Summary: | Character reference parser for new XML parser | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Vicki Pfau <jeffrey+webkit> | ||||||||
Component: | New Bugs | Assignee: | Vicki Pfau <jeffrey+webkit> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Normal | CC: | abarth, jamesr, webkit.review.bot | ||||||||
Priority: | P2 | ||||||||||
Version: | 528+ (Nightly build) | ||||||||||
Hardware: | Unspecified | ||||||||||
OS: | Unspecified | ||||||||||
Bug Depends on: | |||||||||||
Bug Blocks: | 64396 | ||||||||||
Attachments: |
|
Description
Vicki Pfau
2011-07-12 14:48:14 PDT
Created attachment 100586 [details]
Patch
Comment on attachment 100586 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=100586&action=review > Source/WebCore/xml/parser/XMLCharacterReferenceParser.cpp:37 > +namespace { No need to indent for namespaces. > Source/WebCore/xml/parser/XMLCharacterReferenceParser.cpp:46 > + inline UChar adjustEntity(UChar32 value) You probably don't need this for XML. This is a strange HTML legacy thing. > Source/WebCore/xml/parser/XMLCharacterReferenceParser.cpp:94 > +bool consumeCharacterReference(SegmentedString& source, Vector<UChar, 16>& decodedCharacter, bool& notEnoughCharacters) You should be able to share almost all of this code with the HTML version by taking convertToUTF16 as a template parameter. Comment on attachment 100586 [details]
Patch
I'm going to make this r- to encourage you to share more code using a template.
Created attachment 100743 [details]
Patch
Comment on attachment 100743 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=100743&action=review This looks great. We could od some work to fixup some naming mistakes I made when writing this code originally, but that's nto a big deal. (The most pervasive one is renaming "cc" to either currentCharacter or "c" as appropriate.) > Source/WebCore/xml/parser/CharacterReferenceParserInlineMethods.h:28 > +#ifndef CharacterReferenceParserInlineMethods_h > +#define CharacterReferenceParserInlineMethods_h I'd drop the word "Methods" from the name of this file/ > Source/WebCore/xml/parser/CharacterReferenceParserInlineMethods.h:51 > +template <typename ParserFunctions> > +inline bool consumeCharacterReference(SegmentedString& source, Vector<UChar, 16>& decodedCharacter, bool& notEnoughCharacters, UChar additionalAllowedCharacter) jamesr says to get rid of this inline keyword. That's probably the right. The compiler will probably just generate an unconditional jump anyway. > Source/WebCore/xml/parser/XMLCharacterReferenceParser.cpp:45 > + return 0xFFFD; bad indents in this file. > Source/WebCore/xml/parser/XMLCharacterReferenceParser.cpp:66 > + ASSERT_NOT_REACHED(); Do you mean notImplemented() ? You should also listen to whatever suggestions jamesr has. Created attachment 100840 [details]
Patch for landing
Comment on attachment 100840 [details] Patch for landing Clearing flags on attachment: 100840 Committed r91025: <http://trac.webkit.org/changeset/91025> All reviewed patches have been landed. Closing bug. |