DevTools: do not clear memory cache when restoring emulation [chromium/src : main]
Alex Rudenko (Gerrit)
Alex Rudenko voted and added 3 comments![]( "Open in Gerrit")
Votes added by Alex Rudenko
| Commit-Queue | +1 |
3 comments
void ActivateDevToolsTransform(const DeviceEmulationParams&,Alex RudenkoBlink Style Guide: Prefer enums or StrongAliases to bare bools for function parameters. The meaning of the 'restore' boolean is not clear at call sites. Please consider using a `base::StrongAlias` or an enum to improve readability, for example `using RestoreState = base::StrongAlias<class RestoreStateTag, bool>;`.
_To keep this interaction as brief and non-intrusive as possible, please consider responding with one of following options:
**Done** | **Won't fix**: reason | **b/<bug_id>** | **Invalid:** reasonThis comment was generated by [Experimental Blink C++ Code Review Agent](http://go/blink-c++-code-review-agent).AI reviews can sometimes be inaccurate; We appreciate your 🙏 feedback 🙏 to help us improve.[File a bug](http://go/blink-c++-code-review-agent-feedback) | [Provide feedback on chat](https://chat.google.com/room/AAQA0zhQHe0?cls=4) | [Opt-out](https://ganpati2.corp.google.com/group/peep-genai-blink-agent-optout.prod)_
Done
void ChangeEmulationParams(const DeviceEmulationParams& params,Alex RudenkoBlink Style Guide: Prefer enums or StrongAliases to bare bools for function parameters. The meaning of the 'restore' boolean is not clear at call sites. Please consider using a `base::StrongAlias` or an enum to improve readability, for example `using RestoreState = base::StrongAlias<class RestoreStateTag, bool>;`.
_To keep this interaction as brief and non-intrusive as possible, please consider responding with one of following options:
**Done** | **Won't fix**: reason | **b/<bug_id>** | **Invalid:** reasonThis comment was generated by [Experimental Blink C++ Code Review Agent](http://go/blink-c++-code-review-agent).AI reviews can sometimes be inaccurate; We appreciate your 🙏 feedback 🙏 to help us improve.[File a bug](http://go/blink-c++-code-review-agent-feedback) | [Provide feedback on chat](https://chat.google.com/room/AAQA0zhQHe0?cls=4) | [Opt-out](https://ganpati2.corp.google.com/group/peep-genai-blink-agent-optout.prod)_
Done
void EnableDeviceEmulation(const DeviceEmulationParams& parameters,Alex RudenkoBlink Style Guide: Prefer enums or StrongAliases to bare bools for function parameters. The meaning of the 'restore' boolean is not clear at call sites. Please consider using a `base::StrongAlias` or an enum to improve readability, for example `using RestoreState = base::StrongAlias<class RestoreStateTag, bool>;`.
_To keep this interaction as brief and non-intrusive as possible, please consider responding with one of following options:
**Done** | **Won't fix**: reason | **b/<bug_id>** | **Invalid:** reasonThis comment was generated by [Experimental Blink C++ Code Review Agent](http://go/blink-c++-code-review-agent).AI reviews can sometimes be inaccurate; We appreciate your 🙏 feedback 🙏 to help us improve.[File a bug](http://go/blink-c++-code-review-agent-feedback) | [Provide feedback on chat](https://chat.google.com/room/AAQA0zhQHe0?cls=4) | [Opt-out](https://ganpati2.corp.google.com/group/peep-genai-blink-agent-optout.prod)_
Done
Related details
- Philip Pfaffe
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Philip Pfaffe (Gerrit)
Philip Pfaffe voted and added 3 comments![]( "Open in Gerrit")
Votes added by Philip Pfaffe
| Code-Review | +1 |
3 comments
const blink::mojom::DeviceEmulationUpdateReason restore) override {}const ref?
const mojom::blink::DeviceEmulationUpdateReason reason) override;const ref?
const mojom::blink::DeviceEmulationUpdateReason reason) {const ref?
Related details
- Alex Rudenko
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Alex Rudenko (Gerrit)
Alex Rudenko added 3 comments![]( "Open in Gerrit")
const blink::mojom::DeviceEmulationUpdateReason restore) override {}Alex Rudenkoconst ref?
it is not compatible with the base class generated based on mojom so I think it's not possible?
const mojom::blink::DeviceEmulationUpdateReason reason) override;Alex Rudenkoconst ref?
ditto
const mojom::blink::DeviceEmulationUpdateReason reason) {Alex Rudenkoconst ref?
ditto
Related details
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
gwsq (Gerrit)
Message from gwsq![]( "Open in Gerrit")
From googleclient/chrome/chromium_gwsq/ipc/config.gwsq:
IPC: arthurs...@chromium.org
📎 It looks like you’re making a possibly security-sensitive change! 📎 IPC security review isn’t a rubberstamp, so your friendly security reviewer will need a fair amount of context to review your CL effectively. Please review your CL description and code comments to make sure they provide context for someone unfamiliar with your project/area. Pay special attention to where data comes from and which processes it flows between (and their privilege levels). Feel free to point your security reviewer at design docs, bugs, or other links if you can’t reasonably make a self-contained CL description. (Also see https://cbea.ms/git-commit/).
IPC reviewer(s): arthurs...@chromium.org
Note: IPC gwsq added no new reviewers; existing reviewers satisfied requirements!
Reviewer source(s):
arthurs...@chromium.org is from context(googleclient/chrome/chromium_gwsq/ipc/config.gwsq)
Related details
- Arthur Sonzogni
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Daniel Cheng (Gerrit)
Daniel Cheng added 2 comments![]( "Open in Gerrit")
EnableDeviceEmulation(DeviceEmulationParams parameters, DeviceEmulationUpdateReason reason);Nit: wrap at 80 (git cl format --mojom might fix this)
reason != mojom::blink::DeviceEmulationUpdateReason::kRestore) {I don't think it's entirely obvious why:
- we have to evict the memory cache at all to begin with
- why `kRestore` is exempt from this
I think comments here and perhaps different names for the reason variants might help?
Related details
- Alex Rudenko
- Arthur Sonzogni
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Arthur Sonzogni (Gerrit)
Arthur Sonzogni added 2 comments![]( "Open in Gerrit")
const blink::mojom::DeviceEmulationUpdateReason restore) override {}The parameter is named `reason` in every other part of the patch (the Mojo interface, implementation files, etc.). Renaming `restore` to `reason` here would improve consistency, even in a test mock.
reason != mojom::blink::DeviceEmulationUpdateReason::kRestore) {I don't think it's entirely obvious why:
- we have to evict the memory cache at all to begin with
- why `kRestore` is exempt from this
I think comments here and perhaps different names for the reason variants might help?
+1.
Related details
- Alex Rudenko
Alex Rudenko (Gerrit)
Alex Rudenko voted and added 3 comments![]( "Open in Gerrit")
Votes added by Alex Rudenko
| Commit-Queue | +1 |
3 comments
const blink::mojom::DeviceEmulationUpdateReason restore) override {}The parameter is named `reason` in every other part of the patch (the Mojo interface, implementation files, etc.). Renaming `restore` to `reason` here would improve consistency, even in a test mock.
Done
EnableDeviceEmulation(DeviceEmulationParams parameters, DeviceEmulationUpdateReason reason);Nit: wrap at 80 (git cl format --mojom might fix this)
Thanks, `git cl format --mojom` fixed it but there were a bunch of unrelated fixes (I reverted those). Curious if there is there a reason why mojom formatter does not run by default?
reason != mojom::blink::DeviceEmulationUpdateReason::kRestore) {Arthur SonzogniI don't think it's entirely obvious why:
- we have to evict the memory cache at all to begin with
- why `kRestore` is exempt from this
I think comments here and perhaps different names for the reason variants might help?
+1.
Thanks, I have added a comment here explaining the situation. I have not renamed the variable and enums so far to hear if you have any suggestions on what names would make it clearer?
Related details
- Arthur Sonzogni
- Daniel Cheng
Daniel Cheng (Gerrit)
Daniel Cheng added 1 comment![]( "Open in Gerrit")
reason != mojom::blink::DeviceEmulationUpdateReason::kRestore) {Arthur SonzogniI don't think it's entirely obvious why:
- we have to evict the memory cache at all to begin with
- why `kRestore` is exempt from this
I think comments here and perhaps different names for the reason variants might help?
Alex Rudenko+1.
Thanks, I have added a comment here explaining the situation. I have not renamed the variable and enums so far to hear if you have any suggestions on what names would make it clearer?
Maybe it should just be named for what it does: whether or not to invalidate the memory cache?
Related details
- Alex Rudenko
- Arthur Sonzogni
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Alex Rudenko (Gerrit)
Alex Rudenko added 1 comment![]( "Open in Gerrit")
reason != mojom::blink::DeviceEmulationUpdateReason::kRestore) {Arthur SonzogniI don't think it's entirely obvious why:
- we have to evict the memory cache at all to begin with
- why `kRestore` is exempt from this
I think comments here and perhaps different names for the reason variants might help?
Alex Rudenko+1.
Daniel ChengThanks, I have added a comment here explaining the situation. I have not renamed the variable and enums so far to hear if you have any suggestions on what names would make it clearer?
Maybe it should just be named for what it does: whether or not to invalidate the memory cache?
I checked and it does appear that memory cache behavior is the only piece of the per-process logic here so I renamed the enum accordingly:
```
enum DeviceEmulationCacheBehavior {
// Clear memory cache when emulation parameters change.
kClearCache,
// Keep the memory cache when emulation parameters change.
kKeepCache,
};
```
Related details
- Arthur Sonzogni
- Daniel Cheng
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Daniel Cheng (Gerrit)
Daniel Cheng voted and added 3 comments![]( "Open in Gerrit")
Votes added by Daniel Cheng
| Code-Review | +1 |
3 comments
// Keep the memory cache when emulation parameters change.It would be nice to give a brief example here of when a caller would use it. Something like:
"Typically used when committing a navigation in a new Document, which has to re-sync the device emulation state, but should not clobber the global memory cache since the parameters did not actually change."
DeviceEmulationParams parameters, DeviceEmulationCacheBehavior cache_behavior);Nit: wrapping :)
Related details
- Alex Rudenko
- Arthur Sonzogni
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Alex Rudenko (Gerrit)
Alex Rudenko added 2 comments![]( "Open in Gerrit")
// Keep the memory cache when emulation parameters change.It would be nice to give a brief example here of when a caller would use it. Something like:
"Typically used when committing a navigation in a new Document, which has to re-sync the device emulation state, but should not clobber the global memory cache since the parameters did not actually change."
Done, thanks!
DeviceEmulationParams parameters, DeviceEmulationCacheBehavior cache_behavior);Alex RudenkoNit: wrapping :)
Done
Related details
- Arthur Sonzogni
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Arthur Sonzogni (Gerrit)
Arthur Sonzogni voted Code-Review+1![]( "Open in Gerrit")
- Alex Rudenko
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Alex Rudenko (Gerrit)
Alex Rudenko voted Commit-Queue+2![]( "Open in Gerrit")
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Chromium LUCI CQ (Gerrit)
Chromium LUCI CQ submitted the change![]( "Open in Gerrit")
Change information
DevTools: do not clear memory cache when restoring emulation
With RenderDocument, the codepath that syncs device emulation
settings with new renderers runs for every navigation. That prevents
memory caches from working as expected on navigation (and reload).
This CL adds the indication of the reason for the device emulation
parameters update to skip the cache logic.
Drive-by: adds missing tests for the Network.restoredFromCache.
- M content/browser/devtools/protocol/emulation_handler.cc
- M content/browser/devtools/protocol/emulation_handler.h
- M content/public/test/fake_frame_widget.h
- M third_party/blink/public/mojom/page/widget.mojom
- M third_party/blink/renderer/core/exported/web_view_impl.cc
- M third_party/blink/renderer/core/exported/web_view_impl.h
- M third_party/blink/renderer/core/frame/screen_metrics_emulator.cc
- M third_party/blink/renderer/core/frame/screen_metrics_emulator.h
- M third_party/blink/renderer/core/frame/web_frame_widget_impl.cc
- M third_party/blink/renderer/core/frame/web_frame_widget_impl.h
- M third_party/blink/renderer/core/inspector/dev_tools_emulator.cc
- M third_party/blink/renderer/core/inspector/dev_tools_emulator.h
- A third_party/blink/web_tests/http/tests/inspector-protocol/network/request-served-from-cache-expected.txt
- A third_party/blink/web_tests/http/tests/inspector-protocol/network/request-served-from-cache-with-emulation-expected.txt
- A third_party/blink/web_tests/http/tests/inspector-protocol/network/request-served-from-cache-with-emulation.js
- A third_party/blink/web_tests/http/tests/inspector-protocol/network/request-served-from-cache.js
- A third_party/blink/web_tests/http/tests/inspector-protocol/network/resources/one-script.html
- A third_party/blink/web_tests/http/tests/inspector-protocol/network/resources/one-script.php
Code-Review: +1 by Daniel Cheng, +1 by Arthur Sonzogni, +1 by Philip Pfaffe
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |