Bug 106256 - HTMLTreeBuilder should not depend on Frame
Summary: HTMLTreeBuilder should not depend on Frame
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Adam Barth
URL:
Keywords:
Depends on:
Blocks: 106127
  Show dependency treegraph
 
Reported: 2013-01-07 14:32 PST by Adam Barth
Modified: 2013-01-07 17:23 PST (History)
2 users (show)

See Also:


Attachments
Patch (22.48 KB, patch)
2013-01-07 14:37 PST, Adam Barth
no flags Details | Formatted Diff | Diff
Patch for landing (21.08 KB, patch)
2013-01-07 16:38 PST, Adam Barth
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Adam Barth 2013-01-07 14:32:52 PST
HTMLTreeBuilder should not depend on Frame
Comment 1 Adam Barth 2013-01-07 14:37:58 PST
Created attachment 181569 [details]
Patch
Comment 2 Eric Seidel (no email) 2013-01-07 14:46:20 PST
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.
Comment 3 Adam Barth 2013-01-07 14:54:34 PST
> 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.
Comment 4 Eric Seidel (no email) 2013-01-07 14:55:59 PST
(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.
Comment 5 Eric Seidel (no email) 2013-01-07 14:56:18 PST
In general I'm not a big fan of HTMLParserOptions have a default constructor or any smarts of its own. :)
Comment 6 Adam Barth 2013-01-07 15:18:16 PST
> 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.
Comment 7 Adam Barth 2013-01-07 16:38:15 PST
Created attachment 181597 [details]
Patch for landing
Comment 8 WebKit Review Bot 2013-01-07 17:23:04 PST
Comment on attachment 181597 [details]
Patch for landing

Clearing flags on attachment: 181597

Committed r139020: <http://trac.webkit.org/changeset/139020>
Comment 9 WebKit Review Bot 2013-01-07 17:23:08 PST
All reviewed patches have been landed.  Closing bug.