New issue
Advanced search Search tips

Issue 921243 link

Starred by 2 users
Status: Fixed
Owner:
bsep@chromium.org
Closed: Jan 17
Cc:
a...@chromium.org
pkasting@chromium.org
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Bug



Sign in to add a comment

Remove tab pulse when hovering "Close tabs to the right" or "Close other tabs"

Project Member Reported by bsep@chromium.org, Jan 12 
When hovering tab context menu items "Close tabs to the right" or "Close other tabs", the affected tabs will pulse. This behavior is very difficult to discover and probably not necessary, since the description of their effects is pretty clear. It's also buggy (easy for the pulsing to get de-synced) and not implemented on Mac.

I will remove this feature.

Comment 1 by a...@chromium.org, Jan 12 

Note that this is the only remaining use of the tab pulsing code, so you'll be removing that code as well.

Comment 2 by sky@google.com, Jan 12 

For what it's worth I believe the pulse was more visible when it was added, and it grew harder to discern as the tab/frame colors changed.
Project Member

Comment 3 by bugdroid1@chromium.org, Jan 15 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/1dd90008554cac4a0a1637f7ec1d45aeb16eb254

commit 1dd90008554cac4a0a1637f7ec1d45aeb16eb254
Author: Bret Sepulveda <bsep@chromium.org>
Date: Tue Jan 15 00:40:49 2019

Delete tab pulsing feature.

Previously, when hovering tab context menu items "Close tabs to the
right" and "Close other tabs", the affected tabs would pulse. This patch
deletes that feature. This caused SimpleMenuModel::CommandIdHighlighted
and transitively MenuModel::HighlightChangedTo to become unused. This
also caused TabStrip::animation_container_ to become effectively unused,
since pulsing was the only animation that needed to be coordinated
between tabs.

Bug:  921243 
Change-Id: Iae4ef39d979fa5f5a96ffc465275f25385f5949f
Reviewed-on: https://chromium-review.googlesource.com/c/1407958
Commit-Queue: Bret Sepulveda <bsep@chromium.org>
Reviewed-by: Drew Wilson <atwilson@chromium.org>
Reviewed-by: Scott Violet <sky@chromium.org>
Reviewed-by: Avi Drissman <avi@chromium.org>
Cr-Commit-Position: refs/heads/master@{#622657}
[modify] https://crrev.com/1dd90008554cac4a0a1637f7ec1d45aeb16eb254/chrome/browser/status_icons/status_icon_menu_model.cc
[modify] https://crrev.com/1dd90008554cac4a0a1637f7ec1d45aeb16eb254/chrome/browser/status_icons/status_icon_menu_model.h
[modify] https://crrev.com/1dd90008554cac4a0a1637f7ec1d45aeb16eb254/chrome/browser/ui/tabs/tab_strip_model.cc
[modify] https://crrev.com/1dd90008554cac4a0a1637f7ec1d45aeb16eb254/chrome/browser/ui/tabs/tab_strip_model.h
[modify] https://crrev.com/1dd90008554cac4a0a1637f7ec1d45aeb16eb254/chrome/browser/ui/tabs/tab_strip_model_unittest.cc
[modify] https://crrev.com/1dd90008554cac4a0a1637f7ec1d45aeb16eb254/chrome/browser/ui/toolbar/back_forward_menu_model.cc
[modify] https://crrev.com/1dd90008554cac4a0a1637f7ec1d45aeb16eb254/chrome/browser/ui/toolbar/back_forward_menu_model.h
[modify] https://crrev.com/1dd90008554cac4a0a1637f7ec1d45aeb16eb254/chrome/browser/ui/views/menu_model_adapter_test.cc
[modify] https://crrev.com/1dd90008554cac4a0a1637f7ec1d45aeb16eb254/chrome/browser/ui/views/tabs/browser_tab_strip_controller.cc
[modify] https://crrev.com/1dd90008554cac4a0a1637f7ec1d45aeb16eb254/chrome/browser/ui/views/tabs/browser_tab_strip_controller.h
[modify] https://crrev.com/1dd90008554cac4a0a1637f7ec1d45aeb16eb254/chrome/browser/ui/views/tabs/tab.cc
[modify] https://crrev.com/1dd90008554cac4a0a1637f7ec1d45aeb16eb254/chrome/browser/ui/views/tabs/tab.h
[modify] https://crrev.com/1dd90008554cac4a0a1637f7ec1d45aeb16eb254/chrome/browser/ui/views/tabs/tab_strip.cc
[modify] https://crrev.com/1dd90008554cac4a0a1637f7ec1d45aeb16eb254/chrome/browser/ui/views/tabs/tab_strip.h
[modify] https://crrev.com/1dd90008554cac4a0a1637f7ec1d45aeb16eb254/chrome/browser/ui/views/tabs/tab_unittest.cc
[modify] https://crrev.com/1dd90008554cac4a0a1637f7ec1d45aeb16eb254/chrome/browser/ui/views/toolbar/toolbar_button_views_unittest.cc
[modify] https://crrev.com/1dd90008554cac4a0a1637f7ec1d45aeb16eb254/ui/base/models/menu_model.h
[modify] https://crrev.com/1dd90008554cac4a0a1637f7ec1d45aeb16eb254/ui/base/models/simple_menu_model.cc
[modify] https://crrev.com/1dd90008554cac4a0a1637f7ec1d45aeb16eb254/ui/base/models/simple_menu_model.h
[modify] https://crrev.com/1dd90008554cac4a0a1637f7ec1d45aeb16eb254/ui/views/controls/combobox/combobox.cc
[modify] https://crrev.com/1dd90008554cac4a0a1637f7ec1d45aeb16eb254/ui/views/controls/menu/menu_controller.cc
[modify] https://crrev.com/1dd90008554cac4a0a1637f7ec1d45aeb16eb254/ui/views/controls/menu/menu_delegate.h
[modify] https://crrev.com/1dd90008554cac4a0a1637f7ec1d45aeb16eb254/ui/views/controls/menu/menu_model_adapter.cc
[modify] https://crrev.com/1dd90008554cac4a0a1637f7ec1d45aeb16eb254/ui/views/controls/menu/menu_model_adapter.h
[modify] https://crrev.com/1dd90008554cac4a0a1637f7ec1d45aeb16eb254/ui/views/controls/menu/menu_model_adapter_unittest.cc

Comment 4 by bsep@chromium.org, Jan 15

Status: Fixed (was: Started)
Project Member

Comment 5 by bugdroid1@chromium.org, Jan 15 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/c75dbc59782cbab8703a74674f449b9f76b95ad9

commit c75dbc59782cbab8703a74674f449b9f76b95ad9
Author: Kevin Marshall <kmarshall@chromium.org>
Date: Tue Jan 15 01:42:13 2019

Revert "Delete tab pulsing feature."

This reverts commit 1dd90008554cac4a0a1637f7ec1d45aeb16eb254.

Reason for revert: breaks linux-jumbo-rel (failure log: https://logs.chromium.org/logs/chromium/buildbucket/cr-buildbucket.appspot.com/8924308573690053872/+/steps/compile/0/stdout)

Original change's description:
> Delete tab pulsing feature.
> 
> Previously, when hovering tab context menu items "Close tabs to the
> right" and "Close other tabs", the affected tabs would pulse. This patch
> deletes that feature. This caused SimpleMenuModel::CommandIdHighlighted
> and transitively MenuModel::HighlightChangedTo to become unused. This
> also caused TabStrip::animation_container_ to become effectively unused,
> since pulsing was the only animation that needed to be coordinated
> between tabs.
> 
> Bug:  921243 
> Change-Id: Iae4ef39d979fa5f5a96ffc465275f25385f5949f
> Reviewed-on: https://chromium-review.googlesource.com/c/1407958
> Commit-Queue: Bret Sepulveda <bsep@chromium.org>
> Reviewed-by: Drew Wilson <atwilson@chromium.org>
> Reviewed-by: Scott Violet <sky@chromium.org>
> Reviewed-by: Avi Drissman <avi@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#622657}

TBR=avi@chromium.org,sky@chromium.org,atwilson@chromium.org,bsep@chromium.org

Change-Id: I96285c3f0b2601cc59c98b5d8abde97fd04afdf4
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug:  921243 
Reviewed-on: https://chromium-review.googlesource.com/c/1409886
Reviewed-by: Kevin Marshall <kmarshall@chromium.org>
Commit-Queue: Kevin Marshall <kmarshall@chromium.org>
Cr-Commit-Position: refs/heads/master@{#622668}
[modify] https://crrev.com/c75dbc59782cbab8703a74674f449b9f76b95ad9/chrome/browser/status_icons/status_icon_menu_model.cc
[modify] https://crrev.com/c75dbc59782cbab8703a74674f449b9f76b95ad9/chrome/browser/status_icons/status_icon_menu_model.h
[modify] https://crrev.com/c75dbc59782cbab8703a74674f449b9f76b95ad9/chrome/browser/ui/tabs/tab_strip_model.cc
[modify] https://crrev.com/c75dbc59782cbab8703a74674f449b9f76b95ad9/chrome/browser/ui/tabs/tab_strip_model.h
[modify] https://crrev.com/c75dbc59782cbab8703a74674f449b9f76b95ad9/chrome/browser/ui/tabs/tab_strip_model_unittest.cc
[modify] https://crrev.com/c75dbc59782cbab8703a74674f449b9f76b95ad9/chrome/browser/ui/toolbar/back_forward_menu_model.cc
[modify] https://crrev.com/c75dbc59782cbab8703a74674f449b9f76b95ad9/chrome/browser/ui/toolbar/back_forward_menu_model.h
[modify] https://crrev.com/c75dbc59782cbab8703a74674f449b9f76b95ad9/chrome/browser/ui/views/menu_model_adapter_test.cc
[modify] https://crrev.com/c75dbc59782cbab8703a74674f449b9f76b95ad9/chrome/browser/ui/views/tabs/browser_tab_strip_controller.cc
[modify] https://crrev.com/c75dbc59782cbab8703a74674f449b9f76b95ad9/chrome/browser/ui/views/tabs/browser_tab_strip_controller.h
[modify] https://crrev.com/c75dbc59782cbab8703a74674f449b9f76b95ad9/chrome/browser/ui/views/tabs/tab.cc
[modify] https://crrev.com/c75dbc59782cbab8703a74674f449b9f76b95ad9/chrome/browser/ui/views/tabs/tab.h
[modify] https://crrev.com/c75dbc59782cbab8703a74674f449b9f76b95ad9/chrome/browser/ui/views/tabs/tab_strip.cc
[modify] https://crrev.com/c75dbc59782cbab8703a74674f449b9f76b95ad9/chrome/browser/ui/views/tabs/tab_strip.h
[modify] https://crrev.com/c75dbc59782cbab8703a74674f449b9f76b95ad9/chrome/browser/ui/views/tabs/tab_unittest.cc
[modify] https://crrev.com/c75dbc59782cbab8703a74674f449b9f76b95ad9/chrome/browser/ui/views/toolbar/toolbar_button_views_unittest.cc
[modify] https://crrev.com/c75dbc59782cbab8703a74674f449b9f76b95ad9/ui/base/models/menu_model.h
[modify] https://crrev.com/c75dbc59782cbab8703a74674f449b9f76b95ad9/ui/base/models/simple_menu_model.cc
[modify] https://crrev.com/c75dbc59782cbab8703a74674f449b9f76b95ad9/ui/base/models/simple_menu_model.h
[modify] https://crrev.com/c75dbc59782cbab8703a74674f449b9f76b95ad9/ui/views/controls/combobox/combobox.cc
[modify] https://crrev.com/c75dbc59782cbab8703a74674f449b9f76b95ad9/ui/views/controls/menu/menu_controller.cc
[modify] https://crrev.com/c75dbc59782cbab8703a74674f449b9f76b95ad9/ui/views/controls/menu/menu_delegate.h
[modify] https://crrev.com/c75dbc59782cbab8703a74674f449b9f76b95ad9/ui/views/controls/menu/menu_model_adapter.cc
[modify] https://crrev.com/c75dbc59782cbab8703a74674f449b9f76b95ad9/ui/views/controls/menu/menu_model_adapter.h
[modify] https://crrev.com/c75dbc59782cbab8703a74674f449b9f76b95ad9/ui/views/controls/menu/menu_model_adapter_unittest.cc

Comment 6 by a...@chromium.org, Jan 15

Status: Assigned (was: Fixed)
bsep, try again.

The failure that caused the revert is completely bogus. Your patch was legit and the jumbo builder is having issues. See the email I sent to cxx and  bug 921967 .
Project Member

Comment 7 by bugdroid1@chromium.org, Jan 17 (6 days ago) 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/47a251dbd0b47aa784e05124ba755b79fbc540d8

commit 47a251dbd0b47aa784e05124ba755b79fbc540d8
Author: Bret Sepulveda <bsep@chromium.org>
Date: Thu Jan 17 00:45:26 2019

Reland "Delete tab pulsing feature."

This is a reland of 1dd90008554cac4a0a1637f7ec1d45aeb16eb254

Original change's description:
> Delete tab pulsing feature.
>
> Previously, when hovering tab context menu items "Close tabs to the
> right" and "Close other tabs", the affected tabs would pulse. This patch
> deletes that feature. This caused SimpleMenuModel::CommandIdHighlighted
> and transitively MenuModel::HighlightChangedTo to become unused. This
> also caused TabStrip::animation_container_ to become effectively unused,
> since pulsing was the only animation that needed to be coordinated
> between tabs.
>
> Bug:  921243 
> Change-Id: Iae4ef39d979fa5f5a96ffc465275f25385f5949f
> Reviewed-on: https://chromium-review.googlesource.com/c/1407958
> Commit-Queue: Bret Sepulveda <bsep@chromium.org>
> Reviewed-by: Drew Wilson <atwilson@chromium.org>
> Reviewed-by: Scott Violet <sky@chromium.org>
> Reviewed-by: Avi Drissman <avi@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#622657}

TBR=avi@chromium.org,sky@chromium.org,atwilson@chromium.org

Bug:  921243 
Change-Id: I528cbdde5bf03674c7e65bc3c6e1eb8691efb3d2
Reviewed-on: https://chromium-review.googlesource.com/c/1413453
Reviewed-by: Bret Sepulveda <bsep@chromium.org>
Reviewed-by: Scott Violet <sky@chromium.org>
Reviewed-by: Avi Drissman <avi@chromium.org>
Commit-Queue: Bret Sepulveda <bsep@chromium.org>
Cr-Commit-Position: refs/heads/master@{#623465}
[modify] https://crrev.com/47a251dbd0b47aa784e05124ba755b79fbc540d8/chrome/browser/status_icons/status_icon_menu_model.cc
[modify] https://crrev.com/47a251dbd0b47aa784e05124ba755b79fbc540d8/chrome/browser/status_icons/status_icon_menu_model.h
[modify] https://crrev.com/47a251dbd0b47aa784e05124ba755b79fbc540d8/chrome/browser/ui/tabs/tab_strip_model.cc
[modify] https://crrev.com/47a251dbd0b47aa784e05124ba755b79fbc540d8/chrome/browser/ui/tabs/tab_strip_model.h
[modify] https://crrev.com/47a251dbd0b47aa784e05124ba755b79fbc540d8/chrome/browser/ui/tabs/tab_strip_model_unittest.cc
[modify] https://crrev.com/47a251dbd0b47aa784e05124ba755b79fbc540d8/chrome/browser/ui/toolbar/back_forward_menu_model.cc
[modify] https://crrev.com/47a251dbd0b47aa784e05124ba755b79fbc540d8/chrome/browser/ui/toolbar/back_forward_menu_model.h
[modify] https://crrev.com/47a251dbd0b47aa784e05124ba755b79fbc540d8/chrome/browser/ui/views/menu_model_adapter_test.cc
[modify] https://crrev.com/47a251dbd0b47aa784e05124ba755b79fbc540d8/chrome/browser/ui/views/tabs/browser_tab_strip_controller.cc
[modify] https://crrev.com/47a251dbd0b47aa784e05124ba755b79fbc540d8/chrome/browser/ui/views/tabs/browser_tab_strip_controller.h
[modify] https://crrev.com/47a251dbd0b47aa784e05124ba755b79fbc540d8/chrome/browser/ui/views/tabs/tab.cc
[modify] https://crrev.com/47a251dbd0b47aa784e05124ba755b79fbc540d8/chrome/browser/ui/views/tabs/tab.h
[modify] https://crrev.com/47a251dbd0b47aa784e05124ba755b79fbc540d8/chrome/browser/ui/views/tabs/tab_strip.cc
[modify] https://crrev.com/47a251dbd0b47aa784e05124ba755b79fbc540d8/chrome/browser/ui/views/tabs/tab_strip.h
[modify] https://crrev.com/47a251dbd0b47aa784e05124ba755b79fbc540d8/chrome/browser/ui/views/tabs/tab_unittest.cc
[modify] https://crrev.com/47a251dbd0b47aa784e05124ba755b79fbc540d8/chrome/browser/ui/views/toolbar/toolbar_button_views_unittest.cc
[modify] https://crrev.com/47a251dbd0b47aa784e05124ba755b79fbc540d8/ui/base/models/menu_model.h
[modify] https://crrev.com/47a251dbd0b47aa784e05124ba755b79fbc540d8/ui/base/models/simple_menu_model.cc
[modify] https://crrev.com/47a251dbd0b47aa784e05124ba755b79fbc540d8/ui/base/models/simple_menu_model.h
[modify] https://crrev.com/47a251dbd0b47aa784e05124ba755b79fbc540d8/ui/views/controls/combobox/combobox.cc
[modify] https://crrev.com/47a251dbd0b47aa784e05124ba755b79fbc540d8/ui/views/controls/menu/menu_controller.cc
[modify] https://crrev.com/47a251dbd0b47aa784e05124ba755b79fbc540d8/ui/views/controls/menu/menu_delegate.h
[modify] https://crrev.com/47a251dbd0b47aa784e05124ba755b79fbc540d8/ui/views/controls/menu/menu_model_adapter.cc
[modify] https://crrev.com/47a251dbd0b47aa784e05124ba755b79fbc540d8/ui/views/controls/menu/menu_model_adapter.h
[modify] https://crrev.com/47a251dbd0b47aa784e05124ba755b79fbc540d8/ui/views/controls/menu/menu_model_adapter_unittest.cc

Comment 8 by bsep@chromium.org, Jan 17 (6 days ago)

Status: Fixed (was: Assigned)
►

Sign in to add a comment