Bug 45119

Summary: Implement HTML5 definition of document.readyState
Product: WebKit Reporter: Tony Gentilcore <tonyg>
Component: New BugsAssignee: Tony Gentilcore <tonyg>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, ap, commit-queue, eric, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Other   
OS: OS X 10.5   
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch for landing none

Description Tony Gentilcore 2010-09-02 11:38:46 PDT
Implement HTML5 definition of document.readyState
Comment 1 Tony Gentilcore 2010-09-02 13:32:47 PDT
Created attachment 66405 [details]
Patch
Comment 2 Eric Seidel (no email) 2010-09-02 14:11:13 PDT
Attachment 66405 [details] did not build on mac:
Build output: http://queues.webkit.org/results/3927063
Comment 3 Alexey Proskuryakov 2010-09-02 15:47:53 PDT
Maybe the test could have e.g. an image subresource to make a difference between DOMContentLoaded and onload?
Comment 4 Tony Gentilcore 2010-09-02 16:00:20 PDT
> Maybe the test could have e.g. an image subresource to make a difference between DOMContentLoaded and onload?

Good call. But an image subresource could actually load before or after DCL, so the readyState might be loading or interactive depending on timing.

How about I add two subresources:
1. An external script (guaranteed to block parsing) and check readyState == "loading" in its onload handler.
2. Dynamically append an external script during the DCL event and check readyState == "interactive" in its onload handler.
Comment 5 Adam Barth 2010-09-02 16:12:10 PDT
Comment on attachment 66405 [details]
Patch

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

> WebCore/dom/Document.cpp:964
> +    default:
> +        ASSERT_NOT_REACHED();
> +        return String();
Generally we omit the default case.  The compiler will warn us if we forget something from the enum.

> WebCore/dom/XMLDocumentParserLibxml2.cpp:1319
> +        // FIXME: This hack probably isn't the best way to accomplish this.
> +        document()->setReadyState(Document::Complete); // Make the doc think it's done, so it will apply xsl sheets.
Yuck.  XML -> tears.

> WebCore/loader/FrameLoader.cpp:418
> -        m_frame->document()->setParsing(false);
> +        // FIXME: HTML5 doesn't tell us to set the state to complete when aborting, but we do anyway to match legacy behavior.
> +        // http://www.whatwg.org/specs/web-apps/current-work/#the-end
> +        m_frame->document()->setReadyState(Document::Complete);
Maybe file a bug against the spec?
Comment 6 Tony Gentilcore 2010-09-05 21:02:11 PDT
Created attachment 66603 [details]
Patch
Comment 7 Tony Gentilcore 2010-09-05 21:04:32 PDT
> Generally we omit the default case.  The compiler will warn us if we forget something from the enum.
Done.

> Maybe file a bug against the spec?
Done. Linked in FIXME.

I was hoping to replace m_bParsing with either "!m_parser || !m_parser->isParsing()" or "m_readyState == Loading", but it turns out m_bParsing is a little more subtle than that. I believe I can still clean it up, but probably not best in this patch.
Comment 8 Tony Gentilcore 2010-09-05 21:35:14 PDT
Created attachment 66605 [details]
Patch
Comment 9 Adam Barth 2010-09-05 23:13:58 PDT
Comment on attachment 66605 [details]
Patch

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

Looks great.  Please be on the lookout for any compat issues.

> LayoutTests/ChangeLog:9
> +        * fast/dom/Document/readystate-expected.txt: Added.
> +        * fast/dom/Document/readystate.html: Added. Reads readyState inline script, external script, deferred script, DOMContentLoaded, onload, and dynamic post-onload script.
Out of curiosity, did you cross-test this with other browsers?

> LayoutTests/fast/dom/Document/readystate.html:5
> +<link rel="stylesheet" href="../../js/resources/js-test-style.css">
> +<script src="../../js/resources/js-test-pre.js"></script>
Woah, crazy.  It's like a script test that doesn't use an external JS file.  In my ideal world, doing something like this would be as easy as dump-as-markup: just add a script tag.

> WebCore/dom/Document.cpp:962
> +    switch (m_readyState) {
> +    case Loading:
> +        return "loading";
> +    case Interactive:
> +        return "interactive";
> +    case Complete:
> +        return "complete";
Should we use DECLARE_STATIC_LOCAL so we don't have to create these string objects every time this API is called?
Comment 10 Tony Gentilcore 2010-09-06 10:40:10 PDT
> Out of curiosity, did you cross-test this with other browsers?

Yes.

Legacy Chrome/Safari behave the same except defer isn't supported so that runs earlier and is "loading", and "interactive" is called "loaded".

Minefield behaves the same with one exception. The script which is dynamically appended in the DCL event runs before onload and thus is "interactive". In WebKit it runs after onload and thus is "complete". This behavioral difference should probably be investigated but is totally orthogonal to this change.

> Woah, crazy.  It's like a script test that doesn't use an external JS file.  In my ideal world, doing something like this would be as easy as dump-as-markup: just add a script tag.

I liked being about to use shouldBeEqualToString for this test. Is there another pattern I should prefer?

> Should we use DECLARE_STATIC_LOCAL so we don't have to create these string objects every time this API is called?

Done.
Comment 11 Adam Barth 2010-09-06 10:42:50 PDT
> I liked being about to use shouldBeEqualToString for this test. Is there another pattern I should prefer?

I meant "crazy" in an awesome way.
Comment 12 Tony Gentilcore 2010-09-06 10:50:09 PDT
(In reply to comment #11)
> > I liked being about to use shouldBeEqualToString for this test. Is there another pattern I should prefer?
> 
> I meant "crazy" in an awesome way.

I'm still slightly conflicted. On one hand it is awesome that tests like this are clean and just involve writing a single file. But on the other hand, they can't be easily run in other browsers (due to the data: hack). For testing the other browsers, I had to write a separate test.
Comment 13 Tony Gentilcore 2010-09-06 10:52:51 PDT
Created attachment 66658 [details]
Patch for landing
Comment 14 WebKit Commit Bot 2010-09-06 11:33:22 PDT
Comment on attachment 66658 [details]
Patch for landing

Clearing flags on attachment: 66658

Committed r66841: <http://trac.webkit.org/changeset/66841>
Comment 15 WebKit Commit Bot 2010-09-06 11:33:27 PDT
All reviewed patches have been landed.  Closing bug.
Comment 16 WebKit Review Bot 2010-09-06 15:20:20 PDT
http://trac.webkit.org/changeset/66841 might have broken Leopard Intel Debug (Tests)
Comment 17 Alexey Proskuryakov 2010-09-07 09:58:21 PDT
> I'm still slightly conflicted.

I think that for a test like this, being able to compare to all other major browsers is important. It's likely that someone will need to modify it in the future for this purpose.