Bug 63955 – Add framework for a new/dummy XMLDocumentParser

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Vicki Pfau]

Add framework for a new/dummy XMLDocumentParser

[Vicki Pfau]

Created attachment 99749 [details]
Patch

[WebKit Review Bot]

Comment on attachment 99749 [details]
Patch

Attachment 99749 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/8982914

[Gyuyoung Kim]

Comment on attachment 99749 [details]
Patch

Attachment 99749 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/8987467

[Vicki Pfau]

Created attachment 99761 [details]
Patch

[Adam Barth]

[+hyatt]

Were you planning to address Hyatt's feedback on webkit-dev before moving forward with this effort?

[Mark Rowe (bdash)]

Comment on attachment 99761 [details]
Patch

There’s no need for ENABLE_NEW_XML to be in FeatureDefines.xcconfig.

[WebKit Review Bot]

The commit-queue encountered the following flaky tests while processing attachment 99761 [details]:

media/adopt-node-crash.html bug 64014 (authors: [e-mail], [e-mail], [e-mail], and [e-mail])
The commit-queue is continuing to process your patch.

[Dave Hyatt]

(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.

[Vicki Pfau]

Created attachment 99895 [details]
Patch

[Adam Barth]

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?

[Adam Barth]

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.

[Vicki Pfau]

Created attachment 100139 [details]
Patch

[Adam Barth]

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.

[Vicki Pfau]

Created attachment 100142 [details]
Patch

[WebKit Review Bot]

Comment on attachment 100142 [details]
Patch

Clearing flags on attachment: 100142

Committed r90654: <http://trac.webkit.org/changeset/90654>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.