Refactor code around ScriptLoader::FetchScript() according to the spec

third_party/WebKit/Source/core/dom/ScriptLoader.cpp

Index: third_party/WebKit/Source/core/dom/ScriptLoader.cpp
diff --git a/third_party/WebKit/Source/core/dom/ScriptLoader.cpp b/third_party/WebKit/Source/core/dom/ScriptLoader.cpp
index 77cc040f3400f0f5af8bc9f49e42ae0b41fe73e9..316051e3523c299f550f5ec3ded2c80df77478b4 100644
--- a/third_party/WebKit/Source/core/dom/ScriptLoader.cpp
+++ b/third_party/WebKit/Source/core/dom/ScriptLoader.cpp
@@ -42,9 +42,9 @@
 #include "core/frame/LocalFrame.h"
 #include "core/frame/SubresourceIntegrity.h"
 #include "core/frame/csp/ContentSecurityPolicy.h"
-#include "core/html/CrossOriginAttribute.h"
 #include "core/html/imports/HTMLImport.h"
 #include "core/html/parser/HTMLParserIdioms.h"
+#include "core/loader/resource/ScriptResource.h"
 #include "platform/WebFrameScheduler.h"
 #include "platform/loader/fetch/AccessControlStatus.h"
 #include "platform/loader/fetch/FetchParameters.h"
@@ -267,32 +267,108 @@ bool ScriptLoader::PrepareScript(const TextPosition& script_start_position,
   if (!IsScriptForEventSupported())
     return false;
 
-  // 14. "If the script element has a charset attribute,

[hiroshige, 2017/04/14 00:13:29]

Step 14 is moved below.

-  //      then let encoding be the result of
-  //      getting an encoding from the value of the charset attribute."
-  //     "If the script element does not have a charset attribute,
-  //      or if getting an encoding failed, let encoding
-  //      be the same as the encoding of the script element's node document."
-  // TODO(hiroshige): Should we handle failure in getting an encoding?
-  String encoding;
-  if (!element_->CharsetAttributeValue().IsEmpty())
-    encoding = element_->CharsetAttributeValue();
-  else
-    encoding = element_document.characterSet();
-
-  // Steps 15--20 are handled in fetchScript().
+  // 14. is handled below.
+
+  // 15. "Let CORS setting be the current state of the element's

[hiroshige, 2017/04/14 00:13:29]

Step 15 is moved from FetchScript().

+  //      crossorigin content attribute."
+  CrossOriginAttributeValue cross_origin =
+      GetCrossOriginAttributeValue(element_->CrossOriginAttributeValue());
+
+  // 16. will be handled below once module script support is implemented.
+
+  // 17. "If the script element has a nonce attribute,

[hiroshige, 2017/04/14 00:13:29]

Step 17 is moved from FetchScript().

+  //      then let cryptographic nonce be that attribute's value.
+  //      Otherwise, let cryptographic nonce be the empty string."
+  String nonce;
+  if (element_->IsNonceableElement())
+    nonce = element_->nonce();
+
+  // 18. is handled below.
+
+  // 19. "Let parser state be "parser-inserted"

[hiroshige, 2017/04/14 00:13:29]

Step 19 is moved from FetchScript().

+  //      if the script element has been flagged as "parser-inserted",
+  //      and "not parser-inserted" otherwise."
+  ParserDisposition parser_state =
+      IsParserInserted() ? kParserInserted : kNotParserInserted;
 
   // 21. "If the element has a src content attribute, run these substeps:"
   if (element_->HasSourceAttribute()) {
-    FetchParameters::DeferOption defer = FetchParameters::kNoDefer;
-    if (!parser_inserted_ || element_->AsyncAttributeValue() ||
-        element_->DeferAttributeValue())
-      defer = FetchParameters::kLazyLoad;
-    if (document_write_intervention_ ==
-        DocumentWriteIntervention::kFetchDocWrittenScriptDeferIdle)
-      defer = FetchParameters::kIdleLoad;
-    if (!FetchScript(element_->SourceAttributeValue(), encoding, defer))
+    // 21.1. Let src be the value of the element's src attribute.
+    String src =
+        StripLeadingAndTrailingHTMLSpaces(element_->SourceAttributeValue());

[hiroshige, 2017/04/14 00:13:29]

Moved from Line 489.

+
+    // 21.2. "If src is the empty string, queue a task to

[hiroshige, 2017/04/14 00:13:29]

Moved from Line 579.

+    //        fire an event named error at the element, and abort these steps."
+    if (src.IsEmpty()) {
+      // TODO(hiroshige): Make this asynchronous. Currently we fire the error
+      // event synchronously to keep the existing behavior.
+      DispatchErrorEvent();
+      return false;
+    }
+
+    // 21.3. "Set the element's from an external file flag."

[hiroshige, 2017/04/14 00:13:29]

21.3 is moved from FetchScript().
Actually in this CL |is_external_script_| is set a little earlier (i.e. before
ScriptResource::Fetch()), but I hope this doesn't cause a problem.

+    is_external_script_ = true;
+
+    // 21.4. "Parse src relative to the element's node document."
+    KURL url = element_document.CompleteURL(src);

[hiroshige, 2017/04/14 00:13:29]

This might cause slight behavior change: previously, we call CompleteURL() with
string before StripLeadingAndTrailingHTMLSpaces(). In this CL, we call
CompleteURL() after stripping. Therefore |url| might be different if the src
attribute contains leading/trailing whitespaces. I'll revert this change if this
might be a problem and leave a TODO.

According to the test result,
http/tests/local/absolute-url-strip-whitespace.html is turned from FAIL to PASS,
so probably this causes a good behavior change -- but anyway I'll split the
behavior change into a separate CL.

+
+    // 21.5. "If the previous step failed, queue a task to

[hiroshige, 2017/04/14 00:13:29]

Moved from Line 579.

+    //        fire an event named error at the element, and abort these steps."
+    if (!url.IsValid()) {
+      // TODO(hiroshige): Make this asynchronous. Currently we fire the error
+      // event synchronously to keep the existing behavior.
+      DispatchErrorEvent();
+      return false;
+    }
+
+    DCHECK(!resource_);
+
+    // 21.6. "Switch on the script's type:"
+    // - "classic":
+
+    // 14. "If the script element has a charset attribute,

[hiroshige, 2017/04/14 00:13:29]

Step 14 is moved from above.

+    //      then let encoding be the result of
+    //      getting an encoding from the value of the charset attribute."
+    //     "If the script element does not have a charset attribute,
+    //      or if getting an encoding failed, let encoding
+    //      be the same as the encoding of the script element's node
+    //      document."
+    // TODO(hiroshige): Should we handle failure in getting an encoding?
+    String encoding;
+    if (!element_->CharsetAttributeValue().IsEmpty())
+      encoding = element_->CharsetAttributeValue();
+    else
+      encoding = element_document.characterSet();
+
+    // Step 16 is skipped because "module script credentials" is not used
+    // for classic scripts.
+
+    // 18. "If the script element has an integrity attribute,

[hiroshige, 2017/04/14 00:13:29]

Step 18 is moved from FetchScript().

+    //      then let integrity metadata be that attribute's value.
+    //      Otherwise, let integrity metadata be the empty string."
+    String integrity_attr = element_->IntegrityAttributeValue();
+    IntegrityMetadataSet integrity_metadata;
+    if (!integrity_attr.IsEmpty()) {
+      SubresourceIntegrity::ParseIntegrityAttribute(
+          integrity_attr, integrity_metadata, &element_document);
+    }
+
+    if (!FetchClassicScript(url, element_document.Fetcher(), nonce,
+                            integrity_metadata, parser_state, cross_origin,
+                            element_document.GetSecurityOrigin(), encoding))
       return false;

[hiroshige, 2017/04/14 00:13:29]

Moved from Line 579.
Oh, I should call DispatchErrorEvent() here (fixed in Patch Set 5, but no test
failure...so not covered by tests!)

+
+    // - "module":
+    // TODO(hiroshige): Implement this.
+
+    // "When the chosen algorithm asynchronously completes, set
+    //  the script's script to the result. At that time, the script is ready."
+    // When the script is ready, PendingScriptClient::pendingScriptFinished()
+    // is used as the notification, and the action to take when
+    // the script is ready is specified later, in
+    // - ScriptLoader::PrepareScript(), or
+    // - HTMLParserScriptRunner,
+    // depending on the conditions in Step 23 of "prepare a script".
   }
 
   // 22. "If the element does not have a src content attribute,
@@ -476,108 +552,62 @@ bool ScriptLoader::PrepareScript(const TextPosition& script_start_position,
   return true;
 }
 
-// Steps 15--21 of https://html.spec.whatwg.org/#prepare-a-script
-bool ScriptLoader::FetchScript(const String& source_url,
-                               const String& encoding,
-                               FetchParameters::DeferOption defer) {
-  Document* element_document = &(element_->GetDocument());
-  if (!element_->IsConnected() || element_->GetDocument() != element_document)
-    return false;
-
-  DCHECK(!resource_);
-  // 21. "If the element has a src content attribute, run these substeps:"
-  if (!StripLeadingAndTrailingHTMLSpaces(source_url).IsEmpty()) {
-    // 21.4. "Parse src relative to the element's node document."
-    ResourceRequest resource_request(element_document->CompleteURL(source_url));
-
-    // [Intervention]
-    if (document_write_intervention_ ==
-        DocumentWriteIntervention::kFetchDocWrittenScriptDeferIdle) {
-      resource_request.SetHTTPHeaderField(
-          "Intervention",
-          "<https://www.chromestatus.com/feature/5718547946799104>");
-    }
-
-    FetchParameters params(resource_request, element_->InitiatorName());
+bool ScriptLoader::FetchClassicScript(
+    const KURL& url,
+    ResourceFetcher* fetcher,
+    const String& nonce,
+    const IntegrityMetadataSet& integrity_metadata,
+    ParserDisposition parser_state,
+    CrossOriginAttributeValue cross_origin,
+    SecurityOrigin* security_origin,
+    const String& encoding) {
+  // https://html.spec.whatwg.org/#prepare-a-script
+  // 21.6, "classic":
+  // "Fetch a classic script given url, settings, ..."
+  ResourceRequest resource_request(url);
 
-    // 15. "Let CORS setting be the current state of the element's

[hiroshige, 2017/04/14 00:13:29]

Lines 503--544 (Steps 15, 17, 18, 19) are moved to PrepareScript(), except for
params.setFoo() calls.

-    //      crossorigin content attribute."
-    CrossOriginAttributeValue cross_origin =
-        GetCrossOriginAttributeValue(element_->CrossOriginAttributeValue());
-
-    // 16. "Let module script credentials mode be determined by switching
-    //      on CORS setting:"
-    // TODO(hiroshige): Implement this step for "module".
-
-    // 21.6, "classic": "Fetch a classic script given ... CORS setting
-    //                   ... and encoding."
-    if (cross_origin != kCrossOriginAttributeNotSet) {
-      params.SetCrossOriginAccessControl(element_document->GetSecurityOrigin(),
-                                         cross_origin);
-    }
-
-    params.SetCharset(encoding);
-
-    // 17. "If the script element has a nonce attribute,
-    //      then let cryptographic nonce be that attribute's value.
-    //      Otherwise, let cryptographic nonce be the empty string."
-    if (element_->IsNonceableElement())
-      params.SetContentSecurityPolicyNonce(element_->nonce());
+  // [Intervention]
+  if (document_write_intervention_ ==
+      DocumentWriteIntervention::kFetchDocWrittenScriptDeferIdle) {
+    resource_request.SetHTTPHeaderField(
+        "Intervention",
+        "<https://www.chromestatus.com/feature/5718547946799104>");
+  }
 
-    // 19. "Let parser state be "parser-inserted"
-    //      if the script element has been flagged as "parser-inserted",
-    //      and "not parser-inserted" otherwise."
-    params.SetParserDisposition(IsParserInserted() ? kParserInserted
-                                                   : kNotParserInserted);
+  FetchParameters params(resource_request, element_->InitiatorName());
 
-    params.SetDefer(defer);
+  // "... cryptographic nonce, ..."
+  params.SetContentSecurityPolicyNonce(nonce);
 
-    // 18. "If the script element has an integrity attribute,
-    //      then let integrity metadata be that attribute's value.
-    //      Otherwise, let integrity metadata be the empty string."
-    String integrity_attr = element_->IntegrityAttributeValue();
-    if (!integrity_attr.IsEmpty()) {
-      IntegrityMetadataSet metadata_set;
-      SubresourceIntegrity::ParseIntegrityAttribute(
-          integrity_attr, metadata_set, element_document);
-      params.SetIntegrityMetadata(metadata_set);
-    }
+  // "... integrity metadata, ..."
+  params.SetIntegrityMetadata(integrity_metadata);
 
-    // 21.6. "Switch on the script's type:"
+  // "... parser state, ..."
+  params.SetParserDisposition(parser_state);
 
-    // - "classic":
-    //   "Fetch a classic script given url, settings, cryptographic nonce,
-    //    integrity metadata, parser state, CORS setting, and encoding."
-    resource_ = ScriptResource::Fetch(params, element_document->Fetcher());
+  // "... CORS setting, ..."
+  if (cross_origin != kCrossOriginAttributeNotSet) {
+    params.SetCrossOriginAccessControl(security_origin, cross_origin);
+  }
 
-    // - "module":
-    //   "Fetch a module script graph given url, settings, "script",
-    //    cryptographic nonce, parser state, and
-    //    module script credentials mode."
-    // TODO(kouhei, hiroshige): Implement this.
+  // "... and encoding."
+  params.SetCharset(encoding);
 
-    // "When the chosen algorithm asynchronously completes, set
-    //  the script's script to the result. At that time, the script is ready."
-    // When the script is ready, PendingScriptClient::pendingScriptFinished()
-    // is used as the notification, and the action to take when
-    // the script is ready is specified later, in
-    // - ScriptLoader::prepareScript(), or
-    // - HTMLParserScriptRunner,
-    // depending on the conditions in Step 23 of "prepare a script".
+  // This DeferOption logic is only for classic scripts, as we always set

[hiroshige, 2017/04/14 00:13:29]

This logic is moved from PrepareScript() to clarify that this is only for
classic script (and because this is not spec'ed).

+  // |kLazyLoad| for module scripts in ModuleScriptLoader.
+  FetchParameters::DeferOption defer = FetchParameters::kNoDefer;
+  if (!parser_inserted_ || element_->AsyncAttributeValue() ||
+      element_->DeferAttributeValue())
+    defer = FetchParameters::kLazyLoad;
+  if (document_write_intervention_ ==
+      DocumentWriteIntervention::kFetchDocWrittenScriptDeferIdle)
+    defer = FetchParameters::kIdleLoad;
+  params.SetDefer(defer);
 
-    // 21.3. "Set the element's from an external file flag."
-    is_external_script_ = true;

[hiroshige, 2017/04/14 00:13:29]

Step 21.3 is moved to FetchScript().

-  }
+  resource_ = ScriptResource::Fetch(params, fetcher);
 
-  if (!resource_) {
-    // 21.2. "If src is the empty string, queue a task to
-    //        fire an event named error at the element, and abort these steps."
-    // 21.5. "If the previous step failed, queue a task to
-    //        fire an event named error at the element, and abort these steps."
-    // TODO(hiroshige): Make this asynchronous.
-    DispatchErrorEvent();

[hiroshige, 2017/04/14 00:13:29]

This clause handles three different cases, and are moved into separate blocks in
this CL to make the code more directly corresponds to the spec:
- 21.2 -> Line 300,
- 21.5 -> Line 315,
- fetch failures due to other reasons (I don't know the details of the cause) ->
Line 359.

+  if (!resource_)
     return false;
-  }
 
   // [Intervention]
   if (created_during_document_write_ &&