[Webkit-unassigned] [Bug 20710] WebKit should support defer and async on script elements

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=20710


Adam Barth <abarth at webkit.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #57565|review?                     |review-
               Flag|                            |




--- Comment #24 from Adam Barth <abarth at webkit.org>  2010-06-01 11:11:39 PST ---
(From update of attachment 57565)
We'll want Eric to do the final review, but here are some comments for now.

LayoutTests/http/tests/local/resources/async-script.js:1
 +  window.status_ += " async ";
The trailing underscore is a Google-ism.  It's probably ok for tests to use a variety of styles, but I just mention it so you're aware.

LayoutTests/ChangeLog:5
 +          Added tests for defer and async script attributes.
Might be worth noting there that these tests are supposed to pass in the HTML5 parser but fail in the old parser.

LayoutTests/ChangeLog:14
 +          * http/tests/local/script-defer.html: Added.
If you run-webkit-tests, it should generate the missing expectation files.  You can specify tests to run on the command line if you don't want to run the whole suite.

LayoutTests/http/tests/local/resources/slow-async-script.cgi:4
 +  sleep(0.5);
Boo.  This will make the test somewhat slow and possibly flaky...  Not sure how to make it better though...

LayoutTests/http/tests/local/script-async.html:28
 +          layoutTestController.notifyDone();
If you're going to call notifyDone in the onload handler anyway, you don't need to waitUntilDone().  The default end of the test is after the onload handler finishes.

LayoutTests/http/tests/local/script-async.html:32
 +  <script src="http://127.0.0.1:8000/local/resources/slow-async-script.cgi" async="ASYNC"></script>
Do we want to test some syntactic variations of these attributes?

LayoutTests/http/tests/local/script-defer.html:20
 +      if (window.status_ != expected)
If there's just one expected result, we can just dump out the result and manage the expectations in the -expected.txt file, but it doesn't really matter.

LayoutTests/ChangeLog:12
 +          * http/tests/local/resources/slow-defer-script.cgi: Added.
Why are these tests in the "local" directory?  They don't seem to interact with the file system (which is why things go in local).

WebCore/ChangeLog:9
 +          the legacy parser.
Would be nice to have more information in the ChangeLog for future developers trying to understand why you did what you did.

WebCore/dom/ScriptElement.cpp:314
 +      return !m_scriptElement->sourceAttributeValue().isEmpty() && m_scriptElement->asyncAttributeValue();
Can you add a link to the spec where this is defined?  It's not obvious to me that this is the correct resolution when an inline script has an async attribute.

WebCore/html/HTML5ScriptRunner.cpp:148
 +      m_scriptsToExecuteAfterParsing.removeFirst();
There isn't a takeFirst method that does both of these?  Maybe we should add one?

WebCore/html/HTML5ScriptRunner.cpp:151
 +          return executeScriptsWaitingForParsing();
Will this recursion explode if there are thousands of these scripts?  Maybe a looping construct is better?  Can running the script cause re-entrancy here we need to worry about?

WebCore/dom/ScriptElement.cpp:320
 +      return !m_scriptElement->sourceAttributeValue().isEmpty() && !m_scriptElement->asyncAttributeValue() && m_scriptElement->deferAttributeValue();
We need a test for this logic.

WebCore/dom/ScriptElement.cpp:314
 +      return !m_scriptElement->sourceAttributeValue().isEmpty() && m_scriptElement->asyncAttributeValue();
Also, we need a test for this logic.

WebCore/html/HTML5ScriptRunner.cpp:147
 +      m_parsingBlockingScript = m_scriptsToExecuteAfterParsing.first();
How can a script be blocking parsing if we're done parsing?

WebCore/html/HTML5ScriptRunner.cpp:238
 +      PendingScript pendingScript = PendingScript();
No need to call the default constructor explicitly.

WebCore/html/HTML5ScriptRunner.cpp:239
 +      requestScript(&pendingScript, scriptElement);
If pendingScript is a value type, we should pass it by non-const reference.  If pendingScript is an output parameter, we should lass it last.

WebCore/html/HTML5ScriptRunner.cpp:253
 +          // Asynchronous scripts are not handled by the script runner because they do not block parsing.
But deferred scripts do block parsing?

WebCore/html/HTML5Tokenizer.cpp:91
 +      if (shouldContinueParsing)
Do we need a not here?  If we should continue parsing, then we don't want to finish the tree builder.  If this branch goes the other way, who will call finish on the tree builder?

WebCore/html/HTMLScriptElement.cpp:164
 +      return deferAttributeValue();
Why add this virtual method call?  Maybe have the virtual one call the non-virtual one?

WebCore/html/HTMLScriptElement.cpp:159
 +      setAttribute(asyncAttr, async ? "" : 0);
Please add a test for this.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.