Sinan Sahin (Gerrit)

unread,

Apr 30, 2026, 5:56:58 PM (10 days ago) Apr 30

to jingjing gu, Jinsuk Kim, Peter Conn, Chromium LUCI CQ, android-bu...@system.gserviceaccount.com, chromium...@chromium.org, Peter Beverloo, lizeb+watch...@chromium.org

Attention needed from Peter Conn and jingjing gu

Sinan Sahin voted Code-Review+1

Code-Review

+1

Open in Gerrit

Related details

Attention is currently required from:

Peter Conn
jingjing gu

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

Sinan Sahin (Gerrit)

unread,

May 4, 2026, 6:39:21 PM (6 days ago) May 4

to jingjing gu, Jinsuk Kim, Peter Conn, Chromium LUCI CQ, android-bu...@system.gserviceaccount.com, chromium...@chromium.org, Peter Beverloo, lizeb+watch...@chromium.org

Attention needed from Peter Conn and jingjing gu

Sinan Sahin added 2 comments

File chrome/android/java/src/org/chromium/chrome/browser/customtabs/BaseCustomTabRootUiCoordinator.java

Line 511, Patchset 33 (Parent):

            Callback<Runnable> softInputCallback =
                    ((PartialCustomTabDisplayManager) mCustomTabHeightStrategy)::onShowSoftInput;

            var tabController = mTabController.get();
            tabController.registerTabObserver(new PartialCustomTabTabObserver(softInputCallback));
            var csManager = mContextualSearchManagerSupplier.get();
            if (csManager != null) {
                tabController.registerTabObserver(
                        new EmptyTabObserver() {
                            @Override
                            public void didFirstVisuallyNonEmptyPaint(Tab tab) {
                                csManager.setCanHideAndroidBrowserControls(false);
                            }
                        });
            }

Sinan Sahin . unresolved

@jins...@chromium.org I just wanted to make sure we didn't miss anything here. I don't recognize this part.

File chrome/android/java/src/org/chromium/chrome/browser/customtabs/features/toolbar/CustomTabToolbar.java

Line 2263, Patchset 33 (Latest): switch (OPTIONAL_BUTTON_FOR_METRIC) {

Sinan Sahin . unresolved

Is this right? This is always UNKNOWN afaict.

Open in Gerrit

Related details

Attention is currently required from:

Peter Conn
jingjing gu

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

jingjing gu (Gerrit)

unread,

May 5, 2026, 1:06:57 AM (5 days ago) May 5

to Sinan Sahin, Jinsuk Kim, Peter Conn, Chromium LUCI CQ, android-bu...@system.gserviceaccount.com, chromium...@chromium.org, Peter Beverloo, lizeb+watch...@chromium.org

Attention needed from Peter Conn and Sinan Sahin

jingjing gu added 2 comments

File chrome/android/java/src/org/chromium/chrome/browser/customtabs/BaseCustomTabRootUiCoordinator.java

Line 511, Patchset 33 (Parent):

            Callback<Runnable> softInputCallback =
                    ((PartialCustomTabDisplayManager) mCustomTabHeightStrategy)::onShowSoftInput;

            var tabController = mTabController.get();
            tabController.registerTabObserver(new PartialCustomTabTabObserver(softInputCallback));
            var csManager = mContextualSearchManagerSupplier.get();
            if (csManager != null) {
                tabController.registerTabObserver(
                        new EmptyTabObserver() {
                            @Override
                            public void didFirstVisuallyNonEmptyPaint(Tab tab) {
                                csManager.setCanHideAndroidBrowserControls(false);
                            }
                        });
            }

Sinan Sahin . unresolved

@jins...@chromium.org I just wanted to make sure we didn't miss anything here. I don't recognize this part.

jingjing gu

Thanks for calling this out. I checked again: the code kept here is the old `if (ChromeFeatureList.sCctToolbarRefactor.isEnabled())` branch around lines 440-478, which used to return before the fallback below. The deleted block was the old disabled-flag fallback path.

File chrome/android/java/src/org/chromium/chrome/browser/customtabs/features/toolbar/CustomTabToolbar.java

Line 2263, Patchset 33 (Latest): switch (OPTIONAL_BUTTON_FOR_METRIC) {

Sinan Sahin . unresolved

Is this right? This is always UNKNOWN afaict.

jingjing gu

Good point. This used to be updated by the old optional-button path, which this CL removes, so it now effectively stays `UNKNOWN`.

Do you prefer simplifying this metric logic here, or keeping the metric cleanup for a follow-up CL?

Open in Gerrit

Related details

Attention is currently required from:

Peter Conn
Sinan Sahin

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

Jinsuk Kim (Gerrit)

unread,

May 5, 2026, 8:47:29 AM (5 days ago) May 5

to jingjing gu, Sinan Sahin, Peter Conn, Chromium LUCI CQ, android-bu...@system.gserviceaccount.com, chromium...@chromium.org, Peter Beverloo, lizeb+watch...@chromium.org

Attention needed from Peter Conn, Sinan Sahin and jingjing gu

Jinsuk Kim added 2 comments

File chrome/android/java/src/org/chromium/chrome/browser/customtabs/BaseCustomTabRootUiCoordinator.java

Line 511, Patchset 33 (Parent):

            Callback<Runnable> softInputCallback =
                    ((PartialCustomTabDisplayManager) mCustomTabHeightStrategy)::onShowSoftInput;

            var tabController = mTabController.get();
            tabController.registerTabObserver(new PartialCustomTabTabObserver(softInputCallback));
            var csManager = mContextualSearchManagerSupplier.get();
            if (csManager != null) {
                tabController.registerTabObserver(
                        new EmptyTabObserver() {
                            @Override
                            public void didFirstVisuallyNonEmptyPaint(Tab tab) {
                                csManager.setCanHideAndroidBrowserControls(false);
                            }
                        });
            }

Sinan Sahin . unresolved

@jins...@chromium.org I just wanted to make sure we didn't miss anything here. I don't recognize this part.

jingjing gu

Thanks for calling this out. I checked again: the code kept here is the old `if (ChromeFeatureList.sCctToolbarRefactor.isEnabled())` branch around lines 440-478, which used to return before the fallback below. The deleted block was the old disabled-flag fallback path.

Jinsuk Kim

Thanks for catching this part. It it still required code, accidentally skipped with the refactor flag. PCCT usage is relatively low and this path is also rarely taken but it is important logic to sort out the conflict between contextual search bottom sheet and PCCT. Please keep this in the new code.

File chrome/android/java/src/org/chromium/chrome/browser/customtabs/features/toolbar/CustomTabToolbar.java

Line 2263, Patchset 33 (Latest): switch (OPTIONAL_BUTTON_FOR_METRIC) {

Sinan Sahin . unresolved

Is this right? This is always UNKNOWN afaict.

jingjing gu

Good point. This used to be updated by the old optional-button path, which this CL removes, so it now effectively stays `UNKNOWN`.
Do you prefer simplifying this metric logic here, or keeping the metric cleanup for a follow-up CL?

Jinsuk Kim

The entire |logActionButtonComboMetric| can be removed as it was added to do the analysis in stable exp. Not needed any more.

Open in Gerrit

Related details

Attention is currently required from:

Peter Conn
Sinan Sahin
jingjing gu

satisfied_requirement

unsatisfied_requirement

open

diffy

jingjing gu (Gerrit)

unread,

May 5, 2026, 12:46:00 PM (5 days ago) May 5

to Sinan Sahin, Jinsuk Kim, Peter Conn, Chromium LUCI CQ, android-bu...@system.gserviceaccount.com, chromium...@chromium.org, Peter Beverloo, lizeb+watch...@chromium.org

Attention needed from Jinsuk Kim, Peter Conn and Sinan Sahin

jingjing gu added 2 comments

File chrome/android/java/src/org/chromium/chrome/browser/customtabs/BaseCustomTabRootUiCoordinator.java

Line 511, Patchset 33 (Parent):

            Callback<Runnable> softInputCallback =
                    ((PartialCustomTabDisplayManager) mCustomTabHeightStrategy)::onShowSoftInput;

            var tabController = mTabController.get();
            tabController.registerTabObserver(new PartialCustomTabTabObserver(softInputCallback));
            var csManager = mContextualSearchManagerSupplier.get();
            if (csManager != null) {
                tabController.registerTabObserver(
                        new EmptyTabObserver() {
                            @Override
                            public void didFirstVisuallyNonEmptyPaint(Tab tab) {
                                csManager.setCanHideAndroidBrowserControls(false);
                            }
                        });
            }

Sinan Sahin . unresolved

@jins...@chromium.org I just wanted to make sure we didn't miss anything here. I don't recognize this part.

jingjing gu

Thanks for calling this out. I checked again: the code kept here is the old `if (ChromeFeatureList.sCctToolbarRefactor.isEnabled())` branch around lines 440-478, which used to return before the fallback below. The deleted block was the old disabled-flag fallback path.

Jinsuk Kim

Thanks for catching this part. It it still required code, accidentally skipped with the refactor flag. PCCT usage is relatively low and this path is also rarely taken but it is important logic to sort out the conflict between contextual search bottom sheet and PCCT. Please keep this in the new code.

jingjing gu

Thanks, I kept the refactored toolbar initialization path and only carried over the PartialCustomTab contextual search observer setup from the old fallback, since that logic is still required.

Could you please take another look?

File chrome/android/java/src/org/chromium/chrome/browser/customtabs/features/toolbar/CustomTabToolbar.java

Line 2263, Patchset 33: switch (OPTIONAL_BUTTON_FOR_METRIC) {

Sinan Sahin . unresolved

Is this right? This is always UNKNOWN afaict.

jingjing gu

Good point. This used to be updated by the old optional-button path, which this CL removes, so it now effectively stays `UNKNOWN`.
Do you prefer simplifying this metric logic here, or keeping the metric cleanup for a follow-up CL?

Jinsuk Kim

The entire |logActionButtonComboMetric| can be removed as it was added to do the analysis in stable exp. Not needed any more.

jingjing gu

Thanks, removed the obsolete action button combo metric code from CustomTabToolbar.

Open in Gerrit

Related details

Attention is currently required from:

Jinsuk Kim
Peter Conn
Sinan Sahin

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

Jinsuk Kim (Gerrit)

unread,

May 6, 2026, 8:24:02 AM (4 days ago) May 6

to jingjing gu, Sinan Sahin, Peter Conn, Chromium LUCI CQ, android-bu...@system.gserviceaccount.com, chromium...@chromium.org, Peter Beverloo, lizeb+watch...@chromium.org

Attention needed from Peter Conn, Sinan Sahin and jingjing gu

Jinsuk Kim voted and added 4 comments

Votes added by Jinsuk Kim

Code-Review

+1

4 comments

Patchset-level comments

File-level comment, Patchset 36 (Latest):

Jinsuk Kim . resolved

Thanks for addressing the feedback. LGTM % one more

File chrome/android/java/src/org/chromium/chrome/browser/customtabs/BaseCustomTabRootUiCoordinator.java

Line 511, Patchset 33 (Parent):

            Callback<Runnable> softInputCallback =
                    ((PartialCustomTabDisplayManager) mCustomTabHeightStrategy)::onShowSoftInput;

            var tabController = mTabController.get();
            tabController.registerTabObserver(new PartialCustomTabTabObserver(softInputCallback));
            var csManager = mContextualSearchManagerSupplier.get();
            if (csManager != null) {
                tabController.registerTabObserver(
                        new EmptyTabObserver() {
                            @Override
                            public void didFirstVisuallyNonEmptyPaint(Tab tab) {
                                csManager.setCanHideAndroidBrowserControls(false);
                            }
                        });
            }

Sinan Sahin . resolved

@jins...@chromium.org I just wanted to make sure we didn't miss anything here. I don't recognize this part.

jingjing gu

Thanks for calling this out. I checked again: the code kept here is the old `if (ChromeFeatureList.sCctToolbarRefactor.isEnabled())` branch around lines 440-478, which used to return before the fallback below. The deleted block was the old disabled-flag fallback path.

Jinsuk Kim

Thanks for catching this part. It it still required code, accidentally skipped with the refactor flag. PCCT usage is relatively low and this path is also rarely taken but it is important logic to sort out the conflict between contextual search bottom sheet and PCCT. Please keep this in the new code.

jingjing gu

Thanks, I kept the refactored toolbar initialization path and only carried over the PartialCustomTab contextual search observer setup from the old fallback, since that logic is still required.
Could you please take another look?

Jinsuk Kim

Acknowledged

File chrome/android/java/src/org/chromium/chrome/browser/customtabs/features/toolbar/CustomTabToolbar.java

Line 345, Patchset 36 (Parent): // LINT.ThenChange(//tools/metrics/histograms/metadata/custom_tabs/enums.xml:CustomTabsToolbarButtons)

Jinsuk Kim . unresolved

You also need to update (remove) tools/metrics/histograms/metadata/custom_tabs/enums.xml:CustomTabsToolbarButtons

Line 2263, Patchset 33: switch (OPTIONAL_BUTTON_FOR_METRIC) {

Sinan Sahin . resolved

Is this right? This is always UNKNOWN afaict.

jingjing gu

Good point. This used to be updated by the old optional-button path, which this CL removes, so it now effectively stays `UNKNOWN`.
Do you prefer simplifying this metric logic here, or keeping the metric cleanup for a follow-up CL?

Jinsuk Kim

The entire |logActionButtonComboMetric| can be removed as it was added to do the analysis in stable exp. Not needed any more.

jingjing gu

Thanks, removed the obsolete action button combo metric code from CustomTabToolbar.

Jinsuk Kim

Acknowledged

Open in Gerrit

Related details

Attention is currently required from:

Peter Conn
Sinan Sahin
jingjing gu

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

jingjing gu (Gerrit)

unread,

May 6, 2026, 9:16:35 AM (4 days ago) May 6

to Chromium Metrics Reviews, Jinsuk Kim, Sinan Sahin, Peter Conn, Chromium LUCI CQ, android-bu...@system.gserviceaccount.com, chromium...@chromium.org, Peter Beverloo, asvitkine...@chromium.org, lizeb+watch...@chromium.org

Attention needed from Jinsuk Kim, Peter Conn and Sinan Sahin

jingjing gu added 1 comment

File chrome/android/java/src/org/chromium/chrome/browser/customtabs/features/toolbar/CustomTabToolbar.java

Line 345, Patchset 36 (Parent): // LINT.ThenChange(//tools/metrics/histograms/metadata/custom_tabs/enums.xml:CustomTabsToolbarButtons)

Jinsuk Kim . unresolved

You also need to update (remove) tools/metrics/histograms/metadata/custom_tabs/enums.xml:CustomTabsToolbarButtons

jingjing gu

thanks, fixed. Removed CustomTabsToolbarButtons and the corresponding CustomTab.AdaptiveToolbarButton.ActionButtons histogram entry

Open in Gerrit

Related details

Attention is currently required from:

Jinsuk Kim
Peter Conn
Sinan Sahin

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

Jinsuk Kim (Gerrit)

unread,

May 6, 2026, 10:01:43 AM (4 days ago) May 6

to jingjing gu, Chromium Metrics Reviews, Sinan Sahin, Peter Conn, Chromium LUCI CQ, android-bu...@system.gserviceaccount.com, chromium...@chromium.org, Peter Beverloo, asvitkine...@chromium.org, lizeb+watch...@chromium.org

Attention needed from Peter Conn, Sinan Sahin and jingjing gu

Jinsuk Kim voted and added 1 comment

Votes added by Jinsuk Kim

Code-Review

+1

1 comment

Patchset-level comments

File-level comment, Patchset 38 (Latest):

Jinsuk Kim . resolved

Thanks for handling the feedback. LGTM again

Open in Gerrit

Related details

Attention is currently required from:

Peter Conn
Sinan Sahin
jingjing gu

satisfied_requirement

unsatisfied_requirement

open

diffy

jingjing gu (Gerrit)

unread,

May 6, 2026, 11:17:12 PM (4 days ago) May 6

to Jinsuk Kim, Chromium Metrics Reviews, Sinan Sahin, Peter Conn, Chromium LUCI CQ, android-bu...@system.gserviceaccount.com, chromium...@chromium.org, Peter Beverloo, asvitkine...@chromium.org, lizeb+watch...@chromium.org

Attention needed from Peter Conn and Sinan Sahin

jingjing gu added 1 comment

File chrome/android/java/src/org/chromium/chrome/browser/customtabs/features/toolbar/CustomTabToolbar.java

Line 345, Patchset 36 (Parent): // LINT.ThenChange(//tools/metrics/histograms/metadata/custom_tabs/enums.xml:CustomTabsToolbarButtons)

Jinsuk Kim . resolved

You also need to update (remove) tools/metrics/histograms/metadata/custom_tabs/enums.xml:CustomTabsToolbarButtons

jingjing gu

thanks, fixed. Removed CustomTabsToolbarButtons and the corresponding CustomTab.AdaptiveToolbarButton.ActionButtons histogram entry

jingjing gu

Marked as resolved.

Open in Gerrit

Related details

Attention is currently required from:

Peter Conn
Sinan Sahin

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

Peter Conn (Gerrit)

unread,

May 7, 2026, 3:37:26 AM (3 days ago) May 7

to jingjing gu, Jinsuk Kim, Chromium Metrics Reviews, Sinan Sahin, Chromium LUCI CQ, android-bu...@system.gserviceaccount.com, chromium...@chromium.org, Peter Beverloo, asvitkine...@chromium.org, lizeb+watch...@chromium.org

Attention needed from Sinan Sahin and jingjing gu

Peter Conn voted Code-Review+1

Code-Review

+1

Open in Gerrit

Related details

Attention is currently required from:

Sinan Sahin
jingjing gu

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

Sinan Sahin (Gerrit)

unread,

May 7, 2026, 4:42:26 PM (3 days ago) May 7

to jingjing gu, Peter Conn, Jinsuk Kim, Chromium Metrics Reviews, Chromium LUCI CQ, android-bu...@system.gserviceaccount.com, chromium...@chromium.org, Peter Beverloo, asvitkine...@chromium.org, lizeb+watch...@chromium.org

Attention needed from jingjing gu

Sinan Sahin voted and added 1 comment

Votes added by Sinan Sahin

Commit-Queue

+1

1 comment

File chrome/android/java/src/org/chromium/chrome/browser/customtabs/BaseCustomTabRootUiCoordinator.java

Line 487, Patchset 38 (Latest): if (omniboxParams != null && shouldEnableOmnibox) {

Sinan Sahin . unresolved

nit: We set omniboxParams only if shouldEnableOmnibox is true, so this can be simplified.

Open in Gerrit

Related details

Attention is currently required from:

jingjing gu

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

Sinan Sahin (Gerrit)

unread,

May 7, 2026, 4:42:32 PM (3 days ago) May 7

to jingjing gu, Peter Conn, Jinsuk Kim, Chromium Metrics Reviews, Chromium LUCI CQ, android-bu...@system.gserviceaccount.com, chromium...@chromium.org, Peter Beverloo, asvitkine...@chromium.org, lizeb+watch...@chromium.org

Attention needed from jingjing gu

Sinan Sahin voted

Code-Review	+1
Commit-Queue	+0

Open in Gerrit

Related details

Attention is currently required from:

jingjing gu

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

jingjing gu (Gerrit)

unread,

May 7, 2026, 7:38:39 PM (3 days ago) May 7

to Sinan Sahin, Peter Conn, Jinsuk Kim, Chromium Metrics Reviews, Chromium LUCI CQ, android-bu...@system.gserviceaccount.com, chromium...@chromium.org, Peter Beverloo, asvitkine...@chromium.org, lizeb+watch...@chromium.org

Attention needed from Jinsuk Kim, Peter Conn and Sinan Sahin

jingjing gu added 1 comment

File chrome/android/java/src/org/chromium/chrome/browser/customtabs/BaseCustomTabRootUiCoordinator.java

Line 487, Patchset 38: if (omniboxParams != null && shouldEnableOmnibox) {

Sinan Sahin . resolved

nit: We set omniboxParams only if shouldEnableOmnibox is true, so this can be simplified.

jingjing gu

Thanks, fixed. Simplified the guard to `omniboxParams != null`.

Open in Gerrit

Related details

Attention is currently required from:

Jinsuk Kim
Peter Conn
Sinan Sahin

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

Jinsuk Kim (Gerrit)

unread,

May 8, 2026, 7:59:47 AM (2 days ago) May 8

to jingjing gu, Sinan Sahin, Peter Conn, Chromium Metrics Reviews, Chromium LUCI CQ, android-bu...@system.gserviceaccount.com, chromium...@chromium.org, Peter Beverloo, asvitkine...@chromium.org, lizeb+watch...@chromium.org

Attention needed from Peter Conn, Sinan Sahin and jingjing gu

Jinsuk Kim voted Code-Review+1

Code-Review

+1

Open in Gerrit

Related details

Attention is currently required from:

Peter Conn
Sinan Sahin
jingjing gu

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

Peter Conn (Gerrit)

unread,

4:01 AM (9 hours ago) 4:01 AM

to jingjing gu, Jinsuk Kim, Sinan Sahin, Chromium Metrics Reviews, Chromium LUCI CQ, android-bu...@system.gserviceaccount.com, chromium...@chromium.org, Peter Beverloo, asvitkine...@chromium.org, lizeb+watch...@chromium.org

Attention needed from Sinan Sahin and jingjing gu

Peter Conn voted Code-Review+1

Code-Review

+1

Open in Gerrit

Related details

Attention is currently required from:

Sinan Sahin
jingjing gu

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

jingjing gu (Gerrit)

unread,

5:21 AM (8 hours ago) 5:21 AM

to Peter Conn, Jinsuk Kim, Sinan Sahin, Chromium Metrics Reviews, Chromium LUCI CQ, android-bu...@system.gserviceaccount.com, chromium...@chromium.org, Peter Beverloo, asvitkine...@chromium.org, lizeb+watch...@chromium.org

Attention needed from Sinan Sahin

jingjing gu voted Commit-Queue+2

Commit-Queue

+2

Open in Gerrit

Related details

Attention is currently required from:

Sinan Sahin

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,

6:03 AM (7 hours ago) 6:03 AM

to jingjing gu, Peter Conn, Jinsuk Kim, Sinan Sahin, Chromium Metrics Reviews, android-bu...@system.gserviceaccount.com, chromium...@chromium.org, Peter Beverloo, asvitkine...@chromium.org, lizeb+watch...@chromium.org

Chromium LUCI CQ submitted the change

Change information

Commit message:

[customtabs] Remove old CCT toolbar coordinator paths (2/4)

CCTToolbarRefactor has been enabled by default. Remove the runtime flag
branches from the toolbar coordinator and root UI initialization path,
making the refactored toolbar setup unconditional.

- BaseCustomTabRootUiCoordinator: remove the old initializeToolbar()
  fallback while keeping required PartialCustomTab contextual search
  observer setup in the refactored path
- CloseButtonVisibilityManager: use the toolbar buttons coordinator path
  directly
- CustomTabToolbarCoordinator: remove old toolbar manager branches
- CustomTabToolbarCoordinatorUnitTest: remove flag-parameterized
  expectations
- CustomTabToolbar: remove obsolete action button combo metric code and
  metadata

OBSOLETE_HISTOGRAM[CustomTab.AdaptiveToolbarButton.ActionButtons]=Removed because the stable CCT toolbar refactor analysis metric is no longer recorded.

Bug: 506206087

Change-Id: I7fdc066cf6d2321d6c1ada26ce693d3ce83c722a

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

Reviewed-by: Jinsuk Kim <jins...@chromium.org>

Commit-Queue: jingjing gu <jingj...@microsoft.com>

Reviewed-by: Peter Conn <pec...@chromium.org>

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

Files:

M chrome/android/java/src/org/chromium/chrome/browser/customtabs/BaseCustomTabRootUiCoordinator.java
M chrome/android/java/src/org/chromium/chrome/browser/customtabs/CloseButtonVisibilityManager.java
M chrome/android/java/src/org/chromium/chrome/browser/customtabs/features/toolbar/CustomTabToolbar.java
M chrome/android/java/src/org/chromium/chrome/browser/customtabs/features/toolbar/CustomTabToolbarCoordinator.java
M chrome/android/junit/src/org/chromium/chrome/browser/customtabs/features/toolbar/CustomTabToolbarCoordinatorUnitTest.java
M chrome/android/junit/src/org/chromium/chrome/browser/customtabs/features/toolbar/CustomTabToolbarUnitTest.java
M tools/metrics/histograms/metadata/custom_tabs/enums.xml
M tools/metrics/histograms/metadata/custom_tabs/histograms.xml

Change size: L

Delta: 8 files changed, 37 insertions(+), 851 deletions(-)

Branch: refs/heads/main

Submit Requirements:

Code-Review: +1 by Jinsuk Kim, +1 by Peter Conn

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

[customtabs] Remove old CCT toolbar coordinator paths (2/4) [chromium/src : main]

Sinan Sahin (Gerrit)

Sinan Sahin voted Code-Review+1

Related details

Sinan Sahin (Gerrit)

Sinan Sahin added 2 comments

Related details

jingjing gu (Gerrit)

jingjing gu added 2 comments

Related details

Jinsuk Kim (Gerrit)

Jinsuk Kim added 2 comments

Related details

jingjing gu (Gerrit)

jingjing gu added 2 comments

Related details

Jinsuk Kim (Gerrit)

Jinsuk Kim voted and added 4 comments

Votes added by Jinsuk Kim

4 comments

Related details

jingjing gu (Gerrit)

jingjing gu added 1 comment

Related details

Jinsuk Kim (Gerrit)

Jinsuk Kim voted and added 1 comment

Votes added by Jinsuk Kim

1 comment

Related details

jingjing gu (Gerrit)

jingjing gu added 1 comment

Related details

Peter Conn (Gerrit)

Peter Conn voted Code-Review+1

Related details

Sinan Sahin (Gerrit)

Sinan Sahin voted and added 1 comment

Votes added by Sinan Sahin

1 comment

Related details

Sinan Sahin (Gerrit)

Sinan Sahin voted

Related details

jingjing gu (Gerrit)

jingjing gu added 1 comment

Related details

Jinsuk Kim (Gerrit)

Jinsuk Kim voted Code-Review+1

Related details

Peter Conn (Gerrit)

Peter Conn voted Code-Review+1

Related details

jingjing gu (Gerrit)

jingjing gu voted Commit-Queue+2

Related details

Chromium LUCI CQ (Gerrit)

Chromium LUCI CQ submitted the change

Change information