Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2821553002:
Refactor code around ScriptLoader::FetchScript() according to the spec (Closed)
DescriptionRefactor code around ScriptLoader::FetchScript() according to the spec
This CL renames FetchScript() into FetchClassicScript(), and moves most of
spec'ed logic to PrepareScript().
This makes the code structure to correspond to the spec more directly, e.g.
- FetchClassicScript() now directly corresponds to "fetch a classic script"
https://html.spec.whatwg.org/#fetch-a-classic-script
and works just as a bridge between PrepareScript() (dominated by HTML
spec) and the Blink loading interface.
- Empty/invalid URLs are handled explicitly, independently from script
type, and in the steps that directly correspond to the spec, while
previously they are handled as the cases where ScriptResource::Fetch()
returns null.
This also makes it easier to implement module script supports.
This CL shouldn't change the behavior.
BUG=686281
Review-Url: https://codereview.chromium.org/2821553002
Cr-Commit-Position: refs/heads/master@{#465485}
Committed: https://chromium.googlesource.com/chromium/src/+/b8d842d9a8a6d837901ce9ef4629c6ebdaef5694
Patch Set 1 #Patch Set 2 : refine #Patch Set 3 : refine #Patch Set 4 : refine #
Total comments: 16
Patch Set 5 : fix FetchClassicScript() == false case #Patch Set 6 : DCHECK #Patch Set 7 : Revert behavior change #Patch Set 8 : Rebase #Patch Set 9 : Rebase #Patch Set 10 : Rebase #Patch Set 11 : Rebase #
Messages
Total messages: 41 (35 generated)
The CQ bit was checked by hiroshige@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by hiroshige@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Move DeferOption logic from PrepareScript() to FetchScript() BUG= ========== to ========== Refactor code around ScriptLoader::FetchScript() according to the spec This is also preparation for introducing a module-counterpart of FetchClassicScript(). BUG= ==========
The CQ bit was checked by hiroshige@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by hiroshige@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Refactor code around ScriptLoader::FetchScript() according to the spec This is also preparation for introducing a module-counterpart of FetchClassicScript(). BUG= ========== to ========== Refactor code around ScriptLoader::FetchScript() according to the spec This CL renames FetchScript() into FetchClassicScript(), and moves most of spec'ed logic to PrepareScript(). This makes the code structure to correspond to the spec more directly, e.g. - FetchClassicScript() now directly corresponds to "fetch a classic script" https://html.spec.whatwg.org/#fetch-a-classic-script and works just as a bridge between PrepareScript() (dominated by HTML spec) and the Blink loading interface. - Empty/invalid URLs are handled explicitly, independently from script type, and in the steps that directly correspond to the spec, while previously they are handled as the cases where ScriptResource::Fetch() returns null. This also makes it easier to implement module script supports. BUG=686281 ==========
hiroshige@chromium.org changed reviewers: + kouhei@chromium.org
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)
hiroshige@chromium.org changed reviewers: + japhet@chromium.org, sigbjornf@opera.com
PTAL. https://codereview.chromium.org/2821553002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/dom/ScriptLoader.cpp (left): https://codereview.chromium.org/2821553002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/dom/ScriptLoader.cpp:270: // 14. "If the script element has a charset attribute, Step 14 is moved below. https://codereview.chromium.org/2821553002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/dom/ScriptLoader.cpp:503: // 15. "Let CORS setting be the current state of the element's Lines 503--544 (Steps 15, 17, 18, 19) are moved to PrepareScript(), except for params.setFoo() calls. https://codereview.chromium.org/2821553002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/dom/ScriptLoader.cpp:569: is_external_script_ = true; Step 21.3 is moved to FetchScript(). https://codereview.chromium.org/2821553002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/dom/ScriptLoader.cpp:578: DispatchErrorEvent(); 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. https://codereview.chromium.org/2821553002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/dom/ScriptLoader.cpp (right): https://codereview.chromium.org/2821553002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/dom/ScriptLoader.cpp:272: // 15. "Let CORS setting be the current state of the element's Step 15 is moved from FetchScript(). https://codereview.chromium.org/2821553002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/dom/ScriptLoader.cpp:279: // 17. "If the script element has a nonce attribute, Step 17 is moved from FetchScript(). https://codereview.chromium.org/2821553002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/dom/ScriptLoader.cpp:288: // 19. "Let parser state be "parser-inserted" Step 19 is moved from FetchScript(). https://codereview.chromium.org/2821553002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/dom/ScriptLoader.cpp:298: StripLeadingAndTrailingHTMLSpaces(element_->SourceAttributeValue()); Moved from Line 489. https://codereview.chromium.org/2821553002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/dom/ScriptLoader.cpp:300: // 21.2. "If src is the empty string, queue a task to Moved from Line 579. https://codereview.chromium.org/2821553002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/dom/ScriptLoader.cpp:309: // 21.3. "Set the element's from an external file flag." 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. https://codereview.chromium.org/2821553002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/dom/ScriptLoader.cpp:313: KURL url = element_document.CompleteURL(src); 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. https://codereview.chromium.org/2821553002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/dom/ScriptLoader.cpp:315: // 21.5. "If the previous step failed, queue a task to Moved from Line 579. https://codereview.chromium.org/2821553002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/dom/ScriptLoader.cpp:329: // 14. "If the script element has a charset attribute, Step 14 is moved from above. https://codereview.chromium.org/2821553002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/dom/ScriptLoader.cpp:346: // 18. "If the script element has an integrity attribute, Step 18 is moved from FetchScript(). https://codereview.chromium.org/2821553002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/dom/ScriptLoader.cpp:359: return false; Moved from Line 579. Oh, I should call DispatchErrorEvent() here (fixed in Patch Set 5, but no test failure...so not covered by tests!) https://codereview.chromium.org/2821553002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/dom/ScriptLoader.cpp:596: // This DeferOption logic is only for classic scripts, as we always set This logic is moved from PrepareScript() to clarify that this is only for classic script (and because this is not spec'ed).
The CQ bit was checked by hiroshige@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...) android_cronet on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_cron...) android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...) cast_shell_android on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...) linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...) win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was checked by hiroshige@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm
The CQ bit was checked by hiroshige@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Refactor code around ScriptLoader::FetchScript() according to the spec This CL renames FetchScript() into FetchClassicScript(), and moves most of spec'ed logic to PrepareScript(). This makes the code structure to correspond to the spec more directly, e.g. - FetchClassicScript() now directly corresponds to "fetch a classic script" https://html.spec.whatwg.org/#fetch-a-classic-script and works just as a bridge between PrepareScript() (dominated by HTML spec) and the Blink loading interface. - Empty/invalid URLs are handled explicitly, independently from script type, and in the steps that directly correspond to the spec, while previously they are handled as the cases where ScriptResource::Fetch() returns null. This also makes it easier to implement module script supports. BUG=686281 ========== to ========== Refactor code around ScriptLoader::FetchScript() according to the spec This CL renames FetchScript() into FetchClassicScript(), and moves most of spec'ed logic to PrepareScript(). This makes the code structure to correspond to the spec more directly, e.g. - FetchClassicScript() now directly corresponds to "fetch a classic script" https://html.spec.whatwg.org/#fetch-a-classic-script and works just as a bridge between PrepareScript() (dominated by HTML spec) and the Blink loading interface. - Empty/invalid URLs are handled explicitly, independently from script type, and in the steps that directly correspond to the spec, while previously they are handled as the cases where ScriptResource::Fetch() returns null. This also makes it easier to implement module script supports. This CL doesn't change the behavior. BUG=686281 ==========
Description was changed from ========== Refactor code around ScriptLoader::FetchScript() according to the spec This CL renames FetchScript() into FetchClassicScript(), and moves most of spec'ed logic to PrepareScript(). This makes the code structure to correspond to the spec more directly, e.g. - FetchClassicScript() now directly corresponds to "fetch a classic script" https://html.spec.whatwg.org/#fetch-a-classic-script and works just as a bridge between PrepareScript() (dominated by HTML spec) and the Blink loading interface. - Empty/invalid URLs are handled explicitly, independently from script type, and in the steps that directly correspond to the spec, while previously they are handled as the cases where ScriptResource::Fetch() returns null. This also makes it easier to implement module script supports. This CL doesn't change the behavior. BUG=686281 ========== to ========== Refactor code around ScriptLoader::FetchScript() according to the spec This CL renames FetchScript() into FetchClassicScript(), and moves most of spec'ed logic to PrepareScript(). This makes the code structure to correspond to the spec more directly, e.g. - FetchClassicScript() now directly corresponds to "fetch a classic script" https://html.spec.whatwg.org/#fetch-a-classic-script and works just as a bridge between PrepareScript() (dominated by HTML spec) and the Blink loading interface. - Empty/invalid URLs are handled explicitly, independently from script type, and in the steps that directly correspond to the spec, while previously they are handled as the cases where ScriptResource::Fetch() returns null. This also makes it easier to implement module script supports. This CL shouldn't change the behavior. BUG=686281 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by hiroshige@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from kouhei@chromium.org Link to the patchset: https://codereview.chromium.org/2821553002/#ps180001 (title: "Rebase")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
The CQ bit was checked by hiroshige@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from kouhei@chromium.org Link to the patchset: https://codereview.chromium.org/2821553002/#ps200001 (title: "Rebase")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 200001, "attempt_start_ts": 1492564700261920,
"parent_rev": "8267f48654eb20e9d6453b53f2da310953d244f1", "commit_rev":
"b8d842d9a8a6d837901ce9ef4629c6ebdaef5694"}
Message was sent while issue was closed.
Description was changed from ========== Refactor code around ScriptLoader::FetchScript() according to the spec This CL renames FetchScript() into FetchClassicScript(), and moves most of spec'ed logic to PrepareScript(). This makes the code structure to correspond to the spec more directly, e.g. - FetchClassicScript() now directly corresponds to "fetch a classic script" https://html.spec.whatwg.org/#fetch-a-classic-script and works just as a bridge between PrepareScript() (dominated by HTML spec) and the Blink loading interface. - Empty/invalid URLs are handled explicitly, independently from script type, and in the steps that directly correspond to the spec, while previously they are handled as the cases where ScriptResource::Fetch() returns null. This also makes it easier to implement module script supports. This CL shouldn't change the behavior. BUG=686281 ========== to ========== Refactor code around ScriptLoader::FetchScript() according to the spec This CL renames FetchScript() into FetchClassicScript(), and moves most of spec'ed logic to PrepareScript(). This makes the code structure to correspond to the spec more directly, e.g. - FetchClassicScript() now directly corresponds to "fetch a classic script" https://html.spec.whatwg.org/#fetch-a-classic-script and works just as a bridge between PrepareScript() (dominated by HTML spec) and the Blink loading interface. - Empty/invalid URLs are handled explicitly, independently from script type, and in the steps that directly correspond to the spec, while previously they are handled as the cases where ScriptResource::Fetch() returns null. This also makes it easier to implement module script supports. This CL shouldn't change the behavior. BUG=686281 Review-Url: https://codereview.chromium.org/2821553002 Cr-Commit-Position: refs/heads/master@{#465485} Committed: https://chromium.googlesource.com/chromium/src/+/b8d842d9a8a6d837901ce9ef4629... ==========
Message was sent while issue was closed.
Committed patchset #11 (id:200001) as https://chromium.googlesource.com/chromium/src/+/b8d842d9a8a6d837901ce9ef4629... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
