Android: enable short-edges cutout mode for fullscreen and standalone webapps [chromium/src : main]
Helmut Januschka (Gerrit)
Helmut Januschka added 4 comments![]( "Open in Gerrit")
Parent: 2e800ed4 (Roll Chromium Variations from HwVOWY6F5WmG4cDs-... to 85ywFykF1JAOJzfJO...)Helmut JanuschkaNit: The description says 'Use short-edges display cutout mode for fullscreen webapps', but the CL also enables edge-to-edge mode for standalone webapps. Consider updating the description to mention standalone webapps as well.
Done
/** Coordinator for shared functionality between Trusted Web Activities and webapps. */Helmut Januschkaultra nit: It would be nice to list the responsibilities here, e.g. entering an exiting immersive mode, making decisions based on whether the current web contents is in-scope of the app and thus should show in 'app mode', etc.
Done
if (!ChromeFeatureList.sWebAppShortEdgesCutoutMode.isEnabled()) {Helmut JanuschkaThere might be a behavior regression risk here?
When the feature flag is turned off, this returns false. In the past, for Webapp/WebAPK activities, it would return true when resolved display mode is FULLSCREEN. To preserve previous behavior when the flag is off, we should maybe check the flag at the very beginning and fall back to the previous logic if it is disabled?
```
if (!ChromeFeatureList.sWebAppShortEdgesCutoutMode.isEnabled()) {
return baseCustomTabActivity.getIntentDataProvider().getResolvedDisplayMode() == DisplayMode.FULLSCREEN;
}
```
omg, thanks, changed `isInBrowserFullscreen()` to preserve legacy behavior when `WebAppShortEdgesCutoutMode` is disabled.
int left = Math.max(0, systemBars.left);Helmut Januschkatests are failing here as systemBars is null? Maybe my suggestion was bad? Or we need to update the test mock?
this is indeed fallout from the `getInsets(Type.systemBars())` change.updated the mock! (should pass now)
Related details
- Daniel Murphy
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Daniel Murphy (Gerrit)
Daniel Murphy voted and added 2 comments![]( "Open in Gerrit")
Votes added by Daniel Murphy
| Code-Review | +1 |
2 comments
if (!baseCustomTabActivity.getIntentDataProvider().isWebappOrWebApkActivity()) {just to make sure I'm understanding - isInBrowserFullscreen - we say we aren't when we're not in a webapp or webapk. So it's not possible for the `baseCustomTabActivity.getIntentDataProvider().getResolvedDisplayMode()` to return FULLSCREEN if we are in a browser tab, then?
Related details
- Helmut Januschka
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. |
Helmut Januschka (Gerrit)
Helmut Januschka added 1 comment![]( "Open in Gerrit")
if (!baseCustomTabActivity.getIntentDataProvider().isWebappOrWebApkActivity()) {just to make sure I'm understanding - isInBrowserFullscreen - we say we aren't when we're not in a webapp or webapk. So it's not possible for the `baseCustomTabActivity.getIntentDataProvider().getResolvedDisplayMode()` to return FULLSCREEN if we are in a browser tab, then?
correct.it returns false before checking display mode unless the activity is a webapp/webapk activity, same as before this change.
Related details
- Charles Hager
- Jinsuk Kim
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Jinsuk Kim (Gerrit)
Jinsuk Kim added 2 comments![]( "Open in Gerrit")
peconn@ would you help review Immersive mode controller?
return mIntentDataProvider == null || !mIntentDataProvider.isPartialCustomTab();mIntentDataProvider should be non-null by this time - you may assumeNonNull
Related details
- Charles Hager
- Daniel Murphy
- Helmut Januschka
- Peter Conn
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. |
Daniel Murphy (Gerrit)
Daniel Murphy voted and added 1 comment![]( "Open in Gerrit")
Votes added by Daniel Murphy
| Code-Review | +1 |
- Charles Hager
- Helmut Januschka
- Peter Conn
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. |
Charles Hager (Gerrit)
Charles Hager added 1 comment![]( "Open in Gerrit")
Use short-edges display cutout mode for fullscreen and standaloneCan you include screenshots of before / after? Thanks!
Related details
- Helmut Januschka
- Peter Conn
Daniel Murphy (Gerrit)
Daniel Murphy added 1 comment![]( "Open in Gerrit")
There's a lot of changes happening in the CL now - let me know when you've thoroughly tested this and things have settled down
Related details
- Helmut Januschka
- Peter Conn
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. |
Helmut Januschka (Gerrit)
Helmut Januschka added 5 comments![]( "Open in Gerrit")
still lgtm I think?
there where small changes discovered on android13. but should be all sorted now.
There's a lot of changes happening in the CL now - let me know when you've thoroughly tested this and things have settled down
sorry for the noise, yes, there's a issue with view-port:cover i am trying to chase, i'll send replies to prev. comments once i know what the real fix is.
@dmu...@chromium.org reporter spotted two follow-ups on real devices: standalone PWAs were going edge-to-edge without `viewport-fit=cover`, and the status bar stayed painted with `theme_color` instead of going truly transparent. PS21 fixes both. also addressed open feedback, and posted links to screenshots.
sorry for the noise, next time i remove you from attention list first, just needed to upload to CL to pull from different machine, where my android13 (arm) is connected.
Use short-edges display cutout mode for fullscreen and standaloneCan you include screenshots of before / after? Thanks!
screenshots before/after, with and without notch see:
https://static.januschka.com/i-407420295/index.html
it's mostly "case 2" that was broken, and is fixed now.
there is also download links to pre-built apks, if you trust a random internet citizen to test it on any device/emu.
return mIntentDataProvider == null || !mIntentDataProvider.isPartialCustomTab();mIntentDataProvider should be non-null by this time - you may assumeNonNull
- Charles Hager
- Daniel Murphy
- Jinsuk Kim
- Peter Conn
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Peter Conn (Gerrit)
Peter Conn added 2 comments![]( "Open in Gerrit")
mEdgeToEdgeStateProvider.acquireSetDecorFitsSystemWindowToken();It may just be because I find "decor fits system window" to be a really hard to parse (and I appreciate this comes from the Android API), but would renaming these methods make everything a bit clearer?
Something like `acquireEdgeToEdgeToken` and `releaseEdgeToEdgeToken` (and then updating the name of this method to just be `updateEdgeToEdge`)?
mSetDecorFitsSystemWindowToken = TokenHolder.INVALID_TOKEN;Making this class deal with / know about `INVALID_TOKEN` is a bit awkward, instead could you add a `EdgeToEdgeStateProvider.TokenHolder` class (or `EdgeToEdgeTokenHolder` or something), so that all of this code becomes:
```
if (...) {
edgeToEdgeTokenHolder.acquireTokenIfEmpty();
} else {
edgeToEdgeTokenHolder.release();
}
```
Related details
- Charles Hager
- Daniel Murphy
- Helmut Januschka
- Jinsuk Kim
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. |
Charles Hager (Gerrit)
Charles Hager added 3 comments![]( "Open in Gerrit")
* embedder reports browser fullscreen mode.Ideally, could you add some more flag-guarding using sWebAppShortEdgesCutoutMode, just to be safe? It would be nice to be able to turn this off in case of any regressions.
int left = Math.max(0, systemBars.left);Are the system bar insets ever negative? Is that for floating windows or something? I don't remember negative insets, of the top of my head.
Related details
- Helmut Januschka
- Jinsuk Kim
- 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. |
Charles Hager (Gerrit)
Charles Hager added 1 comment![]( "Open in Gerrit")
mEdgeToEdgeStateProvider.acquireSetDecorFitsSystemWindowToken();It may just be because I find "decor fits system window" to be a really hard to parse (and I appreciate this comes from the Android API), but would renaming these methods make everything a bit clearer?
Something like `acquireEdgeToEdgeToken` and `releaseEdgeToEdgeToken` (and then updating the name of this method to just be `updateEdgeToEdge`)?
Sure, I could maybe look into that. I agree the naming from the Android API is a bit confusing.
Related details
- Helmut Januschka
- Jinsuk Kim
- 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. |
Helmut Januschka (Gerrit)
Helmut Januschka added 4 comments![]( "Open in Gerrit")
mEdgeToEdgeStateProvider.acquireSetDecorFitsSystemWindowToken();It may just be because I find "decor fits system window" to be a really hard to parse (and I appreciate this comes from the Android API), but would renaming these methods make everything a bit clearer?
Something like `acquireEdgeToEdgeToken` and `releaseEdgeToEdgeToken` (and then updating the name of this method to just be `updateEdgeToEdge`)?
done
mSetDecorFitsSystemWindowToken = TokenHolder.INVALID_TOKEN;Making this class deal with / know about `INVALID_TOKEN` is a bit awkward, instead could you add a `EdgeToEdgeStateProvider.TokenHolder` class (or `EdgeToEdgeTokenHolder` or something), so that all of this code becomes:
```
if (...) {
edgeToEdgeTokenHolder.acquireTokenIfEmpty();
} else {
edgeToEdgeTokenHolder.release();
}
```
Done. Added `EdgeToEdgeTokenHolder` in `ui/edge_to_edge/`
* embedder reports browser fullscreen mode.Ideally, could you add some more flag-guarding using sWebAppShortEdgesCutoutMode, just to be safe? It would be nice to be able to turn this off in case of any regressions.
added more guards, also added a about://Flags flag, hope it's ok in this case, explaining regular users how to use apk with dash dash flag is difficult
int left = Math.max(0, systemBars.left);Are the system bar insets ever negative? Is that for floating windows or something? I don't remember negative insets, of the top of my head.
left over from despratly trying to get android10 working, but its true,removed the unnecessary `Math.max(0, ...)` clamps in `getBrowserSafeAreaInsets()`.
the outer .max is still required.
Related details
- Charles Hager
- Peter Conn
- Wenyu Fu
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Peter Conn (Gerrit)
Peter Conn voted Code-Review+1![]( "Open in Gerrit")
- Charles Hager
- Helmut Januschka
- Wenyu Fu
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |