Changeset 205652 in webkit


Ignore:
Timestamp:
Sep 8, 2016 10:49:43 AM (8 years ago)
Author:
Yusuke Suzuki
Message:

ScriptRunner should be driven by PendingScript rather than ScriptElement
​https://bugs.webkit.org/show_bug.cgi?id=161726

Reviewed by Ryosuke Niwa.

Source/WebCore:

ScriptRunner is driven by ScriptElement::notifyFinished. While ScriptRunner is driven by this,
HTMLScriptRunner does not use it. Instead, HTMLScriptRunner uses PendingScriptClient. As a result,
ScriptElement::notifyFinished is used only when the script is annotated with "defer" or "async"
while all the external script will load the LoadableScript. It is confusing.
This patch removes ScriptElement::notifyFinished and use PendingScript's observability
in ScriptRunner instead.

This patch also fixes the behavior about ignore-destructive-writes counter[1]. When dispatching
the load and error events, this ignore-destructive-writes counter should not be incremeneted by
this execution. The added tests ensure this behavior.

[1]: ​https://html.spec.whatwg.org/multipage/scripting.html#execute-the-script-block

  • dom/ScriptElement.cpp: Drop LoadableScriptClient interface.

(WebCore::ScriptElement::prepareScript): Do not use addClient. ScriptRunner use PendingScript::{setClient,clearClient} instead.
(WebCore::ScriptElement::executeScriptForScriptRunner): IgnoreDestructiveWriteCountIncrementer will be done in ScriptElement::executeScript.
So no need to do it here, that's duplicated.
(WebCore::ScriptElement::~ScriptElement): Deleted. ScriptElement does not use addClient/removeClient.
(WebCore::ScriptElement::stopLoadRequest): Deleted.
(WebCore::ScriptElement::executeScriptForHTMLScriptRunner): Deleted. executeScriptForHTMLScriptRunner and executeScriptForScriptRunner are
merged into executeScriptForRunner.
(WebCore::ScriptElement::notifyFinished): Deleted.

  • dom/ScriptElement.h:

(WebCore::ScriptElement::~ScriptElement):
(WebCore::ScriptElement::willExecuteInOrder): Used in ScriptRunner to determine whether the script is async or defer.
(WebCore::ScriptElement::willExecuteWhenDocumentFinishedParsing): Deleted.

  • dom/ScriptRunner.cpp:

(WebCore::ScriptRunner::~ScriptRunner): HashSet's iterator will return const PendingScript&.
Another option is using HashSet<RefPtr<PendingScript>>. Here, we use a little bit weired const_cast.
(WebCore::ScriptRunner::queueScriptForExecution): Use PendingScript::setClient to wait loading.
(WebCore::ScriptRunner::notifyFinished): Notify the script ready here.
(WebCore::ScriptRunner::timerFired): Use executeScriptForScriptRunner.
(WebCore::ScriptRunner::notifyScriptReady): Deleted.

  • dom/ScriptRunner.h:
  • html/parser/HTMLScriptRunner.cpp:

(WebCore::HTMLScriptRunner::executePendingScriptAndDispatchEvent): Use executeScriptForScriptRunner.

LayoutTests:

  • js/dom/document-write-in-error-event-expected.txt: Added.
  • js/dom/document-write-in-error-event.html: Added.
  • js/dom/document-write-in-load-event-expected.txt: Added.
  • js/dom/document-write-in-load-event.html: Added.
  • js/dom/script-tests/dummy.js: Added.
Location:
trunk
Files:
5 added
7 edited

Legend:

Unmodified
Added
Removed

  • trunk/LayoutTests/ChangeLog

    r205579 r205652  
     12016-09-08  Yusuke Suzuki  <utatane.tea@gmail.com>
     2
     3        ScriptRunner should be driven by PendingScript rather than ScriptElement
     4        https://bugs.webkit.org/show_bug.cgi?id=161726
     5
     6        Reviewed by Ryosuke Niwa.
     7
     8        * js/dom/document-write-in-error-event-expected.txt: Added.
     9        * js/dom/document-write-in-error-event.html: Added.
     10        * js/dom/document-write-in-load-event-expected.txt: Added.
     11        * js/dom/document-write-in-load-event.html: Added.
     12        * js/dom/script-tests/dummy.js: Added.
     13
    1142016-09-07  Simon Fraser  <simon.fraser@apple.com>
    215

  • trunk/Source/WebCore/ChangeLog

    r205650 r205652  
     12016-09-08  Yusuke Suzuki  <utatane.tea@gmail.com>
     2
     3        ScriptRunner should be driven by PendingScript rather than ScriptElement
     4        https://bugs.webkit.org/show_bug.cgi?id=161726
     5
     6        Reviewed by Ryosuke Niwa.
     7
     8        ScriptRunner is driven by ScriptElement::notifyFinished. While ScriptRunner is driven by this,
     9        HTMLScriptRunner does not use it. Instead, HTMLScriptRunner uses PendingScriptClient. As a result,
     10        ScriptElement::notifyFinished is used only when the script is annotated with "defer" or "async"
     11        while all the external script will load the LoadableScript. It is confusing.
     12        This patch removes ScriptElement::notifyFinished and use PendingScript's observability
     13        in ScriptRunner instead.
     14
     15        This patch also fixes the behavior about ignore-destructive-writes counter[1]. When dispatching
     16        the load and error events, this ignore-destructive-writes counter should not be incremeneted by
     17        this execution. The added tests ensure this behavior.
     18
     19        [1]: https://html.spec.whatwg.org/multipage/scripting.html#execute-the-script-block
     20
     21        * dom/ScriptElement.cpp: Drop LoadableScriptClient interface.
     22        (WebCore::ScriptElement::prepareScript): Do not use addClient. ScriptRunner use PendingScript::{setClient,clearClient} instead.
     23        (WebCore::ScriptElement::executeScriptForScriptRunner): IgnoreDestructiveWriteCountIncrementer will be done in ScriptElement::executeScript.
     24        So no need to do it here, that's duplicated.
     25        (WebCore::ScriptElement::~ScriptElement): Deleted. ScriptElement does not use addClient/removeClient.
     26        (WebCore::ScriptElement::stopLoadRequest): Deleted.
     27        (WebCore::ScriptElement::executeScriptForHTMLScriptRunner): Deleted. executeScriptForHTMLScriptRunner and executeScriptForScriptRunner are
     28        merged into executeScriptForRunner.
     29        (WebCore::ScriptElement::notifyFinished): Deleted.
     30        * dom/ScriptElement.h:
     31        (WebCore::ScriptElement::~ScriptElement):
     32        (WebCore::ScriptElement::willExecuteInOrder): Used in ScriptRunner to determine whether the script is async or defer.
     33        (WebCore::ScriptElement::willExecuteWhenDocumentFinishedParsing): Deleted.
     34        * dom/ScriptRunner.cpp:
     35        (WebCore::ScriptRunner::~ScriptRunner): HashSet's iterator will return `const PendingScript&`.
     36        Another option is using HashSet<RefPtr<PendingScript>>. Here, we use a little bit weired const_cast.
     37        (WebCore::ScriptRunner::queueScriptForExecution): Use PendingScript::setClient to wait loading.
     38        (WebCore::ScriptRunner::notifyFinished): Notify the script ready here.
     39        (WebCore::ScriptRunner::timerFired): Use executeScriptForScriptRunner.
     40        (WebCore::ScriptRunner::notifyScriptReady): Deleted.
     41        * dom/ScriptRunner.h:
     42        * html/parser/HTMLScriptRunner.cpp:
     43        (WebCore::HTMLScriptRunner::executePendingScriptAndDispatchEvent): Use executeScriptForScriptRunner.
     44
    1452016-09-08  Alex Christensen  <achristensen@webkit.org>
    246

  • trunk/Source/WebCore/dom/ScriptElement.cpp

    r205581 r205652  
    7676}
    7777
    78 ScriptElement::~ScriptElement()
    79 {
    80     stopLoadRequest();
    81 }
    82 
    8378bool ScriptElement::shouldCallFinishedInsertingSubtree(ContainerNode& insertionPoint)
    8479{
    … …  
    246241        m_willExecuteInOrder = true;
    247242        document.scriptRunner()->queueScriptForExecution(this, *m_loadableScript, ScriptRunner::IN_ORDER_EXECUTION);
    248         m_loadableScript->addClient(*this);
    249243    } else if (hasSourceAttribute()) {
    250244        ASSERT(m_loadableScript);
    251245        m_element.document().scriptRunner()->queueScriptForExecution(this, *m_loadableScript, ScriptRunner::ASYNC_EXECUTION);
    252         m_loadableScript->addClient(*this);
    253246    } else {
    254247        // Reset line numbering for nested writes.
    … …  
    337330}
    338331
    339 void ScriptElement::stopLoadRequest()
    340 {
    341     if (m_loadableScript) {
    342         if (!m_willBeParserExecuted)
    343             m_loadableScript->removeClient(*this);
    344         m_loadableScript = nullptr;
    345     }
    346 }
    347 
    348332void ScriptElement::executeScriptAndDispatchEvent(LoadableScript& loadableScript)
    349333{
    … …  
    359343}
    360344
    361 void ScriptElement::executeScriptForScriptRunner(LoadableScript& loadableScript)
    362 {
    363     ASSERT(!m_willBeParserExecuted);
    364     executeScriptAndDispatchEvent(loadableScript);
    365     loadableScript.removeClient(*this);
    366 }
    367 
    368 void ScriptElement::executeScriptForHTMLScriptRunner(PendingScript& pendingScript)
    369 {
    370     IgnoreDestructiveWriteCountIncrementer ignoreDestructiveWriteCountIncrementer(&m_element.document());
     345void ScriptElement::executeScriptForScriptRunner(PendingScript& pendingScript)
     346{
    371347    if (auto* loadableScript = pendingScript.loadableScript())
    372348        executeScriptAndDispatchEvent(*loadableScript);
    … …  
    378354}
    379355
    380 void ScriptElement::notifyFinished(LoadableScript&)
    381 {
    382     ASSERT(!m_willBeParserExecuted);
    383 
    384     // LoadableScript possibly invokes this notifyFinished() more than
    385     // once because ScriptElement doesn't unsubscribe itself from
    386     // LoadableScript here and does it in execute() instead.
    387     // We use m_loadableScript to check if this function is already called.
    388     if (!m_loadableScript)
    389         return;
    390 
    391     if (m_willExecuteInOrder)
    392         m_element.document().scriptRunner()->notifyScriptReady(this, ScriptRunner::IN_ORDER_EXECUTION);
    393     else
    394         m_element.document().scriptRunner()->notifyScriptReady(this, ScriptRunner::ASYNC_EXECUTION);
    395 
    396     m_loadableScript = nullptr;
    397 }
    398 
    399356bool ScriptElement::ignoresLoadRequest() const
    400357{

  • trunk/Source/WebCore/dom/ScriptElement.h

    r205581 r205652  
    3939class ScriptSourceCode;
    4040
    41 class ScriptElement : private LoadableScriptClient {
     41class ScriptElement {
    4242public:
    43     virtual ~ScriptElement();
     43    virtual ~ScriptElement() { }
    4444
    4545    Element& element() { return m_element; }
    … …  
    5353    void executeScript(const ScriptSourceCode&);
    5454
    55     void executeScriptForScriptRunner(LoadableScript&);
    56     void executeScriptForHTMLScriptRunner(PendingScript&);
     55    void executeScriptForScriptRunner(PendingScript&);
    5756
    5857    // XML parser calls these
    … …  
    6463    bool readyToBeParserExecuted() const { return m_readyToBeParserExecuted; }
    6564    bool willExecuteWhenDocumentFinishedParsing() const { return m_willExecuteWhenDocumentFinishedParsing; }
     65    bool willExecuteInOrder() const { return m_willExecuteInOrder; }
    6666    LoadableScript* loadableScript() { return m_loadableScript.get(); }
    6767
    … …  
    9393
    9494    bool requestClassicScript(const String& sourceURL);
    95     void stopLoadRequest();
    96 
    97     void notifyFinished(LoadableScript&) override;
    9895
    9996    virtual String sourceAttributeValue() const = 0;

  • trunk/Source/WebCore/dom/ScriptRunner.cpp

    r205581 r205652  
    4141ScriptRunner::~ScriptRunner()
    4242{
    43     for (size_t i = 0; i < m_scriptsToExecuteSoon.size(); ++i)
     43    for (auto& pendingScript : m_scriptsToExecuteSoon) {
     44        UNUSED_PARAM(pendingScript);
    4445        m_document.decrementLoadEventDelayCount();
    45     for (size_t i = 0; i < m_scriptsToExecuteInOrder.size(); ++i)
     46    }
     47    for (auto& pendingScript : m_scriptsToExecuteInOrder) {
     48        if (pendingScript->watchingForLoad())
     49            pendingScript->clearClient();
    4650        m_document.decrementLoadEventDelayCount();
    47     for (unsigned i = 0; i < m_pendingAsyncScripts.size(); ++i)
     51    }
     52    for (auto& pendingScript : m_pendingAsyncScripts) {
     53        if (pendingScript->watchingForLoad())
     54            const_cast<PendingScript&>(pendingScript.get()).clearClient();
    4855        m_document.decrementLoadEventDelayCount();
     56    }
    4957}
    5058
    … …  
    5866    m_document.incrementLoadEventDelayCount();
    5967
     68    Ref<PendingScript> pendingScript = PendingScript::create(element, loadableScript);
    6069    switch (executionType) {
    6170    case ASYNC_EXECUTION:
    62         m_pendingAsyncScripts.add(scriptElement, PendingScript::create(element, loadableScript));
     71        m_pendingAsyncScripts.add(pendingScript.copyRef());
    6372        break;
    6473
    6574    case IN_ORDER_EXECUTION:
    66         m_scriptsToExecuteInOrder.append(PendingScript::create(element, loadableScript));
     75        m_scriptsToExecuteInOrder.append(pendingScript.copyRef());
    6776        break;
    6877    }
     78    pendingScript->setClient(this);
    6979}
    7080
    … …  
    8090}
    8191
    82 void ScriptRunner::notifyScriptReady(ScriptElement* scriptElement, ExecutionType executionType)
     92void ScriptRunner::notifyFinished(PendingScript& pendingScript)
    8393{
    84     switch (executionType) {
    85     case ASYNC_EXECUTION:
    86         ASSERT(m_pendingAsyncScripts.contains(scriptElement));
    87         m_scriptsToExecuteSoon.append(m_pendingAsyncScripts.take(scriptElement)->ptr());
    88         break;
    89 
    90     case IN_ORDER_EXECUTION:
     94    auto* scriptElement = toScriptElementIfPossible(&pendingScript.element());
     95    ASSERT(scriptElement);
     96    if (scriptElement->willExecuteInOrder())
    9197        ASSERT(!m_scriptsToExecuteInOrder.isEmpty());
    92         break;
     98    else {
     99        ASSERT(m_pendingAsyncScripts.contains(pendingScript));
     100        m_scriptsToExecuteSoon.append(m_pendingAsyncScripts.take(pendingScript)->ptr());
    93101    }
     102    pendingScript.clearClient();
    94103    m_timer.startOneShot(0);
    95104}
    … …  
    117126        ASSERT(scriptElement);
    118127        ASSERT(script->needsLoading());
    119         scriptElement->executeScriptForScriptRunner(*script->loadableScript());
     128        scriptElement->executeScriptForScriptRunner(*script);
    120129        m_document.decrementLoadEventDelayCount();
    121130    }

  • trunk/Source/WebCore/dom/ScriptRunner.h

    r205581 r205652  
    2626#pragma once
    2727
     28#include "PendingScriptClient.h"
    2829#include "Timer.h"
    29 #include <wtf/HashMap.h>
     30#include <wtf/HashSet.h>
    3031#include <wtf/Noncopyable.h>
    3132#include <wtf/Vector.h>
    … …  
    3839class LoadableScript;
    3940
    40 class ScriptRunner {
     41class ScriptRunner : private PendingScriptClient {
    4142    WTF_MAKE_NONCOPYABLE(ScriptRunner); WTF_MAKE_FAST_ALLOCATED;
    4243public:
    … …  
    5455    void timerFired();
    5556
     57    void notifyFinished(PendingScript&) override;
     58
    5659    Document& m_document;
    5760    Vector<Ref<PendingScript>> m_scriptsToExecuteInOrder;
    5861    Vector<RefPtr<PendingScript>> m_scriptsToExecuteSoon; // http://www.whatwg.org/specs/web-apps/current-work/#set-of-scripts-that-will-execute-as-soon-as-possible
    59     HashMap<ScriptElement*, Ref<PendingScript>> m_pendingAsyncScripts;
     62    HashSet<Ref<PendingScript>> m_pendingAsyncScripts;
    6063    Timer m_timer;
    6164};

  • trunk/Source/WebCore/html/parser/HTMLScriptRunner.cpp

    r205581 r205652  
    122122    if (auto* scriptElement = toScriptElementIfPossible(&pendingScript->element())) {
    123123        NestingLevelIncrementer nestingLevelIncrementer(m_scriptNestingLevel);
    124         scriptElement->executeScriptForHTMLScriptRunner(*pendingScript);
     124        scriptElement->executeScriptForScriptRunner(*pendingScript);
    125125    }
    126126    ASSERT(!isExecutingScript());
Note: See TracChangeset for help on using the changeset viewer.