Bug 45310

Created attachment 66741 [details] Patch

Comment on attachment 66741 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=66741&action=prettypatch Looks fantastic. Please figure out the non-async blocking onload or not before landing. > WebCore/dom/AsyncScriptRunner.cpp:88 > + if (Frame* frame = m_document->frame()) > + frame->loader()->checkCompleted(); You should add your comment from the changelog here. It's not immediately obvious why we do this. > WebCore/loader/FrameLoader.cpp:841 > + if (m_frame->document()->asyncScriptRunner()->hasPendingScripts()) Will this return true for non-asyc, non-parser inserted scripts? i.e. an inline <script> element inserting a non-async script tag. That shouldn't delay onload, should it? Do we have a test for that?

> You should add your comment from the changelog here. It's not immediately obvious why we do this. Done. > Will this return true for non-asyc, non-parser inserted scripts? i.e. an inline <script> element inserting a non-async script tag. That shouldn't delay onload, should it? Do we have a test for that? That is a really good question. This patch would make those dynamically inserted scripts block the load event. I wrote a quick test that loads a slow script by appending it to the head from an inline script and seeing if it executes before the body's onload handler. Here are the results: Blocks load Chrome 6: NO FF3.6: YES FF4: YES IE8: NO IE9: NO OP10.5: YES Safari 5: NO There is no strong consensus about the behavior :-( However, it looks to me like the HTML5 spec requires that it block the load event. In http://www.whatwg.org/specs/web-apps/current-work/#script step #9, the script is added to the "set of scripts that will execute as soon as possible" whether it is actually marked "async" by the parser or whether it just wasn't parser inserted. Then http://www.whatwg.org/specs/web-apps/current-work/#the-end step #5 causes us to block the load event until all scripts that will execute as soon as possible are complete. This change actually does break the readystate test that I added in https://bugs.webkit.org/show_bug.cgi?id=45119 (but I didn't notice it before uploading the patch). It makes it behave the same as FF4 as mentioned in https://bugs.webkit.org/show_bug.cgi?id=45119#c10. So options are: 1. HTML5 (leave the code as-is, update the readystate expectations) 2. Legacy (add a special case to the code so that only explicitly marked async scripts block load, leave expectations as-is). My preference is #1, and we can write #2 if #1 causes compat problems. What do you think?

Created attachment 66780 [details] Update readystate expectations. Use increment/decrementLoadEventDelayCount()

> Created an attachment (id=66780) [details] > Update readystate expectations. Use increment/decrementLoadEventDelayCount() I updated the patch for #1 and discovered the increment/decrementLoadEventDelayCount which simplifies the change a bit.

Comment on attachment 66780 [details] Update readystate expectations. Use increment/decrementLoadEventDelayCount() View in context: https://bugs.webkit.org/attachment.cgi?id=66780&action=prettypatch Looks good to me. It would be good for Eric to look at this change too before landing. > WebCore/dom/AsyncScriptRunner.cpp:46 > + m_scriptsToExecuteSoon[i].first->element()->deref(); // Balances ref() in executeScriptSoon(). Manual ref counting is sadness. Is there no way to do this with RefPtr?

> > + m_scriptsToExecuteSoon[i].first->element()->deref(); // Balances ref() in executeScriptSoon(). > Manual ref counting is sadness. Is there no way to do this with RefPtr? Totally agree. I factored this code out of Document and left it exactly as-is during the refactor. My plan is to refactor this and ScriptElementData a bit so that it can use PendingScripts instead. I'll wait for Eric before landing.

Comment on attachment 66780 [details] Update readystate expectations. Use increment/decrementLoadEventDelayCount() View in context: https://bugs.webkit.org/attachment.cgi?id=66780&action=prettypatch LGTM! > WebCore/dom/AsyncScriptRunner.cpp:60 > m_scriptsToExecuteSoon.append(make_pair(data, cachedScript)); Clearly make_pair should instead be a class, which does the load event increment and ref counting for you. But that can be a second patch. You should consider implementing for= event= support for <script> if you've been enjoying this. That should be an easy fix and will fix some IE-only sites. (comedycentral.com used to depend on for= event= support for instance)

> Clearly make_pair should instead be a class, which does the load event increment and ref counting for you. But that can be a second patch. Yep. Like I mentioned to abarth, I think the right thing to do here is to use PendingScript. Definitely a separate patch. > You should consider implementing for= event= support for <script> if you've been enjoying this. That should be an easy fix and will fix some IE-only sites. (comedycentral.com used to depend on for= event= support for instance) Thanks. I'll think about those some more, but I'm not really wild about the patterns they encourage and they are listed as non-conforming features. Other than possible compat issues w/ IE-specific sites, do you have any arguments for why they should exist?

Comment on attachment 66780 [details] Update readystate expectations. Use increment/decrementLoadEventDelayCount() Rejecting patch 66780 from commit-queue. Failed to run "['WebKitTools/Scripts/run-webkit-tests', '--no-launch-safari', '--exit-after-n-failures=1', '--wait-for-httpd', '--ignore-tests', 'compositing,media', '--quiet']" exit_code: 1 Running build-dumprendertree Compiling Java tests make: Nothing to be done for `default'. Running tests from /Users/eseidel/Projects/CommitQueue/LayoutTests Testing 20949 test cases. http/tests/misc/script-async.html -> failed Exiting early after 1 failures. 20193 tests run. 490.79s total testing time 20192 test cases (99%) succeeded 1 test case (<1%) had incorrect layout 26 test cases (<1%) had stderr output Full output: http://queues.webkit.org/results/3948330

Created attachment 67040 [details] Patch for landing

> > You should consider implementing for= event= support for <script> if you've been enjoying this. That should be an easy fix and will fix some IE-only sites. (comedycentral.com used to depend on for= event= support for instance) > Thanks. I'll think about those some more, but I'm not really wild about the patterns they encourage and they are listed as non-conforming features. Other than possible compat issues w/ IE-specific sites, do you have any arguments for why they should exist? I caught up w/ Adam about this. He explained to me that HTML5 doesn't suggest these attributes to be implemented the way that IE implements them, it suggests neutering them. That seems like a reasonable way to have sites that use them degrade more gracefully without polluting the web platform. I'll put a patch together.

Comment on attachment 67040 [details] Patch for landing Clearing flags on attachment: 67040 Committed r67163: <http://trac.webkit.org/changeset/67163>

All reviewed patches have been landed. Closing bug.