Summary: | Add framework for a new/dummy XMLDocumentParser | ||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Vicki Pfau <jeffrey+webkit> | ||||||||||||
Component: | New Bugs | Assignee: | Vicki Pfau <jeffrey+webkit> | ||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||
Severity: | Normal | CC: | abarth, catfish.man, dglazkov, hyatt, webkit.review.bot | ||||||||||||
Priority: | P2 | ||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||
Hardware: | Unspecified | ||||||||||||||
OS: | Unspecified | ||||||||||||||
Attachments: |
|
Description
Vicki Pfau
2011-07-05 14:55:45 PDT
Created attachment 99749 [details]
Patch
Comment on attachment 99749 [details] Patch Attachment 99749 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/8982914 Comment on attachment 99749 [details] Patch Attachment 99749 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/8987467 Created attachment 99761 [details]
Patch
[+hyatt] Were you planning to address Hyatt's feedback on webkit-dev before moving forward with this effort? Comment on attachment 99761 [details]
Patch
There’s no need for ENABLE_NEW_XML to be in FeatureDefines.xcconfig.
The commit-queue encountered the following flaky tests while processing attachment 99761 [details]: media/adopt-node-crash.html bug 64014 (authors: annacc@chromium.org, eric.carlson@apple.com, jamesr@chromium.org, and vrk@chromium.org) The commit-queue is continuing to process your patch. (In reply to comment #5) > [+hyatt] > > Were you planning to address Hyatt's feedback on webkit-dev before moving forward with this effort? I think it's fine to try the experiment. Maybe it ends up being so much better performance-wise and footprint-wise that it's worth replacing libxml2. A possible bridge step for XSLT would be to abort parsing with the new XML parser and revert to using libxml2/libxslt when XSLT stylesheets are encountered. Since they are at the top of the file, this would not be a big deal. Created attachment 99895 [details]
Patch
Comment on attachment 99895 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=99895&action=review > Source/WebCore/xml/parser/XMLDocumentParserNew.cpp:40 > +XMLDocumentParser::XMLDocumentParser(Document* document, FrameView* frameView) A FrameView? That's very strange. Are you sure you need that? If anything, you might need a Frame, but the HTMLDocumentParser gets away without one, so you probably don't need it. Certainly there's no necessary connection between parsing XML and FrameView. > Source/WebCore/xml/parser/XMLDocumentParserNew.cpp:54 > + , m_requestingScript(false) In the HTML parser, we tried to move as much state as possible into subobjects because the main parser class tends to get complicated. For example, that's why we have the ScriptRunner class. Does XHTML support defer and async? If so, there's probably some logic in ScriptRunner that you'll want to re-use. > Source/WebCore/xml/parser/XMLDocumentParserNew.cpp:95 > +void XMLDocumentParser::doWrite(const String&) > +{ > +} What is doWrite? I'd skip these for now because they're not need to implement the DocumentParser interface. > Source/WebCore/xml/parser/XMLDocumentParserNew.cpp:132 > + attrsOK = false; Generally, we prefer variable names made out of full english words. This function appears to be unused, so maybe we should add it when we need it? Comment on attachment 99895 [details]
Patch
I think you're falling into a bad trap with this patch. You're accepting XMLDocumentParser.h has given, but that header has a bunch of junk in it that we don't want to have contaminate the new parser. Can we start with a cleaner slate? You should be able to just create a new subclass of ScriptableDocumentParser and use an ifdef at the construction site.
Created attachment 100139 [details]
Patch
Comment on attachment 100139 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=100139&action=review > Source/WebCore/xml/parser/NewXMLDocumentParser.h:37 > +class NewXMLDocumentParser : public ScriptableDocumentParser, public CachedResourceClient { NewXMLDocumentParser probably won't need to be a CachedResourceClient (or at least we can add that when needed). > Source/WebCore/xml/parser/NewXMLDocumentParser.h:53 > + NewXMLDocumentParser(Document*); This can be private. > Source/WebCore/xml/parser/NewXMLDocumentParser.h:68 > + // CachedResourceClient > + virtual void notifyFinished(CachedResource*); This can probably also be removed. Created attachment 100142 [details]
Patch
Comment on attachment 100142 [details] Patch Clearing flags on attachment: 100142 Committed r90654: <http://trac.webkit.org/changeset/90654> All reviewed patches have been landed. Closing bug. |