Bug 106256 – HTMLTreeBuilder should not depend on Frame

• Component: New Bugs
• Status: RESOLVED FIXED

[Adam Barth]

HTMLTreeBuilder should not depend on Frame

[Adam Barth]

Created attachment 181569 [details]
Patch

[Eric Seidel (no email)]

Comment on attachment 181569 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=181569&action=review

I'm slightly confused as to why you changed the test.  But otherwise this looks fine.  I think you should move to a more explicit method name for getting the right options before landing.

> Source/WebCore/html/parser/HTMLMetaCharsetParser.cpp:44
> +    : m_tokenizer(HTMLTokenizer::create(HTMLParserOptions())) // No pre-HTML5 parser quirks.

This feels a bit odd.  Maybe we should give it a better name?  Like HTMLParserOptions::defaultOptions()?  or HTMLParserOptions::allOptionsDisabled()?

> Source/WebCore/html/parser/HTMLParserOptions.h:44
>          , maximumDOMTreeDepth(0)

Is 0 the value you want for "default" in my previous comment?  That's part of where my confusion comes from.  It's not clear what the empty/default constructor is supposed to return there.

[Adam Barth]

> I'm slightly confused as to why you changed the test.

Previously, the HTMLTreeBuilder would pull the "is scripting enabled" bit from the Frame.  Now, we push the information when we start parsing.  That means if the Frame changes its mind between when it starts parsing and when the <noscript> tag gets processed, we'll parse the <noscript> tag differently.

In this specific case, the overridePreference API on testRunner is twiddling the "is scripting enabled" bit on the Frame (really on the Frame's Page's Settings).

> > Source/WebCore/html/parser/HTMLMetaCharsetParser.cpp:44
> > +    : m_tokenizer(HTMLTokenizer::create(HTMLParserOptions())) // No pre-HTML5 parser quirks.
> 
> This feels a bit odd.  Maybe we should give it a better name?  Like HTMLParserOptions::defaultOptions()?  or HTMLParserOptions::allOptionsDisabled()?

Good idea.

> > Source/WebCore/html/parser/HTMLParserOptions.h:44
> >          , maximumDOMTreeDepth(0)
> 
> Is 0 the value you want for "default" in my previous comment?  That's part of where my confusion comes from.  It's not clear what the empty/default constructor is supposed to return there.

Yeah, we should probably use the default value from Settings.  I'll fix that before landing too.

[Eric Seidel (no email)]

(In reply to comment #3)
> > > Source/WebCore/html/parser/HTMLMetaCharsetParser.cpp:44
> > > +    : m_tokenizer(HTMLTokenizer::create(HTMLParserOptions())) // No pre-HTML5 parser quirks.
> > 
> > This feels a bit odd.  Maybe we should give it a better name?  Like HTMLParserOptions::defaultOptions()?  or HTMLParserOptions::allOptionsDisabled()?
> 
> Good idea.

You could also just construct a HTMLParserOptions as normal here, and then twiddle the one bit you care about.  That would also be clearer.

[Eric Seidel (no email)]

In general I'm not a big fan of HTMLParserOptions have a default constructor or any smarts of its own. :)

[Adam Barth]

> In general I'm not a big fan of HTMLParserOptions have a default constructor or any smarts of its own. :)

Ok, I'll remove the default constructor.  I agree that it's likely to be misused.

[Adam Barth]

Created attachment 181597 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 181597 [details]
Patch for landing

Clearing flags on attachment: 181597

Committed r139020: <http://trac.webkit.org/changeset/139020>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.