|
|
Created:
3 years, 8 months ago by yhirano Modified:
3 years, 7 months ago CC:
chromium-reviews, Yoav Weiss, gavinp+prerender_chromium.org, tyoshino+watch_chromium.org, loading-reviews_chromium.org, gavinp+loader_chromium.org, blink-reviews, loading-reviews+fetch_chromium.org, kinuko+watch, Nate Chapin Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionSeparate preaload matching from MemoryCache
This change decouples preload matching from MemoryCache lookup. With this change,
preload matching is done for ResourceFetcher::prealods_.
This is the first step and part of the second step in the design document[1].
1: https://docs.google.com/document/d/1oq8ixPnaDxuAlKUTRQ3WoYHlJenVkzNU9xnkuZX_dWM/edit#
BUG=695939
Review-Url: https://codereview.chromium.org/2834733003
Cr-Commit-Position: refs/heads/master@{#469046}
Committed: https://chromium.googlesource.com/chromium/src/+/bb924e1897f5b4c67325dbfbdf2441e7f0686d53
Patch Set 1 : fix #
Total comments: 2
Patch Set 2 : rebase #Patch Set 3 : rebase #
Total comments: 8
Patch Set 4 : rebase #Patch Set 5 : fix #Patch Set 6 : fix #Patch Set 7 : fix #
Total comments: 2
Patch Set 8 : fix #Messages
Total messages: 107 (98 generated)
The CQ bit was checked by yhirano@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 yhirano@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 yhirano@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...) cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...) chromeos_amd64-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...) chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...) linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by yhirano@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: cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...) linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by yhirano@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 yhirano@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 yhirano@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: 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 yhirano@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: win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
The CQ bit was checked by yhirano@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: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by yhirano@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 yhirano@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: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by yhirano@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: 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 yhirano@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 yhirano@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 yhirano@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...
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
Patchset #1 (id:40001) has been deleted
Patchset #1 (id:60001) has been deleted
Patchset #1 (id:80001) has been deleted
Patchset #1 (id:100001) has been deleted
Patchset #1 (id:120001) has been deleted
Patchset #1 (id:140001) has been deleted
Patchset #1 (id:160001) has been deleted
Patchset #1 (id:180001) has been deleted
Patchset #1 (id:200001) has been deleted
Patchset #1 (id:220001) has been deleted
Patchset #1 (id:240001) has been deleted
The CQ bit was checked by yhirano@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...
Patchset #2 (id:280001) has been deleted
Description was changed from ========== [WIP] Do preload matching separately from MemoryCache matching BUG=695939 ========== to ========== Separate preaload matching from MemoryCache This change decouples preload matching from MemoryCache lookup. With this change, preload matching is done for ResourceFetcher::prealods_. This is the first step and part of the second step in the design document[1]. 1: https://docs.google.com/document/d/1oq8ixPnaDxuAlKUTRQ3WoYHlJenVkzNU9xnkuZX_d... BUG=695939 ==========
yhirano@chromium.org changed reviewers: + japhet@chromium.org, yoav@yoav.ws
yhirano@chromium.org changed reviewers: - japhet@chromium.org, yoav@yoav.ws
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Patchset #1 (id:260001) has been deleted
https://codereview.chromium.org/2834733003/diff/300001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/http/tests/preload/multiple-meta-csp.html (left): https://codereview.chromium.org/2834733003/diff/300001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/http/tests/preload/multiple-meta-csp.html:21: assert_true(internals.isPreloaded('../resources/testharnessreport.js')); In this test, speculative preloads are made AFTER actual requests are made (regardless of the change). Without this change, the speculative preloads are merged to the actual requets and tracked as preloaded resources. With this change, the speculative preloads are merged to the actual requets, and they are not treated as preloaded resources. https://codereview.chromium.org/2834733003/diff/300001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/http/tests/resources/srcset-helper.js (right): https://codereview.chromium.org/2834733003/diff/300001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/http/tests/resources/srcset-helper.js:26: internals.evictAllResources(); Without this, preloading requets match with MemoryCache entries, and they are not treated as preloaded resources.
yhirano@chromium.org changed reviewers: + japhet@chromium.org, yoav@yoav.ws
+japhet@, yoav@
The CQ bit was checked by yhirano@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: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by yhirano@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.
On 2017/04/26 01:31:10, yhirano wrote: > +japhet@, yoav@ ping
Sorry for the delay... https://codereview.chromium.org/2834733003/diff/340001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/loader/fetch/PreloadKey.h (right): https://codereview.chromium.org/2834733003/diff/340001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/loader/fetch/PreloadKey.h:44: static KURL FragmentRemovedUrl(const KURL& src) { RemoveFragmentFromUrl() would be a little clearer. https://codereview.chromium.org/2834733003/diff/340001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/loader/fetch/Resource.cpp (right): https://codereview.chromium.org/2834733003/diff/340001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/loader/fetch/Resource.cpp:425: loader_->Fetcher()->RemovePreload(this); I think this can move to ResourceFetcher::HandleLoaderError(), which would remove the need to reference ResourceFetcher from Resource. https://codereview.chromium.org/2834733003/diff/340001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/loader/fetch/ResourceFetcher.cpp (right): https://codereview.chromium.org/2834733003/diff/340001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/loader/fetch/ResourceFetcher.cpp:691: InsertAsPreloadIfNecessary(resource, params, factory.GetType()); It's unfortunate to need to do this in two places :/ https://codereview.chromium.org/2834733003/diff/340001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/loader/fetch/ResourceFetcher.cpp:877: if ((params.IsSpeculativePreload() || params.IsLinkPreload()) && This if() statement would probably be easier to ready as two early-exit if()s instead.
The CQ bit was checked by yhirano@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 yhirano@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: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by yhirano@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 yhirano@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.
The CQ bit was checked by yhirano@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...
https://codereview.chromium.org/2834733003/diff/340001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/loader/fetch/PreloadKey.h (right): https://codereview.chromium.org/2834733003/diff/340001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/loader/fetch/PreloadKey.h:44: static KURL FragmentRemovedUrl(const KURL& src) { On 2017/05/02 00:02:16, Nate Chapin wrote: > RemoveFragmentFromUrl() would be a little clearer. Done. https://codereview.chromium.org/2834733003/diff/340001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/loader/fetch/Resource.cpp (right): https://codereview.chromium.org/2834733003/diff/340001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/loader/fetch/Resource.cpp:425: loader_->Fetcher()->RemovePreload(this); On 2017/05/02 00:02:16, Nate Chapin wrote: > I think this can move to ResourceFetcher::HandleLoaderError(), which would > remove the need to reference ResourceFetcher from Resource. Done. https://codereview.chromium.org/2834733003/diff/340001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/loader/fetch/ResourceFetcher.cpp (right): https://codereview.chromium.org/2834733003/diff/340001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/loader/fetch/ResourceFetcher.cpp:691: InsertAsPreloadIfNecessary(resource, params, factory.GetType()); On 2017/05/02 00:02:16, Nate Chapin wrote: > It's unfortunate to need to do this in two places :/ Acknowledged. https://codereview.chromium.org/2834733003/diff/340001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/loader/fetch/ResourceFetcher.cpp:877: if ((params.IsSpeculativePreload() || params.IsLinkPreload()) && On 2017/05/02 00:02:16, Nate Chapin wrote: > This if() statement would probably be easier to ready as two early-exit if()s > instead. Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
LGTM https://codereview.chromium.org/2834733003/diff/420001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/loader/fetch/ResourceFetcher.cpp (right): https://codereview.chromium.org/2834733003/diff/420001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/loader/fetch/ResourceFetcher.cpp:876: if (!(params.IsSpeculativePreload() || params.IsLinkPreload())) Nit: I prefer to avoid paren usage in if() statements, so I'd lean toward: if (!params.IsSpeculativePreload() && !params.IsLinkPreload()) ...but I won't fight it if you strongly prefer this.
The CQ bit was checked by yhirano@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...
https://codereview.chromium.org/2834733003/diff/420001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/loader/fetch/ResourceFetcher.cpp (right): https://codereview.chromium.org/2834733003/diff/420001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/loader/fetch/ResourceFetcher.cpp:876: if (!(params.IsSpeculativePreload() || params.IsLinkPreload())) On 2017/05/02 23:06:31, Nate Chapin wrote: > Nit: I prefer to avoid paren usage in if() statements, so I'd lean toward: > if (!params.IsSpeculativePreload() && !params.IsLinkPreload()) > > ...but I won't fight it if you strongly prefer this. Done.
The CQ bit was unchecked by yhirano@chromium.org
The CQ bit was checked by yhirano@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from japhet@chromium.org Link to the patchset: https://codereview.chromium.org/2834733003/#ps440001 (title: "fix")
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": 440001, "attempt_start_ts": 1493829269121260, "parent_rev": "47f9431d5a69ae9e81faa6472d8b5a188eb63242", "commit_rev": "bb924e1897f5b4c67325dbfbdf2441e7f0686d53"}
Message was sent while issue was closed.
Description was changed from ========== Separate preaload matching from MemoryCache This change decouples preload matching from MemoryCache lookup. With this change, preload matching is done for ResourceFetcher::prealods_. This is the first step and part of the second step in the design document[1]. 1: https://docs.google.com/document/d/1oq8ixPnaDxuAlKUTRQ3WoYHlJenVkzNU9xnkuZX_d... BUG=695939 ========== to ========== Separate preaload matching from MemoryCache This change decouples preload matching from MemoryCache lookup. With this change, preload matching is done for ResourceFetcher::prealods_. This is the first step and part of the second step in the design document[1]. 1: https://docs.google.com/document/d/1oq8ixPnaDxuAlKUTRQ3WoYHlJenVkzNU9xnkuZX_d... BUG=695939 Review-Url: https://codereview.chromium.org/2834733003 Cr-Commit-Position: refs/heads/master@{#469046} Committed: https://chromium.googlesource.com/chromium/src/+/bb924e1897f5b4c67325dbfbdf24... ==========
Message was sent while issue was closed.
Committed patchset #8 (id:440001) as https://chromium.googlesource.com/chromium/src/+/bb924e1897f5b4c67325dbfbdf24... |