[Script Spec Annotation] Annotate HTMLParserScriptRunner

third_party/WebKit/Source/core/html/parser/HTMLParserScriptRunner.cpp

 /*
  * Copyright (C) 2010 Google, Inc. All Rights Reserved.
  *
  * Redistribution and use in source and binary forms, with or without
  * modification, are permitted provided that the following conditions
  * are met:
  * 1. Redistributions of source code must retain the above copyright
  *    notice, this list of conditions and the following disclaimer.
  * 2. Redistributions in binary form must reproduce the above copyright
  *    notice, this list of conditions and the following disclaimer in the
@@ skipping 170 matching lines @@
   // may continue to run pending scripts after the parser has
   // detached.
 }
 
 bool HTMLParserScriptRunner::isParserBlockingScriptReady() {
   if (!m_document->isScriptExecutionReady())
     return false;
   return m_parserBlockingScript->isReady();
 }
 
+// This has two callers and corresponds to different concepts in the spec:
+// - When called from executeParsingBlockingScripts(), this corresponds to some
+//   steps of the "Otherwise" Clause of 'An end tag whose tag name is "script"'
+//   https://html.spec.whatwg.org/#scriptEndTag
+// - When called from executeScriptsWaitingForParsing(), this corresponds
+//   https://html.spec.whatwg.org/#execute-the-script-block
+//   and thus currently this function does more than specced.
+// TODO(hiroshige): Make the spec and implementation consistent.
 void HTMLParserScriptRunner::executePendingScriptAndDispatchEvent(
     PendingScript* pendingScript,
     ScriptStreamer::Type pendingScriptType) {
   bool errorOccurred = false;
   ScriptSourceCode sourceCode = pendingScript->getSource(
       documentURLForScriptExecution(m_document), errorOccurred);
 
   // Stop watching loads before executeScript to prevent recursion if the script
   // reloads itself.
   // TODO(kouhei): Consider merging this w/ pendingScript->dispose() after the
   // if block.
   pendingScript->stopWatchingForLoad();
 
   if (!isExecutingScript()) {
     Microtask::performCheckpoint(V8PerIsolateData::mainThreadIsolate());
     if (pendingScriptType == ScriptStreamer::ParsingBlocking) {
       // The parser cannot be unblocked as a microtask requested another
       // resource
       if (!m_document->isScriptExecutionReady())
         return;
     }
   }
 
   TextPosition scriptStartPosition = pendingScript->startingPosition();
   double scriptParserBlockingTime =
       pendingScript->parserBlockingLoadStartTime();
+  Element* element = pendingScript->element();
+
+  // 1. "Let the script be the pending parsing-blocking script.
+  //     There is no longer a pending parsing-blocking script."
   // Clear the pending script before possible re-entrancy from executeScript()
-  Element* element = pendingScript->element();
   pendingScript->dispose();
 
   if (ScriptLoader* scriptLoader = toScriptLoaderIfPossible(element)) {
+    // 7. "Increment the parser's script nesting level by one (it should be
+    //     zero before this step, so this sets it to one)."
     HTMLParserReentryPermit::ScriptNestingLevelIncrementer
         nestingLevelIncrementer =
             m_reentryPermit->incrementScriptNestingLevel();
+
     IgnoreDestructiveWriteCountIncrementer
         ignoreDestructiveWriteCountIncrementer(m_document);
+
+    // 8. "Execute the script."
     if (errorOccurred) {
       TRACE_EVENT_WITH_FLOW1(
           "blink", "HTMLParserScriptRunner ExecuteScriptFailed", element,
           TRACE_EVENT_FLAG_FLOW_IN, "data",
           getTraceArgsForScriptElement(element, scriptStartPosition));
       scriptLoader->dispatchErrorEvent();
     } else {
       DCHECK(isExecutingScript());
       if (scriptParserBlockingTime > 0.0) {
         DocumentParserTiming::from(*m_document)
             .recordParserBlockedOnScriptLoadDuration(
                 monotonicallyIncreasingTime() - scriptParserBlockingTime,
                 scriptLoader->wasCreatedDuringDocumentWrite());
       }
       if (!doExecuteScript(element, sourceCode, scriptStartPosition)) {
         scriptLoader->dispatchErrorEvent();
       } else {
         element->dispatchEvent(Event::create(EventTypeNames::load));
       }
     }
+
+    // 9. "Decrement the parser's script nesting level by one.
+    //     If the parser's script nesting level is zero
+    //     (which it always should be at this point),
+    //     then set the parser pause flag to false."
+    // This is implemented by scope out of ScriptNestingLevelIncrementer.
   }
 
   DCHECK(!isExecutingScript());
 }
 
 void fetchBlockedDocWriteScript(Element* script,
                                 bool isParserInserted,
                                 const TextPosition& scriptStartPosition) {
   DCHECK(script);
 
@@ skipping 81 matching lines @@
   }
 
   // If the script was blocked as part of document.write intervention,
   // then send an asynchronous GET request with an interventions header.
   possiblyFetchBlockedDocWriteScript(pendingScript);
 
   m_host->notifyScriptLoaded(pendingScript);
 }
 
 // Implements the steps for 'An end tag whose tag name is "script"'
-// http://whatwg.org/html#scriptEndTag
+// https://html.spec.whatwg.org/#scriptEndTag
 // Script handling lives outside the tree builder to keep each class simple.
 void HTMLParserScriptRunner::processScriptElement(
     Element* scriptElement,
     const TextPosition& scriptStartPosition) {
   DCHECK(scriptElement);
   TRACE_EVENT1(
       "blink", "HTMLParserScriptRunner::execute", "data",
       getTraceArgsForScriptElement(scriptElement, scriptStartPosition));
   // FIXME: If scripting is disabled, always just return.
 
   bool hadPreloadScanner = m_host->hasPreloadScanner();
 
+  // Initial steps of 'An end tag whose tag name is "script"'.
   // Try to execute the script given to us.
   processScriptElementInternal(scriptElement, scriptStartPosition);
 
+  // "At this stage, if there is a pending parsing-blocking script, then:"
   if (hasParserBlockingScript()) {
+    // - "If the script nesting level is not zero:"
     if (isExecutingScript()) {
+      // "Set the parser pause flag to true, and abort the processing of any
+      // nested invocations of the tokenizer, yielding control back to the
+      // caller. (Tokenization will resume when the caller returns to the
+      // "outer" tree construction stage.)"
+      // TODO(hiroshige): set the parser pause flag to true here.
+
       // Unwind to the outermost HTMLParserScriptRunner::processScriptElement
       // before continuing parsing.
       return;
     }
 
+    // - "Otherwise":
+
     traceParserBlockingScript(m_parserBlockingScript.get(),
                               !m_document->isScriptExecutionReady());
     m_parserBlockingScript->markParserBlockingLoadStartTime();
 
     // If preload scanner got created, it is missing the source after the
     // current insertion point. Append it and scan.
     if (!hadPreloadScanner && m_host->hasPreloadScanner())
       m_host->appendCurrentInputStreamToPreloadScannerAndScan();
+
     executeParsingBlockingScripts();
   }
 }
 
 bool HTMLParserScriptRunner::hasParserBlockingScript() const {
   return !!m_parserBlockingScript->element();
 }
 
+// Implements the "Otherwise" Clause of 'An end tag whose tag name is "script"'

[dominicc (has gone to gerrit), 2017/02/16 08:41:00]

I would make this more succinct by dropping "implements"; I think that is clear
implicitly. Just mention the part of the spec and the link.

[hiroshige, 2017/02/20 23:56:40]

Done.

+// https://html.spec.whatwg.org/#scriptEndTag
 void HTMLParserScriptRunner::executeParsingBlockingScripts() {
+  // 3. "If (1) the parser's Document has a style sheet that is blocking scripts
+  //     or (2) the script's "ready to be parser-executed" flag is not set:
+  //     spin the event loop
+  //     until the parser's Document has no style sheet that is blocking scripts
+  //     and the script's "ready to be parser-executed" flag is set."
+  // These conditions correspond to isParserBlockingScriptReady() and

[dominicc (has gone to gerrit), 2017/02/16 08:41:00]

Great comment; thank you so much for doing this.

Don't be afraid of using blank lines in the comment with // alone to break
commentary from spec prose to make it clearer.

BTW, would this be clearer if the spec text did not have quotes, and the asides
were quoted with [] or something? 3. " looks a bit messy; 3. is also part of the
spec text. The indentation with quotes is not consistent cf line 246, and it
also gets a bit ugly when the quoted text contains quotes as on line 394.

[hiroshige, 2017/02/20 23:56:40]

Done.
As for formatting of spec excerpts, I'd like to keep the format mostly as-is
(because I've committed spec comments in the same style) and do the reformatting
later as a separate issue, to prevent module implementation from being blocked.

+  // if it is false, executeParsingBlockingScripts() will be called later
+  // when isParserBlockingScriptReady() becomes true:
+  // (1) from HTMLParserScriptRunner::executeScriptsWaitingForResources(), or
+  // (2) from HTMLParserScriptRunner::executeScriptsWaitingForLoad().
   while (hasParserBlockingScript() && isParserBlockingScriptReady()) {
     DCHECK(m_document);
     DCHECK(!isExecutingScript());
     DCHECK(m_document->isScriptExecutionReady());
 
+    // 6. "Let the insertion point be just before the next input character."
     InsertionPointRecord insertionPointRecord(m_host->inputStream());
+
+    // 1., 7.--9.
     executePendingScriptAndDispatchEvent(m_parserBlockingScript.get(),
                                          ScriptStreamer::ParsingBlocking);
+
+    // 10. "Let the insertion point be undefined again."
+    // Implemented as scope out of InsertionPointRecord.

[dominicc (has gone to gerrit), 2017/02/16 08:41:00]

"scope out of" is a bit awkward. Maybe say something including
"InsertionPointRecord goes out of scope" or "~InsertionPointRecord" or
something? Ditto above and below.

[hiroshige, 2017/02/20 23:56:40]

Done.

+
+    // 11. "If there is once again a pending parsing-blocking script, then
+    //      repeat these steps from step 1."
   }
 }
 
 void HTMLParserScriptRunner::executeScriptsWaitingForLoad(
     PendingScript* pendingScript) {
   TRACE_EVENT0("blink", "HTMLParserScriptRunner::executeScriptsWaitingForLoad");
   DCHECK(!isExecutingScript());
   DCHECK(hasParserBlockingScript());
   DCHECK_EQ(pendingScript, m_parserBlockingScript);
   DCHECK(m_parserBlockingScript->isReady());
   executeParsingBlockingScripts();
 }
 
 void HTMLParserScriptRunner::executeScriptsWaitingForResources() {
   TRACE_EVENT0("blink",
                "HTMLParserScriptRunner::executeScriptsWaitingForResources");
   DCHECK(m_document);
   DCHECK(!isExecutingScript());
   DCHECK(m_document->isScriptExecutionReady());
   executeParsingBlockingScripts();
 }
 
+// Step 3 of https://html.spec.whatwg.org/#the-end:
+// "If the list of scripts that will execute when the document has
+//  finished parsing is not empty, run these substeps:"
 bool HTMLParserScriptRunner::executeScriptsWaitingForParsing() {
   TRACE_EVENT0("blink",
                "HTMLParserScriptRunner::executeScriptsWaitingForParsing");
+
   while (!m_scriptsToExecuteAfterParsing.isEmpty()) {
     DCHECK(!isExecutingScript());
     DCHECK(!hasParserBlockingScript());
     DCHECK(m_scriptsToExecuteAfterParsing.first()->resource());
+
+    // 1. "Spin the event loop until the first script in the list of scripts
+    //     that will execute when the document has finished parsing
+    //     has its "ready to be parser-executed" flag set and
+    //     the parser's Document has no style sheet that is blocking scripts."
+    // TODO(hiroshige): Is the latter part checked anywhere?
     if (!m_scriptsToExecuteAfterParsing.first()->isReady()) {
       m_scriptsToExecuteAfterParsing.first()->watchForLoad(this);
       traceParserBlockingScript(m_scriptsToExecuteAfterParsing.first().get(),
                                 !m_document->isScriptExecutionReady());
       m_scriptsToExecuteAfterParsing.first()->markParserBlockingLoadStartTime();
       return false;
     }
+
+    // 3. "Remove the first script element from the list of scripts that will
+    //     execute when the document has finished parsing (i.e. shift out the
+    //     first entry in the list)."
     PendingScript* first = m_scriptsToExecuteAfterParsing.takeFirst();
+
+    // 2. "Execute the first script in the list of scripts that will execute
+    //     when the document has finished parsing."
     executePendingScriptAndDispatchEvent(first, ScriptStreamer::Deferred);
+
     // FIXME: What is this m_document check for?
     if (!m_document)
       return false;
+
+    // 4. "If the list of scripts that will execute when the document has
+    //     finished parsing is still not empty, repeat these substeps again
+    //     from substep 1."
   }
   return true;
 }
 
 // 2nd Clause, Step 23 of https://html.spec.whatwg.org/#prepare-a-script
 void HTMLParserScriptRunner::requestParsingBlockingScript(Element* element) {
   // "The element is the pending parsing-blocking script of the Document of
   //  the parser that created the element.
   //  (There can only be one such script per Document at a time.)"
   if (!requestPendingScript(m_parserBlockingScript.get(), element))
@@ skipping 51 matching lines @@
   ScriptResource* resource = toScriptLoaderIfPossible(script)->resource();
   if (!resource) {
     DVLOG(1) << "Not implemented.";  // Dispatch error event.
     return false;
   }
   pendingScript->setScriptResource(resource);
   return true;
 }
 
 // Implements the initial steps for 'An end tag whose tag name is "script"'
-// http://whatwg.org/html#scriptEndTag
+// https://html.spec.whatwg.org/#scriptEndTag
 void HTMLParserScriptRunner::processScriptElementInternal(
     Element* script,
     const TextPosition& scriptStartPosition) {
   DCHECK(m_document);
   DCHECK(!hasParserBlockingScript());
   {
     ScriptLoader* scriptLoader = toScriptLoaderIfPossible(script);
 
     // This contains both a DCHECK and a null check since we should not
     // be getting into the case of a null script element, but seem to be from
     // time to time. The assertion is left in to help find those cases and
     // is being tracked by <https://bugs.webkit.org/show_bug.cgi?id=60559>.
     DCHECK(scriptLoader);
     if (!scriptLoader)
       return;
 
     DCHECK(scriptLoader->isParserInserted());
 
     if (!isExecutingScript())
       Microtask::performCheckpoint(V8PerIsolateData::mainThreadIsolate());
 
+    // "Let the old insertion point have the same value as the current
+    //  insertion point.
+    //  Let the insertion point be just before the next input character."
     InsertionPointRecord insertionPointRecord(m_host->inputStream());
+
+    // "Increment the parser's script nesting level by one."
     HTMLParserReentryPermit::ScriptNestingLevelIncrementer
         nestingLevelIncrementer =
             m_reentryPermit->incrementScriptNestingLevel();
 
+    // "Prepare the script. This might cause some script to execute, which
+    //  might cause new characters to be inserted into the tokenizer, and
+    //  might cause the tokenizer to output more tokens, resulting in a
+    //  reentrant invocation of the parser."
     scriptLoader->prepareScript(scriptStartPosition);
 
     // A part of Step 23 of https://html.spec.whatwg.org/#prepare-a-script:
     if (!scriptLoader->willBeParserExecuted())
       return;
 
     if (scriptLoader->willExecuteWhenDocumentFinishedParsing()) {
       // 1st Clause of Step 23.
       requestDeferredScript(script);
     } else if (scriptLoader->readyToBeParserExecuted()) {
@@ skipping 15 matching lines @@
         m_parserBlockingScript->dispose();
         ScriptSourceCode sourceCode(script->textContent(),
                                     documentURLForScriptExecution(m_document),
                                     scriptStartPosition);
         doExecuteScript(script, sourceCode, scriptStartPosition);
       }
     } else {
       // 2nd Clause of Step 23.
       requestParsingBlockingScript(script);
     }
+
+    // "Decrement the parser's script nesting level by one.
+    //  If the parser's script nesting level is zero, then set the parser
+    //  pause flag to false."
+    // Implemented by scope out of ScriptNestingLevelIncrementer.
+
+    // "Let the insertion point have the value of the old insertion point."
+    // Implemented by scope out of InsertionPointRecord.
   }
 }
 
 DEFINE_TRACE(HTMLParserScriptRunner) {
   visitor->trace(m_document);
   visitor->trace(m_host);
   visitor->trace(m_parserBlockingScript);
   visitor->trace(m_scriptsToExecuteAfterParsing);
   PendingScriptClient::trace(visitor);
 }
 
 }  // namespace blink