Bug 63955

Summary: Add framework for a new/dummy XMLDocumentParser
Product: WebKit Reporter: Vicki Pfau <jeffrey+webkit>
Component: New BugsAssignee: 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 Flags
Patch
none
Patch
none
Patch
none
Patch
none
Patch none

Description Vicki Pfau 2011-07-05 14:55:45 PDT
Add framework for a new/dummy XMLDocumentParser
Comment 1 Vicki Pfau 2011-07-05 14:57:53 PDT
Created attachment 99749 [details]
Patch
Comment 2 WebKit Review Bot 2011-07-05 15:17:09 PDT
Comment on attachment 99749 [details]
Patch

Attachment 99749 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/8982914
Comment 3 Gyuyoung Kim 2011-07-05 15:20:53 PDT
Comment on attachment 99749 [details]
Patch

Attachment 99749 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/8987467
Comment 4 Vicki Pfau 2011-07-05 16:37:18 PDT
Created attachment 99761 [details]
Patch
Comment 5 Adam Barth 2011-07-06 11:45:00 PDT
[+hyatt]

Were you planning to address Hyatt's feedback on webkit-dev before moving forward with this effort?
Comment 6 Mark Rowe (bdash) 2011-07-06 11:58:05 PDT
Comment on attachment 99761 [details]
Patch

There’s no need for ENABLE_NEW_XML to be in FeatureDefines.xcconfig.
Comment 7 WebKit Review Bot 2011-07-06 12:15:59 PDT
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.
Comment 8 Dave Hyatt 2011-07-06 14:30:34 PDT
(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.
Comment 9 Vicki Pfau 2011-07-06 15:38:56 PDT
Created attachment 99895 [details]
Patch
Comment 10 Adam Barth 2011-07-07 01:21:54 PDT
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 11 Adam Barth 2011-07-07 01:24:02 PDT
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.
Comment 12 Vicki Pfau 2011-07-08 11:11:55 PDT
Created attachment 100139 [details]
Patch
Comment 13 Adam Barth 2011-07-08 11:17:38 PDT
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.
Comment 14 Vicki Pfau 2011-07-08 11:45:17 PDT
Created attachment 100142 [details]
Patch
Comment 15 WebKit Review Bot 2011-07-08 12:31:48 PDT
Comment on attachment 100142 [details]
Patch

Clearing flags on attachment: 100142

Committed r90654: <http://trac.webkit.org/changeset/90654>
Comment 16 WebKit Review Bot 2011-07-08 12:31:54 PDT
All reviewed patches have been landed.  Closing bug.