Created attachment 184792 [details] Patch

Comment on attachment 184792 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=184792&action=review > Source/WebCore/ChangeLog:11 > + No new tests because covered by existing tests. Do you have an idea as to how many this causes to pass? > Source/WebCore/html/parser/HTMLDocumentParser.cpp:186 > +bool HTMLDocumentParser::shouldUseThreading() const > +{ > + return m_options.useThreading && !isParsingFragment() && !document()->url().isBlankURL(); > +} We had this exact logic in an earlier version, but removed it in favor of a solution which could handle empty documnts (in addition to about:blank, which just happens to be empty). I'm surprised javascript is counted as a blank url?

(In reply to comment #2) > (From update of attachment 184792 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=184792&action=review > > > Source/WebCore/ChangeLog:11 > > + No new tests because covered by existing tests. > > Do you have an idea as to how many this causes to pass? No. I only ran the tests in loading/ and it caused us to pick up 3 there. Not sure how many other tests like that are out there. > > > Source/WebCore/html/parser/HTMLDocumentParser.cpp:186 > > +bool HTMLDocumentParser::shouldUseThreading() const > > +{ > > + return m_options.useThreading && !isParsingFragment() && !document()->url().isBlankURL(); > > +} > > We had this exact logic in an earlier version, but removed it in favor of a solution which could handle empty documnts (in addition to about:blank, which just happens to be empty). I'm surprised javascript is counted as a blank url? Yeah, JavaScript is counted as a blank URL. The thing that gets piped to append() is the result of the JavaScript execution, not the JavaScript itself and at that time that document has an empty URL.

> Yeah, JavaScript is counted as a blank URL. The thing that gets piped to append() is the result of the JavaScript execution, not the JavaScript itself and at that time that document has an empty URL. Maybe we should switch it to using insert instead?

Comment on attachment 184792 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=184792&action=review > Source/WebCore/html/parser/HTMLDocumentParser.cpp:185 > + return m_options.useThreading && !isParsingFragment() && !document()->url().isBlankURL(); We should put this logic in HTMLParserOptions so that m_options reflects reality. IMHO, disabling threading for blank URLs makes sense. Given that we've wanted to do this twice, it's probably the right thing to do.

Created attachment 185009 [details] Patch

> > Source/WebCore/html/parser/HTMLDocumentParser.cpp:185 > > + return m_options.useThreading && !isParsingFragment() && !document()->url().isBlankURL(); > > We should put this logic in HTMLParserOptions so that m_options reflects reality. Done

Comment on attachment 185009 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=185009&action=review > Source/WebCore/html/parser/HTMLParserOptions.cpp:44 > - useThreading = settings && settings->threadedHTMLParser(); > + useThreading = settings && settings->threadedHTMLParser() && (!document || !document->url().isBlankURL()); settings being non-zero implies that document is non-zero, so you can remove the check for !document.

Created attachment 185016 [details] Patch for landing

Comment on attachment 185016 [details] Patch for landing Clearing flags on attachment: 185016 Committed r140986: <http://trac.webkit.org/changeset/140986>

All reviewed patches have been landed. Closing bug.