Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2274233002:
[Devtools] Fix reload behavior when devtools is open (Closed)
|
Created:
4 years, 3 months ago by allada Modified:
4 years, 3 months ago CC:
Nate Chapin, chromium-reviews, extensions-reviews_chromium.org, caseq+blink_chromium.org, lushnikov+blink_chromium.org, pfeldman+blink_chromium.org, apavlov+blink_chromium.org, devtools-reviews_chromium.org, blink-reviews, chromium-apps-reviews_chromium.org, kozyatinskiy+blink_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[Devtools] Fix reload behavior when devtools is open
Fixed a bug where devtools was changing the reload behavior of
chrome while it was open. This patch makes devtools follow
the same behavior chrome does when refreshing.
The memory cache was being ignored in the case when devtools was
open devtools consumes the reload events and dispatches them to
devtools front-end for directives on what to do. Front end was then
dispatching a reload event back to browser but the directive that
devtools was instructing was to ignore memory cache (same behavior
that window.location.reload() would use), however if the user was
to reload with devtools closed it would try and use the items in
memory cache if available.
Patch worth noting: https://codereview.chromium.org/1975753006
R=dgozman
BUG=640687
Committed: https://crrev.com/ce98e6c4a756a41f57ff444a40267a66f52e91e8
Cr-Commit-Position: refs/heads/master@{#417669}
Patch Set 1 : [Devtools] Fix reload behavior when devtools is open #
Total comments: 15
Patch Set 2 : new methods #Messages
Total messages: 42 (26 generated)
Description was changed from ========== [Devtools] Fix reload behavior when devtools is open Fixed a bug where devtools was changing the reload behavior of chrome while it was open. This patch makes devtools follow the same behavior chrome does when refreshing. The memory cache was being ignored in the case when devtools was open devtools consumes the reload events and dispatches them to devtools front-end for directives on what to do. Front end was then dispatching a reload event back to browser but the directive that devtools was instructing was to ignore memory cache (same behavior that window.location.reload() would use), however if the user was to reload with devtools closed it would try and use the items in memory cache if available. B=640687 R=dgozman BUG= ========== to ========== [Devtools] Fix reload behavior when devtools is open Fixed a bug where devtools was changing the reload behavior of chrome while it was open. This patch makes devtools follow the same behavior chrome does when refreshing. The memory cache was being ignored in the case when devtools was open devtools consumes the reload events and dispatches them to devtools front-end for directives on what to do. Front end was then dispatching a reload event back to browser but the directive that devtools was instructing was to ignore memory cache (same behavior that window.location.reload() would use), however if the user was to reload with devtools closed it would try and use the items in memory cache if available. R=dgozman BUG=640687 ==========
The CQ bit was checked by allada@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 ========== [Devtools] Fix reload behavior when devtools is open Fixed a bug where devtools was changing the reload behavior of chrome while it was open. This patch makes devtools follow the same behavior chrome does when refreshing. The memory cache was being ignored in the case when devtools was open devtools consumes the reload events and dispatches them to devtools front-end for directives on what to do. Front end was then dispatching a reload event back to browser but the directive that devtools was instructing was to ignore memory cache (same behavior that window.location.reload() would use), however if the user was to reload with devtools closed it would try and use the items in memory cache if available. R=dgozman BUG=640687 ========== to ========== [Devtools] Fix reload behavior when devtools is open Fixed a bug where devtools was changing the reload behavior of chrome while it was open. This patch makes devtools follow the same behavior chrome does when refreshing. The memory cache was being ignored in the case when devtools was open devtools consumes the reload events and dispatches them to devtools front-end for directives on what to do. Front end was then dispatching a reload event back to browser but the directive that devtools was instructing was to ignore memory cache (same behavior that window.location.reload() would use), however if the user was to reload with devtools closed it would try and use the items in memory cache if available. Patch worth noting: https://codereview.chromium.org/1975753006 R=dgozman BUG=640687 ==========
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...)
Patchset #1 (id:1) has been deleted
The CQ bit was checked by allada@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...
PTAL and tell me what you think.
allada@chromium.org changed reviewers: + toyoshim@chromium.org
toyoshim@ could you also give me a lgtm on this too?
dgozman@chromium.org changed reviewers: + caseq@chromium.org
+caseq for extension test - could you please take a look? https://codereview.chromium.org/2274233002/diff/20001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/http/tests/inspector-protocol/reload-memory-cache-expected.txt (right): https://codereview.chromium.org/2274233002/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/http/tests/inspector-protocol/reload-memory-cache-expected.txt:1: Tests that reloads when choming from protocol still use memory cache for reloads. typo: choming, reloads mentioned twice https://codereview.chromium.org/2274233002/diff/20001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/http/tests/inspector-protocol/reload-memory-cache.html (right): https://codereview.chromium.org/2274233002/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/http/tests/inspector-protocol/reload-memory-cache.html:53: InspectorTest.log("THIS SHOULD NOT BE REACHED!!!!"); nit: I don't like the shouty case :-) https://codereview.chromium.org/2274233002/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/http/tests/inspector-protocol/reload-memory-cache.html:53: InspectorTest.log("THIS SHOULD NOT BE REACHED!!!!"); completeTest in this case as well, or it will timeout. https://codereview.chromium.org/2274233002/diff/20001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/http/tests/inspector/resource-har-conversion-expected.txt (right): https://codereview.chromium.org/2274233002/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/http/tests/inspector/resource-har-conversion-expected.txt:32: compression : <number> Why did this change?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2274233002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/inspector/InspectorPageAgent.cpp (left): https://codereview.chromium.org/2274233002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/inspector/InspectorPageAgent.cpp:419: m_inspectedFrames->root()->reload(optionalBypassCache.fromMaybe(false) ? FrameLoadTypeReloadBypassingCache : FrameLoadTypeReload, ClientRedirectPolicy::NotClientRedirect); When is this method called? Now, FrameLoadTypeReload is a default behavior, but FrameLoadTypeReloadMainResource is used behind a flag. If this is called when user push reload button on the DevTools UI, or on similar situation via a remote control, I think it's better to change this when we enable the flag by default. If so, can you leave following comment for me? TODO(crbug.com/612701): Change reload type when new reload implementation is enabled by default. If this is triggered even when a user clicks reload button from Chrome UI, this will fix a known issue that I need to fix for the new reload.
https://codereview.chromium.org/2274233002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/inspector/InspectorPageAgent.cpp (left): https://codereview.chromium.org/2274233002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/inspector/InspectorPageAgent.cpp:419: m_inspectedFrames->root()->reload(optionalBypassCache.fromMaybe(false) ? FrameLoadTypeReloadBypassingCache : FrameLoadTypeReload, ClientRedirectPolicy::NotClientRedirect); I checked by myself. This will fix the issue that means this method is called for reload UI. So the right fix is plumbing the flag from the browser process, and switch reload mode here, but probably it's enough to leave a TODO and update the mode when the feature is enabled by default.
> +caseq for extension test - could you please take a look? extensions tests lgtm
https://codereview.chromium.org/2274233002/diff/20001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/http/tests/inspector/resource-har-conversion-expected.txt (right): https://codereview.chromium.org/2274233002/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/http/tests/inspector/resource-har-conversion-expected.txt:32: compression : <number> On 2016/08/24 23:09:42, dgozman wrote: > Why did this change? Because now it is sending with a compression, before i was sending just a 304 so there was no content to compress. https://codereview.chromium.org/2274233002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/inspector/InspectorPageAgent.cpp (left): https://codereview.chromium.org/2274233002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/inspector/InspectorPageAgent.cpp:419: m_inspectedFrames->root()->reload(optionalBypassCache.fromMaybe(false) ? FrameLoadTypeReloadBypassingCache : FrameLoadTypeReload, ClientRedirectPolicy::NotClientRedirect); On 2016/08/25 10:52:39, toyoshim wrote: > I checked by myself. This will fix the issue that means this method is called > for reload UI. > > So the right fix is plumbing the flag from the browser process, and switch > reload mode here, but probably it's enough to leave a TODO and update the mode > when the feature is enabled by default. I see that's where I was confused, in my ToT version of chromium it was on by default. I manually disabled it and it goes back to FrameLoadTypeReload behavior. So would you prefer a TODO with your name or would you like me to plum it? I think it may be worth to write a browser test to ensure devtools does not change behavior of reload instead of doing it in protocol (since protocol causes the bug). I have a feeling a little bit of refactoring in the way devtools handles reload is due at some point though.
https://codereview.chromium.org/2274233002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/inspector/InspectorPageAgent.cpp (left): https://codereview.chromium.org/2274233002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/inspector/InspectorPageAgent.cpp:419: m_inspectedFrames->root()->reload(optionalBypassCache.fromMaybe(false) ? FrameLoadTypeReloadBypassingCache : FrameLoadTypeReload, ClientRedirectPolicy::NotClientRedirect); Yes, if the setting is default, 50% of ToT/Canary/Dev and 10% of Beta users get a new behavior. About plumbing or TODO, I'm fine with both. It's up to you. For testing, I agreed that we should have tests for reload, and actually I partially added it recently as a content browser test. content/browser/loader/reload_cache_control_browsertest.cc is the one. It has a TODO to have additional tests with DevTools open. I planned to add it when I fix this issue, but it hasn't happened yet.
https://codereview.chromium.org/2274233002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/inspector/InspectorPageAgent.cpp (left): https://codereview.chromium.org/2274233002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/inspector/InspectorPageAgent.cpp:419: m_inspectedFrames->root()->reload(optionalBypassCache.fromMaybe(false) ? FrameLoadTypeReloadBypassingCache : FrameLoadTypeReload, ClientRedirectPolicy::NotClientRedirect); FYI, I will plumb the flag for another purpose. See patch set 5 of https://codereview.chromium.org/2257743002/. If you need this flag and will submit before my change, please feel free to use the code in this CL.
Any other problems that block this CL? https://codereview.chromium.org/2274233002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/inspector/InspectorPageAgent.cpp (left): https://codereview.chromium.org/2274233002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/inspector/InspectorPageAgent.cpp:419: m_inspectedFrames->root()->reload(optionalBypassCache.fromMaybe(false) ? FrameLoadTypeReloadBypassingCache : FrameLoadTypeReload, ClientRedirectPolicy::NotClientRedirect); Now the flag is available in Blink. So, this could be RuntimeEnabledFeatures::reloadwithoutSubResourceCacheRevalidationEnabled() ? FrameLoadTypeReloadMainResource : FrameLoadTypeReload
On 2016/09/07 06:48:20, toyoshim wrote: > Any other problems that block this CL? > > https://codereview.chromium.org/2274233002/diff/20001/third_party/WebKit/Sour... > File third_party/WebKit/Source/core/inspector/InspectorPageAgent.cpp (left): > > https://codereview.chromium.org/2274233002/diff/20001/third_party/WebKit/Sour... > third_party/WebKit/Source/core/inspector/InspectorPageAgent.cpp:419: > m_inspectedFrames->root()->reload(optionalBypassCache.fromMaybe(false) ? > FrameLoadTypeReloadBypassingCache : FrameLoadTypeReload, > ClientRedirectPolicy::NotClientRedirect); > Now the flag is available in Blink. > So, this could be > > RuntimeEnabledFeatures::reloadwithoutSubResourceCacheRevalidationEnabled() ? > FrameLoadTypeReloadMainResource : FrameLoadTypeReload No, I will patch it in and see how it works. Thanks!
The CQ bit was checked by allada@chromium.org to run a CQ dry run
PTL toyoshim@ and dgozman@ https://codereview.chromium.org/2274233002/diff/20001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/http/tests/inspector-protocol/reload-memory-cache-expected.txt (right): https://codereview.chromium.org/2274233002/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/http/tests/inspector-protocol/reload-memory-cache-expected.txt:1: Tests that reloads when choming from protocol still use memory cache for reloads. On 2016/08/24 23:09:42, dgozman wrote: > typo: choming, reloads mentioned twice Done. https://codereview.chromium.org/2274233002/diff/20001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/http/tests/inspector-protocol/reload-memory-cache.html (right): https://codereview.chromium.org/2274233002/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/http/tests/inspector-protocol/reload-memory-cache.html:53: InspectorTest.log("THIS SHOULD NOT BE REACHED!!!!"); On 2016/08/24 23:09:42, dgozman wrote: > nit: I don't like the shouty case :-) Done. https://codereview.chromium.org/2274233002/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/http/tests/inspector-protocol/reload-memory-cache.html:53: InspectorTest.log("THIS SHOULD NOT BE REACHED!!!!"); On 2016/08/24 23:09:42, dgozman wrote: > completeTest in this case as well, or it will timeout. Done. https://codereview.chromium.org/2274233002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/inspector/InspectorPageAgent.cpp (left): https://codereview.chromium.org/2274233002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/inspector/InspectorPageAgent.cpp:419: m_inspectedFrames->root()->reload(optionalBypassCache.fromMaybe(false) ? FrameLoadTypeReloadBypassingCache : FrameLoadTypeReload, ClientRedirectPolicy::NotClientRedirect); On 2016/09/07 06:48:20, toyoshim wrote: > Now the flag is available in Blink. > So, this could be > > RuntimeEnabledFeatures::reloadwithoutSubResourceCacheRevalidationEnabled() ? > FrameLoadTypeReloadMainResource : FrameLoadTypeReload Done.
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 allada@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.
Patchset #2 (id:40001) has been deleted
reload LGTM
The CQ bit was checked by allada@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from caseq@chromium.org Link to the patchset: https://codereview.chromium.org/2274233002/#ps60001 (title: "new methods")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== [Devtools] Fix reload behavior when devtools is open Fixed a bug where devtools was changing the reload behavior of chrome while it was open. This patch makes devtools follow the same behavior chrome does when refreshing. The memory cache was being ignored in the case when devtools was open devtools consumes the reload events and dispatches them to devtools front-end for directives on what to do. Front end was then dispatching a reload event back to browser but the directive that devtools was instructing was to ignore memory cache (same behavior that window.location.reload() would use), however if the user was to reload with devtools closed it would try and use the items in memory cache if available. Patch worth noting: https://codereview.chromium.org/1975753006 R=dgozman BUG=640687 ========== to ========== [Devtools] Fix reload behavior when devtools is open Fixed a bug where devtools was changing the reload behavior of chrome while it was open. This patch makes devtools follow the same behavior chrome does when refreshing. The memory cache was being ignored in the case when devtools was open devtools consumes the reload events and dispatches them to devtools front-end for directives on what to do. Front end was then dispatching a reload event back to browser but the directive that devtools was instructing was to ignore memory cache (same behavior that window.location.reload() would use), however if the user was to reload with devtools closed it would try and use the items in memory cache if available. Patch worth noting: https://codereview.chromium.org/1975753006 R=dgozman BUG=640687 ==========
Message was sent while issue was closed.
Committed patchset #2 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== [Devtools] Fix reload behavior when devtools is open Fixed a bug where devtools was changing the reload behavior of chrome while it was open. This patch makes devtools follow the same behavior chrome does when refreshing. The memory cache was being ignored in the case when devtools was open devtools consumes the reload events and dispatches them to devtools front-end for directives on what to do. Front end was then dispatching a reload event back to browser but the directive that devtools was instructing was to ignore memory cache (same behavior that window.location.reload() would use), however if the user was to reload with devtools closed it would try and use the items in memory cache if available. Patch worth noting: https://codereview.chromium.org/1975753006 R=dgozman BUG=640687 ========== to ========== [Devtools] Fix reload behavior when devtools is open Fixed a bug where devtools was changing the reload behavior of chrome while it was open. This patch makes devtools follow the same behavior chrome does when refreshing. The memory cache was being ignored in the case when devtools was open devtools consumes the reload events and dispatches them to devtools front-end for directives on what to do. Front end was then dispatching a reload event back to browser but the directive that devtools was instructing was to ignore memory cache (same behavior that window.location.reload() would use), however if the user was to reload with devtools closed it would try and use the items in memory cache if available. Patch worth noting: https://codereview.chromium.org/1975753006 R=dgozman BUG=640687 Committed: https://crrev.com/ce98e6c4a756a41f57ff444a40267a66f52e91e8 Cr-Commit-Position: refs/heads/master@{#417669} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/ce98e6c4a756a41f57ff444a40267a66f52e91e8 Cr-Commit-Position: refs/heads/master@{#417669} |
