Gazal Agarwal (Gerrit)

unread,

12:23 AM (18 hours ago) 12:23 AM

to Hao Dong, Hao Dong, Chromium LUCI CQ, chromium...@chromium.org, davidj...@chromium.org, gogeral...@chromium.org, hanxi...@chromium.org, mattsimm...@chromium.org, meilian...@chromium.org, wychen...@chromium.org, yuezhang...@chromium.org, yusufo...@chromium.org

Attention needed from Hao Dong and Hao Dong

Gazal Agarwal added 1 comment

Patchset-level comments

File-level comment, Patchset 1 (Latest):

Gazal Agarwal . unresolved

@spdo...@chromium.org Mine is a smaller change, so I can rebase on yours if there are conflicts.

Open in Gerrit

Related details

Attention is currently required from:

Hao Dong
Hao Dong

Submit Requirements:

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.

Gerrit

satisfied_requirement

unsatisfied_requirement

open

diffy

Hao Dong (Gerrit)

unread,

1:07 AM (18 hours ago) 1:07 AM

to Gazal Agarwal, Hao Dong, Chromium LUCI CQ, chromium...@chromium.org, davidj...@chromium.org, gogeral...@chromium.org, hanxi...@chromium.org, mattsimm...@chromium.org, meilian...@chromium.org, wychen...@chromium.org, yuezhang...@chromium.org, yusufo...@chromium.org

Attention needed from Gazal Agarwal and Hao Dong

Hao Dong voted and added 3 comments

Votes added by Hao Dong

Code-Review

+1

3 comments

Patchset-level comments

File-level comment, Patchset 1 (Latest):

Gazal Agarwal . resolved

@spdo...@chromium.org Mine is a smaller change, so I can rebase on yours if there are conflicts.

Hao Dong

Thanks! They should not conflict if I updated mPinnedTabsRecyclerView to pinnedTabsRecyclerView back, but I could also rebase on this one if it lands first and there is any conflict.

Commit Message

Line 12, Patchset 1 (Latest):Bug: 550922629, 337042619

Hao Dong . unresolved

wrong bug numbers?

File chrome/android/features/tab_ui/java/src/org/chromium/chrome/browser/tasks/tab_management/vertical_tabs/VerticalTabListCoordinator.java

Line 754, Patchset 1 (Latest):

            } else {
                mPinnedTabsRecyclerView.setVisibility(View.VISIBLE);
                mPinnedTabsRecyclerView.setDisableItemAnimations(false);
            }

Hao Dong . unresolved

just curious: do we still need this with ` mPinnedTabsRecyclerView.runOnNextLayout(() -> mPinnedTabsRecyclerView.setDisableItemAnimations(false));` above?

Open in Gerrit

Related details

Attention is currently required from:

Gazal Agarwal
Hao Dong

Submit Requirements:

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.

Gerrit

satisfied_requirement

unsatisfied_requirement

open

diffy

Gazal Agarwal (Gerrit)

unread,

1:51 AM (17 hours ago) 1:51 AM

to Hao Dong, Hao Dong, Chromium LUCI CQ, chromium...@chromium.org, davidj...@chromium.org, gogeral...@chromium.org, hanxi...@chromium.org, mattsimm...@chromium.org, meilian...@chromium.org, wychen...@chromium.org, yuezhang...@chromium.org, yusufo...@chromium.org

Attention needed from Hao Dong and Hao Dong

Gazal Agarwal voted and added 3 comments

Votes added by Gazal Agarwal

Auto-Submit	+1
Commit-Queue	+1

3 comments

Patchset-level comments

File-level comment, Patchset 1:

Gazal Agarwal . resolved

@spdo...@chromium.org Mine is a smaller change, so I can rebase on yours if there are conflicts.

Hao Dong

Thanks! They should not conflict if I updated mPinnedTabsRecyclerView to pinnedTabsRecyclerView back, but I could also rebase on this one if it lands first and there is any conflict.

Gazal Agarwal

SG!

Commit Message

Line 12, Patchset 1:Bug: 550922629, 337042619

Hao Dong . resolved

wrong bug numbers?

Gazal Agarwal

Yeah pasted the second bug after the first 5 by mistake. Updated.

File chrome/android/features/tab_ui/java/src/org/chromium/chrome/browser/tasks/tab_management/vertical_tabs/VerticalTabListCoordinator.java

Line 754, Patchset 1:

            } else {
                mPinnedTabsRecyclerView.setVisibility(View.VISIBLE);
                mPinnedTabsRecyclerView.setDisableItemAnimations(false);
            }

Hao Dong . resolved

just curious: do we still need this with ` mPinnedTabsRecyclerView.runOnNextLayout(() -> mPinnedTabsRecyclerView.setDisableItemAnimations(false));` above?

Gazal Agarwal

That's a valid point. If the RV was already visible, `updatePinnedTabsVisibility` won't be called.

I realized I can just use a`swapAdapter`, so that all old views are recycled and the flash is gone. Updated.

Open in Gerrit

Related details

Attention is currently required from:

Hao Dong
Hao Dong

Submit Requirements:

Code-Coverage
Code-Owners
Code-Review
Review-Enforcement

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

satisfied_requirement

unsatisfied_requirement

open

diffy

Hao Dong (Gerrit)

unread,

11:56 AM (7 hours ago) 11:56 AM

to Gazal Agarwal, Hao Dong, Chromium LUCI CQ, chromium...@chromium.org, davidj...@chromium.org, gogeral...@chromium.org, hanxi...@chromium.org, mattsimm...@chromium.org, meilian...@chromium.org, wychen...@chromium.org, yuezhang...@chromium.org, yusufo...@chromium.org

Attention needed from Gazal Agarwal and Hao Dong

Hao Dong voted and added 1 comment

Votes added by Hao Dong

Code-Review

+1

1 comment

File chrome/android/features/tab_ui/java/src/org/chromium/chrome/browser/tasks/tab_management/vertical_tabs/VerticalTabListCoordinator.java

Line 748, Patchset 4 (Latest):

            mPinnedTabsRecyclerView.setVisibility(View.GONE);
            mPinnedTabsRecyclerView.swapAdapter(
                    mPinnedTabsAdapter, /* removeAndRecycleExistingViews= */ true);

Hao Dong . unresolved

super nit: guard this with `if (mPinnedTabsRecyclerView.getVisibility() != View.GONE)` to avoid redundant swapAdapter calls.

Open in Gerrit

Related details

Attention is currently required from:

Gazal Agarwal
Hao Dong

Submit Requirements:

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.

Gerrit

satisfied_requirement

unsatisfied_requirement

open

diffy

Gazal Agarwal (Gerrit)

unread,

12:23 PM (6 hours ago) 12:23 PM

to Hao Dong, Hao Dong, Chromium LUCI CQ, chromium...@chromium.org, davidj...@chromium.org, gogeral...@chromium.org, hanxi...@chromium.org, mattsimm...@chromium.org, meilian...@chromium.org, wychen...@chromium.org, yuezhang...@chromium.org, yusufo...@chromium.org

Attention needed from Hao Dong

Gazal Agarwal added 1 comment

File chrome/android/features/tab_ui/java/src/org/chromium/chrome/browser/tasks/tab_management/vertical_tabs/VerticalTabListCoordinator.java

Line 748, Patchset 4: mPinnedTabsRecyclerView.setVisibility(View.GONE);


            mPinnedTabsRecyclerView.swapAdapter(
                    mPinnedTabsAdapter, /* removeAndRecycleExistingViews= */ true);

Hao Dong . resolved

super nit: guard this with `if (mPinnedTabsRecyclerView.getVisibility() != View.GONE)` to avoid redundant swapAdapter calls.

Gazal Agarwal

Done, thanks.

Open in Gerrit

Related details

Attention is currently required from:

Hao Dong

Submit Requirements:

Code-Coverage
Code-Owners
Code-Review

Review-Enforcement

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

satisfied_requirement

open

diffy

Gazal Agarwal (Gerrit)

unread,

12:23 PM (6 hours ago) 12:23 PM

to Hao Dong, Hao Dong, Chromium LUCI CQ, chromium...@chromium.org, davidj...@chromium.org, gogeral...@chromium.org, hanxi...@chromium.org, mattsimm...@chromium.org, meilian...@chromium.org, wychen...@chromium.org, yuezhang...@chromium.org, yusufo...@chromium.org

Attention needed from Hao Dong

Gazal Agarwal voted Commit-Queue+2

Commit-Queue

+2

Open in Gerrit

Related details

Attention is currently required from:

Hao Dong

Submit Requirements:

Code-Coverage
Code-Owners
Code-Review
Review-Enforcement

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

satisfied_requirement

open

diffy

Chromium LUCI CQ (Gerrit)

unread,

1:23 PM (5 hours ago) 1:23 PM

to Gazal Agarwal, Hao Dong, Hao Dong, chromium...@chromium.org, davidj...@chromium.org, gogeral...@chromium.org, hanxi...@chromium.org, mattsimm...@chromium.org, meilian...@chromium.org, wychen...@chromium.org, yuezhang...@chromium.org, yusufo...@chromium.org

Chromium LUCI CQ submitted the change with unreviewed changes

Unreviewed changes

4 is the latest approved patch-set.
The change was submitted with unreviewed changes in the following files:

```
The name of the file: chrome/android/features/tab_ui/java/src/org/chromium/chrome/browser/tasks/tab_management/vertical_tabs/VerticalTabListCoordinator.java
Insertions: 5, Deletions: 3.

The diff is too large to show. Please review the diff.
```

Change information

Commit message:

[Vertical Tabs] Fix pinned tab flash on visibility change

Call swapAdapter on the pinned tabs RecyclerView when it becomes empty.
This detaches old views immediately to prevent them from animating on
screen when waking up from GONE. Additionally, adds clip animations to
pinned tabs RV.

Bug: 509226293, 537042619

Change-Id: I99c60d14e93c27d4a4049d4a20f4aaa846da47d4

Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/8127761

Commit-Queue: Gazal Agarwal <aga...@google.com>

Reviewed-by: Hao Dong <spdo...@google.com>

Auto-Submit: Gazal Agarwal <aga...@google.com>

Cr-Commit-Position: refs/heads/main@{#1665594}

Files:

M chrome/android/features/tab_ui/java/src/org/chromium/chrome/browser/tasks/tab_management/vertical_tabs/VerticalTabListCoordinator.java

Change size: S

Delta: 1 file changed, 13 insertions(+), 2 deletions(-)

Branch: refs/heads/main

Submit Requirements:

Code-Review: +1 by Hao Dong

Open in Gerrit

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

open

diffy

satisfied_requirement

Reply all

Reply to author

Forward

[Vertical Tabs] Fix pinned tab flash on visibility change [chromium/src : main]

Gazal Agarwal (Gerrit)

Gazal Agarwal added 1 comment

Related details

Hao Dong (Gerrit)

Hao Dong voted and added 3 comments

Votes added by Hao Dong

3 comments

Related details

Gazal Agarwal (Gerrit)

Gazal Agarwal voted and added 3 comments

Votes added by Gazal Agarwal

3 comments

Related details

Hao Dong (Gerrit)

Hao Dong voted and added 1 comment

Votes added by Hao Dong

1 comment

Related details

Gazal Agarwal (Gerrit)

Gazal Agarwal added 1 comment

Related details

Gazal Agarwal (Gerrit)

Gazal Agarwal voted Commit-Queue+2

Related details

Chromium LUCI CQ (Gerrit)

Chromium LUCI CQ submitted the change with unreviewed changes

Unreviewed changes

Change information