[Toolbar] Don't capture on desktop [chromium/src : main]
Peilin Wang (Gerrit)
Peilin Wang added 1 comment![]( "Open in Gerrit")
Peilin WangDoes this cause any issues when, e.g. the tabstrip is hidden/shown (due to window width changes) or when entering/leaving floating window mode?
Patrick Nolandwidth change: At the moment, the tabstrip just fades in/out without the toolbar moving. As long as if the toolbar doesn't move, there won't be any issues.
floating window mode: ah I didn't know about this feature (good thing you're reviewing this CL!) how can I test this?
Peilin WangOn an AL device in floating window mode, click the app icon/caret in the top left to change window modes. In fullscreen mode, long press the notch at the top center of the screen to get the same menu.
Patrick NolandOk I just tried it, I don't see any issues.
Peilin WangInteresting; we force a capture for those scenarios so I thought we would need one, but perhaps that's not true on AL. Adding Wenyu for a second set of eyes.
+1 for Wenyu's review.
At least for the width change, the behavior is different. On tablets, the tabstrip disappears and the toolbar moves up. For desktop, the tabstrip just disappears and the toolbar doesn't move up because we can't have it overlap the app icon.
Related details
- Patrick Noland
- Wenyu Fu
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Wenyu Fu (Gerrit)
Wenyu Fu added 1 comment![]( "Open in Gerrit")
if (DeviceInfo.isDesktop()) {We should use some other signals than form factors. [TopControlsLockCoordinator](https://source.chromium.org/chromium/chromium/src/+/main:chrome/browser/ui/android/desktop_windowing/java/src/org/chromium/chrome/browser/ui/desktop_windowing/TopControlsLockCoordinator.java;l=27?q=-file:%5Eout%2F%20-file:%5Ethird_party%2F%20TopControlsLockCoordinator&ss=chromium%2Fchromium%2Fsrc) controls the behavior where toolbar is considered "locked"
Related details
- Patrick Noland
- Peilin Wang
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Wenyu Fu (Gerrit)
Wenyu Fu added 1 comment![]( "Open in Gerrit")
if (DeviceInfo.isDesktop()) {We should use some other signals than form factors. [TopControlsLockCoordinator](https://source.chromium.org/chromium/chromium/src/+/main:chrome/browser/ui/android/desktop_windowing/java/src/org/chromium/chrome/browser/ui/desktop_windowing/TopControlsLockCoordinator.java;l=27?q=-file:%5Eout%2F%20-file:%5Ethird_party%2F%20TopControlsLockCoordinator&ss=chromium%2Fchromium%2Fsrc) controls the behavior where toolbar is considered "locked"
*To clarify, I suggest if we can either expose `TopControlsLockCoordinator.shouldLock`, or `TopControlsStacker.isScrollingDisabled` and check such method, instead of checking form factor.
Sirisha Kavuluru (Gerrit)
Sirisha Kavuluru added 3 comments![]( "Open in Gerrit")
Adding Mark who was also considering this chnage
I don't know of any case where we show the composited toolbar on desktop (no toolbar swipe, toolbar doesn't scroll) so we should not do toolbar captures.One scenario I have noticed is for Contextual Search - steps to repro:
- Long press on text on web content --> shows contextual search bottom sheet
- Drag the bottom sheet up to show scrim --> I think here we use cc layers for toolbar and scrim
if (DeviceInfo.isDesktop()) {We should use some other signals than form factors. [TopControlsLockCoordinator](https://source.chromium.org/chromium/chromium/src/+/main:chrome/browser/ui/android/desktop_windowing/java/src/org/chromium/chrome/browser/ui/desktop_windowing/TopControlsLockCoordinator.java;l=27?q=-file:%5Eout%2F%20-file:%5Ethird_party%2F%20TopControlsLockCoordinator&ss=chromium%2Fchromium%2Fsrc) controls the behavior where toolbar is considered "locked"
+1 - we should disable this based on topcontrols locked signal.
Can we also flag guard this change?
Related details
- Mark Schillaci
- Patrick Noland
- Peilin Wang
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Peilin Wang (Gerrit)
Peilin Wang added 1 comment![]( "Open in Gerrit")
if (DeviceInfo.isDesktop()) {We should use some other signals than form factors. [TopControlsLockCoordinator](https://source.chromium.org/chromium/chromium/src/+/main:chrome/browser/ui/android/desktop_windowing/java/src/org/chromium/chrome/browser/ui/desktop_windowing/TopControlsLockCoordinator.java;l=27?q=-file:%5Eout%2F%20-file:%5Ethird_party%2F%20TopControlsLockCoordinator&ss=chromium%2Fchromium%2Fsrc) controls the behavior where toolbar is considered "locked"
TopControlsLockCoordinator.shouldLockTopControls returns true for desktop (I guess I"m just duplicating the logic in this CL. Should I check `isDesktop && shouldLockTopControls` instead, in case if the logic in shouldLockTopControls changes in the future?)
This was just meant to be a quick change to not capture on desktop (should be a strictly better change) If we want investigate cases on other form factors where we lock and don't need to capture, I can do that some other time. I think it would require more time to investigate correctness and impact on performance.
Related details
- Mark Schillaci
- Patrick Noland
- Wenyu Fu
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Peilin Wang (Gerrit)
Peilin Wang added 1 comment![]( "Open in Gerrit")
I don't know of any case where we show the composited toolbar on desktop (no toolbar swipe, toolbar doesn't scroll) so we should not do toolbar captures.One scenario I have noticed is for Contextual Search - steps to repro:
- Long press on text on web content --> shows contextual search bottom sheet
- Drag the bottom sheet up to show scrim --> I think here we use cc layers for toolbar and scrim
I can't get contextual search to open on desktop. Is it because I'm using an emulator and can't long press?
Related details
- Mark Schillaci
- Patrick Noland
- Sirisha Kavuluru
- Wenyu Fu
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Sirisha Kavuluru (Gerrit)
Sirisha Kavuluru added 1 comment![]( "Open in Gerrit")
I don't know of any case where we show the composited toolbar on desktop (no toolbar swipe, toolbar doesn't scroll) so we should not do toolbar captures.Peilin WangOne scenario I have noticed is for Contextual Search - steps to repro:
- Long press on text on web content --> shows contextual search bottom sheet
- Drag the bottom sheet up to show scrim --> I think here we use cc layers for toolbar and scrim
I can't get contextual search to open on desktop. Is it because I'm using an emulator and can't long press?
I think so - contextual search only shows up on touch and not with peripherals
Related details
- Mark Schillaci
- Patrick Noland
- Peilin Wang
- Wenyu Fu
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Wenyu Fu (Gerrit)
Wenyu Fu added 1 comment![]( "Open in Gerrit")
if (DeviceInfo.isDesktop()) {Peilin WangWe should use some other signals than form factors. [TopControlsLockCoordinator](https://source.chromium.org/chromium/chromium/src/+/main:chrome/browser/ui/android/desktop_windowing/java/src/org/chromium/chrome/browser/ui/desktop_windowing/TopControlsLockCoordinator.java;l=27?q=-file:%5Eout%2F%20-file:%5Ethird_party%2F%20TopControlsLockCoordinator&ss=chromium%2Fchromium%2Fsrc) controls the behavior where toolbar is considered "locked"
TopControlsLockCoordinator.shouldLockTopControls returns true for desktop (I guess I"m just duplicating the logic in this CL. Should I check `isDesktop && shouldLockTopControls` instead, in case if the logic in shouldLockTopControls changes in the future?)
This was just meant to be a quick change to not capture on desktop (should be a strictly better change) If we want investigate cases on other form factors where we lock and don't need to capture, I can do that some other time. I think it would require more time to investigate correctness and impact on performance.
I think the primary concern is this adds another `DeviceInfo.isDesktop()`.
I suggest if we can read the signal `shouldLockTopControls` with reasonable amount of work, we should just do that right now. This also makes the behavior more testable IMO
Related details
- Mark Schillaci
- Patrick Noland
- Peilin Wang
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Peilin Wang (Gerrit)
Peilin Wang added 1 comment![]( "Open in Gerrit")
if (DeviceInfo.isDesktop()) {Peilin WangWe should use some other signals than form factors. [TopControlsLockCoordinator](https://source.chromium.org/chromium/chromium/src/+/main:chrome/browser/ui/android/desktop_windowing/java/src/org/chromium/chrome/browser/ui/desktop_windowing/TopControlsLockCoordinator.java;l=27?q=-file:%5Eout%2F%20-file:%5Ethird_party%2F%20TopControlsLockCoordinator&ss=chromium%2Fchromium%2Fsrc) controls the behavior where toolbar is considered "locked"
Wenyu FuTopControlsLockCoordinator.shouldLockTopControls returns true for desktop (I guess I"m just duplicating the logic in this CL. Should I check `isDesktop && shouldLockTopControls` instead, in case if the logic in shouldLockTopControls changes in the future?)
This was just meant to be a quick change to not capture on desktop (should be a strictly better change) If we want investigate cases on other form factors where we lock and don't need to capture, I can do that some other time. I think it would require more time to investigate correctness and impact on performance.
I think the primary concern is this adds another `DeviceInfo.isDesktop()`.
I suggest if we can read the signal `shouldLockTopControls` with reasonable amount of work, we should just do that right now. This also makes the behavior more testable IMO
Ok, in that case we would also be effecting other form factors. I'll make the change and flag guard this (and hopefully we should see some perf wins)
Related details
- Mark Schillaci
- Patrick Noland
- Sirisha Kavuluru
- Wenyu Fu
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Mark Schillaci (Gerrit)
Mark Schillaci added 1 comment![]( "Open in Gerrit")
Just a random thought - we have an experiment about to run right now (currently going through QA before starting Finch rollout) where we change the snapshot dimensions for the toolbar, hidden behind the ToolbarSnapshotRefactor flag. This probably isn't a risky change, but it might be nice to put this behind the same flag (just in case) but also to get a measure of net performance improvement from this (specifically ram).
lmkwyt +cc eleanor as fyi
Related details
- Patrick Noland
- Peilin Wang
- Sirisha Kavuluru
- Wenyu Fu
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |