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

Issue 2542183002: Don't remove CSSPreloaderResourceClient for unused speculative markup preloads (Closed)

Created:
4 years ago by Charlie Harrison
Modified:
4 years ago
CC:
chromium-reviews, blink-reviews, loading-reviews+fetch_chromium.org, tyoshino+watch_chromium.org, Nate Chapin, gavinp+loader_chromium.org
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

Don't remove CSSPreloaderResourceClient for unused speculative markup preloads The CSS @import scanner attaches a passive resource client to a css preload request. This passive client should not affect the policy decisions of the preload and should just observe notifications passively. This patch fixes a bug where removing a passive client from an otherwise unused preload ends up cancelling it, which removes the preload from memory cache. This is very wrong behavior, and causes the optimization to be less effective, and report bad metrics. Simply not removing the client will not cause the resource to live longer than necessary, because the client holds only weak references to the resource. BUG=670295, 662999 Committed: https://crrev.com/dff0384e2ab5ade040a08a38e72eece575ea0aee Cr-Commit-Position: refs/heads/master@{#436312}

Patch Set 1 #

Patch Set 2 : allow dynamic link preload removal #

Total comments: 4

Patch Set 3 : yhirano review #

Unified diffs Side-by-side diffs Delta from patch set Stats (+12 lines, -2 lines) Patch
M third_party/WebKit/LayoutTests/TestExpectations View 1 chunk +0 lines, -2 lines 0 comments Download
M third_party/WebKit/Source/core/html/parser/CSSPreloadScanner.cpp View 1 2 1 chunk +12 lines, -0 lines 0 comments Download

Messages

Total messages: 35 (20 generated)
Charlie Harrison
PTAL!
4 years ago (2016-12-01 19:34:22 UTC) #2
Charlie Harrison
Hm hold off, a couple layout tests failures look related, sorry.
4 years ago (2016-12-01 21:49:12 UTC) #7
Charlie Harrison
Bots are happy after I special cased link preloads. PTAL?
4 years ago (2016-12-02 00:49:00 UTC) #12
yhirano
Thank you for working on this issue. https://codereview.chromium.org/2542183002/diff/20001/third_party/WebKit/Source/core/fetch/Resource.cpp File third_party/WebKit/Source/core/fetch/Resource.cpp (right): https://codereview.chromium.org/2542183002/diff/20001/third_party/WebKit/Source/core/fetch/Resource.cpp#newcode762 third_party/WebKit/Source/core/fetch/Resource.cpp:762: if (isUnusedPreload() ...
4 years ago (2016-12-02 01:32:25 UTC) #13
Charlie Harrison
https://codereview.chromium.org/2542183002/diff/20001/third_party/WebKit/Source/core/fetch/Resource.cpp File third_party/WebKit/Source/core/fetch/Resource.cpp (right): https://codereview.chromium.org/2542183002/diff/20001/third_party/WebKit/Source/core/fetch/Resource.cpp#newcode762 third_party/WebKit/Source/core/fetch/Resource.cpp:762: if (isUnusedPreload() && !isLinkPreload()) On 2016/12/02 01:32:25, yhirano wrote: ...
4 years ago (2016-12-02 04:20:22 UTC) #14
yhirano
+hiroshige@ fyi https://codereview.chromium.org/2542183002/diff/20001/third_party/WebKit/Source/core/fetch/Resource.cpp File third_party/WebKit/Source/core/fetch/Resource.cpp (right): https://codereview.chromium.org/2542183002/diff/20001/third_party/WebKit/Source/core/fetch/Resource.cpp#newcode762 third_party/WebKit/Source/core/fetch/Resource.cpp:762: if (isUnusedPreload() && !isLinkPreload()) On 2016/12/02 04:20:22, ...
4 years ago (2016-12-02 08:50:29 UTC) #17
Charlie Harrison
https://codereview.chromium.org/2542183002/diff/20001/third_party/WebKit/Source/core/fetch/Resource.cpp File third_party/WebKit/Source/core/fetch/Resource.cpp (right): https://codereview.chromium.org/2542183002/diff/20001/third_party/WebKit/Source/core/fetch/Resource.cpp#newcode762 third_party/WebKit/Source/core/fetch/Resource.cpp:762: if (isUnusedPreload() && !isLinkPreload()) On 2016/12/02 08:50:29, yhirano wrote: ...
4 years ago (2016-12-02 13:14:16 UTC) #18
Charlie Harrison
Alright, the change you requested is running through the bots now. I *think* it should ...
4 years ago (2016-12-02 15:58:13 UTC) #21
yhirano
On 2016/12/02 15:58:13, Charlie Harrison wrote: > Alright, the change you requested is running through ...
4 years ago (2016-12-05 02:22:41 UTC) #24
Charlie Harrison
Thanks! Yoav, I'll wait for your stamp to land, as this is related to the ...
4 years ago (2016-12-05 04:03:26 UTC) #25
Yoav Weiss
On 2016/12/05 04:03:26, Charlie Harrison wrote: > Thanks! Yoav, I'll wait for your stamp to ...
4 years ago (2016-12-05 14:38:38 UTC) #27
Charlie Harrison
Thanks! Let me push to CQ.
4 years ago (2016-12-05 14:41:14 UTC) #28
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/2542183002/40001
4 years ago (2016-12-05 14:41:34 UTC) #30
commit-bot: I haz the power
Committed patchset #3 (id:40001)
4 years ago (2016-12-05 16:25:16 UTC) #33
commit-bot: I haz the power
4 years ago (2016-12-05 16:28:56 UTC) #35
Message was sent while issue was closed.
Patchset 3 (id:??) landed as
https://crrev.com/dff0384e2ab5ade040a08a38e72eece575ea0aee
Cr-Commit-Position: refs/heads/master@{#436312}

Powered by Google App Engine
This is Rietveld 408576698