Bug 109607 – Threaded HTML parser should pass the remaining fast/tokenizer tests

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

[Adam Barth]

Threaded HTML parser should pass the remaining fast/tokenizer tests

[Adam Barth]

Created attachment 187917 [details]
Patch

[Eric Seidel (no email)]

Comment on attachment 187917 [details]
Patch

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

I think we're gonna need to chat/vc about this one.

> Source/WebCore/html/parser/HTMLDocumentParser.cpp:317
> +    ASSERT(checkpoint->unparsedInput.isSafeToSendToAnotherThread());

Seems we should just call isSafeToSentToAnotherThread on checkpoint itself?

[Adam Barth]

(In reply to comment #2)
> (From update of attachment 187917 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=187917&action=review
> 
> I think we're gonna need to chat/vc about this one.

Ok.  Let me know when you're ready.

> > Source/WebCore/html/parser/HTMLDocumentParser.cpp:317
> > +    ASSERT(checkpoint->unparsedInput.isSafeToSendToAnotherThread());
> 
> Seems we should just call isSafeToSentToAnotherThread on checkpoint itself?

Sure, we could do that.

[Eric Seidel (no email)]

Comment on attachment 187917 [details]
Patch

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

> Source/WebCore/html/parser/HTMLDocumentParser.cpp:251
> +    ASSERT(!m_haveBackgroundParser || mode == ForceSynchronous);

These scare me.  We should just make sure both of these ASSERT changes are necessary.

[Eric Seidel (no email)]

Comment on attachment 187917 [details]
Patch

LGTM.  Please check the ASSERTS before landing.

[Adam Barth]

We talked through the ASSERTs on VC.  It's for the case when you have a script-created parser that document.writes something like "<script src=...></script>Foo".  We'll parse the script tag synchronously but then wait for the external script to arrive.  Once the external script arrives, we'll parse the "Foo" asynchronously.  In that case, shouldUseBackgroundParser is true but we don't m_haveBackgroundParser.

We discussed several approaches for cleaning up these state variables, and we'll rename or rework them to be clearer in a future patch.

[WebKit Review Bot]

Comment on attachment 187917 [details]
Patch

Clearing flags on attachment: 187917

Committed r142673: <http://trac.webkit.org/changeset/142673>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.