I have enclosed to this email a review of kurmiashish's code and recommendations on what to merge upstream and when.
I'm mostly happy with what he did so far, namely: - A BMP filter - Extend the current HTML filter to support new attributes (HTML5.0) - Parse XHTML as XML (imho we need a better, generic purpose XML filter) - A brand new CSS filter Currently he is working on an ATOM 1.0 filter and I expect it to be ready soon (it's relatively easy to do). I am not sure at this stage whether we will have a RSS filter (they are many versions of the norm, no-one really agree on one; and it's complex). His next assignments are in order: - XHTML support in ATOM - ThawIndex filter (it's basic XML: trivial) - SVG support (including in HTML) - More work on XHTML (parse it as XML) ... Then we will see how far we are in the program... And whether we want to go for audio/video formats or RSS... Suggestions from there on will be welcome. He probably shouldn't be the one merging his code upstream. As I refuse to commit to freenet-staging someone else should probably do it (it's trivial, they are no conflicts and 3rd party reviewing won't harm). On a side note: Ashish has been busy with personal issues lately; I hope the best for what he has on his mind. I will do his mid-term evaluation this week-end: for me he's passing.
BMPFilter.java: Get rid of readInt and readShort; use what we have in freenet.support.Fields for instance... Magic numbers are *bad* and that code is error-prone and not self-explicit Appart from that I think it's ready to be enabled upstream. ############################################################################### CSSTokenizerFilter.java: Rename it to CSSFilter.java Try to declare each of the imports you need; We usually don't use wildcards. Don't use your own log method: use the logging infrastructure. We use to have problem with nested locking on the Logger class and its sub-implementations. Register a LogThresholdCallback statically and then prefix your log statements with if(logMINOR) ... Performance wise it does matter. Why do you catch IOException in parse()? You shouldn't. Your concat() method ought to be in freenet.support elementVerifiers, allelementVerifiers, auxilaryVerifiers and friends ought to be static AND final Is there a reason why auxilaryVerifiers is 100 PropertyVerifiers? We can have arbitrary limits but justify them; at least put a comment... getVerifier(String element) is far away from being thread safe... Whether we want to have locking here or not is open to discussion. in HTMLelementVerifier(String), don't use StringBuffer: use StringBuilders (see the javadoc for the difference) filterCSS() : I don't like that one. constants ought to belong to the class itself and to have descriptive names! You are using comments but not javadoc here, which is bad because IDEs don't pick it up. Factor it out and put descriptive names and use enums. The rest of the method lacks comments. I would suggest you replace all the HashSets by enums. (allowedUnits, colorKeywords, ...) should be enums, not HashSets imho is*Checker(String) : those methods should log the exceptions they catch using the logger. Hmmm, this code is clearly not ready to be deployed but I'd still suggest it's merged upstream and proposed as an alternative filter (maybe through a config. option). It's *safer* than the current CSS filter but requires testing. ############################################################################### CSSParser.java: get rid of it alltogether; Usually we use TMCI and the FILTER: keyword to test filters. ############################################################################### ElementInfo.java: Same comments as above: I think we should use enums there too... ############################################################################### HTMLFilter.java: openElements should be final the XHTML part should check that the basic document structure is there... and that the encoding specified is the one the filter is supposed to operate with. Should be merged upstream so that we have support of the new tags.
signature.asc
Description: Digital signature
_______________________________________________ Devl mailing list Devl@freenetproject.org http://emu.freenetproject.org/cgi-bin/mailman/listinfo/devl
