Stream scripts to V8 as they load - Blink side.

Source/core/dom/PendingScript.cpp

Index: Source/core/dom/PendingScript.cpp
diff --git a/Source/core/dom/PendingScript.cpp b/Source/core/dom/PendingScript.cpp
index 58b27c2ce913bfdbdfbb7deaea6564e817a692be..c2fb4afbcac358565bb14619660c338336b5e55f 100644
--- a/Source/core/dom/PendingScript.cpp
+++ b/Source/core/dom/PendingScript.cpp
@@ -26,6 +26,7 @@
 #include "config.h"
 #include "core/dom/PendingScript.h"
 
+#include "bindings/core/v8/ScriptStreamer.h"
 #include "core/dom/Element.h"
 #include "core/fetch/ScriptResource.h"
 
@@ -35,23 +36,33 @@ PendingScript::~PendingScript()
 {
 }
 
-void PendingScript::watchForLoad(ResourceClient* client)
+void PendingScript::watchForLoad(ScriptResourceClient* client)
 {
     ASSERT(!m_watchingForLoad);
     ASSERT(!resource()->isLoaded());
-    // addClient() will call notifyFinished() if the load is complete. Callers
-    // do not expect to be re-entered from this call, so they should not become
-    // a client of an already-loaded Resource.
-    resource()->addClient(client);
+    if (m_streamer) {
+        m_streamer->addClient(client);
+    } else {
+        // addClient() will call notifyFinished() if the load is
+        // complete. Callers do not expect to be re-entered from this call, so
+        // they should not become a client of an already-loaded Resource.
+        resource()->addClient(client);
+    }
     m_watchingForLoad = true;
 }
 
-void PendingScript::stopWatchingForLoad(ResourceClient* client)
+void PendingScript::stopWatchingForLoad(ScriptResourceClient* client)
 {
-    if (resource() && m_watchingForLoad) {
+    if (!m_watchingForLoad) {
+        return;
+    }
+    if (m_streamer) {
+        m_streamer->cancel();
+        m_streamer->removeClient(client);
+    } else if (resource()) {

[haraken, 2014/09/10 05:57:52]

How is it possible that resource() returns 0 here?

I guess this branch should be:

  else {
    ASSERT(resource());
  }

[marja, 2014/09/11 09:15:37]

This code was originally in HTMLScriptRunner like this:

if (m_parserBlockingScript.resource() &&
m_parserBlockingScript.watchingForLoad())


and in that case, it's clear that it can be that resource() is NULL. However, I
think that check was unnecessarily complicated, and 

if (m_parserBlockingScript.watchingForLoad())

would've been enough.

Anyhow, added the ASSERT here as you suggested.

         resource()->removeClient(client);
-        m_watchingForLoad = false;
     }
+    m_watchingForLoad = false;
 }
 
 PassRefPtrWillBeRawPtr<Element> PendingScript::releaseElementAndClear()
@@ -65,10 +76,25 @@ PassRefPtrWillBeRawPtr<Element> PendingScript::releaseElementAndClear()
 void PendingScript::setScriptResource(ScriptResource* resource)
 {
     setResource(resource);
+    // This function is only called 1) during construction (we're not yet
+    // streaming) 2) after loading has completed (and streaming too, if we were
+    // streaming).
+    ASSERT(!m_streamer || !m_streamer->streamingInProgress());

[haraken, 2014/09/10 05:57:52]

ASSERT(!isStreaming());

[haraken, 2014/09/10 05:57:52]

Add ASSERT(!m_watchingForLoad);

[marja, 2014/09/11 09:15:37]

Done.

[marja, 2014/09/11 09:15:37]

Done.

[marja, 2014/09/16 11:20:38]

Actually, this is not true in the current code (independent of script
streaming).

The code path where this is not true:

#3 blink::PendingScript::setScriptResource()
#4 blink::PendingScript::PendingScript()
#5 WTF::Deque<>::takeFirst()
#6 blink::HTMLScriptRunner::executeScriptsWaitingForParsing()
#7 blink::HTMLDocumentParser::attemptToRunDeferredScriptsAndEnd()
#8 blink::HTMLDocumentParser::notifyScriptLoaded()

... in general, it's not guaranteed that HTMLScriptRunner calls
stopWatchingForLoad when it has already been notified of the load having
finished.

+    m_streamer.release();
 }
 
 void PendingScript::notifyFinished(Resource*)
 {
+    if (m_streamer) {
+        m_streamer->notifyFinished();
+    }
+}
+
+void PendingScript::notifyAppendData(ScriptResource*)
+{
+    if (m_streamer) {
+        m_streamer->notifyAppendData();
+    }
 }
 
 void PendingScript::trace(Visitor* visitor)
@@ -81,10 +107,18 @@ ScriptSourceCode PendingScript::getSource(const KURL& documentURL, bool& errorOc
     if (resource()) {
         errorOccurred = resource()->errorOccurred();
         ASSERT(resource()->isLoaded());
+        if (m_streamer && !m_streamer->streamingSuppressed()) {

[haraken, 2014/09/10 05:57:51]

Help me understand: Why do we need this branch? Can we just always return
ScriptSourceCode(resource()) ?

I think I just don't understand how the suppressing works.

[marja, 2014/09/11 09:15:37]

We need to somehow take the results of streaming into use when compiling, so
this is why we cannot do ScriptSourceCode(resource()) - that would just discard
everything we have done, and we'd end up parsing and compiling the script like
before. We need ScriptSourceCode reflect the fact that we have parsed the code
already and the only thing left is compilation.

Suppressing means that ScriptStreamer is still watching the resource load, just
not giving data to V8. So in those cases, we get the source code via the normal,
non-streaming code path. And from that point on, we just forget that we ever
streamed, and we parse and compile the code like before.

+            return ScriptSourceCode(m_streamer, resource());
+        }
         return ScriptSourceCode(resource());
     }
     errorOccurred = false;
     return ScriptSourceCode(m_element->textContent(), documentURL, startingPosition());
 }
 
+bool PendingScript::isStreaming() const
+{
+    return m_streamer && m_streamer->streamingInProgress();
+}
+
 }