Refactor Script(Loader|Runner): don't access Resources all over the place...

Source/core/dom/ScriptLoader.cpp

 /*
  * Copyright (C) 1999 Lars Knoll (knoll@kde.org)
  *           (C) 1999 Antti Koivisto (koivisto@kde.org)
  *           (C) 2001 Dirk Mueller (mueller@kde.org)
  * Copyright (C) 2003, 2004, 2005, 2006, 2007, 2008 Apple Inc. All rights reserved.
  * Copyright (C) 2008 Nikolas Zimmermann <zimmermann@kde.org>
  *
  * This library is free software; you can redistribute it and/or
  * modify it under the terms of the GNU Library General Public
  * License as published by the Free Software Foundation; either
@@ skipping 57 matching lines @@
     , m_forceAsync(!parserInserted)
     , m_willExecuteInOrder(false)
 {
     ASSERT(m_element);
     if (parserInserted && element->document().scriptableDocumentParser() && !element->document().isInDocumentWrite())
         m_startLineNumber = element->document().scriptableDocumentParser()->lineNumber();
 }
 
 ScriptLoader::~ScriptLoader()
 {
-    stopLoadRequest();
+    m_pendingScript.stopWatchingForLoad(this);

[haraken, 2014/10/15 15:09:41]

We might want to avoid calling stopWatchingForLoad twice. It's possible that
notifyFinished called stopWatchingForLoad before.

[marja, 2014/10/16 15:19:03]

No, stopWatchingForLoad handles it gracefully - which is really handy here
because it's tedious to track it otherwise. The hack which used to be in
ScriptLoader::stopLoadRequest() ("we're watching if the Resource is non-null and
if !m_willBeParserExecuted) is horrible.

We'd basically end up tracking the exact same thing ("are we watching or not")
which PendingScript already does internally.

 }
 
 void ScriptLoader::didNotifySubtreeInsertionsToDocument()
 {
     if (!m_parserInserted)
         prepareScript(); // FIXME: Provide a real starting line number here.
 }
 
 void ScriptLoader::childrenChanged()
 {
@@ skipping 139 matching lines @@
     if (client->hasSourceAttribute() && client->deferAttributeValue() && m_parserInserted && !client->asyncAttributeValue()) {
         m_willExecuteWhenDocumentFinishedParsing = true;
         m_willBeParserExecuted = true;
     } else if (client->hasSourceAttribute() && m_parserInserted && !client->asyncAttributeValue()) {
         m_willBeParserExecuted = true;
     } else if (!client->hasSourceAttribute() && m_parserInserted && !elementDocument.isRenderingReady()) {
         m_willBeParserExecuted = true;
         m_readyToBeParserExecuted = true;
     } else if (client->hasSourceAttribute() && !client->asyncAttributeValue() && !m_forceAsync) {
         m_willExecuteInOrder = true;
-        contextDocument->scriptRunner()->queueScriptForExecution(this, m_resource, ScriptRunner::IN_ORDER_EXECUTION);
-        m_resource->addClient(this);
+        m_pendingScript = PendingScript(m_element, m_resource.get());
+        contextDocument->scriptRunner()->queueScriptForExecution(this, ScriptRunner::IN_ORDER_EXECUTION);
+        // Note that watchForLoad can immediately call notifyFinished.
+        m_pendingScript.watchForLoad(this);
     } else if (client->hasSourceAttribute()) {
-        contextDocument->scriptRunner()->queueScriptForExecution(this, m_resource, ScriptRunner::ASYNC_EXECUTION);
-        m_resource->addClient(this);
+        m_pendingScript = PendingScript(m_element, m_resource.get());
+        contextDocument->scriptRunner()->queueScriptForExecution(this, ScriptRunner::ASYNC_EXECUTION);
+        // Note that watchForLoad can immediately call notifyFinished.
+        m_pendingScript.watchForLoad(this);
     } else {
         // Reset line numbering for nested writes.
         TextPosition position = elementDocument.isInDocumentWrite() ? TextPosition() : scriptStartPosition;
         KURL scriptURL = (!elementDocument.isInDocumentWrite() && m_parserInserted) ? elementDocument.url() : KURL();
         executeScript(ScriptSourceCode(scriptContent(), scriptURL, position));
     }
 
     return true;
 }
 
@@ skipping 96 matching lines @@
     // block's source and the script block's type.
     // Note: This is where the script is compiled and actually executed.
     frame->script().executeScriptInMainWorld(sourceCode, corsCheck, compilationFinishTime);
 
     if (isHTMLScriptLoader(m_element)) {
         ASSERT(contextDocument->currentScript() == m_element);
         contextDocument->popCurrentScript();
     }
 }
 
-void ScriptLoader::stopLoadRequest()
-{
-    if (m_resource) {
-        if (!m_willBeParserExecuted)
-            m_resource->removeClient(this);
-        m_resource = 0;
-    }
-}
-
-void ScriptLoader::execute(ScriptResource* resource)
+void ScriptLoader::execute()
 {
     ASSERT(!m_willBeParserExecuted);
-    ASSERT(resource);
-    if (resource->errorOccurred()) {
+    ASSERT(m_pendingScript.resource());
+    bool errorOccurred = false;
+    ScriptSourceCode source = m_pendingScript.getSource(KURL(), errorOccurred);
+    RefPtr<Element> element = m_pendingScript.releaseElementAndClear();
+    if (errorOccurred) {
         dispatchErrorEvent();
-    } else if (!resource->wasCanceled()) {
-        executeScript(ScriptSourceCode(resource));
+    } else if (!m_resource->wasCanceled()) {
+        executeScript(source);
         dispatchLoadEvent();
     }
-    resource->removeClient(this);
+    m_resource = 0;

[haraken, 2014/10/15 15:09:41]

Just help me understand: Don't we need to call
m_pendingScript.stopWatchingForLoad(this) here?

[marja, 2014/10/16 15:19:03]

No, because it's already called in execute().

 }
 
 void ScriptLoader::notifyFinished(Resource* resource)
 {
     ASSERT(!m_willBeParserExecuted);
 
     RefPtrWillBeRawPtr<Document> elementDocument(m_element->document());
     RefPtrWillBeRawPtr<Document> contextDocument = elementDocument->contextDocument().get();
     if (!contextDocument)
         return;
 
-    // Resource possibly invokes this notifyFinished() more than
-    // once because ScriptLoader doesn't unsubscribe itself from
-    // Resource here and does it in execute() instead.
-    // We use m_resource to check if this function is already called.
     ASSERT_UNUSED(resource, resource == m_resource);
-    if (!m_resource)
-        return;
+

[haraken, 2014/10/15 15:09:41]

Shall we add ASSERT(m_resource)?

[marja, 2014/10/16 15:19:03]

It's a bit unnecessary since we already ASSERT resource == m_resource and this
is not called with NULL...

     if (m_resource->errorOccurred()) {
         dispatchErrorEvent();
         contextDocument->scriptRunner()->notifyScriptLoadError(this, m_willExecuteInOrder ? ScriptRunner::IN_ORDER_EXECUTION : ScriptRunner::ASYNC_EXECUTION);
         return;
     }
     if (m_willExecuteInOrder)
         contextDocument->scriptRunner()->notifyScriptReady(this, ScriptRunner::IN_ORDER_EXECUTION);
     else
         contextDocument->scriptRunner()->notifyScriptReady(this, ScriptRunner::ASYNC_EXECUTION);
 
-    m_resource = 0;
+    m_pendingScript.stopWatchingForLoad(this);
 }
 
 bool ScriptLoader::ignoresLoadRequest() const
 {
     return m_alreadyStarted || m_isExternalScript || m_parserInserted || !element() || !element()->inDocument();
 }
 
 bool ScriptLoader::isScriptForEventSupported() const
 {
     String eventAttribute = client()->eventAttributeValue();
@@ skipping 32 matching lines @@
     if (isHTMLScriptLoader(element))
         return toHTMLScriptElement(element)->loader();
 
     if (isSVGScriptLoader(element))
         return toSVGScriptElement(element)->loader();
 
     return 0;
 }
 
 }