Created attachment 181569 [details] Patch

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.

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

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

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

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

Created attachment 181597 [details] Patch for landing

Comment on attachment 181597 [details] Patch for landing Clearing flags on attachment: 181597 Committed r139020: <http://trac.webkit.org/changeset/139020>

All reviewed patches have been landed. Closing bug.