Bug 109607 - Threaded HTML parser should pass the remaining fast/tokenizer tests
Summary: Threaded HTML parser should pass the remaining fast/tokenizer tests
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-02-12 13:12 PST by Adam Barth
Modified: 2013-02-12 14:46 PST (History)
4 users (show)

See Also:


Attachments
Patch (8.80 KB, patch)
2013-02-12 13:14 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-02-12 13:12:32 PST
Threaded HTML parser should pass the remaining fast/tokenizer tests
Comment 1 Adam Barth 2013-02-12 13:14:56 PST
Created attachment 187917 [details]
Patch
Comment 2 Eric Seidel (no email) 2013-02-12 13:17:48 PST
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?
Comment 3 Adam Barth 2013-02-12 13:23:23 PST
(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.
Comment 4 Eric Seidel (no email) 2013-02-12 13:28:52 PST
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.
Comment 5 Eric Seidel (no email) 2013-02-12 13:32:24 PST
Comment on attachment 187917 [details]
Patch

LGTM.  Please check the ASSERTS before landing.
Comment 6 Adam Barth 2013-02-12 13:41:52 PST
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.
Comment 7 WebKit Review Bot 2013-02-12 14:46:04 PST
Comment on attachment 187917 [details]
Patch

Clearing flags on attachment: 187917

Committed r142673: <http://trac.webkit.org/changeset/142673>
Comment 8 WebKit Review Bot 2013-02-12 14:46:07 PST
All reviewed patches have been landed.  Closing bug.