[webkit-reviews] review denied: [Bug 40934] Support <script defer> as specified by HTML5 : [Attachment 59276] Patch

[bugzilla-daemon at webkit.org]

Eric Seidel <eric at webkit.org> has denied 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 59276: Patch
https://bugs.webkit.org/attachment.cgi?id=59276&action=review

------- Additional Comments from Eric Seidel <eric at webkit.org>
First of all, thank you! It's wonderful to have you working on this!

This is a complicated patch.  It may take me a couple passes to review it, but
I'll go through it once with comments now.

LayoutTests/http/tests/loading/resources/defer-script.js:1
 +  window.status += " defer ";
I think I probably would have used some other logging mechanism. :)

LayoutTests/http/tests/loading/script-defer-expected.txt:6
 +  main frame - didFinishLoadForFrame
I'm not sure you want to have this in the loading directory, I don't think you
need/want these callbacks.  Maybe you do, but it makes the tests possibly more
complicated than needed.

LayoutTests/http/tests/loading/script-defer.html:8
 +    window.status += (' DCL ')
function log(text) {
   document.getElementById("preTag").innerHTML += text + "\n";
}
is the easy hack for logging.  It's also just as easy to create text nodes and
br nodes in a normal div, like the "console" div and log() function for
script-tests do.

LayoutTests/http/tests/loading/script-defer.html:17
 +	var results = "PASS";
You can actually use the shouldBe, log, etc. from script-tests w/o being a
template-based script-test by including fast/js/js-test-pre.js.  I think that's
exposed in some manner to http tests, but I could be wrong.

Honestly all the window.status stuff is fine. :)  It was just a little strange
at first to read.

WebCore/html/HTML5DocumentParser.cpp:306
 +	m_endWasDelayed = !m_scriptRunner->executeScriptsWaitingForParsing();
This seems very strange.  You mean we'd end up calling end() twice?  Since we
delayed the end inside end?

WebCore/html/HTML5DocumentParser.cpp:306
 +	m_endWasDelayed = !m_scriptRunner->executeScriptsWaitingForParsing();
Maybe we should have a wrapper function for calling end instead? 
runAnyDeferredScriptsAndEnd() or similar?

WebCore/html/HTML5ScriptRunner.cpp:233
 +		watchForLoad(m_parsingBlockingScript);
Feels very strange to me to pull this watchForLoad call out from right next to
where we make the request.  Seems like it would be easy to end up watching
twice or not at all.  If we fail to "watch" a script, it will end up with no
clients and can end up purging its data from the cache any time we hit the
run-loop.

WebCore/html/HTML5ScriptRunner.cpp:241
 +  void HTML5ScriptRunner::requestScript(PendingScript& pendingScript,
Element* scriptElement)
I think this is a great change.  It is definitely better to pass in the
m_parsingBlockingScript as a reference than to use it directly!

WebCore/html/HTML5ScriptRunner.cpp:279
 +	m_scriptsToExecuteAfterParsing.append(pendingScript);
I think the lack of watchForLoad is going to bite us here.  I think instead the
load function needs to know to ignore the notification.  That will be slightly
sad as we may have to remove more ASSERTS (which are the only thing which makes
this sort of async programming possible).

WebCore/html/HTML5ScriptRunner.cpp:294
 +		requestDeferredScript(script);
This is nice and clean. :)

WebCore/html/HTML5ScriptRunner.h:114
 +	Deque<PendingScript> m_scriptsToExecuteAfterParsing;
I think this design seems totally workable.  I wonder if we want the
ScriptRunner to be in charge of running defer scripts, or should something else
like the Document do so (since it happens after parsing is done?)  How are
async scripts going to wire in?  Do those need to be run by the ScriptRunner or
Document or something else?

We may need to split the scripting logic out of Document ont something else. 
Maybe the ScriptRunner should be owned by the document directly and just used
by the parser?	Or maybe a new ScriptRunner-type-class needs to exist for the
Documetns' script running needs?

Those big design questions don't need to be answered by this patch, but should
enter into your thinking in terms of planning async and defer. :)

r- because I think this will end up with defer scripts being purged from the
cache before being run (thus causing flakiness and ASSERTs in debug mode).