Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1997833002:
Don't keep a separate m_revalidatingRequest on Resource (Closed)
|
Created:
4 years, 7 months ago by Nate Chapin Modified:
4 years, 6 months ago CC:
chromium-reviews, tyoshino+watch_chromium.org, Yoav Weiss, blink-reviews-bindings_chromium.org, gavinp+loader_chromium.org, blink-reviews, loading-reviews+fetch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionDon't keep a separate m_revalidatingRequest on Resource
Have Resource::setRevalidatingRequest() just overwrite
m_resourceRequest, and keep an m_isRevalidating bit to track
whether a revalidation is progress.
Ideally we'd get to a point where the m_isRevalidating bit
isn't needed, or at least doesn't need to be accessible
outside of Resource and its subclasses.
BUG=
Committed: https://crrev.com/3a7c82704c5428a6e7be008e9a65e58f30ef285f
Cr-Commit-Position: refs/heads/master@{#398173}
Patch Set 1 #
Total comments: 18
Patch Set 2 : Address yhirano's comments #Patch Set 3 : Rebase #
Messages
Total messages: 37 (15 generated)
The CQ bit was checked by japhet@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1997833002/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1997833002/1
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== Don't keep a separate m_revalidatingRequest on Resource BUG= ========== to ========== Don't keep a separate m_revalidatingRequest on Resource Have Resource::setRevalidatingRequest() just overwrite m_resourceRequest, and keep an m_isRevalidating bit to track whether a revalidation is progress. Ideally we'd get to a point where the m_isRevalidating bit isn't needed, or at least doesn't need to be accessible outside of Resource and its subclasses. This patch removes a few uses of that public getter, isCacheValidator(). BUG= ==========
japhet@chromium.org changed reviewers: + hiroshige@chromium.org, yhirano@chromium.org
Does this seem reasonable? https://codereview.chromium.org/1997833002/diff/1/third_party/WebKit/Source/b... File third_party/WebKit/Source/bindings/core/v8/ScriptStreamer.cpp (left): https://codereview.chromium.org/1997833002/diff/1/third_party/WebKit/Source/b... third_party/WebKit/Source/bindings/core/v8/ScriptStreamer.cpp:656: if (resource->isCacheValidator()) { I think this this was a cache for the old way of revalidating with a separate resource. I can't find any tests that indicate it's still needed. https://codereview.chromium.org/1997833002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/fetch/Resource.cpp (left): https://codereview.chromium.org/1997833002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/fetch/Resource.cpp:581: if (!memoryCache()->contains(this) || hasClientsOrObservers() || !m_revalidatingRequest.isNull() || !m_loadFinishTime || !isSafeToUnlock()) Switched this to !isLoaded(), which I think is what these bits are trying to express.
https://codereview.chromium.org/1997833002/diff/1/third_party/WebKit/Source/b... File third_party/WebKit/Source/bindings/core/v8/ScriptStreamer.cpp (left): https://codereview.chromium.org/1997833002/diff/1/third_party/WebKit/Source/b... third_party/WebKit/Source/bindings/core/v8/ScriptStreamer.cpp:656: if (resource->isCacheValidator()) { On 2016/05/19 22:18:38, Nate Chapin wrote: > I think this this was a cache for the old way of revalidating with a separate > resource. I can't find any tests that indicate it's still needed. It seems this branch was introduced at https://codereview.chromium.org/368283002#msg11, but I still don't understand why it was needed and why it isn't needed any more. I'll try again tomorrow. https://codereview.chromium.org/1997833002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/fetch/Resource.cpp (right): https://codereview.chromium.org/1997833002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/fetch/Resource.cpp:550: void Resource::setRevalidatingRequest(const ResourceRequest& request) DCHECK(!request.isNull())? https://codereview.chromium.org/1997833002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/fetch/Resource.cpp:703: m_clients.add(client); Not related to this CL but it looks a |didAddClient| call is missing. https://codereview.chromium.org/1997833002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/fetch/Resource.cpp:919: } m_isRevalidating = false; https://codereview.chromium.org/1997833002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/fetch/Resource.cpp:924: m_redirectChain.clear(); ditto https://codereview.chromium.org/1997833002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/fetch/Resource.h (right): https://codereview.chromium.org/1997833002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/fetch/Resource.h:304: bool m_isRevalidating; This member can be in the private section. Its getter is already provided. https://codereview.chromium.org/1997833002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/fetch/Resource.h:304: bool m_isRevalidating; [optional] Perhaps it might be good to have this boolean as a bitfield.
Generally looking good but I don't understand ScriptStreamer change. I'll try to understand tomorrow.
https://codereview.chromium.org/1997833002/diff/1/third_party/WebKit/Source/b... File third_party/WebKit/Source/bindings/core/v8/ScriptStreamer.cpp (left): https://codereview.chromium.org/1997833002/diff/1/third_party/WebKit/Source/b... third_party/WebKit/Source/bindings/core/v8/ScriptStreamer.cpp:656: if (resource->isCacheValidator()) { On 2016/05/23 11:40:38, yhirano wrote: > On 2016/05/19 22:18:38, Nate Chapin wrote: > > I think this this was a cache for the old way of revalidating with a separate > > resource. I can't find any tests that indicate it's still needed. > > It seems this branch was introduced at > https://codereview.chromium.org/368283002#msg11, but I still don't understand > why it was needed and why it isn't needed any more. I'll try again tomorrow. I think this branch is needed because ScriptResource doesn't call notifyAppendData when the revalidation succeeds. I couldn't find any layout tests testing ScriptStreamer specifically by the way (I skimmed ScriptStream.cpp changes but none of them updated LayoutTests except https://codereview.chromium.org/1807193003).
https://codereview.chromium.org/1997833002/diff/1/third_party/WebKit/Source/b... File third_party/WebKit/Source/bindings/core/v8/ScriptStreamer.cpp (left): https://codereview.chromium.org/1997833002/diff/1/third_party/WebKit/Source/b... third_party/WebKit/Source/bindings/core/v8/ScriptStreamer.cpp:656: if (resource->isCacheValidator()) { On 2016/05/24 11:18:51, yhirano wrote: > On 2016/05/23 11:40:38, yhirano wrote: > > On 2016/05/19 22:18:38, Nate Chapin wrote: > > > I think this this was a cache for the old way of revalidating with a > separate > > > resource. I can't find any tests that indicate it's still needed. > > > > It seems this branch was introduced at > > https://codereview.chromium.org/368283002#msg11, but I still don't understand > > why it was needed and why it isn't needed any more. I'll try again tomorrow. > > I think this branch is needed because ScriptResource doesn't call > notifyAppendData when the revalidation succeeds. > > I couldn't find any layout tests testing ScriptStreamer specifically by the way > (I skimmed ScriptStream.cpp changes but none of them updated LayoutTests except > https://codereview.chromium.org/1807193003). Ok, the lack of tests is annoying, but I agree that the lack of notifyAppendData() calls is a plausible reason, so I'm reverting this block. That may be something to think about if we refactor ResourceClient callbacks at some point. https://codereview.chromium.org/1997833002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/fetch/Resource.cpp (right): https://codereview.chromium.org/1997833002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/fetch/Resource.cpp:550: void Resource::setRevalidatingRequest(const ResourceRequest& request) On 2016/05/23 11:40:38, yhirano wrote: > DCHECK(!request.isNull())? Done. https://codereview.chromium.org/1997833002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/fetch/Resource.cpp:703: m_clients.add(client); On 2016/05/23 11:40:38, yhirano wrote: > Not related to this CL but it looks a |didAddClient| call is missing. I don't think we want didAddClient here: that would cause the client to get the data from the old load, not the revalidation. https://codereview.chromium.org/1997833002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/fetch/Resource.cpp:919: } On 2016/05/23 11:40:38, yhirano wrote: > m_isRevalidating = false; This is now set in responseReceived(), just before revalidationSucceeded/revalidationFailed is called. https://codereview.chromium.org/1997833002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/fetch/Resource.cpp:924: m_redirectChain.clear(); On 2016/05/23 11:40:38, yhirano wrote: > ditto ditto :) https://codereview.chromium.org/1997833002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/fetch/Resource.h (right): https://codereview.chromium.org/1997833002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/fetch/Resource.h:304: bool m_isRevalidating; On 2016/05/23 11:40:38, yhirano wrote: > This member can be in the private section. Its getter is already provided. Done. https://codereview.chromium.org/1997833002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/fetch/Resource.h:304: bool m_isRevalidating; On 2016/05/23 11:40:38, yhirano wrote: > [optional] Perhaps it might be good to have this boolean as a bitfield. Done.
The CQ bit was checked by japhet@chromium.org to run a CQ dry run
Description was changed from ========== Don't keep a separate m_revalidatingRequest on Resource Have Resource::setRevalidatingRequest() just overwrite m_resourceRequest, and keep an m_isRevalidating bit to track whether a revalidation is progress. Ideally we'd get to a point where the m_isRevalidating bit isn't needed, or at least doesn't need to be accessible outside of Resource and its subclasses. This patch removes a few uses of that public getter, isCacheValidator(). BUG= ========== to ========== Don't keep a separate m_revalidatingRequest on Resource Have Resource::setRevalidatingRequest() just overwrite m_resourceRequest, and keep an m_isRevalidating bit to track whether a revalidation is progress. Ideally we'd get to a point where the m_isRevalidating bit isn't needed, or at least doesn't need to be accessible outside of Resource and its subclasses. BUG= ==========
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1997833002/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1997833002/20001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Dry run: None
lgtm https://codereview.chromium.org/1997833002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/fetch/Resource.cpp (right): https://codereview.chromium.org/1997833002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/fetch/Resource.cpp:703: m_clients.add(client); On 2016/05/26 21:43:02, Nate Chapin wrote: > On 2016/05/23 11:40:38, yhirano wrote: > > Not related to this CL but it looks a |didAddClient| call is missing. > > I don't think we want didAddClient here: that would cause the client to get the > data from the old load, not the revalidation. Ah, I see, thank you.
This CL looks good, but conflicts with my CL [1]. Because I want to merge [1] to beta etc., I want to land [1] first and then this CL. Could you take a look at [1]? Thanks. [1] https://codereview.chromium.org/2017883002/
On 2016/05/27 07:16:01, hiroshige wrote: > This CL looks good, but conflicts with my CL [1]. > Because I want to merge [1] to beta etc., I want to land [1] first and then this > CL. Could you take a look at [1]? Thanks. > > [1] https://codereview.chromium.org/2017883002/ Reviewed. I'll wait to land this until that one is in.
On 2016/05/27 17:44:46, Nate Chapin wrote: > On 2016/05/27 07:16:01, hiroshige wrote: > > This CL looks good, but conflicts with my CL [1]. > > Because I want to merge [1] to beta etc., I want to land [1] first and then > this > > CL. Could you take a look at [1]? Thanks. > > > > [1] https://codereview.chromium.org/2017883002/ > > Reviewed. I'll wait to land this until that one is in. Thanks! I'm committing [1]. Because [1] introduces a new callsite of revalidationFailed(), could you modify the code around the new revalidationFailed() callsite after rebase?
On 2016/06/03 05:38:57, hiroshige (slow) wrote: > On 2016/05/27 17:44:46, Nate Chapin wrote: > > On 2016/05/27 07:16:01, hiroshige wrote: > > > This CL looks good, but conflicts with my CL [1]. > > > Because I want to merge [1] to beta etc., I want to land [1] first and then > > this > > > CL. Could you take a look at [1]? Thanks. > > > > > > [1] https://codereview.chromium.org/2017883002/ > > > > Reviewed. I'll wait to land this until that one is in. > > Thanks! > I'm committing [1]. > Because [1] introduces a new callsite of revalidationFailed(), could you modify > the code around the new revalidationFailed() callsite after rebase? Now that there are multiple callsites for revaldationFailed(), I moved "m_isRevalidating = false;" back to revalidationFailed/revalidationSucceeded.
The CQ bit was checked by japhet@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/1997833002/#ps40001 (title: "Rebase")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1997833002/40001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was checked by japhet@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1997833002/40001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_android_rel_ng on tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by japhet@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1997833002/40001
Message was sent while issue was closed.
Description was changed from ========== Don't keep a separate m_revalidatingRequest on Resource Have Resource::setRevalidatingRequest() just overwrite m_resourceRequest, and keep an m_isRevalidating bit to track whether a revalidation is progress. Ideally we'd get to a point where the m_isRevalidating bit isn't needed, or at least doesn't need to be accessible outside of Resource and its subclasses. BUG= ========== to ========== Don't keep a separate m_revalidatingRequest on Resource Have Resource::setRevalidatingRequest() just overwrite m_resourceRequest, and keep an m_isRevalidating bit to track whether a revalidation is progress. Ideally we'd get to a point where the m_isRevalidating bit isn't needed, or at least doesn't need to be accessible outside of Resource and its subclasses. BUG= ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== Don't keep a separate m_revalidatingRequest on Resource Have Resource::setRevalidatingRequest() just overwrite m_resourceRequest, and keep an m_isRevalidating bit to track whether a revalidation is progress. Ideally we'd get to a point where the m_isRevalidating bit isn't needed, or at least doesn't need to be accessible outside of Resource and its subclasses. BUG= ========== to ========== Don't keep a separate m_revalidatingRequest on Resource Have Resource::setRevalidatingRequest() just overwrite m_resourceRequest, and keep an m_isRevalidating bit to track whether a revalidation is progress. Ideally we'd get to a point where the m_isRevalidating bit isn't needed, or at least doesn't need to be accessible outside of Resource and its subclasses. BUG= Committed: https://crrev.com/3a7c82704c5428a6e7be008e9a65e58f30ef285f Cr-Commit-Position: refs/heads/master@{#398173} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/3a7c82704c5428a6e7be008e9a65e58f30ef285f Cr-Commit-Position: refs/heads/master@{#398173} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
