Bug 45119 – Implement HTML5 definition of document.readyState

• Component: New Bugs
• Status: RESOLVED FIXED

[Tony Gentilcore]

Implement HTML5 definition of document.readyState

[Tony Gentilcore]

Created attachment 66405 [details]
Patch

[Eric Seidel (no email)]

Attachment 66405 [details] did not build on mac:
Build output: http://queues.webkit.org/results/3927063

[Alexey Proskuryakov]

Maybe the test could have e.g. an image subresource to make a difference between DOMContentLoaded and onload?

[Tony Gentilcore]

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

[Adam Barth]

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?

[Tony Gentilcore]

Created attachment 66603 [details]
Patch

[Tony Gentilcore]

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

[Tony Gentilcore]

Created attachment 66605 [details]
Patch

[Adam Barth]

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?

[Tony Gentilcore]

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

[Adam Barth]

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

I meant "crazy" in an awesome way.

[Tony Gentilcore]

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

[Tony Gentilcore]

Created attachment 66658 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 66658 [details]
Patch for landing

Clearing flags on attachment: 66658

Committed r66841: <http://trac.webkit.org/changeset/66841>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[WebKit Review Bot]

http://trac.webkit.org/changeset/66841 might have broken Leopard Intel Debug (Tests)

[Alexey Proskuryakov]

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