Shakti Sahu (Gerrit)

unread,

Jan 9, 2026, 8:41:59 PM (20 hours ago) Jan 9

to Anthony Cui, Sophie Chang, Tommy Nyquist, Chromium LUCI CQ, chromium...@chromium.org, dewitt...@chromium.org

Attention needed from Anthony Cui

Shakti Sahu voted and added 4 comments

Votes added by Shakti Sahu

Code-Review

+1

4 comments

Patchset-level comments

File-level comment, Patchset 1 (Latest):

Shakti Sahu . resolved

LGTM % nits.

sophiechang@, nyquist@ FYI.

File chrome/browser/glic/browser_ui/tab_underline_view_controller_impl.cc

Line 262, Patchset 1 (Latest): ShowAndAnimateUnderline(true);

Shakti Sahu . unresolved

nit: `/*triggered_by_glic=*/true`

Line 346, Patchset 1 (Latest): HideUnderline();

Shakti Sahu . unresolved

Do we need to do the reverse logic here? Will glic ever send `TabNotInPinnedSet` signals while glic panel isn't showing (and AIM could be showing at that instant)?

Line 374, Patchset 1 (Latest):

      // Ignore the signal from contextual tasks if the underline is being shown
      // due to a signal from Glic

Shakti Sahu . unresolved

nit: // If the underline is currently being shown due to a Glic side panel, ignore reset signals from contextual tasks as the backend sends reset signals much more frequently (e.g. for every tab switch event) even though contextual tasks side panel isn't open.

Open in Gerrit

Related details

Attention is currently required from:

Anthony Cui

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

Anthony Cui (Gerrit)

unread,

Jan 9, 2026, 9:21:35 PM (19 hours ago) Jan 9

to Shakti Sahu, Sophie Chang, Tommy Nyquist, Chromium LUCI CQ, chromium...@chromium.org, dewitt...@chromium.org

Anthony Cui added 3 comments

File chrome/browser/glic/browser_ui/tab_underline_view_controller_impl.cc

Line 262, Patchset 1: ShowAndAnimateUnderline(true);

Shakti Sahu . resolved

nit: `/*triggered_by_glic=*/true`

Anthony Cui

Done

Line 346, Patchset 1: HideUnderline();

Shakti Sahu . resolved

Do we need to do the reverse logic here? Will glic ever send `TabNotInPinnedSet` signals while glic panel isn't showing (and AIM could be showing at that instant)?

Anthony Cui

Done

Line 374, Patchset 1: // Ignore the signal from contextual tasks if the underline is being shown


      // due to a signal from Glic

Shakti Sahu . resolved

nit: // If the underline is currently being shown due to a Glic side panel, ignore reset signals from contextual tasks as the backend sends reset signals much more frequently (e.g. for every tab switch event) even though contextual tasks side panel isn't open.

Anthony Cui

Done

Open in Gerrit

Related details

Attention set is empty

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

Anthony Cui (Gerrit)

unread,

Jan 9, 2026, 9:43:05 PM (19 hours ago) Jan 9

to Shakti Sahu, Sophie Chang, Tommy Nyquist, Chromium LUCI CQ, chromium...@chromium.org, dewitt...@chromium.org

Anthony Cui voted Commit-Queue+2

Commit-Queue

+2

Open in Gerrit

Related details

Attention set is empty

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,

Jan 9, 2026, 10:10:42 PM (18 hours ago) Jan 9

to Anthony Cui, Shakti Sahu, Sophie Chang, Tommy Nyquist, chromium...@chromium.org, dewitt...@chromium.org

Chromium LUCI CQ submitted the change with unreviewed changes

Unreviewed changes

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

```
The name of the file: chrome/browser/glic/browser_ui/tab_underline_view_controller_impl.cc
Insertions: 26, Deletions: 21.

@@ -259,9 +259,9 @@
       // Active follow tab underline should be newly shown, pinned tabs should
       // re-animate or be newly shown if not already visible.
       if (IsUnderlineTabSharedThroughActiveFollow()) {
-        ShowAndAnimateUnderline(true);
+        ShowAndAnimateUnderline(/*triggered_by_glic=*/true);
       }
-      ShowOrAnimatePinnedUnderline(true);
+      ShowOrAnimatePinnedUnderline(/*triggered_by_glic=*/true);
       break;
     }
     case UpdateUnderlineReason::kContextAccessIndicatorOff: {
@@ -271,7 +271,7 @@
           (GlicEnabling::IsMultiInstanceEnabled() || IsGlicWindowShowing())) {
         break;
       }
-      HideUnderline();
+      HideUnderline(/*triggered_by_glic=*/true);
       break;
     }
     case UpdateUnderlineReason::kFocusedTabChanged_NoFocusChange: {
@@ -287,7 +287,7 @@
       // follow. Pinned tabs should not react as the set of shared tabs has
       // not changed.
       if (IsUnderlineTabSharedThroughActiveFollow()) {
-        ShowAndAnimateUnderline(true);
+        ShowAndAnimateUnderline(/*triggered_by_glic=*/true);
       }
       break;
     }
@@ -298,7 +298,7 @@
       if (IsUnderlineTabPinned() && context_access_indicator_enabled_) {
         AnimateUnderline();
       } else if (!IsUnderlineTabPinned()) {
-        HideUnderline();
+        HideUnderline(/*triggered_by_glic=*/true);
       }
       break;
     }
@@ -306,19 +306,19 @@
       // Active follow tab underline should be newly shown, pinned tabs should
       // re-animate or be newly shown if not already visible.
       if (IsUnderlineTabSharedThroughActiveFollow()) {
-        ShowAndAnimateUnderline(true);
+        ShowAndAnimateUnderline(/*triggered_by_glic=*/true);
       }
-      ShowOrAnimatePinnedUnderline(true);
+      ShowOrAnimatePinnedUnderline(/*triggered_by_glic=*/true);
       break;
     case UpdateUnderlineReason::kFocusedTabChanged_ChromeLostFocus:
       // Underline should be hidden, with exception to pinned tabs.
       if (!IsUnderlineTabPinned()) {
-        HideUnderline();
+        HideUnderline(/*triggered_by_glic=*/true);
       }
       break;
     case UpdateUnderlineReason::kPinnedTabsChanged_TabInPinnedSet:
       if (GlicEnabling::IsMultiInstanceEnabled()) {
-        ShowAndAnimateUnderline(true);
+        ShowAndAnimateUnderline(/*triggered_by_glic=*/true);
       } else {
         // If `underline_view_` is not visible, then this tab was just added
         // to the set of pinned tabs.
@@ -327,7 +327,7 @@
           // is open. For multi-instance this is controlled via the pinned
           // tabs api.
           if (IsGlicWindowShowing()) {
-            ShowAndAnimateUnderline(true);
+            ShowAndAnimateUnderline(/*triggered_by_glic=*/true);
           }
         } else {
           // This tab was already pinned - re-animate to reflect the change in
@@ -343,20 +343,20 @@
         return;
       }
       // This tab may have just been removed from the pinned set.
-      HideUnderline();
+      HideUnderline(/*triggered_by_glic=*/true);
       break;
     case UpdateUnderlineReason::kPanelStateChanged_PanelShowing:
       // Visibility of underlines of pinned tabs should follow visibility of
       // the glic panel.
       if (IsUnderlineTabPinned()) {
-        ShowAndAnimateUnderline(true);
+        ShowAndAnimateUnderline(/*triggered_by_glic=*/true);
       }
       break;
     case UpdateUnderlineReason::kPanelStateChanged_PanelHidden:
       // Visibility of underlines of pinned tabs should follow visibility of
       // the glic panel.
       if (IsUnderlineTabPinned()) {
-        HideUnderline();
+        HideUnderline(/*triggered_by_glic=*/true);
       }
       break;
     case UpdateUnderlineReason::kUserInputSubmitted:
@@ -366,13 +366,15 @@
       break;
     case UpdateUnderlineReason::kContextualTask_TabInContext:
       if (!underline_view_->IsShowing()) {
-        ShowAndAnimateUnderline(false);
+        ShowAndAnimateUnderline(/*triggered_by_glic=*/false);
       }
       break;
     case UpdateUnderlineReason::kContextualTask_TabNotInContext:
       // TODO(crbug.com/467739947): Consider reenabling hide animation.
-      // Ignore the signal from contextual tasks if the underline is being shown
-      // due to a signal from Glic
+      // If the underline is currently being shown due to a Glic side panel,
+      // ignore reset signals from contextual tasks as the backend sends reset
+      // signals much more frequently (e.g. for every tab switch event) even
+      // though contextual tasks side panel isn't open.
       if (state_ == UnderlineState::kShowingForGlic) {
         return;
       }
@@ -389,11 +391,18 @@
   underline_view_->Show();
 }
 
-void TabUnderlineViewControllerImpl::HideUnderline() {
+void TabUnderlineViewControllerImpl::HideUnderline(bool triggered_by_glic) {
   if (!underline_view_->IsShowing()) {
     return;
   }
 
+  // Ignore the signal from Glic if the underline is being shown
+  // due to a signal from contextual tasks.
+  if (triggered_by_glic &&
+      state_ == UnderlineState::kShowingForContextualTasks) {
+    return;
+  }
+
   state_ = UnderlineState::kHidden;
 
   if (base::FeatureList::IsEnabled(features::kGlicDisableUnderlineAnimations)) {
@@ -510,8 +519,4 @@
   return base::FeatureList::IsEnabled(contextual_tasks::kContextualTasks);
 }
 
-// bool TabUnderlineViewControllerImpl::ShouldSkipUpdateSignal(
-//     bool triggered_by_glic) {
-// }
-
 }  // namespace glic
```
```
The name of the file: chrome/browser/glic/browser_ui/tab_underline_view_controller_impl.h
Insertions: 1, Deletions: 1.

@@ -131,7 +131,7 @@
   // the beginning.
   void ShowAndAnimateUnderline(bool triggered_by_glic);
 
-  void HideUnderline();
+  void HideUnderline(bool triggered_by_glic);
 
   // Replay the animation without hiding and re-showing the view.
   void AnimateUnderline();
```

Change information

Commit message:

[GlicUnderlines] Ignore hide signals from contextual tasks to glic-triggered underlines

Bug: b:469102481

Change-Id: I4340c0a513e26e5613cf391b1ebbb36bb02a8f36

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

Commit-Queue: Anthony Cui <cuian...@chromium.org>

Reviewed-by: Shakti Sahu <shakt...@chromium.org>

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

Files:

M chrome/browser/glic/browser_ui/tab_underline_view_controller_impl.cc
M chrome/browser/glic/browser_ui/tab_underline_view_controller_impl.h

Change size: M

Delta: 2 files changed, 51 insertions(+), 21 deletions(-)

Branch: refs/heads/main

Submit Requirements:

Code-Review: +1 by Shakti Sahu

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

[GlicUnderlines] Ignore hide signals from contextual tasks to glic-triggered underlines [chromium/src : main]

Shakti Sahu (Gerrit)

Shakti Sahu voted and added 4 comments

Votes added by Shakti Sahu

4 comments

Related details

Anthony Cui (Gerrit)

Anthony Cui added 3 comments

Related details

Anthony Cui (Gerrit)

Anthony Cui voted Commit-Queue+2

Related details

Chromium LUCI CQ (Gerrit)

Chromium LUCI CQ submitted the change with unreviewed changes

Unreviewed changes

Change information