[webkit-reviews] review granted: [Bug 40934] Support <script defer> as specified by HTML5 : [Attachment 63029] Fix tab in ChangeLog

[bugzilla-daemon at webkit.org]

Eric Seidel <eric at webkit.org> has granted Tony Gentilcore
<tonyg at chromium.org>'s request for review:
Bug 40934: Support <script defer> as specified by HTML5
https://bugs.webkit.org/show_bug.cgi?id=40934

Attachment 63029: Fix tab in ChangeLog
https://bugs.webkit.org/attachment.cgi?id=63029&action=review

------- Additional Comments from Eric Seidel <eric at webkit.org>
LayoutTests/fast/dom/HTMLScriptElement/defer-double-defer-write.html:14
 +	onbeforeload="doubleWrite(4)"
Why does this onbeforeload run at a different time than the one in the next
test?  This one seems to run after the 3, the next test it runs immediately? 
Is it racey?

LayoutTests/fast/dom/HTMLScriptElement/defer-double-write.html:14
 +	onbeforeload="doubleWrite(3)"
Here is the other onbeforeload your'e expecting to run at a different time than
the other one.	I'm not sure how tehse two tests are different.

Oh, I see, the first one uses a "double defer".  But I still wouldn't expect
the onbeforeload time to
change?LayoutTests/fast/dom/HTMLScriptElement/defer-inline-script.html:11
 +  <script defer>
Excellent.  This test coudl probably be simpler with just a little
document.write(). :)  The script-testing stuff is useful, but you don't really
seem to be uing it here. :)

LayoutTests/fast/dom/HTMLScriptElement/defer-onbeforeload.html:8
 +  Checks that deferred scripts fire onbeforeload immediately and that it is
cancellable.
This test seems to have conflicting expectations with teh first one...

LayoutTests/fast/dom/HTMLScriptElement/defer-onbeforeload.html:10
 +  <script src="resources/shouldnotexecute.js" onbeforeload="return false;"
defer></script>
Seems you'd want to log here, no?

LayoutTests/fast/dom/HTMLScriptElement/defer-script-invalid-url.html:10
 +  <script src="http://invalid:999999/" defer></script>
Do we need/want to check timing expectations of onerror?  That might be racey
though.

LayoutTests/fast/dom/HTMLScriptElement/resources/shouldnotexecute.js:1
 +  debug("should not execute");
Might put FAIL in there.

LayoutTests/http/tests/misc/resources/script-debug-body-background.js:1
 +  debug('Body background: ' +
getComputedStyle(document.body)['background-color']);
This should ideally contain the expectation in it.  Since you're using the
full-blown js-test-pre.js harness, you could use shouldBe here or
shouldBeEqualToString

LayoutTests/http/tests/misc/script-defer-write-slow-stylesheet.html:18
 +  <script
src="http://127.0.0.1:8000/misc/resources/script-write-slow-stylesheet.js"
defer="defer"></script>
So this is testing that writing a stylesheet from a defer script will delay
DOMContentLoaded, right?

WebCore/html/HTMLScriptRunner.cpp:251
 +	AtomicString srcValue = scriptElement->getAttribute(srcAttr);
const AtomicString& srcValue should be sufficient (and prevents one uneeded
ref).

WebCore/html/HTMLScriptRunner.cpp:257
 +	pendingScript.element = scriptElement;
Do we need to end up clearing this in the failure case?  I guess not, since we
weren't doing so before?

WebCore/html/HTMLScriptRunner.cpp:276
 +	if (madeRequest && !m_parsingBlockingScript.cachedScript()->isLoaded())

I think I would have made the m_parsingBlockingScript.cachedScript() dependance
explicit here:

if (madeRequest) {
    ASSERT(m_parsingBlockingScript.cachedScript());
    if (!m_parsingBlockingScript.cachedScript()->isLoaded())
	 watchForLoad(m_parsingBlockingScript);
}

Since requestScript can return w/o a cachedScript(), we should ASSERT that the
bool and cachedScript() always agree somewhere.

WebCore/html/HTMLScriptRunner.cpp:283
 +	bool madeRequest = requestScript(pendingScript, scriptElement);
This is a success bool, it seems.

I'm not sure the name "madeRequest" helps me here.  My first thought was "oh,
do I need to call it again?"

Maybe "requestSuccessfull"?  Or just throw it in the if block itself w/o a
local variable would be OK too.

(This is a nit, and not a big deal.)

In general this looks fine.  I think we could go one more round for nits.