Uh... "Attach a file" isn't working right now, so I cannot attach the screen cast.

The hover highlight opacity seems very different between the two tabs being hovered in this video.

Interesting!  So my first thinking is that maybe Tab::UpdateOpacities isn't getting called as often as it should.  I thought Tab construction and OnThemeChange would be sufficient.  What else can change the tab colors?  This is the relevant code before modifying opacities based on color & contrast:

  // In UpdateOpacities ...
  const SkColor active_tab_bg_color =
      controller_->GetTabBackgroundColor(TAB_ACTIVE);
  const SkColor inactive_tab_bg_color =
      controller_->GetTabBackgroundColor(TAB_INACTIVE);
  // ... compute opacity based on colors and contrast, and keep the values around for a while ...

I did some more investigation on this. I don't know if my explanation will make any sense, but I'll try.

I restarted the Canary build with around 20 open tabs. The active tab was around the 6th from the right side. Upon restart, it returns to being the active tab.

Now, all the hover highlights of the inactive tabs to the *left* of the active tab are darker and more visible. Whereas, the hover highlights to the right of the active tab are much lighter (more subtle).

If I change the active tab, the point at which the hover highlight changes opacity is still at the same tab. So all the tabs with the darker highlight remain darker and all the ones with the lighter highlight remain lighter. The tab that was selected on startup, will have the darker inactive highlight.

Clear as mud?

Uh oh, looks like this call comes back with different colors depending on active state.  I thought it was reliable depending only on theme / stable coloring.  So the bug is that I need to know the standard stable theme tab background colors, not something that depends on changing state.

We were writing at same time.  I appreciate the repro steps!  It seems in line with my code analysis.  Active state determines what comes back from GetTabBackgroundColor -- but I didn't know it was stateful.  I will try to repro with some logging to confirm this theory.

Yeah, I am thinking of changing BrowserNonClientFrameView::GetTabBackgroundColor -- maybe it should take another parameter so we can be sure of what it will return... or perhaps we can create a new version of the method that is less volatile, with state depending only on theme.

A quick chat with pkasting@ revised my earlier thinking that this could be done with 2 stable colors that change only with theme.  Changing window active state can change colors, and hence computed opacities must update.  So these are candidates for the fix:

* Within Tab, simply move UpdateOpacities() from OnThemeChanged to OnButtonColorMaybeChanged.  (And possibly rename this function since it is getting heavier use lately.)  The downside to this approach is that we're using more memory and calculations by doing this for all tabs.
* Move the stored opacities to TabStrip and update them from TabStrip::FrameColorsChanged.  This seems to get called from various code-paths when the relevant state & colors change, and is more efficient.

I will see about getting a CL for the second approach soon.  pkasting@ is busy.  kylixrd@, do you have time to review today?

orinj@ if you can get a CL in soon, I can review it early tomorrow morning. I'm OOO on Friday, Monday, Tuesday.

crrev.com/c/1168140 should do the trick.  I am testing it now.

Thanks kylixrd@ -- pkasting@ went ahead with the review today, so tomorrow's canary should include the fix.  I am hoping you can side-by-side repro to confirm this change resolves the problem.  I haven't been able to repro on linux (I was mistaken in #9) so this CL is based on pure code analysis. }:)  I will test on Windows tomorrow as well, though.

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

commit 944434030c905c00deec8919a5afc9e100d9f365
Author: Orin Jaworski <orinj@chromium.org>
Date: Thu Aug 09 06:04:33 2018

Move tab hover opacity state handling from Tab to TabStrip

The opacities based on colors and contrast ratios need to be
recalculated when window active state or colors change, and
doing this for all tabs is not efficient, so the stored
opacities are moved to TabStrip for use by all its tabs.
The motivation for this CL was to fix a bug where some tabs
had different hover opacities than others because they
were not updating with change in window active state.

Bug:  872310 
Change-Id: Ia7367227df005470364da10b43df429f9e4e8e47
Reviewed-on: https://chromium-review.googlesource.com/1168140
Commit-Queue: Orin Jaworski <orinj@chromium.org>
Reviewed-by: Peter Kasting <pkasting@chromium.org>
Cr-Commit-Position: refs/heads/master@{#581806}
[modify] https://crrev.com/944434030c905c00deec8919a5afc9e100d9f365/chrome/browser/ui/views/tabs/tab.cc
[modify] https://crrev.com/944434030c905c00deec8919a5afc9e100d9f365/chrome/browser/ui/views/tabs/tab.h
[modify] https://crrev.com/944434030c905c00deec8919a5afc9e100d9f365/chrome/browser/ui/views/tabs/tab_controller.h
[modify] https://crrev.com/944434030c905c00deec8919a5afc9e100d9f365/chrome/browser/ui/views/tabs/tab_strip.cc
[modify] https://crrev.com/944434030c905c00deec8919a5afc9e100d9f365/chrome/browser/ui/views/tabs/tab_strip.h
[modify] https://crrev.com/944434030c905c00deec8919a5afc9e100d9f365/chrome/browser/ui/views/tabs/tab_unittest.cc

I wonder if this commit missed the cut-off for today's Canary build? I'm still seeing the issue on today's 70.0.3517.0. I'm update/rebasing my working copy right now to make sure I have your fix.

I tried this several times with my local working copy and it didn't reproduce. It does appear that today's Canary in fact didn't include this commit.

That Canary version was cut at r581729 (see http://omahaproxy.appspot.com/ ).  So yes, this missed the cut for it.

Yeah, my CL was green across the board, but didn't merge until late evening because the ios-simulator trybot wasn't trying.  I had to re-submit a few times because of job timeout.  So, as I understand the situation:

* Today's canary just missed the CL thanks to lazy ios-simulator, boo. :(
* But the CL solves the problem, yay! :)

I think if you can repro the issue on local working copy after reverting this commit specifically, that would be quite conclusive, but already confidence is high.  Thanks for the update!

This bug requires manual review: M69 has already been promoted to the beta branch, so this requires manual review
Please contact the milestone owner if you have questions.
Owners: amineer@(Android), kariahda@(iOS), cindyb@(ChromeOS), govind@(Desktop)

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot

How is the change listed at #14 looking in canary? Is it fully safe to merge to M69?

(This was approved in https://bugs.chromium.org/p/chromium/issues/detail?id=856893#c36 )

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

commit 6f6d7edfae674879a8e116a4c2d140b22f115a60
Author: Peter Kasting <pkasting@chromium.org>
Date: Tue Aug 14 00:50:11 2018

(Merge to M69 branch) Move tab hover opacity state handling from Tab to TabStrip

The opacities based on colors and contrast ratios need to be
recalculated when window active state or colors change, and
doing this for all tabs is not efficient, so the stored
opacities are moved to TabStrip for use by all its tabs.
The motivation for this CL was to fix a bug where some tabs
had different hover opacities than others because they
were not updating with change in window active state.

(cherry picked from commit 944434030c905c00deec8919a5afc9e100d9f365)

Bug:  872310 
Change-Id: Ia7367227df005470364da10b43df429f9e4e8e47
Reviewed-on: https://chromium-review.googlesource.com/1168140
Commit-Queue: Orin Jaworski <orinj@chromium.org>
Reviewed-by: Peter Kasting <pkasting@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#581806}
Reviewed-on: https://chromium-review.googlesource.com/1173659
Cr-Commit-Position: refs/branch-heads/3497@{#607}
Cr-Branched-From: 271eaf50594eb818c9295dc78d364aea18c82ea8-refs/heads/master@{#576753}
[modify] https://crrev.com/6f6d7edfae674879a8e116a4c2d140b22f115a60/chrome/browser/ui/views/tabs/tab.cc
[modify] https://crrev.com/6f6d7edfae674879a8e116a4c2d140b22f115a60/chrome/browser/ui/views/tabs/tab.h
[modify] https://crrev.com/6f6d7edfae674879a8e116a4c2d140b22f115a60/chrome/browser/ui/views/tabs/tab_controller.h
[modify] https://crrev.com/6f6d7edfae674879a8e116a4c2d140b22f115a60/chrome/browser/ui/views/tabs/tab_strip.cc
[modify] https://crrev.com/6f6d7edfae674879a8e116a4c2d140b22f115a60/chrome/browser/ui/views/tabs/tab_strip.h
[modify] https://crrev.com/6f6d7edfae674879a8e116a4c2d140b22f115a60/chrome/browser/ui/views/tabs/tab_unittest.cc

+abdulsyed@ fyi, M69 merges taken for Proj-MdRefresh .