Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1867493003:
remove FetchContext::getCachePolicy() (Closed)
|
Created:
4 years, 8 months ago by Takashi Toyoshima Modified:
3 years, 9 months ago CC:
chromium-reviews, nasko+codewatch_chromium.org, dcheng, apavlov+blink_chromium.org, kinuko+watch, cbentzel+watch_chromium.org, jam, dglazkov+blink, caseq+blink_chromium.org, darin-cc_chromium.org, gavinp+loader_chromium.org, devtools-reviews_chromium.org, loading-reviews_chromium.org, blink-reviews-api_chromium.org, mlamouri+watch-content_chromium.org, creis+watch_chromium.org, Yoav Weiss, lushnikov+blink_chromium.org, blink-reviews, loading-reviews+fetch_chromium.org, Nate Chapin, tyoshino+watch_chromium.org, mlamouri+watch-blink_chromium.org, pfeldman+blink_chromium.org, mkwst+moarreviews-renderer_chromium.org, sergeyv+blink_chromium.org, kozyatinskiy+blink_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
Descriptionremove FetchContext::getCachePolicy()
CachePolicy represents per-frame memory cache policy, and used to
determine WebCachePolicy for resourceRequestCachePolicy in
FrameFetchContext.
Since the last caller of getCachePolicy() is only in FrameFetchContext,
remove the virtual method completely, and replace it with some
anonymous functions.
BUG=602900
Review-Url: https://codereview.chromium.org/1867493003
Cr-Commit-Position: refs/heads/master@{#458708}
Committed: https://chromium.googlesource.com/chromium/src/+/fa78cf3a4331be2113c320020e10d1223127c125
Patch Set 1 #Patch Set 2 : diff from 1858533002 #Patch Set 3 : again #Patch Set 4 : update #Patch Set 5 : . #Patch Set 6 : try #Patch Set 7 : build fix #Patch Set 8 : rebase #Patch Set 9 : rebase #Patch Set 10 : plan b #Patch Set 11 : plan c #Patch Set 12 : remove intentional behavior changes from this CL #Patch Set 13 : one more #Patch Set 14 : move determineFrameWCP outof class #
Total comments: 10
Patch Set 15 : review #33 #
Total comments: 8
Patch Set 16 : review #35 #Patch Set 17 : review #43 #Patch Set 18 : git merge master #
Total comments: 6
Patch Set 19 : review #45 #
Total comments: 2
Patch Set 20 : review #51 #
Messages
Total messages: 58 (38 generated)
Description was changed from ========== Merge fetch/CachePolicy to WebCachePolicy CachePolicy is used only for memory cache policy, but we have another, WebCachePolicy for disk cache. Basically these should be mapped 1:1, and using the same enum class can avoid confusion. BUG=599364 ========== to ========== Merge fetch/CachePolicy to WebCachePolicy CachePolicy is used only for memory cache policy, but we have another, WebCachePolicy for disk cache. Basically these should be mapped 1:1, and using the same enum class can avoid confusion. BUG=599364 ==========
toyoshim@chromium.org changed reviewers: + japhet@chromium.org
Nate: this change will follow https://codereview.chromium.org/1858533002
It's a bit confusing to have the same enum mean different things in different contexts. Is there any hope of merging getMemoryCachePolicy() and resourceRequestCachePolicy()?
toyoshim@chromium.org changed reviewers: + yhirano@chromium.org
The CQ bit was checked by toyoshim@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_compile_dbg on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)
The CQ bit was checked by toyoshim@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_...)
Description was changed from ========== Merge fetch/CachePolicy to WebCachePolicy CachePolicy is used only for memory cache policy, but we have another, WebCachePolicy for disk cache. Basically these should be mapped 1:1, and using the same enum class can avoid confusion. BUG=599364 ========== to ========== remove FetchContext::getCachePolicy CachePolicy represents per-frame memory cache policy, and used to determine WebCachePolicy in FrameFetchContext. Since the last caller for this method is inside FrameFetchContext, we can remove the virtual method completely with some code changes. BUG=599364 ==========
Description was changed from ========== remove FetchContext::getCachePolicy CachePolicy represents per-frame memory cache policy, and used to determine WebCachePolicy in FrameFetchContext. Since the last caller for this method is inside FrameFetchContext, we can remove the virtual method completely with some code changes. BUG=599364 ========== to ========== remove FetchContext::getCachePolicy CachePolicy represents per-frame memory cache policy, and used to determine WebCachePolicy in FrameFetchContext. Since the last caller for this method is inside FrameFetchContext, we can remove the virtual method completely with some code changes. BUG=602900 ==========
The CQ bit was checked by toyoshim@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 toyoshim@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...
Description was changed from ========== remove FetchContext::getCachePolicy CachePolicy represents per-frame memory cache policy, and used to determine WebCachePolicy in FrameFetchContext. Since the last caller for this method is inside FrameFetchContext, we can remove the virtual method completely with some code changes. BUG=602900 ========== to ========== remove FetchContext::getCachePolicy() CachePolicy represents per-frame memory cache policy, and used to determine WebCachePolicy for resourceRequestCachePolicy in FrameFetchContext. Since the last caller of getCachePolicy() is only in FrameFetchContext, remove the virtual method completely, and replace it with some anonymous functions and static method. BUG=602900 ==========
The CQ bit was checked by toyoshim@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_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 toyoshim@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...
Description was changed from ========== remove FetchContext::getCachePolicy() CachePolicy represents per-frame memory cache policy, and used to determine WebCachePolicy for resourceRequestCachePolicy in FrameFetchContext. Since the last caller of getCachePolicy() is only in FrameFetchContext, remove the virtual method completely, and replace it with some anonymous functions and static method. BUG=602900 ========== to ========== remove FetchContext::getCachePolicy() CachePolicy represents per-frame memory cache policy, and used to determine WebCachePolicy for resourceRequestCachePolicy in FrameFetchContext. Since the last caller of getCachePolicy() is only in FrameFetchContext, remove the virtual method completely, and replace it with some anonymous functions. BUG=602900 ==========
PTAL patch set 14
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_...)
Awesome! Is there any reason to consider unwinding the dependency on the parent frame first (or as part of this patch)? If not, will resolving these TODOs happen soon? https://codereview.chromium.org/1867493003/diff/260001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/loader/FrameFetchContext.cpp (right): https://codereview.chromium.org/1867493003/diff/260001/third_party/WebKit/Sou... third_party/WebKit/Source/core/loader/FrameFetchContext.cpp:218: enum class RequestMethod { kIsPost, kIsNotPost }; I'm behind on the style guide: is kEnumValue preferred now over EnumValue? https://codereview.chromium.org/1867493003/diff/260001/third_party/WebKit/Sou... third_party/WebKit/Source/core/loader/FrameFetchContext.cpp:254: WebCachePolicy determineFrameWebCachePolicy(Frame* frame, Resource::Type type) { Can we use the local ResourceType instead of Resource::Type here? https://codereview.chromium.org/1867493003/diff/260001/third_party/WebKit/Sou... third_party/WebKit/Source/core/loader/FrameFetchContext.cpp:257: if (!frame->isLocalFrame()) Should this have a TODO to remove when |type| is removed? https://codereview.chromium.org/1867493003/diff/260001/third_party/WebKit/Sou... third_party/WebKit/Source/core/loader/FrameFetchContext.cpp:262: toLocalFrame(frame)->document()->fetcher()->context().isLoadComplete()) { Optional: Just check loadEventFinished() directly instead of routing to the FetchContext? https://codereview.chromium.org/1867493003/diff/260001/third_party/WebKit/Sou... third_party/WebKit/Source/core/loader/FrameFetchContext.cpp:267: // TODO(toyoshim): Adopt BypassingCache even for MainResource. Why is the MainResource special case necessary?
In this CL, I'm trying to exclude logical changes as much as possible, and I expect all TODO added in this CL will be fixed soon by small followup CLs. https://codereview.chromium.org/1867493003/diff/260001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/loader/FrameFetchContext.cpp (right): https://codereview.chromium.org/1867493003/diff/260001/third_party/WebKit/Sou... third_party/WebKit/Source/core/loader/FrameFetchContext.cpp:218: enum class RequestMethod { kIsPost, kIsNotPost }; Yeah, I also recently noticed this style change through a code review. https://www.chromium.org/blink/coding-style According to the rule 12, "kInterCaps is preferable for new code." https://codereview.chromium.org/1867493003/diff/260001/third_party/WebKit/Sou... third_party/WebKit/Source/core/loader/FrameFetchContext.cpp:254: WebCachePolicy determineFrameWebCachePolicy(Frame* frame, Resource::Type type) { Ah, yes. Sorry, I just forgot to revert this type change. When I changed this function to a class static method, I changed it to Resource::Type, but now it's an anonymous function and can be back to ResourceType. https://codereview.chromium.org/1867493003/diff/260001/third_party/WebKit/Sou... third_party/WebKit/Source/core/loader/FrameFetchContext.cpp:257: if (!frame->isLocalFrame()) Ah, yes. Subresources will not need to check parents' policy too. https://codereview.chromium.org/1867493003/diff/260001/third_party/WebKit/Sou... third_party/WebKit/Source/core/loader/FrameFetchContext.cpp:262: toLocalFrame(frame)->document()->fetcher()->context().isLoadComplete()) { On 2017/03/09 20:02:47, Nate Chapin wrote: > Optional: Just check loadEventFinished() directly instead of routing to the > FetchContext? Done. https://codereview.chromium.org/1867493003/diff/260001/third_party/WebKit/Sou... third_party/WebKit/Source/core/loader/FrameFetchContext.cpp:267: // TODO(toyoshim): Adopt BypassingCache even for MainResource. Just because the old behavior is so. I already confirmed that all tests pass even if I remove this main resource exception, but I just want to exclude behavior changes from this CL as I can as possible so that I can easily revert behavior change case by case when I need.
description: s/anonymous functions/functions in an unnamed namespace/ https://codereview.chromium.org/1867493003/diff/280001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/loader/FrameFetchContext.cpp (left): https://codereview.chromium.org/1867493003/diff/280001/third_party/WebKit/Sou... third_party/WebKit/Source/core/loader/FrameFetchContext.cpp:392: if (m_documentLoader->getRequest().httpMethod() == "POST") { This part is missing in the new implementation. https://codereview.chromium.org/1867493003/diff/280001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/loader/FrameFetchContext.cpp (right): https://codereview.chromium.org/1867493003/diff/280001/third_party/WebKit/Sou... third_party/WebKit/Source/core/loader/FrameFetchContext.cpp:234: case FrameLoadTypeInitialHistoryLoad: Where is this clause from? https://codereview.chromium.org/1867493003/diff/280001/third_party/WebKit/Sou... third_party/WebKit/Source/core/loader/FrameFetchContext.cpp:253: // TODO(toyoshim): Remove |type|. See comments in resourceRequestCachePolicy(). |resourceType|?
kinuko@chromium.org changed reviewers: + kinuko@chromium.org
I like this change. Would it be possible to have some unit tests for this?
The CQ bit was checked by toyoshim@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/1867493003/diff/280001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/loader/FrameFetchContext.cpp (left): https://codereview.chromium.org/1867493003/diff/280001/third_party/WebKit/Sou... third_party/WebKit/Source/core/loader/FrameFetchContext.cpp:392: if (m_documentLoader->getRequest().httpMethod() == "POST") { We do not need this in the new code. The old logic determines main resource policy first. This main resource policy was muted for history navigation at line 310 in the original, and following lines just revert the mutation for subresources. This is what the comment above explains. In the new logic, determineFrameWebCachePolicy() calls determineWebCachePolicy() with kIsNotMainResource. In this case, determineWebCachePolicy() returns ReturnCacheDataElseLoad directly. Since we do not mutate the policy, we do not need to revert it. FYI, expectations for post requests are - ReturnCacheDataDontLoad for the main resource - ReturnCacheDataElseLoad for subresources https://codereview.chromium.org/1867493003/diff/280001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/loader/FrameFetchContext.cpp (right): https://codereview.chromium.org/1867493003/diff/280001/third_party/WebKit/Sou... third_party/WebKit/Source/core/loader/FrameFetchContext.cpp:234: case FrameLoadTypeInitialHistoryLoad: This wasn't handled in the original, and used UseProtocolCachePolicy. But I think this is the consistent policy for the initial history load. We may want to split this behavior change into a separate CL. https://codereview.chromium.org/1867493003/diff/280001/third_party/WebKit/Sou... third_party/WebKit/Source/core/loader/FrameFetchContext.cpp:253: // TODO(toyoshim): Remove |type|. See comments in resourceRequestCachePolicy(). On 2017/03/14 08:34:35, yhirano wrote: > |resourceType|? Done.
kinuko: We already had some unit tests in FrameFetchContextTest.MainResource and WebFrameTest. Also some browser tests and layout tests cover this. But, probably it would be better that FrameFetchContextTest cover more cases. If we want to have more tests in this CL, I will change test expectations too in the followup CLs that will contain behavior changes. That may cost, but makes it clear what is changed in each CL. Also I probably add some tests to browser test because some behavior changes will affect only memory cache policy, but won't for disk cache. Do you have any preference? Anyway I will start creating tests in a separate CL so that it can be submitted before or after this CL.
On 2017/03/15 03:31:19, Takashi Toyoshima wrote: > kinuko: We already had some unit tests in FrameFetchContextTest.MainResource and > WebFrameTest. > Also some browser tests and layout tests cover this. > > But, probably it would be better that FrameFetchContextTest cover more cases. > > If we want to have more tests in this CL, I will change test expectations too in > the followup CLs that will contain behavior changes. > That may cost, but makes it clear what is changed in each CL. Also I probably > add some tests to browser test because some behavior changes will affect only > memory cache policy, but won't for disk cache. > > Do you have any preference? Anyway I will start creating tests in a separate CL > so that it can be submitted before or after this CL. Having some specific tests to cover follow-up changes sgtm, I'm fine not having them included in this CL too (as you say we already should have basic coverage).
https://codereview.chromium.org/1867493003/diff/280001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/loader/FrameFetchContext.cpp (right): https://codereview.chromium.org/1867493003/diff/280001/third_party/WebKit/Sou... third_party/WebKit/Source/core/loader/FrameFetchContext.cpp:234: case FrameLoadTypeInitialHistoryLoad: On 2017/03/15 03:13:51, Takashi Toyoshima wrote: > This wasn't handled in the original, and used UseProtocolCachePolicy. But I > think this is the consistent policy for the initial history load. We may want to > split this behavior change into a separate CL. Yeah, splitting the CL helps me understand the change.
https://codereview.chromium.org/1867493003/diff/280001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/loader/FrameFetchContext.cpp (right): https://codereview.chromium.org/1867493003/diff/280001/third_party/WebKit/Sou... third_party/WebKit/Source/core/loader/FrameFetchContext.cpp:234: case FrameLoadTypeInitialHistoryLoad: On 2017/03/15 05:26:20, yhirano wrote: > On 2017/03/15 03:13:51, Takashi Toyoshima wrote: > > This wasn't handled in the original, and used UseProtocolCachePolicy. But I > > think this is the consistent policy for the initial history load. We may want > to > > split this behavior change into a separate CL. > > Yeah, splitting the CL helps me understand the change. Done.
https://codereview.chromium.org/1867493003/diff/340001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/loader/FrameFetchContext.cpp (right): https://codereview.chromium.org/1867493003/diff/340001/third_party/WebKit/Sou... third_party/WebKit/Source/core/loader/FrameFetchContext.cpp:235: case FrameLoadTypeInitialInChildFrame: (resourceType == kIsMainResource && (requestType == kIsConditional || method == kIsPost) => ValidatingCacheData https://codereview.chromium.org/1867493003/diff/340001/third_party/WebKit/Sou... third_party/WebKit/Source/core/loader/FrameFetchContext.cpp:240: case FrameLoadTypeInitialHistoryLoad: Ditto https://codereview.chromium.org/1867493003/diff/340001/third_party/WebKit/Sou... third_party/WebKit/Source/core/loader/FrameFetchContext.cpp:256: case FrameLoadTypeReloadBypassingCache: ditto
new patch set was uploaded, but let me see if bots are happy with this. https://codereview.chromium.org/1867493003/diff/340001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/loader/FrameFetchContext.cpp (right): https://codereview.chromium.org/1867493003/diff/340001/third_party/WebKit/Sou... third_party/WebKit/Source/core/loader/FrameFetchContext.cpp:235: case FrameLoadTypeInitialInChildFrame: oops. good eyes. https://codereview.chromium.org/1867493003/diff/340001/third_party/WebKit/Sou... third_party/WebKit/Source/core/loader/FrameFetchContext.cpp:240: case FrameLoadTypeInitialHistoryLoad: On 2017/03/17 12:55:48, yhirano wrote: > Ditto Done. https://codereview.chromium.org/1867493003/diff/340001/third_party/WebKit/Sou... third_party/WebKit/Source/core/loader/FrameFetchContext.cpp:256: case FrameLoadTypeReloadBypassingCache: On 2017/03/17 12:55:49, yhirano wrote: > ditto Done.
The CQ bit was checked by toyoshim@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...
lgtm
https://codereview.chromium.org/1867493003/diff/360001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/loader/FrameFetchContext.cpp (right): https://codereview.chromium.org/1867493003/diff/360001/third_party/WebKit/Sou... third_party/WebKit/Source/core/loader/FrameFetchContext.cpp:245: // keep legacy logic as is for a while. nit: This CL has lots of TODOs, for the ones you plan to change in immediate follow-up patches could we just write so, e.g. 'to be changed in a follow-up patch'? ('for a while' sounds like indefinite period)
On 2017/03/22 08:09:44, kinuko wrote: > https://codereview.chromium.org/1867493003/diff/360001/third_party/WebKit/Sou... > File third_party/WebKit/Source/core/loader/FrameFetchContext.cpp (right): > > https://codereview.chromium.org/1867493003/diff/360001/third_party/WebKit/Sou... > third_party/WebKit/Source/core/loader/FrameFetchContext.cpp:245: // keep legacy > logic as is for a while. > nit: This CL has lots of TODOs, for the ones you plan to change in immediate > follow-up patches could we just write so, e.g. 'to be changed in a follow-up > patch'? ('for a while' sounds like indefinite period) (lgtm/2, btw)
I will submit the last patch set. Nate, I think you agreed the direction of this change. If you still have some concerns, please let me fix it in follow-up changes. https://codereview.chromium.org/1867493003/diff/360001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/loader/FrameFetchContext.cpp (right): https://codereview.chromium.org/1867493003/diff/360001/third_party/WebKit/Sou... third_party/WebKit/Source/core/loader/FrameFetchContext.cpp:245: // keep legacy logic as is for a while. On 2017/03/22 08:09:44, kinuko wrote: > nit: This CL has lots of TODOs, for the ones you plan to change in immediate > follow-up patches could we just write so, e.g. 'to be changed in a follow-up > patch'? ('for a while' sounds like indefinite period) Done.
The CQ bit was checked by toyoshim@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from kinuko@chromium.org, yhirano@chromium.org Link to the patchset: https://codereview.chromium.org/1867493003/#ps380001 (title: "review #51")
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": 380001, "attempt_start_ts": 1490172780058590,
"parent_rev": "d086be303574fcf89fe19de16307b2a582dd3362", "commit_rev":
"fa78cf3a4331be2113c320020e10d1223127c125"}
Message was sent while issue was closed.
Description was changed from ========== remove FetchContext::getCachePolicy() CachePolicy represents per-frame memory cache policy, and used to determine WebCachePolicy for resourceRequestCachePolicy in FrameFetchContext. Since the last caller of getCachePolicy() is only in FrameFetchContext, remove the virtual method completely, and replace it with some anonymous functions. BUG=602900 ========== to ========== remove FetchContext::getCachePolicy() CachePolicy represents per-frame memory cache policy, and used to determine WebCachePolicy for resourceRequestCachePolicy in FrameFetchContext. Since the last caller of getCachePolicy() is only in FrameFetchContext, remove the virtual method completely, and replace it with some anonymous functions. BUG=602900 Review-Url: https://codereview.chromium.org/1867493003 Cr-Commit-Position: refs/heads/master@{#458708} Committed: https://chromium.googlesource.com/chromium/src/+/fa78cf3a4331be2113c320020e10... ==========
Message was sent while issue was closed.
Committed patchset #20 (id:380001) as https://chromium.googlesource.com/chromium/src/+/fa78cf3a4331be2113c320020e10... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
