Default initialize edge-to-edge and its system insets without waiting for the first opted-in website [chromium/src : main]

2 views
Skip to first unread message

Charles Hager (Gerrit)

unread,
Jun 28, 2024, 8:03:54 PM (4 days ago) Jun 28
to Wenyu Fu, Theresa Sullivan, AyeAye, Chromium LUCI CQ, chromium...@chromium.org, hanxi...@chromium.org, blundell+...@chromium.org
Attention needed from Theresa Sullivan and Wenyu Fu

Charles Hager added 1 comment

Patchset-level comments
File-level comment, Patchset 2 (Latest):
Charles Hager . resolved

Will update this once this is merged to help fix the last failing tests
https://chromium-review.googlesource.com/c/chromium/src/+/5664105

Open in Gerrit

Related details

Attention is currently required from:
  • Theresa Sullivan
  • Wenyu Fu
Submit Requirements:
  • requirement satisfiedCode-Coverage
  • requirement is not satisfiedCode-Review
Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. DiffyGerrit
Gerrit-MessageType: comment
Gerrit-Project: chromium/src
Gerrit-Branch: main
Gerrit-Change-Id: I640b43cd76d28f36861379c55f2299518bb1617a
Gerrit-Change-Number: 5661560
Gerrit-PatchSet: 2
Gerrit-Owner: Charles Hager <clh...@google.com>
Gerrit-Reviewer: Charles Hager <clh...@google.com>
Gerrit-Reviewer: Theresa Sullivan <twell...@chromium.org>
Gerrit-Reviewer: Wenyu Fu <wen...@chromium.org>
Gerrit-Attention: Wenyu Fu <wen...@chromium.org>
Gerrit-Attention: Theresa Sullivan <twell...@chromium.org>
Gerrit-Comment-Date: Sat, 29 Jun 2024 00:03:40 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
satisfied_requirement
unsatisfied_requirement
open
diffy

Theresa Sullivan (Gerrit)

unread,
Jul 1, 2024, 12:49:18 PM (yesterday) Jul 1
to Charles Hager, Wenyu Fu, AyeAye, Chromium LUCI CQ, chromium...@chromium.org, hanxi...@chromium.org, blundell+...@chromium.org
Attention needed from Charles Hager and Wenyu Fu

Theresa Sullivan added 5 comments

File chrome/browser/ui/android/edge_to_edge/internal/java/src/org/chromium/chrome/browser/ui/edge_to_edge/EdgeToEdgeControllerImpl.java
Line 65, Patchset 2 (Latest): private boolean mIsOptedIntoEdgeToEdge;
Theresa Sullivan . unresolved

Let's add a JavaDoc for this. I think it'll be true when the current webpage is opted into edge to edge or (in the future) we're showing a native page that draws edge to edge

Line 129, Patchset 2 (Latest): mSystemInsets =
getSystemInsets(
WindowInsetsCompat.toWindowInsetsCompat(
mActivity.getWindow().getDecorView().getRootWindowInsets()));

mEdgeToEdgeOSWrapper.setDecorFitsSystemWindows(mActivity.getWindow(), false);
Theresa Sullivan . unresolved

Does the ordering of these calls matter? Or is getting the system insets using the Root View not impacted by #setDecorFitsSystemWindows? (I'm guessing it's the latter)

Line 280, Patchset 2 (Latest): // It's possible for toEdge to change more than once prior to the first time
// #handleWindowInsets is called. #handleWindowInsets will call #adjustEdges using
// the current mIsActivityToEdge once insets are available.
Theresa Sullivan . unresolved

Now that we initialize mSystemInsets in the ctor, I wonder if this comment is still relevant?

I added this in crrev.com/c/5378615 as part of an NPE fix

Line 313, Patchset 2 (Latest): private boolean shouldPadAdjusters() {
boolean keyboardIsVisible = mKeyboardInsets != null && mKeyboardInsets.bottom > 0;
return mIsOptedIntoEdgeToEdge && !keyboardIsVisible;
}
Theresa Sullivan . unresolved

I think we'll need to revisit this for the chin. For example, when the chin is scrolled off on a webpage that is not opted in we should be padding snackbars, while right now I suspect we aren't.

Maybe we could file a bug & add a TODO here?

File chrome/browser/ui/android/edge_to_edge/java/src/org/chromium/chrome/browser/ui/edge_to_edge/EdgeToEdgeController.java
Line 23, Patchset 2 (Latest): * @return whether the UI has opted into being drawn fully edge to edge. Note that a page may
* still draw edge-to-edge without this being true if the bottom chin is enabled and has
* been fully scrolled off.
Theresa Sullivan . unresolved
```suggestion
* @return Whether the current webpage (via opt-in) or native page is drawing edge to edge to on initial page load. Note that a page may
* still draw beneath the OS navigation bar without this being true if the bottom chin is enabled and has
* been fully scrolled off.
```
Open in Gerrit

Related details

Attention is currently required from:
  • Charles Hager
  • Wenyu Fu
Submit Requirements:
    • requirement satisfiedCode-Coverage
    • requirement is not satisfiedCode-Review
    • requirement is not satisfiedNo-Unresolved-Comments
    Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. DiffyGerrit
    Gerrit-MessageType: comment
    Gerrit-Project: chromium/src
    Gerrit-Branch: main
    Gerrit-Change-Id: I640b43cd76d28f36861379c55f2299518bb1617a
    Gerrit-Change-Number: 5661560
    Gerrit-PatchSet: 2
    Gerrit-Owner: Charles Hager <clh...@google.com>
    Gerrit-Reviewer: Charles Hager <clh...@google.com>
    Gerrit-Reviewer: Theresa Sullivan <twell...@chromium.org>
    Gerrit-Reviewer: Wenyu Fu <wen...@chromium.org>
    Gerrit-Attention: Wenyu Fu <wen...@chromium.org>
    Gerrit-Attention: Charles Hager <clh...@google.com>
    Gerrit-Comment-Date: Mon, 01 Jul 2024 16:49:04 +0000
    Gerrit-HasComments: Yes
    Gerrit-Has-Labels: No
    satisfied_requirement
    unsatisfied_requirement
    open
    diffy

    Wenyu Fu (Gerrit)

    unread,
    Jul 1, 2024, 2:02:22 PM (yesterday) Jul 1
    to Charles Hager, Theresa Sullivan, AyeAye, Chromium LUCI CQ, chromium...@chromium.org, hanxi...@chromium.org, blundell+...@chromium.org
    Attention needed from Charles Hager and Theresa Sullivan

    Wenyu Fu added 4 comments

    File chrome/browser/ui/android/edge_to_edge/internal/java/src/org/chromium/chrome/browser/ui/edge_to_edge/EdgeToEdgeControllerImpl.java
    Line 130, Patchset 2 (Latest): getSystemInsets(
    WindowInsetsCompat.toWindowInsetsCompat(
    mActivity.getWindow().getDecorView().getRootWindowInsets()));
    Wenyu Fu . unresolved

    nit: We should read the system insets from `mInsetObserver.getLastSeenRawInsets()` instead to avoid inconsistency during start up.

    Line 129, Patchset 2 (Latest): mSystemInsets =
    getSystemInsets(
    WindowInsetsCompat.toWindowInsetsCompat(
    mActivity.getWindow().getDecorView().getRootWindowInsets()));

    mEdgeToEdgeOSWrapper.setDecorFitsSystemWindows(mActivity.getWindow(), false);
    Theresa Sullivan . unresolved

    Does the ordering of these calls matter? Or is getting the system insets using the Root View not impacted by #setDecorFitsSystemWindows? (I'm guessing it's the latter)

    Wenyu Fu

    Technically it doesn't matter, since when the window draws E2E, the root view will receive a different window insets

    Line 134, Patchset 2 (Latest): mEdgeToEdgeOSWrapper.setDecorFitsSystemWindows(mActivity.getWindow(), false);
    Wenyu Fu . unresolved

    not blocking: We are always making this call when a controller instance is created. This is currently called when native is initialized:

    https://source.chromium.org/chromium/chromium/src/+/main:chrome/browser/ui/android/edge_to_edge/internal/java/src/org/chromium/chrome/browser/ui/edge_to_edge/EdgeToEdgeControllerFactory.java;l=52;bpv=1;bpt=1?q=-file:%5Eout%2F%20-file:%5Ethird_party%2F%20EdgeToEdgeControllerImpl&ss=chromium%2Fchromium%2Fsrc

    If we no longer have to observe the website opt-in state, I wonder if we can create the controller instance a bit earlier in the start up?

    File chrome/browser/ui/android/edge_to_edge/java/src/org/chromium/chrome/browser/ui/edge_to_edge/EdgeToEdgeController.java
    Line 27, Patchset 2 (Latest): boolean isOptedIntoEdgeToEdge();
    Wenyu Fu . unresolved

    naming nit: I think it might be more clear if we baked in "Page" / "Tab" in the method name to made clear the scope of E2E.

    Chrome is always drawing E2E after this controller is created, and this method is basically used for clients to adjust their UI based on the bottom insets on different pages.

    Open in Gerrit

    Related details

    Attention is currently required from:
    • Charles Hager
    • Theresa Sullivan
    Submit Requirements:
    • requirement satisfiedCode-Coverage
    • requirement is not satisfiedCode-Review
    • requirement is not satisfiedNo-Unresolved-Comments
    Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. DiffyGerrit
    Gerrit-MessageType: comment
    Gerrit-Project: chromium/src
    Gerrit-Branch: main
    Gerrit-Change-Id: I640b43cd76d28f36861379c55f2299518bb1617a
    Gerrit-Change-Number: 5661560
    Gerrit-PatchSet: 2
    Gerrit-Owner: Charles Hager <clh...@google.com>
    Gerrit-Reviewer: Charles Hager <clh...@google.com>
    Gerrit-Reviewer: Theresa Sullivan <twell...@chromium.org>
    Gerrit-Reviewer: Wenyu Fu <wen...@chromium.org>
    Gerrit-Attention: Charles Hager <clh...@google.com>
    Gerrit-Attention: Theresa Sullivan <twell...@chromium.org>
    Gerrit-Comment-Date: Mon, 01 Jul 2024 18:02:06 +0000
    Gerrit-HasComments: Yes
    Gerrit-Has-Labels: No
    Comment-In-Reply-To: Theresa Sullivan <twell...@chromium.org>
    satisfied_requirement
    unsatisfied_requirement
    open
    diffy

    Theresa Sullivan (Gerrit)

    unread,
    Jul 1, 2024, 2:18:16 PM (yesterday) Jul 1
    to Charles Hager, Wenyu Fu, AyeAye, Chromium LUCI CQ, chromium...@chromium.org, hanxi...@chromium.org, blundell+...@chromium.org
    Attention needed from Charles Hager

    Theresa Sullivan added 1 comment

    File chrome/browser/ui/android/edge_to_edge/internal/java/src/org/chromium/chrome/browser/ui/edge_to_edge/EdgeToEdgeControllerImpl.java
    Line 129, Patchset 2 (Latest): mSystemInsets =
    getSystemInsets(
    WindowInsetsCompat.toWindowInsetsCompat(
    mActivity.getWindow().getDecorView().getRootWindowInsets()));

    mEdgeToEdgeOSWrapper.setDecorFitsSystemWindows(mActivity.getWindow(), false);
    Theresa Sullivan . unresolved

    Does the ordering of these calls matter? Or is getting the system insets using the Root View not impacted by #setDecorFitsSystemWindows? (I'm guessing it's the latter)

    Wenyu Fu

    Technically it doesn't matter, since when the window draws E2E, the root view will receive a different window insets

    Theresa Sullivan

    Since this is doing #getDecorView, won't it include the system UI even without a call to #setDecorFitsSystemWindows?

    Open in Gerrit

    Related details

    Attention is currently required from:
    • Charles Hager
    Submit Requirements:
    • requirement satisfiedCode-Coverage
    • requirement is not satisfiedCode-Review
    • requirement is not satisfiedNo-Unresolved-Comments
    Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. DiffyGerrit
    Gerrit-MessageType: comment
    Gerrit-Project: chromium/src
    Gerrit-Branch: main
    Gerrit-Change-Id: I640b43cd76d28f36861379c55f2299518bb1617a
    Gerrit-Change-Number: 5661560
    Gerrit-PatchSet: 2
    Gerrit-Owner: Charles Hager <clh...@google.com>
    Gerrit-Reviewer: Charles Hager <clh...@google.com>
    Gerrit-Reviewer: Theresa Sullivan <twell...@chromium.org>
    Gerrit-Reviewer: Wenyu Fu <wen...@chromium.org>
    Gerrit-Attention: Charles Hager <clh...@google.com>
    Gerrit-Comment-Date: Mon, 01 Jul 2024 18:18:02 +0000
    Gerrit-HasComments: Yes
    Gerrit-Has-Labels: No
    Comment-In-Reply-To: Wenyu Fu <wen...@chromium.org>
    Comment-In-Reply-To: Theresa Sullivan <twell...@chromium.org>
    satisfied_requirement
    unsatisfied_requirement
    open
    diffy

    Charles Hager (Gerrit)

    unread,
    Jul 1, 2024, 5:14:23 PM (yesterday) Jul 1
    to Wenyu Fu, Theresa Sullivan, AyeAye, Chromium LUCI CQ, chromium...@chromium.org, hanxi...@chromium.org, blundell+...@chromium.org
    Attention needed from Theresa Sullivan and Wenyu Fu

    Charles Hager voted and added 7 comments

    Votes added by Charles Hager

    Commit-Queue+1

    7 comments

    File chrome/browser/ui/android/edge_to_edge/internal/java/src/org/chromium/chrome/browser/ui/edge_to_edge/EdgeToEdgeControllerImpl.java
    Line 65, Patchset 2: private boolean mIsOptedIntoEdgeToEdge;
    Theresa Sullivan . resolved

    Let's add a JavaDoc for this. I think it'll be true when the current webpage is opted into edge to edge or (in the future) we're showing a native page that draws edge to edge

    Charles Hager

    Sure - in my mind, "opted in" included native pages that support E2E. Happy to tweak the naming if you like!

    Line 130, Patchset 2: getSystemInsets(
    WindowInsetsCompat.toWindowInsetsCompat(
    mActivity.getWindow().getDecorView().getRootWindowInsets()));
    Wenyu Fu . unresolved

    nit: We should read the system insets from `mInsetObserver.getLastSeenRawInsets()` instead to avoid inconsistency during start up.

    Charles Hager

    Right now that method is annotated as returning a `@Nullable`, but is it actually `@Nullable`? It always seems to be initialized upon creating the `InsetObserver`

    Line 134, Patchset 2: mEdgeToEdgeOSWrapper.setDecorFitsSystemWindows(mActivity.getWindow(), false);
    Wenyu Fu . unresolved

    not blocking: We are always making this call when a controller instance is created. This is currently called when native is initialized:

    https://source.chromium.org/chromium/chromium/src/+/main:chrome/browser/ui/android/edge_to_edge/internal/java/src/org/chromium/chrome/browser/ui/edge_to_edge/EdgeToEdgeControllerFactory.java;l=52;bpv=1;bpt=1?q=-file:%5Eout%2F%20-file:%5Ethird_party%2F%20EdgeToEdgeControllerImpl&ss=chromium%2Fchromium%2Fsrc

    If we no longer have to observe the website opt-in state, I wonder if we can create the controller instance a bit earlier in the start up?

    Charles Hager

    How early would you like to set it? Originally IIUC the E2EControllerImpl was created independently of checking the website state, just that its method `#isEdgeToEdgeActive()` would be false until reaching an E2E opted-in page.

    Was there a specific piece of logic that was preventing an earlier initialization?

    Line 280, Patchset 2: // It's possible for toEdge to change more than once prior to the first time

    // #handleWindowInsets is called. #handleWindowInsets will call #adjustEdges using
    // the current mIsActivityToEdge once insets are available.
    Theresa Sullivan . resolved

    Now that we initialize mSystemInsets in the ctor, I wonder if this comment is still relevant?

    I added this in crrev.com/c/5378615 as part of an NPE fix

    Charles Hager

    I think this NPE shouldn't happen anymore, now that system insets are never null, so we should be good to remove this comment :)

    Line 313, Patchset 2: private boolean shouldPadAdjusters() {

    boolean keyboardIsVisible = mKeyboardInsets != null && mKeyboardInsets.bottom > 0;
    return mIsOptedIntoEdgeToEdge && !keyboardIsVisible;
    }
    Theresa Sullivan . resolved

    I think we'll need to revisit this for the chin. For example, when the chin is scrolled off on a webpage that is not opted in we should be padding snackbars, while right now I suspect we aren't.

    Maybe we could file a bug & add a TODO here?

    Charles Hager

    Yup, this will definitely need to be revisited. I can add a bug and TODO.

    File chrome/browser/ui/android/edge_to_edge/java/src/org/chromium/chrome/browser/ui/edge_to_edge/EdgeToEdgeController.java
    Line 23, Patchset 2: * @return whether the UI has opted into being drawn fully edge to edge. Note that a page may

    * still draw edge-to-edge without this being true if the bottom chin is enabled and has
    * been fully scrolled off.
    Theresa Sullivan . resolved
    ```suggestion
    * @return Whether the current webpage (via opt-in) or native page is drawing edge to edge to on initial page load. Note that a page may
    * still draw beneath the OS navigation bar without this being true if the bottom chin is enabled and has
    * been fully scrolled off.
    ```
    Charles Hager

    Done

    Line 27, Patchset 2: boolean isOptedIntoEdgeToEdge();
    Wenyu Fu . unresolved

    naming nit: I think it might be more clear if we baked in "Page" / "Tab" in the method name to made clear the scope of E2E.

    Chrome is always drawing E2E after this controller is created, and this method is basically used for clients to adjust their UI based on the bottom insets on different pages.

    Charles Hager

    Sure!

    Chrome is always drawing E2E after this controller is created, and this method is basically used for clients to adjust their UI based on the bottom insets on different pages.

    Yeah, this is exactly the subtlety I was trying to convey. Maybe `isPageOptedIntoEdgeToEdge` or `isTabContentOptedIntoEdgeToEdge`? Didn't want it to get too long...

    Open in Gerrit

    Related details

    Attention is currently required from:
    • Theresa Sullivan
    • Wenyu Fu
    Submit Requirements:
    • requirement satisfiedCode-Coverage
    • requirement is not satisfiedCode-Review
    • requirement is not satisfiedNo-Unresolved-Comments
    Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. DiffyGerrit
    Gerrit-MessageType: comment
    Gerrit-Project: chromium/src
    Gerrit-Branch: main
    Gerrit-Change-Id: I640b43cd76d28f36861379c55f2299518bb1617a
    Gerrit-Change-Number: 5661560
    Gerrit-PatchSet: 3
    Gerrit-Owner: Charles Hager <clh...@google.com>
    Gerrit-Reviewer: Charles Hager <clh...@google.com>
    Gerrit-Reviewer: Theresa Sullivan <twell...@chromium.org>
    Gerrit-Reviewer: Wenyu Fu <wen...@chromium.org>
    Gerrit-Attention: Wenyu Fu <wen...@chromium.org>
    Gerrit-Attention: Theresa Sullivan <twell...@chromium.org>
    Gerrit-Comment-Date: Mon, 01 Jul 2024 21:14:10 +0000
    Gerrit-HasComments: Yes
    Gerrit-Has-Labels: Yes
    satisfied_requirement
    unsatisfied_requirement
    open
    diffy

    findit-for-me@appspot.gserviceaccount.com (Gerrit)

    unread,
    Jul 1, 2024, 5:50:16 PM (yesterday) Jul 1
    to Charles Hager, Wenyu Fu, Theresa Sullivan, AyeAye, Chromium LUCI CQ, chromium...@chromium.org, hanxi...@chromium.org, blundell+...@chromium.org
    Attention needed from Theresa Sullivan and Wenyu Fu

    findit...@appspot.gserviceaccount.com voted Code-Coverage+1

    This change meets the code coverage requirements.

    Code-Coverage+1
    Open in Gerrit

    Related details

    Attention is currently required from:
    • Theresa Sullivan
    • Wenyu Fu
    Submit Requirements:
    • requirement satisfiedCode-Coverage
    • requirement is not satisfiedCode-Review
    • requirement is not satisfiedNo-Unresolved-Comments
    Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. DiffyGerrit
    Gerrit-MessageType: comment
    Gerrit-Project: chromium/src
    Gerrit-Branch: main
    Gerrit-Change-Id: I640b43cd76d28f36861379c55f2299518bb1617a
    Gerrit-Change-Number: 5661560
    Gerrit-PatchSet: 3
    Gerrit-Owner: Charles Hager <clh...@google.com>
    Gerrit-Reviewer: Charles Hager <clh...@google.com>
    Gerrit-Reviewer: Theresa Sullivan <twell...@chromium.org>
    Gerrit-Reviewer: Wenyu Fu <wen...@chromium.org>
    Gerrit-Attention: Wenyu Fu <wen...@chromium.org>
    Gerrit-Attention: Theresa Sullivan <twell...@chromium.org>
    Gerrit-Comment-Date: Mon, 01 Jul 2024 21:49:59 +0000
    Gerrit-HasComments: No
    Gerrit-Has-Labels: Yes
    satisfied_requirement
    unsatisfied_requirement
    open
    diffy

    Theresa Sullivan (Gerrit)

    unread,
    Jul 1, 2024, 5:52:59 PM (yesterday) Jul 1
    to Charles Hager, findit...@appspot.gserviceaccount.com, Wenyu Fu, AyeAye, Chromium LUCI CQ, chromium...@chromium.org, hanxi...@chromium.org, blundell+...@chromium.org
    Attention needed from Charles Hager and Wenyu Fu

    Theresa Sullivan voted and added 3 comments

    Votes added by Theresa Sullivan

    Code-Review+1

    3 comments

    Patchset-level comments
    File-level comment, Patchset 3 (Latest):
    Theresa Sullivan . resolved

    lgtm % open questions & Wenyu's lgtm

    File chrome/browser/ui/android/edge_to_edge/internal/java/src/org/chromium/chrome/browser/ui/edge_to_edge/EdgeToEdgeControllerImpl.java
    Line 129, Patchset 2: mSystemInsets =

    getSystemInsets(
    WindowInsetsCompat.toWindowInsetsCompat(
    mActivity.getWindow().getDecorView().getRootWindowInsets()));

    mEdgeToEdgeOSWrapper.setDecorFitsSystemWindows(mActivity.getWindow(), false);
    Theresa Sullivan . unresolved

    Does the ordering of these calls matter? Or is getting the system insets using the Root View not impacted by #setDecorFitsSystemWindows? (I'm guessing it's the latter)

    Wenyu Fu

    Technically it doesn't matter, since when the window draws E2E, the root view will receive a different window insets

    Theresa Sullivan

    Since this is doing #getDecorView, won't it include the system UI even without a call to #setDecorFitsSystemWindows?

    Theresa Sullivan

    Charles - ping on this open thread

    Line 134, Patchset 2: mEdgeToEdgeOSWrapper.setDecorFitsSystemWindows(mActivity.getWindow(), false);
    Wenyu Fu . unresolved

    not blocking: We are always making this call when a controller instance is created. This is currently called when native is initialized:

    https://source.chromium.org/chromium/chromium/src/+/main:chrome/browser/ui/android/edge_to_edge/internal/java/src/org/chromium/chrome/browser/ui/edge_to_edge/EdgeToEdgeControllerFactory.java;l=52;bpv=1;bpt=1?q=-file:%5Eout%2F%20-file:%5Ethird_party%2F%20EdgeToEdgeControllerImpl&ss=chromium%2Fchromium%2Fsrc

    If we no longer have to observe the website opt-in state, I wonder if we can create the controller instance a bit earlier in the start up?

    Charles Hager

    How early would you like to set it? Originally IIUC the E2EControllerImpl was created independently of checking the website state, just that its method `#isEdgeToEdgeActive()` would be false until reaching an E2E opted-in page.

    Was there a specific piece of logic that was preventing an earlier initialization?

    Theresa Sullivan

    Previously we didn't have anything that could draw edge to edge until after native initialized because of the website opt-in requirement.

    Now, we indeed might want to initialize sooner. I'd be happy with a P3 TODO for revisiting as technical debt unless we know of an issue early init would resolve.

    Open in Gerrit

    Related details

    Attention is currently required from:
    • Charles Hager
    • Wenyu Fu
    Submit Requirements:
    • requirement satisfiedCode-Coverage
    • requirement satisfiedCode-Review
    • requirement is not satisfiedNo-Unresolved-Comments
    Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. DiffyGerrit
    Gerrit-MessageType: comment
    Gerrit-Project: chromium/src
    Gerrit-Branch: main
    Gerrit-Change-Id: I640b43cd76d28f36861379c55f2299518bb1617a
    Gerrit-Change-Number: 5661560
    Gerrit-PatchSet: 3
    Gerrit-Owner: Charles Hager <clh...@google.com>
    Gerrit-Reviewer: Charles Hager <clh...@google.com>
    Gerrit-Reviewer: Theresa Sullivan <twell...@chromium.org>
    Gerrit-Reviewer: Wenyu Fu <wen...@chromium.org>
    Gerrit-Attention: Wenyu Fu <wen...@chromium.org>
    Gerrit-Attention: Charles Hager <clh...@google.com>
    Gerrit-Comment-Date: Mon, 01 Jul 2024 21:52:43 +0000
    Gerrit-HasComments: Yes
    Gerrit-Has-Labels: Yes
    Comment-In-Reply-To: Wenyu Fu <wen...@chromium.org>
    Comment-In-Reply-To: Charles Hager <clh...@google.com>
    Comment-In-Reply-To: Theresa Sullivan <twell...@chromium.org>
    satisfied_requirement
    unsatisfied_requirement
    open
    diffy

    Wenyu Fu (Gerrit)

    unread,
    Jul 1, 2024, 6:07:16 PM (yesterday) Jul 1
    to Charles Hager, Theresa Sullivan, findit...@appspot.gserviceaccount.com, AyeAye, Chromium LUCI CQ, chromium...@chromium.org, hanxi...@chromium.org, blundell+...@chromium.org
    Attention needed from Charles Hager

    Wenyu Fu voted and added 3 comments

    Votes added by Wenyu Fu

    Code-Review+1

    3 comments

    File chrome/browser/ui/android/edge_to_edge/internal/java/src/org/chromium/chrome/browser/ui/edge_to_edge/EdgeToEdgeControllerImpl.java
    Line 130, Patchset 2: getSystemInsets(
    WindowInsetsCompat.toWindowInsetsCompat(
    mActivity.getWindow().getDecorView().getRootWindowInsets()));
    Wenyu Fu . unresolved

    nit: We should read the system insets from `mInsetObserver.getLastSeenRawInsets()` instead to avoid inconsistency during start up.

    Charles Hager

    Right now that method is annotated as returning a `@Nullable`, but is it actually `@Nullable`? It always seems to be initialized upon creating the `InsetObserver`

    Wenyu Fu

    Technically `mLastSeenRawWindowInset` can be null based on the code, but I think in practice it's almost always ready by the time InsetObserver is created. When the first inset is ready, `mLastSeenRawWindowInset` should record the raw insets.

    So, there should never be a time where `mLastSeenRawWindowInset == null` while `getRootWindowInsets() != null`.

    We can also use an assert not null here to be safe, too

    File chrome/browser/ui/android/edge_to_edge/java/src/org/chromium/chrome/browser/ui/edge_to_edge/EdgeToEdgeController.java
    Line 25, Patchset 3 (Latest): * this being true if the bottom chin is enabled and has been fully scrolled off.
    Wenyu Fu . unresolved

    optional nit: Add code link to the BottomChinCoordinator

    Line 27, Patchset 2: boolean isOptedIntoEdgeToEdge();
    Wenyu Fu . unresolved

    naming nit: I think it might be more clear if we baked in "Page" / "Tab" in the method name to made clear the scope of E2E.

    Chrome is always drawing E2E after this controller is created, and this method is basically used for clients to adjust their UI based on the bottom insets on different pages.

    Charles Hager

    Sure!

    Chrome is always drawing E2E after this controller is created, and this method is basically used for clients to adjust their UI based on the bottom insets on different pages.

    Yeah, this is exactly the subtlety I was trying to convey. Maybe `isPageOptedIntoEdgeToEdge` or `isTabContentOptedIntoEdgeToEdge`? Didn't want it to get too long...

    Wenyu Fu

    I think `isPageOptedIntoEdgeToEdge`, or `isPageOptedInToEdge` maybe good enough... not feeling very strong here, it seems we only have one caller of this method:

    ```
    public void setNavigationBarScrimFraction(float fraction) {
    if (mEdgeToEdgeControllerSupplier.get() != null
    && mEdgeToEdgeControllerSupplier.get().isOptedIntoEdgeToEdge()) {
    return;
    }
    ```
    Open in Gerrit

    Related details

    Attention is currently required from:
    • Charles Hager
    Submit Requirements:
    • requirement satisfiedCode-Coverage
    • requirement satisfiedCode-Review
    • requirement is not satisfiedNo-Unresolved-Comments
    Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. DiffyGerrit
    Gerrit-MessageType: comment
    Gerrit-Project: chromium/src
    Gerrit-Branch: main
    Gerrit-Change-Id: I640b43cd76d28f36861379c55f2299518bb1617a
    Gerrit-Change-Number: 5661560
    Gerrit-PatchSet: 3
    Gerrit-Owner: Charles Hager <clh...@google.com>
    Gerrit-Reviewer: Charles Hager <clh...@google.com>
    Gerrit-Reviewer: Theresa Sullivan <twell...@chromium.org>
    Gerrit-Reviewer: Wenyu Fu <wen...@chromium.org>
    Gerrit-Attention: Charles Hager <clh...@google.com>
    Gerrit-Comment-Date: Mon, 01 Jul 2024 22:06:58 +0000
    satisfied_requirement
    unsatisfied_requirement
    open
    diffy

    Charles Hager (Gerrit)

    unread,
    Jul 1, 2024, 11:17:09 PM (yesterday) Jul 1
    to Wenyu Fu, Theresa Sullivan, findit...@appspot.gserviceaccount.com, AyeAye, Chromium LUCI CQ, chromium...@chromium.org, hanxi...@chromium.org, blundell+...@chromium.org
    Attention needed from Theresa Sullivan and Wenyu Fu

    Charles Hager added 5 comments

    File chrome/browser/ui/android/edge_to_edge/internal/java/src/org/chromium/chrome/browser/ui/edge_to_edge/EdgeToEdgeControllerImpl.java
    Line 130, Patchset 2: getSystemInsets(
    WindowInsetsCompat.toWindowInsetsCompat(
    mActivity.getWindow().getDecorView().getRootWindowInsets()));
    Wenyu Fu . resolved

    nit: We should read the system insets from `mInsetObserver.getLastSeenRawInsets()` instead to avoid inconsistency during start up.

    Charles Hager

    Right now that method is annotated as returning a `@Nullable`, but is it actually `@Nullable`? It always seems to be initialized upon creating the `InsetObserver`

    Wenyu Fu

    Technically `mLastSeenRawWindowInset` can be null based on the code, but I think in practice it's almost always ready by the time InsetObserver is created. When the first inset is ready, `mLastSeenRawWindowInset` should record the raw insets.

    So, there should never be a time where `mLastSeenRawWindowInset == null` while `getRootWindowInsets() != null`.

    We can also use an assert not null here to be safe, too

    Charles Hager

    Done

    Line 134, Patchset 2: mEdgeToEdgeOSWrapper.setDecorFitsSystemWindows(mActivity.getWindow(), false);
    Wenyu Fu . resolved

    not blocking: We are always making this call when a controller instance is created. This is currently called when native is initialized:

    https://source.chromium.org/chromium/chromium/src/+/main:chrome/browser/ui/android/edge_to_edge/internal/java/src/org/chromium/chrome/browser/ui/edge_to_edge/EdgeToEdgeControllerFactory.java;l=52;bpv=1;bpt=1?q=-file:%5Eout%2F%20-file:%5Ethird_party%2F%20EdgeToEdgeControllerImpl&ss=chromium%2Fchromium%2Fsrc

    If we no longer have to observe the website opt-in state, I wonder if we can create the controller instance a bit earlier in the start up?

    Charles Hager

    How early would you like to set it? Originally IIUC the E2EControllerImpl was created independently of checking the website state, just that its method `#isEdgeToEdgeActive()` would be false until reaching an E2E opted-in page.

    Was there a specific piece of logic that was preventing an earlier initialization?

    Theresa Sullivan

    Previously we didn't have anything that could draw edge to edge until after native initialized because of the website opt-in requirement.

    Now, we indeed might want to initialize sooner. I'd be happy with a P3 TODO for revisiting as technical debt unless we know of an issue early init would resolve.

    Charles Hager

    Sounds good, filed here crbug.com/350610430 - will add a TODO as well.

    Line 129, Patchset 2: mSystemInsets =
    getSystemInsets(
    WindowInsetsCompat.toWindowInsetsCompat(
    mActivity.getWindow().getDecorView().getRootWindowInsets()));

    mEdgeToEdgeOSWrapper.setDecorFitsSystemWindows(mActivity.getWindow(), false);
    Theresa Sullivan . unresolved

    Does the ordering of these calls matter? Or is getting the system insets using the Root View not impacted by #setDecorFitsSystemWindows? (I'm guessing it's the latter)

    Wenyu Fu

    Technically it doesn't matter, since when the window draws E2E, the root view will receive a different window insets

    Theresa Sullivan

    Since this is doing #getDecorView, won't it include the system UI even without a call to #setDecorFitsSystemWindows?

    Theresa Sullivan

    Charles - ping on this open thread

    Charles Hager

    My best guess is that the system insets would be consistent before and after setting the value, though I wasn't fully certain. I need to test this, but that means fixing up some of the E2E flag values. Will report as soon as I've verified!

    IIRC changing the decor fits value doesn't affect the system insets, it's more the rotations / changing between gesture nav vs 3 button nav

    File chrome/browser/ui/android/edge_to_edge/java/src/org/chromium/chrome/browser/ui/edge_to_edge/EdgeToEdgeController.java
    Line 25, Patchset 3 (Latest): * this being true if the bottom chin is enabled and has been fully scrolled off.
    Wenyu Fu . resolved

    optional nit: Add code link to the BottomChinCoordinator

    Charles Hager

    Done

    Line 27, Patchset 2: boolean isOptedIntoEdgeToEdge();
    Wenyu Fu . resolved

    naming nit: I think it might be more clear if we baked in "Page" / "Tab" in the method name to made clear the scope of E2E.

    Chrome is always drawing E2E after this controller is created, and this method is basically used for clients to adjust their UI based on the bottom insets on different pages.

    Charles Hager

    Sure!

    Chrome is always drawing E2E after this controller is created, and this method is basically used for clients to adjust their UI based on the bottom insets on different pages.

    Yeah, this is exactly the subtlety I was trying to convey. Maybe `isPageOptedIntoEdgeToEdge` or `isTabContentOptedIntoEdgeToEdge`? Didn't want it to get too long...

    Wenyu Fu

    I think `isPageOptedIntoEdgeToEdge`, or `isPageOptedInToEdge` maybe good enough... not feeling very strong here, it seems we only have one caller of this method:

    ```
    public void setNavigationBarScrimFraction(float fraction) {
    if (mEdgeToEdgeControllerSupplier.get() != null
    && mEdgeToEdgeControllerSupplier.get().isOptedIntoEdgeToEdge()) {
    return;
    }
    ```
    Charles Hager

    I'll likely just keep `isPageOptedIntoEdgeToEdge` for precision then...

    Open in Gerrit

    Related details

    Attention is currently required from:
    • Theresa Sullivan
    • Wenyu Fu
    Submit Requirements:
    • requirement satisfiedCode-Coverage
    • requirement satisfiedCode-Review
    • requirement is not satisfiedNo-Unresolved-Comments
    Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. DiffyGerrit
    Gerrit-MessageType: comment
    Gerrit-Project: chromium/src
    Gerrit-Branch: main
    Gerrit-Change-Id: I640b43cd76d28f36861379c55f2299518bb1617a
    Gerrit-Change-Number: 5661560
    Gerrit-PatchSet: 3
    Gerrit-Owner: Charles Hager <clh...@google.com>
    Gerrit-Reviewer: Charles Hager <clh...@google.com>
    Gerrit-Reviewer: Theresa Sullivan <twell...@chromium.org>
    Gerrit-Reviewer: Wenyu Fu <wen...@chromium.org>
    Gerrit-Attention: Wenyu Fu <wen...@chromium.org>
    Gerrit-Attention: Theresa Sullivan <twell...@chromium.org>
    Gerrit-Comment-Date: Tue, 02 Jul 2024 03:16:50 +0000
    Gerrit-HasComments: Yes
    Gerrit-Has-Labels: No
    Comment-In-Reply-To: Wenyu Fu <wen...@chromium.org>
    Comment-In-Reply-To: Charles Hager <clh...@google.com>
    Comment-In-Reply-To: Theresa Sullivan <twell...@chromium.org>
    satisfied_requirement
    unsatisfied_requirement
    open
    diffy

    Theresa Sullivan (Gerrit)

    unread,
    12:04 PM (12 hours ago) 12:04 PM
    to Charles Hager, Wenyu Fu, findit...@appspot.gserviceaccount.com, AyeAye, Chromium LUCI CQ, chromium...@chromium.org, hanxi...@chromium.org, blundell+...@chromium.org
    Attention needed from Charles Hager and Wenyu Fu

    Theresa Sullivan voted and added 2 comments

    Votes added by Theresa Sullivan

    Code-Review+1

    2 comments

    File chrome/browser/ui/android/edge_to_edge/internal/java/src/org/chromium/chrome/browser/ui/edge_to_edge/EdgeToEdgeControllerImpl.java
    Line 129, Patchset 2: mSystemInsets =
    getSystemInsets(
    WindowInsetsCompat.toWindowInsetsCompat(
    mActivity.getWindow().getDecorView().getRootWindowInsets()));

    mEdgeToEdgeOSWrapper.setDecorFitsSystemWindows(mActivity.getWindow(), false);
    Theresa Sullivan . unresolved

    Does the ordering of these calls matter? Or is getting the system insets using the Root View not impacted by #setDecorFitsSystemWindows? (I'm guessing it's the latter)

    Wenyu Fu

    Technically it doesn't matter, since when the window draws E2E, the root view will receive a different window insets

    Theresa Sullivan

    Since this is doing #getDecorView, won't it include the system UI even without a call to #setDecorFitsSystemWindows?

    Theresa Sullivan

    Charles - ping on this open thread

    Charles Hager

    My best guess is that the system insets would be consistent before and after setting the value, though I wasn't fully certain. I need to test this, but that means fixing up some of the E2E flag values. Will report as soon as I've verified!

    IIRC changing the decor fits value doesn't affect the system insets, it's more the rotations / changing between gesture nav vs 3 button nav

    Theresa Sullivan

    IIRC changing the decor fits value doesn't affect the system insets,

    I'd expect it to change the system insets observed by mWindowAndroid.getInsetObserver() since that's observing the insets for the root View of the Activity iirc. Before we call #setDecorFitsSystemWindows(false), the root view is set inside the status bar and nav bar so those should be 0. After we call #setDecorFitsSystemWindows(false), they'd be non-zero

    That said, it might not non-0 immediately after calling #setDecorFitsSystemWindows -- wouldn't be surprised if Android needs to perform a layout pass to draw the view into the sys ui space before the window insets start reporting an updated value.

    Line 132, Patchset 4 (Latest): assert mInsetObserver.getLastRawWindowInsets() != null
    : "The inset observer should have non-null insets by the time the"
    + " EdgeToEdgeControllerImpl is initialized.";
    mSystemInsets = getSystemInsets(mInsetObserver.getLastRawWindowInsets());
    Theresa Sullivan . resolved

    Looking at the impl for InsetObserver ctor, it relies on View#getRootWindowInsets which per the JavaDocs will return null of the View isn't attached.

    For use case, it's probably a fair assumption that the root view is attached by the time EdgeToEdgeControllerImpl is created. If we change init location, we may want to revisit this.

    https://developer.android.com/reference/android/view/View#getRootWindowInsets()

    Open in Gerrit

    Related details

    Attention is currently required from:
    • Charles Hager
    • Wenyu Fu
    Submit Requirements:
    • requirement satisfiedCode-Coverage
    • requirement satisfiedCode-Review
    • requirement is not satisfiedNo-Unresolved-Comments
    Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. DiffyGerrit
    Gerrit-MessageType: comment
    Gerrit-Project: chromium/src
    Gerrit-Branch: main
    Gerrit-Change-Id: I640b43cd76d28f36861379c55f2299518bb1617a
    Gerrit-Change-Number: 5661560
    Gerrit-PatchSet: 4
    Gerrit-Owner: Charles Hager <clh...@google.com>
    Gerrit-Reviewer: Charles Hager <clh...@google.com>
    Gerrit-Reviewer: Theresa Sullivan <twell...@chromium.org>
    Gerrit-Reviewer: Wenyu Fu <wen...@chromium.org>
    Gerrit-Attention: Wenyu Fu <wen...@chromium.org>
    Gerrit-Attention: Charles Hager <clh...@google.com>
    Gerrit-Comment-Date: Tue, 02 Jul 2024 16:04:38 +0000
    Gerrit-HasComments: Yes
    Gerrit-Has-Labels: Yes
    satisfied_requirement
    unsatisfied_requirement
    open
    diffy

    Charles Hager (Gerrit)

    unread,
    1:56 PM (10 hours ago) 1:56 PM
    to Theresa Sullivan, Wenyu Fu, findit...@appspot.gserviceaccount.com, AyeAye, Chromium LUCI CQ, chromium...@chromium.org, hanxi...@chromium.org, blundell+...@chromium.org
    Attention needed from Theresa Sullivan and Wenyu Fu

    Charles Hager added 1 comment

    File chrome/browser/ui/android/edge_to_edge/internal/java/src/org/chromium/chrome/browser/ui/edge_to_edge/EdgeToEdgeControllerImpl.java
    Line 129, Patchset 2: mSystemInsets =
    getSystemInsets(
    WindowInsetsCompat.toWindowInsetsCompat(
    mActivity.getWindow().getDecorView().getRootWindowInsets()));

    mEdgeToEdgeOSWrapper.setDecorFitsSystemWindows(mActivity.getWindow(), false);
    Theresa Sullivan . unresolved

    Does the ordering of these calls matter? Or is getting the system insets using the Root View not impacted by #setDecorFitsSystemWindows? (I'm guessing it's the latter)

    Wenyu Fu

    Technically it doesn't matter, since when the window draws E2E, the root view will receive a different window insets

    Theresa Sullivan

    Since this is doing #getDecorView, won't it include the system UI even without a call to #setDecorFitsSystemWindows?

    Theresa Sullivan

    Charles - ping on this open thread

    Charles Hager

    My best guess is that the system insets would be consistent before and after setting the value, though I wasn't fully certain. I need to test this, but that means fixing up some of the E2E flag values. Will report as soon as I've verified!

    IIRC changing the decor fits value doesn't affect the system insets, it's more the rotations / changing between gesture nav vs 3 button nav

    Theresa Sullivan

    IIRC changing the decor fits value doesn't affect the system insets,

    I'd expect it to change the system insets observed by mWindowAndroid.getInsetObserver() since that's observing the insets for the root View of the Activity iirc. Before we call #setDecorFitsSystemWindows(false), the root view is set inside the status bar and nav bar so those should be 0. After we call #setDecorFitsSystemWindows(false), they'd be non-zero

    That said, it might not non-0 immediately after calling #setDecorFitsSystemWindows -- wouldn't be surprised if Android needs to perform a layout pass to draw the view into the sys ui space before the window insets start reporting an updated value.

    Charles Hager

    I believe the root view is never set inside the system bars, or at least I'm seeing the insets from before the decor fits call match those afterwards.

    By default, does the `window.getDecorView().getRootView()` sit within the system bars?
    https://source.chromium.org/chromium/chromium/src/+/main:ui/android/java/src/org/chromium/ui/base/WindowAndroid.java?q=%22new%20ImmutableWeakReference%3C%3E(window.getDecorView().getRootView()));%22&ss=chromium%2Fchromium%2Fsrc

    Open in Gerrit

    Related details

    Attention is currently required from:
    • Theresa Sullivan
    • Wenyu Fu
    Submit Requirements:
    • requirement satisfiedCode-Coverage
    • requirement satisfiedCode-Review
    • requirement is not satisfiedNo-Unresolved-Comments
    Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. DiffyGerrit
    Gerrit-MessageType: comment
    Gerrit-Project: chromium/src
    Gerrit-Branch: main
    Gerrit-Change-Id: I640b43cd76d28f36861379c55f2299518bb1617a
    Gerrit-Change-Number: 5661560
    Gerrit-PatchSet: 4
    Gerrit-Owner: Charles Hager <clh...@google.com>
    Gerrit-Reviewer: Charles Hager <clh...@google.com>
    Gerrit-Reviewer: Theresa Sullivan <twell...@chromium.org>
    Gerrit-Reviewer: Wenyu Fu <wen...@chromium.org>
    Gerrit-Attention: Wenyu Fu <wen...@chromium.org>
    Gerrit-Attention: Theresa Sullivan <twell...@chromium.org>
    Gerrit-Comment-Date: Tue, 02 Jul 2024 17:55:57 +0000
    Gerrit-HasComments: Yes
    Gerrit-Has-Labels: No
    satisfied_requirement
    unsatisfied_requirement
    open
    diffy

    Theresa Sullivan (Gerrit)

    unread,
    2:05 PM (10 hours ago) 2:05 PM
    to Charles Hager, Wenyu Fu, findit...@appspot.gserviceaccount.com, AyeAye, Chromium LUCI CQ, chromium...@chromium.org, hanxi...@chromium.org, blundell+...@chromium.org
    Attention needed from Charles Hager and Wenyu Fu

    Theresa Sullivan added 1 comment

    File chrome/browser/ui/android/edge_to_edge/internal/java/src/org/chromium/chrome/browser/ui/edge_to_edge/EdgeToEdgeControllerImpl.java
    Theresa Sullivan

    By default, does the window.getDecorView().getRootView() sit within the system bars?

    If we're always getting non-0 insets then InsetObserver wouldn't really be telling us what insets currently apply to our app views... but maybe that was intentional.

    Wenyu, do you recall?

    Open in Gerrit

    Related details

    Attention is currently required from:
    • Charles Hager
    • Wenyu Fu
    Submit Requirements:
    • requirement satisfiedCode-Coverage
    • requirement satisfiedCode-Review
    • requirement is not satisfiedNo-Unresolved-Comments
    Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. DiffyGerrit
    Gerrit-MessageType: comment
    Gerrit-Project: chromium/src
    Gerrit-Branch: main
    Gerrit-Change-Id: I640b43cd76d28f36861379c55f2299518bb1617a
    Gerrit-Change-Number: 5661560
    Gerrit-PatchSet: 4
    Gerrit-Owner: Charles Hager <clh...@google.com>
    Gerrit-Reviewer: Charles Hager <clh...@google.com>
    Gerrit-Reviewer: Theresa Sullivan <twell...@chromium.org>
    Gerrit-Reviewer: Wenyu Fu <wen...@chromium.org>
    Gerrit-Attention: Wenyu Fu <wen...@chromium.org>
    Gerrit-Attention: Charles Hager <clh...@google.com>
    Gerrit-Comment-Date: Tue, 02 Jul 2024 18:05:01 +0000
    satisfied_requirement
    unsatisfied_requirement
    open
    diffy

    Wenyu Fu (Gerrit)

    unread,
    2:12 PM (10 hours ago) 2:12 PM
    to Charles Hager, Theresa Sullivan, findit...@appspot.gserviceaccount.com, AyeAye, Chromium LUCI CQ, chromium...@chromium.org, hanxi...@chromium.org, blundell+...@chromium.org
    Attention needed from Charles Hager and Theresa Sullivan

    Wenyu Fu voted and added 1 comment

    Votes added by Wenyu Fu

    Code-Review+1

    1 comment

    File chrome/browser/ui/android/edge_to_edge/internal/java/src/org/chromium/chrome/browser/ui/edge_to_edge/EdgeToEdgeControllerImpl.java
    Wenyu Fu

    I roughly recall that DecorView always has the window insets applied; during my debugging, by the time InsetsObserver is created, `window.getDecorView().getRootView()` is giving the DecorView itself (which make sense..., since `getRootView` "Finds the topmost view in the current view hierarchy."[1])

    For this code, I think it'll be technically reads more correct to set the SystemInsets after we do the call to `setDecorFitsSystemWindows`, though in practice it probably matter less (since the window layout pass is run in the next looper task, so window insets shouldn't change right after `setDecorFitsSystemWindows`)

    [1] https://developer.android.com/reference/android/view/View#getRootView()

    Open in Gerrit

    Related details

    Attention is currently required from:
    • Charles Hager
    • Theresa Sullivan
    Submit Requirements:
    • requirement satisfiedCode-Coverage
    • requirement satisfiedCode-Review
    • requirement is not satisfiedNo-Unresolved-Comments
    Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. DiffyGerrit
    Gerrit-MessageType: comment
    Gerrit-Project: chromium/src
    Gerrit-Branch: main
    Gerrit-Change-Id: I640b43cd76d28f36861379c55f2299518bb1617a
    Gerrit-Change-Number: 5661560
    Gerrit-PatchSet: 4
    Gerrit-Owner: Charles Hager <clh...@google.com>
    Gerrit-Reviewer: Charles Hager <clh...@google.com>
    Gerrit-Reviewer: Theresa Sullivan <twell...@chromium.org>
    Gerrit-Reviewer: Wenyu Fu <wen...@chromium.org>
    Gerrit-Attention: Charles Hager <clh...@google.com>
    Gerrit-Attention: Theresa Sullivan <twell...@chromium.org>
    Gerrit-Comment-Date: Tue, 02 Jul 2024 18:11:53 +0000
    Gerrit-HasComments: Yes
    Gerrit-Has-Labels: Yes
    satisfied_requirement
    unsatisfied_requirement
    open
    diffy

    Charles Hager (Gerrit)

    unread,
    3:05 PM (9 hours ago) 3:05 PM
    to Wenyu Fu, Theresa Sullivan, findit...@appspot.gserviceaccount.com, AyeAye, Chromium LUCI CQ, chromium...@chromium.org, hanxi...@chromium.org, blundell+...@chromium.org
    Attention needed from Theresa Sullivan

    Charles Hager added 1 comment

    File chrome/browser/ui/android/edge_to_edge/internal/java/src/org/chromium/chrome/browser/ui/edge_to_edge/EdgeToEdgeControllerImpl.java
    Line 129, Patchset 2: mSystemInsets =
    getSystemInsets(
    WindowInsetsCompat.toWindowInsetsCompat(
    mActivity.getWindow().getDecorView().getRootWindowInsets()));

    mEdgeToEdgeOSWrapper.setDecorFitsSystemWindows(mActivity.getWindow(), false);
    Theresa Sullivan . resolved
    Charles Hager

    I'm not seeing them ever change, even if I change the `decorFits` value later between true and false.

    I don't mind switching the order, but I wouldn't expect that to have any effect personally...

    Open in Gerrit

    Related details

    Attention is currently required from:
    • Theresa Sullivan
    Submit Requirements:
    • requirement satisfiedCode-Coverage
    • requirement satisfiedCode-Review
    Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. DiffyGerrit
    Gerrit-MessageType: comment
    Gerrit-Project: chromium/src
    Gerrit-Branch: main
    Gerrit-Change-Id: I640b43cd76d28f36861379c55f2299518bb1617a
    Gerrit-Change-Number: 5661560
    Gerrit-PatchSet: 4
    Gerrit-Owner: Charles Hager <clh...@google.com>
    Gerrit-Reviewer: Charles Hager <clh...@google.com>
    Gerrit-Reviewer: Theresa Sullivan <twell...@chromium.org>
    Gerrit-Reviewer: Wenyu Fu <wen...@chromium.org>
    Gerrit-Attention: Theresa Sullivan <twell...@chromium.org>
    Gerrit-Comment-Date: Tue, 02 Jul 2024 19:04:54 +0000
    Gerrit-HasComments: Yes
    Gerrit-Has-Labels: No
    satisfied_requirement
    open
    diffy

    Charles Hager (Gerrit)

    unread,
    3:05 PM (9 hours ago) 3:05 PM
    to Wenyu Fu, Theresa Sullivan, findit...@appspot.gserviceaccount.com, AyeAye, Chromium LUCI CQ, chromium...@chromium.org, hanxi...@chromium.org, blundell+...@chromium.org
    Attention needed from Theresa Sullivan

    Charles Hager voted Commit-Queue+2

    Commit-Queue+2
    Open in Gerrit

    Related details

    Attention is currently required from:
    • Theresa Sullivan
    Submit Requirements:
    • requirement satisfiedCode-Coverage
    • requirement satisfiedCode-Review
    Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. DiffyGerrit
    Gerrit-MessageType: comment
    Gerrit-Project: chromium/src
    Gerrit-Branch: main
    Gerrit-Change-Id: I640b43cd76d28f36861379c55f2299518bb1617a
    Gerrit-Change-Number: 5661560
    Gerrit-PatchSet: 4
    Gerrit-Owner: Charles Hager <clh...@google.com>
    Gerrit-Reviewer: Charles Hager <clh...@google.com>
    Gerrit-Reviewer: Theresa Sullivan <twell...@chromium.org>
    Gerrit-Reviewer: Wenyu Fu <wen...@chromium.org>
    Gerrit-Attention: Theresa Sullivan <twell...@chromium.org>
    Gerrit-Comment-Date: Tue, 02 Jul 2024 19:05:02 +0000
    Gerrit-HasComments: No
    Gerrit-Has-Labels: Yes
    satisfied_requirement
    open
    diffy

    findit-for-me@appspot.gserviceaccount.com (Gerrit)

    unread,
    8:55 PM (3 hours ago) 8:55 PM
    to Charles Hager, Wenyu Fu, Theresa Sullivan, AyeAye, Chromium LUCI CQ, chromium...@chromium.org, hanxi...@chromium.org, blundell+...@chromium.org

    This change meets the code coverage requirements.

    Code-Coverage+1
    Open in Gerrit

    Related details

    Attention set is empty
    Submit Requirements:
    • requirement satisfiedCode-Coverage
    • requirement satisfiedCode-Review
    Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. DiffyGerrit
    Gerrit-MessageType: comment
    Gerrit-Project: chromium/src
    Gerrit-Branch: main
    Gerrit-Change-Id: I640b43cd76d28f36861379c55f2299518bb1617a
    Gerrit-Change-Number: 5661560
    Gerrit-PatchSet: 5
    Gerrit-Owner: Charles Hager <clh...@google.com>
    Gerrit-Reviewer: Charles Hager <clh...@google.com>
    Gerrit-Reviewer: Theresa Sullivan <twell...@chromium.org>
    Gerrit-Reviewer: Wenyu Fu <wen...@chromium.org>
    Gerrit-Comment-Date: Wed, 03 Jul 2024 00:55:08 +0000
    Gerrit-HasComments: No
    Gerrit-Has-Labels: Yes
    satisfied_requirement
    open
    diffy
    Reply all
    Reply to author
    Forward
    0 new messages