|
|
Created:
3 years, 9 months ago by kinuko Modified:
3 years, 8 months ago CC:
chromium-reviews, Yoav Weiss, tyoshino+watch_chromium.org, loading-reviews_chromium.org, gavinp+loader_chromium.org, blink-reviews, loading-reviews+fetch_chromium.org, kinuko+watch, Nate Chapin, hiroshige Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionMove FrameFetchContext::dispatchDidLoadResourceFromMemoryCache logic into ResourceFetcher
Motivation: want to make FrameFetchContext thinner that only has the
code that hides Frame abstraction
BUG=695279
Review-Url: https://codereview.chromium.org/2751143003
Cr-Commit-Position: refs/heads/master@{#460676}
Committed: https://chromium.googlesource.com/chromium/src/+/918293741a54580488930933602b2d4984378b97
Patch Set 1 #Patch Set 2 : . #Patch Set 3 : . #Patch Set 4 : . #
Total comments: 10
Patch Set 5 : addressed comments #Messages
Total messages: 46 (35 generated)
The CQ bit was checked by kinuko@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_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
Description was changed from ========== Move FrameFetchContext::dispatchDidLoadResourceFromMemoryCache logic into ResourceFetcher Motivation: want to make FrameFetchContext thinner that only has the code that hides Frame abstraction BUG= ========== to ========== Move FrameFetchContext::dispatchDidLoadResourceFromMemoryCache logic into ResourceFetcher Motivation: want to make FrameFetchContext thinner that only has the code that hides Frame abstraction BUG=695279 ==========
The CQ bit was checked by kinuko@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_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 kinuko@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 kinuko@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 kinuko@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.
kinuko@chromium.org changed reviewers: + japhet@chromium.org, yhirano@chromium.org
kinuko@chromium.org changed reviewers: + toyoshim@chromium.org
What do you think? Code moving only, similar change to crrev.com/2747203002/ https://codereview.chromium.org/2751143003/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/loader/FrameFetchContext.cpp (right): https://codereview.chromium.org/2751143003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/loader/FrameFetchContext.cpp:468: Code below is basically same as what dispatchDidResponseInternal was doing https://codereview.chromium.org/2751143003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/loader/FrameFetchContext.cpp:556: resourceResponse); Moved most of the code in this method out of FetchContext into ResourceFetcher
Awesome! https://codereview.chromium.org/2751143003/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/loader/FrameFetchContext.cpp (right): https://codereview.chromium.org/2751143003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/loader/FrameFetchContext.cpp:464: probe::markResourceAsCached(frame(), identifier); Is it important to do this after dispatchDidLoadResourceFromMemoryCache() and dispatchWillSendRequest()? If not, can we leave it in dispatchDidLoadResourceFromMemoryCache()? https://codereview.chromium.org/2751143003/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/loader/fetch/FetchContext.h (right): https://codereview.chromium.org/2751143003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/loader/fetch/FetchContext.h:59: enum class ResourceLoadStartType { kNotFromMemoryCache, kFromMemoryCache }; Nit: it's not really "LoadStart" for the kNotFromMemoryCache case at response time. Maybe we could have a ResourceResponseType enum just for FetchContext::dispatchDidReceiveResponse() and just use the ResourceFetcher::RevalidationPolicy for ResourceFetcher::requestLoadStarted()? I think the only reason we had a special enum for requestLoadStarted() instead of using the RevalidationPolicy was that requestLoadStarted() used to be public and called from outside ResourceFetcher. But it's private now, so I don't think there's a good reason not to just use RevalidationPolicy. https://codereview.chromium.org/2751143003/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/loader/fetch/ResourceFetcher.cpp (right): https://codereview.chromium.org/2751143003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/loader/fetch/ResourceFetcher.cpp:335: const FetchRequest& request) { Nit: take const ResourceRequest& instead of const FetchRequest&, since this function doesn't use anything else from the FetchRequest. https://codereview.chromium.org/2751143003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/loader/fetch/ResourceFetcher.cpp:337: identifier, request.resourceRequest(), resource->response()); We're sending a different ResoruceRequest for dispatchDidLoadResourceFromMemoryCache() and dispatchWillSendRequest(). Can we send the same one? I don't think it matters at the moment, but it might cause confusion later.
lgtm if Nate's comments are addressed.
The CQ bit was checked by kinuko@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...
Thanks! Updated. https://codereview.chromium.org/2751143003/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/loader/FrameFetchContext.cpp (right): https://codereview.chromium.org/2751143003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/loader/FrameFetchContext.cpp:464: probe::markResourceAsCached(frame(), identifier); On 2017/03/22 19:02:02, Nate Chapin wrote: > Is it important to do this after dispatchDidLoadResourceFromMemoryCache() and > dispatchWillSendRequest()? If not, can we leave it in > dispatchDidLoadResourceFromMemoryCache()? I tried it in my initial patch, but if we reverse the order inspector's Network.requestServedFromCache notification is not dispatched at all and it breaks multiple tests. It looks willSendRequest needs to precede because this probe method only takes identifier and inspector can't find associated request otherwise. (Needing to have this line here feels a bit unfortunate though. I added a comment to note the ordering issue) https://codereview.chromium.org/2751143003/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/loader/fetch/FetchContext.h (right): https://codereview.chromium.org/2751143003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/loader/fetch/FetchContext.h:59: enum class ResourceLoadStartType { kNotFromMemoryCache, kFromMemoryCache }; On 2017/03/22 19:02:02, Nate Chapin wrote: > Nit: it's not really "LoadStart" for the kNotFromMemoryCache case at response > time. Maybe we could have a ResourceResponseType enum just for > FetchContext::dispatchDidReceiveResponse() and just use the > ResourceFetcher::RevalidationPolicy for ResourceFetcher::requestLoadStarted()? > > I think the only reason we had a special enum for requestLoadStarted() instead > of using the RevalidationPolicy was that requestLoadStarted() used to be public > and called from outside ResourceFetcher. But it's private now, so I don't think > there's a good reason not to just use RevalidationPolicy. Good point, done. https://codereview.chromium.org/2751143003/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/loader/fetch/ResourceFetcher.cpp (right): https://codereview.chromium.org/2751143003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/loader/fetch/ResourceFetcher.cpp:335: const FetchRequest& request) { On 2017/03/22 19:02:02, Nate Chapin wrote: > Nit: take const ResourceRequest& instead of const FetchRequest&, since this > function doesn't use anything else from the FetchRequest. Done. https://codereview.chromium.org/2751143003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/loader/fetch/ResourceFetcher.cpp:337: identifier, request.resourceRequest(), resource->response()); On 2017/03/22 19:02:02, Nate Chapin wrote: > We're sending a different ResoruceRequest for > dispatchDidLoadResourceFromMemoryCache() and dispatchWillSendRequest(). Can we > send the same one? I don't think it matters at the moment, but it might cause > confusion later. Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
LGTM, sorry for the delay.
The CQ bit was checked by kinuko@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from yhirano@chromium.org Link to the patchset: https://codereview.chromium.org/2751143003/#ps80001 (title: "addressed comments")
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_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 kinuko@chromium.org
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_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 kinuko@chromium.org
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": 80001, "attempt_start_ts": 1490848762091240, "parent_rev": "1a21f83e0deacdee7ec43cf1f2b93e47d51438b3", "commit_rev": "918293741a54580488930933602b2d4984378b97"}
Message was sent while issue was closed.
Description was changed from ========== Move FrameFetchContext::dispatchDidLoadResourceFromMemoryCache logic into ResourceFetcher Motivation: want to make FrameFetchContext thinner that only has the code that hides Frame abstraction BUG=695279 ========== to ========== Move FrameFetchContext::dispatchDidLoadResourceFromMemoryCache logic into ResourceFetcher Motivation: want to make FrameFetchContext thinner that only has the code that hides Frame abstraction BUG=695279 Review-Url: https://codereview.chromium.org/2751143003 Cr-Commit-Position: refs/heads/master@{#460676} Committed: https://chromium.googlesource.com/chromium/src/+/918293741a54580488930933602b... ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001) as https://chromium.googlesource.com/chromium/src/+/918293741a54580488930933602b... |