Implement HTML5 definition of document.readyState
Created attachment 66405 [details] Patch
Attachment 66405 [details] did not build on mac: Build output: http://queues.webkit.org/results/3927063
Maybe the test could have e.g. an image subresource to make a difference between DOMContentLoaded and onload?
> 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 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?
Created attachment 66603 [details] Patch
> 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.
Created attachment 66605 [details] Patch
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?
> 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.
> I liked being about to use shouldBeEqualToString for this test. Is there another pattern I should prefer? I meant "crazy" in an awesome way.
(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.
Created attachment 66658 [details] Patch for landing
Comment on attachment 66658 [details] Patch for landing Clearing flags on attachment: 66658 Committed r66841: <http://trac.webkit.org/changeset/66841>
All reviewed patches have been landed. Closing bug.
http://trac.webkit.org/changeset/66841 might have broken Leopard Intel Debug (Tests)
> 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.