[webkit-reviews] review denied: [Bug 20710] WebKit should support defer and async on script elements : [Attachment 57565] Patch

[bugzilla-daemon at webkit.org]

Adam Barth <abarth at webkit.org> has denied Tony Gentilcore
<tonyg at chromium.org>'s request for review:
Bug 20710: WebKit should support defer and async on script elements
https://bugs.webkit.org/show_bug.cgi?id=20710

Attachment 57565: Patch
https://bugs.webkit.org/attachment.cgi?id=57565&action=review

------- Additional Comments from Adam Barth <abarth at webkit.org>
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.