Assigning to kylixrd for evaluation.

This is a bug in the logic about when to add extra padding.

Issue 853623 has been merged into this issue.

Blocked on close button decision

I don't know if it is, actually -- I think we may have this bug even if background tabs always show close buttons.

Triage: Upgrading to P1 - Flicker or movement now meets the P1 bar.

kylixrd: Can this be load balanced to bsep?

Sure. Done.

> I don't know if it is, actually -- I think we may have this bug even if background tabs always show close buttons.

Yes, this bug is also present even if background tabs always show close buttons now.

The only difference is, that the favicon is pushed a little bit to the right when the speaker and stays then there.

Attached a screencast.

Deleted: Favicon_movement_when_speaker_appears.mov 1.3 MB

	Favicon_movement_when_speaker_appears.mov 1.3 MB View Download

Not P1 anymore, but I'll look at it.

FWIW, the code in this function (that computes icon visibility) is so confused and cryptic compared to its original incarnation that what makes the most sense to me is to try to nuke the function and compute icon visibility on the fly during layout.

So this is not actually a Refresh regression, it reproduces in the old UI. That said, it's pretty obnoxious, so maybe I'll try and fix it anyway...

We likely regressed it during the Touchable refactoring, which seems to have introduced a couple bugs to this code.  I suspect e.g. Chrome 61 wouldn't have this.  Not 100% sure though.

The logic itself is pretty suspect; we just randomly add extra padding when all three icons are shown. I think it's supposed to be a proxy for "is there enough room to add the extra padding" but it needs to be redone.

The logic used to basically be, remove the extra padding at the same time the tab gets narrow enough to hide the close button.  Somehow those two got a bit divorced.  It's been tweaked a couple of times for different bug fixes, you'll want to look at the blame history to see which bugs people were attempting to fix, to make sure you don't repeat those (hopefully we added regression tests but who knows).

Apologies if I'm wrong but is  issue 848438  the same as this?  

 Issue 848438  has been merged into this issue.

also it's not just the audio indicator that causes the favicon to twitch, the recording icon too (in the attached screenshot, I was leaving a voice message on WhatsApp)

Deleted: favitwitch.png 11.6 KB

	favitwitch.png 11.6 KB View Download

 Issue 862790  has been merged into this issue.

Triage: How's this going?

It's in review, but I'm making Bret's life hard*, so it's not landed yet.

*About actual behavior, not just things like style or technical debt.

 Issue 864626  has been merged into this issue.

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

commit bb53e26734e70a1f28d0d1d3fe16f2a7eaf5ca27
Author: Bret Sepulveda <bsep@chromium.org>
Date: Tue Jul 17 22:10:09 2018

Increase size at which close buttons on inactive tabs disappear.

This patch changes the size at which close buttons disappear from being
approximately 48 dips of content area to being a fixed constant. As a
first pass I've set the constant to 68 dips, which feels good to me.

This patch also ties the logic for when to show extra left padding to
the close button visibility, which addresses cases where the favicon
would "twitch" due to the alert indicator appearing.

Bug:  853788 ,  863027 
Change-Id: I681f8b7d530b192f2fd5943436ae16bd93f33fba
Reviewed-on: https://chromium-review.googlesource.com/1125315
Reviewed-by: Peter Kasting <pkasting@chromium.org>
Commit-Queue: Bret Sepulveda <bsep@chromium.org>
Cr-Commit-Position: refs/heads/master@{#575795}
[modify] https://crrev.com/bb53e26734e70a1f28d0d1d3fe16f2a7eaf5ca27/chrome/browser/ui/views/tabs/tab.cc
[modify] https://crrev.com/bb53e26734e70a1f28d0d1d3fe16f2a7eaf5ca27/chrome/browser/ui/views/tabs/tab.h
[modify] https://crrev.com/bb53e26734e70a1f28d0d1d3fe16f2a7eaf5ca27/chrome/browser/ui/views/tabs/tab_strip_unittest.cc
[modify] https://crrev.com/bb53e26734e70a1f28d0d1d3fe16f2a7eaf5ca27/chrome/browser/ui/views/tabs/tab_unittest.cc