Chromium Code Reviews
Help | Chromium Project | Gerrit Changes | Sign in
(13)

Issue 2826213003: Don't lower priority for scripts inserted by doc.write (Closed)

Created:
2 years, 2 months ago by Pat Meenan
Modified:
2 years, 1 month ago
CC:
chromium-reviews, Yoav Weiss, blink-reviews-html_chromium.org, loading-reviews+parser_chromium.org, loading-reviews_chromium.org, dglazkov+blink, fuzzing_chromium.org, gavinp+loader_chromium.org, blink-reviews, loading-reviews+fetch_chromium.org, kinuko+watch, Nate Chapin, tyoshino+watch_chromium.org
Target Ref:
refs/heads/master
Project:
chromium
Visibility:
Public.

Description

Don't lower priority for scripts inserted by doc.write HTML Chunks inserted by document.write() are run through a separate instance of the preload scanner from the main document. If the main document scanner has already discovered an image then all scripts discovered by any preload scanner regardless of where they came from would be considered late-body. This behavior causes a regression where a document.write() in the head that includes multiple script tags will discover them but treat them as late-body scripts and only load them one at a time. BUG=713727, 712338 Review-Url: https://codereview.chromium.org/2826213003 Cr-Commit-Position: refs/heads/master@{#467029} Committed: https://chromium.googlesource.com/chromium/src/+/1e0d3cbbf60c67ba9856eadcb9e829b3c4bec6c7

Patch Set 1 #

Patch Set 2 : Added layout test #

Total comments: 8

Patch Set 3 : Changes from review feedback #

Patch Set 4 : Improved layout test determinism #

Patch Set 5 : Fixed duplicate URLs in Layout test #

Unified diffs Side-by-side diffs Delta from patch set Stats (+150 lines, -66 lines) Patch
M third_party/WebKit/LayoutTests/http/tests/inspector/network/resource-priority.html View 1 2 3 4 5 chunks +35 lines, -21 lines 0 comments Download
M third_party/WebKit/LayoutTests/http/tests/inspector/network/resource-priority-expected.txt View 1 2 3 4 1 chunk +22 lines, -6 lines 0 comments Download
A third_party/WebKit/LayoutTests/http/tests/inspector/network/resources/docwrite.js View 1 2 3 4 1 chunk +3 lines, -0 lines 0 comments Download
M third_party/WebKit/Source/core/html/parser/BackgroundHTMLParser.cpp View 1 2 1 chunk +2 lines, -1 line 0 comments Download
M third_party/WebKit/Source/core/html/parser/HTMLDocumentParser.h View 1 chunk +2 lines, -1 line 0 comments Download
M third_party/WebKit/Source/core/html/parser/HTMLDocumentParser.cpp View 1 2 5 chunks +15 lines, -8 lines 0 comments Download
M third_party/WebKit/Source/core/html/parser/HTMLPreloadScanner.h View 1 2 4 chunks +10 lines, -5 lines 0 comments Download
M third_party/WebKit/Source/core/html/parser/HTMLPreloadScanner.cpp View 1 2 7 chunks +16 lines, -6 lines 0 comments Download
M third_party/WebKit/Source/core/html/parser/HTMLPreloadScannerFuzzer.cpp View 1 2 1 chunk +2 lines, -1 line 0 comments Download
M third_party/WebKit/Source/core/html/parser/HTMLPreloadScannerTest.cpp View 1 2 1 chunk +2 lines, -1 line 0 comments Download
M third_party/WebKit/Source/core/html/parser/PreloadRequest.h View 3 chunks +6 lines, -1 line 0 comments Download
M third_party/WebKit/Source/core/html/parser/PreloadRequest.cpp View 1 2 1 chunk +7 lines, -1 line 0 comments Download
M third_party/WebKit/Source/platform/loader/fetch/FetchParameters.h View 1 2 3 chunks +14 lines, -4 lines 0 comments Download
M third_party/WebKit/Source/platform/loader/fetch/FetchParameters.cpp View 1 2 4 chunks +7 lines, -6 lines 0 comments Download
M third_party/WebKit/Source/platform/loader/fetch/ResourceFetcher.h View 1 2 1 chunk +2 lines, -1 line 0 comments Download
M third_party/WebKit/Source/platform/loader/fetch/ResourceFetcher.cpp View 1 2 3 chunks +5 lines, -3 lines 0 comments Download

Messages

Total messages: 37 (23 generated)
Pat Meenan
yoav@ and kinuko@ could you PTAL? This fixes an issue with preload scanning document.write contents ...
2 years, 2 months ago (2017-04-20 20:25:18 UTC) #6
Charlie Harrison
I looked over the approach and it looks good to me.
2 years, 2 months ago (2017-04-20 20:44:13 UTC) #8
Charlie Harrison
Thanks for making this change!
2 years, 2 months ago (2017-04-20 20:45:16 UTC) #9
kinuko
I see... I read through the bug description and alternatives, looks like this seems to ...
2 years, 2 months ago (2017-04-21 06:22:25 UTC) #12
kinuko
Also: can you update your priority doc once this lands? Thanks!
2 years, 2 months ago (2017-04-21 08:27:21 UTC) #13
Pat Meenan
I'll add a not to the priority doc about how document.write() is handled. I don't ...
2 years, 1 month ago (2017-04-21 16:11:02 UTC) #16
kinuko
On 2017/04/21 16:11:02, Pat Meenan wrote: > I'll add a not to the priority doc ...
2 years, 1 month ago (2017-04-24 03:59:55 UTC) #23
Pat Meenan
On 2017/04/24 03:59:55, kinuko wrote: > On 2017/04/21 16:11:02, Pat Meenan wrote: > > I'll ...
2 years, 1 month ago (2017-04-24 13:21:16 UTC) #26
Pat Meenan
On 2017/04/24 13:21:16, Pat Meenan wrote: > On 2017/04/24 03:59:55, kinuko wrote: > > On ...
2 years, 1 month ago (2017-04-24 13:28:06 UTC) #27
Pat Meenan
On 2017/04/24 13:28:06, Pat Meenan wrote: > On 2017/04/24 13:21:16, Pat Meenan wrote: > > ...
2 years, 1 month ago (2017-04-24 17:34:14 UTC) #30
kinuko
lgtm > And apparently the cleanup fixed the issues on Mac so it's green across ...
2 years, 1 month ago (2017-04-25 05:44:23 UTC) #31
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/2826213003/70001
2 years, 1 month ago (2017-04-25 13:08:29 UTC) #33
commit-bot: I haz the power
Committed patchset #5 (id:70001) as https://chromium.googlesource.com/chromium/src/+/1e0d3cbbf60c67ba9856eadcb9e829b3c4bec6c7
2 years, 1 month ago (2017-04-25 17:14:41 UTC) #36
Pat Meenan
2 years, 1 month ago (2017-04-26 13:22:35 UTC) #37
Message was sent while issue was closed.
A revert of this CL (patchset #5 id:70001) has been created in
https://codereview.chromium.org/2843013002/ by pmeenan@chromium.org.

The reason for reverting is: The layout test was broken and racy
(http://crbug.com/715528).  SHould be trivial to fix (use the same query param
for the preloaded and real image) but best to revert, fix and re-land
separately..

Powered by Google App Engine
This is Rietveld 408576698