Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(981)

Issue 2099853002: Don't preload scripts with invalid type/language attributes (Closed)

Created:
4 years, 6 months ago by Charlie Harrison
Modified:
4 years, 5 months ago
Reviewers:
Yoav Weiss
CC:
blink-reviews, blink-reviews-dom_chromium.org, blink-reviews-html_chromium.org, chromium-reviews, dglazkov+blink, eae+blinkwatch, kinuko+watch, rwlbuis, sof, Yoav Weiss
Base URL:
https://chromium.googlesource.com/chromium/src.git@master
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

Don't preload scripts with invalid type/language attributes This patch keeps track of a script tag's type and language attributes, and gates preloading if they are invalid. This aligns the preload scanner's policy with the ScriptLoader's policy. BUG=623109 Committed: https://crrev.com/23f4df7587f338837a709afb192900102584fc0a Cr-Commit-Position: refs/heads/master@{#403182}

Patch Set 1 #

Total comments: 6

Patch Set 2 : add unit tests and fix threading issue #

Patch Set 3 : case insensitive matching #

Patch Set 4 : other nits etc. #

Total comments: 2

Patch Set 5 : isLegacySupportedJavascriptLanguage requires lowercase input #

Unified diffs Side-by-side diffs Delta from patch set Stats (+69 lines, -33 lines) Patch
M third_party/WebKit/Source/core/dom/ScriptLoader.h View 1 chunk +2 lines, -0 lines 0 comments Download
M third_party/WebKit/Source/core/dom/ScriptLoader.cpp View 1 2 3 4 4 chunks +29 lines, -32 lines 0 comments Download
M third_party/WebKit/Source/core/html/parser/HTMLPreloadScanner.cpp View 1 5 chunks +9 lines, -1 line 0 comments Download
M third_party/WebKit/Source/core/html/parser/HTMLPreloadScannerTest.cpp View 1 3 1 chunk +29 lines, -0 lines 0 comments Download

Messages

Total messages: 17 (6 generated)
Charlie Harrison
Yoav, what do you think about this?
4 years, 6 months ago (2016-06-24 22:10:51 UTC) #2
Yoav Weiss
Thanks for working on this. We should definitely align the behavior with that of ScriptLoader. ...
4 years, 6 months ago (2016-06-25 08:08:16 UTC) #3
Charlie Harrison
Thanks for taking a look. Test failures were the result of a non thread safe ...
4 years, 5 months ago (2016-06-27 19:44:10 UTC) #4
commit-bot: I haz the power
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2099853002/60001
4 years, 5 months ago (2016-06-29 02:11:13 UTC) #6
commit-bot: I haz the power
Dry run: This issue passed the CQ dry run.
4 years, 5 months ago (2016-06-29 03:30:29 UTC) #8
Yoav Weiss
LGTM % perf nit https://codereview.chromium.org/2099853002/diff/60001/third_party/WebKit/Source/core/dom/ScriptLoader.cpp File third_party/WebKit/Source/core/dom/ScriptLoader.cpp (right): https://codereview.chromium.org/2099853002/diff/60001/third_party/WebKit/Source/core/dom/ScriptLoader.cpp#newcode167 third_party/WebKit/Source/core/dom/ScriptLoader.cpp:167: || MIMETypeRegistry::isSupportedJavaScriptMIMEType("text/" + language.lower()) Since ...
4 years, 5 months ago (2016-06-30 09:37:51 UTC) #9
Charlie Harrison
Thanks for the review! https://codereview.chromium.org/2099853002/diff/60001/third_party/WebKit/Source/core/dom/ScriptLoader.cpp File third_party/WebKit/Source/core/dom/ScriptLoader.cpp (right): https://codereview.chromium.org/2099853002/diff/60001/third_party/WebKit/Source/core/dom/ScriptLoader.cpp#newcode167 third_party/WebKit/Source/core/dom/ScriptLoader.cpp:167: || MIMETypeRegistry::isSupportedJavaScriptMIMEType("text/" + language.lower()) On ...
4 years, 5 months ago (2016-06-30 12:47:12 UTC) #11
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2099853002/80001
4 years, 5 months ago (2016-06-30 12:47:28 UTC) #13
commit-bot: I haz the power
Committed patchset #5 (id:80001)
4 years, 5 months ago (2016-06-30 15:31:06 UTC) #14
commit-bot: I haz the power
Patchset 5 (id:??) landed as https://crrev.com/23f4df7587f338837a709afb192900102584fc0a Cr-Commit-Position: refs/heads/master@{#403182}
4 years, 5 months ago (2016-06-30 15:33:21 UTC) #16
Charlie Harrison
4 years, 5 months ago (2016-07-07 12:08:07 UTC) #17
Message was sent while issue was closed.
A revert of this CL (patchset #5 id:80001) has been created in
https://codereview.chromium.org/2130773002/ by csharrison@chromium.org.

The reason for reverting is: Some compat risk for this change was brought up.
Going to send a note to blink-dev before re-landing..

Powered by Google App Engine
This is Rietveld 408576698