Fix favicon disappearing on pinned tabs

Fix favicon disappearing on pinned tabs

Because of https://codereview.chromium.org/1566313002, layout no longer
happens during paint, so any operations that require a layout need to
explicitly request one, rather than just scheduling a paint.

This updates the MediaIndicatorButton to update the parent tab whenever
the media state changes, such as during an animation.

BUG=581202
Committed: https://crrev.com/db5cc6687e396fc77282e2c32c39731e20d4fb94
Cr-Commit-Position: refs/heads/master@{#373384}

[enne (OOO), 2016-02-01 19:56:47]

enne@chromium.org changed reviewers:
+ miu@chromium.org, pkasting@chromium.org

[enne (OOO), 2016-02-01 19:58:51]

Oops, a layout was all that was needed after all.

I added a more specific function to tab than just calling layout.  If you want
me to just call layout directly, I could do that too.  I'm mostly following
sky's guidance on my previous CL to add a separate function here.

[miu, 2016-02-01 20:28:33]

lgtm % one thing:

https://codereview.chromium.org/1653113002/diff/1/chrome/browser/ui/views/tab...
File chrome/browser/ui/views/tabs/media_indicator_button.cc (right):

https://codereview.chromium.org/1653113002/diff/1/chrome/browser/ui/views/tab...
chrome/browser/ui/views/tabs/media_indicator_button.cc:80: bool
previous_media_showing_state = showing_media_state_;
The type should be TabMediaState, not bool.

[enne (OOO), 2016-02-01 20:36:33]

https://codereview.chromium.org/1653113002/diff/1/chrome/browser/ui/views/tab...
File chrome/browser/ui/views/tabs/media_indicator_button.cc (right):

https://codereview.chromium.org/1653113002/diff/1/chrome/browser/ui/views/tab...
chrome/browser/ui/views/tabs/media_indicator_button.cc:80: bool
previous_media_showing_state = showing_media_state_;
On 2016/02/01 at 20:28:32, miu wrote:
> The type should be TabMediaState, not bool.

Eep! Thanks.

[enne (OOO), 2016-02-01 20:36:45]

The CQ bit was checked by enne@chromium.org

[enne (OOO), 2016-02-01 20:36:46]

The patchset sent to the CQ was uploaded after l-g-t-m from miu@chromium.org
Link to the patchset: https://codereview.chromium.org/1653113002/#ps20001
(title: "Change bool => TabMediaState OOPS")

[commit-bot: I haz the power, 2016-02-01 20:39:24]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1653113002/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1653113002/20001

[sky, 2016-02-01 20:51:15]

sky@chromium.org changed reviewers:
+ sky@chromium.org

[sky, 2016-02-01 20:51:15]

LGTM - it would be good to add test coverage of this.

[commit-bot: I haz the power, 2016-02-01 20:52:00]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-02-01 20:52:01]

Try jobs failed on following builders:
  chromeos_amd64-generic_chromium_compile_only_ng on tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...)
  chromeos_daisy_chromium_compile_only_ng on tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...)
  chromeos_x86-generic_chromium_compile_only_ng on tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_x86-ge...)

[Peter Kasting, 2016-02-03 00:06:07]

LGTM.  I'd probably have just called Layout() directly, but if sky prefers this
route that's fine.

[enne (OOO), 2016-02-03 01:02:04]

On 2016/02/01 at 20:51:15, sky wrote:
> LGTM - it would be good to add test coverage of this.

Sure, sure.  Added a test.  I'm not super happy with it, so PTAL.

I don't like "sleep for a hardcoded amount of time until the animation
finishes", but it felt silly to pipe through a custom animation time and
animation observers to the fade animation when nothing else needed it.  I also
cribbed the test from TabStripTest somewhat.  I could make a
tab_strip_test.{h,cc} files that both include from too, but it's pretty short so
also felt unneeded.  Happy to change any of this or take pointers on how to make
it better, but it does test this change.

[Peter Kasting, 2016-02-03 01:18:42]

On 2016/02/03 01:02:04, enne wrote:
> On 2016/02/01 at 20:51:15, sky wrote:
> > LGTM - it would be good to add test coverage of this.
> 
> Sure, sure.  Added a test.  I'm not super happy with it, so PTAL.
> 
> I don't like "sleep for a hardcoded amount of time until the animation
> finishes", but it felt silly to pipe through a custom animation time and
> animation observers to the fade animation when nothing else needed it.  I also
> cribbed the test from TabStripTest somewhat.  I could make a
> tab_strip_test.{h,cc} files that both include from too, but it's pretty short
so
> also felt unneeded.  Happy to change any of this or take pointers on how to
make
> it better, but it does test this change.

I'm exceedingly un-thrilled with Sleep()s in tests.  I would rather have no test
than that.

Do we have to test at such a high level that we have a fadeout at all?  Can we
do a lower-level test that basically just fires the various animation state
changes and verifies that the class does a new layout in response?

[enne (OOO), 2016-02-03 01:30:02]

On 2016/02/03 at 01:18:42, pkasting wrote:

> Do we have to test at such a high level that we have a fadeout at all?  Can we
do a lower-level test that basically just fires the various animation state
changes and verifies that the class does a new layout in response?

The MediaIndicatorButton fires off a fade animation when changing states, and
it's the animation itself that then changes the button state at the end and does
the layout.  Even doing that, it seems like I'd have to run the animation
somehow, which is back to run loops and sleeping.  Personally, I think just
testing that the animation controller does a layout is too fine of a test to
actually catch any real regressions in the future.

[Peter Kasting, 2016-02-03 01:44:35]

I would probably not add a test at all here then.

Perhaps Scott has better feedback.

[sky, 2016-02-03 16:41:13]

Can you add a way for the test to alter the animation? Or a test only
function you can call to force it to end?

On Tue, Feb 2, 2016 at 5:44 PM,  <pkasting@chromium.org> wrote:
> I would probably not add a test at all here then.
>
> Perhaps Scott has better feedback.
>
> https://codereview.chromium.org/1653113002/

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[enne (OOO), 2016-02-03 19:11:37]

How about this in terms of stopping the animation directly?

[sky, 2016-02-03 22:02:13]

LGTM

https://codereview.chromium.org/1653113002/diff/60001/chrome/browser/ui/views...
File chrome/browser/ui/views/tabs/media_indicator_button_unittest.cc (right):

https://codereview.chromium.org/1653113002/diff/60001/chrome/browser/ui/views...
chrome/browser/ui/views/tabs/media_indicator_button_unittest.cc:1: // Copyright
(c) 2016 The Chromium Authors. All rights reserved.
nit: no (c)

[enne (OOO), 2016-02-03 22:13:18]

https://codereview.chromium.org/1653113002/diff/60001/chrome/browser/ui/views...
File chrome/browser/ui/views/tabs/media_indicator_button_unittest.cc (right):

https://codereview.chromium.org/1653113002/diff/60001/chrome/browser/ui/views...
chrome/browser/ui/views/tabs/media_indicator_button_unittest.cc:1: // Copyright
(c) 2016 The Chromium Authors. All rights reserved.
On 2016/02/03 at 22:02:13, sky wrote:
> nit: no (c)

Done.  Thanks!

[enne (OOO), 2016-02-03 22:13:24]

The CQ bit was checked by enne@chromium.org

[enne (OOO), 2016-02-03 22:13:24]

The patchset sent to the CQ was uploaded after l-g-t-m from
pkasting@chromium.org, miu@chromium.org, sky@chromium.org
Link to the patchset: https://codereview.chromium.org/1653113002/#ps80001
(title: "Fix copyright; fix variable names")

[commit-bot: I haz the power, 2016-02-03 22:14:01]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1653113002/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1653113002/80001

[Peter Kasting, 2016-02-03 22:36:39]

LGTM

https://codereview.chromium.org/1653113002/diff/80001/chrome/browser/ui/views...
File chrome/browser/ui/views/tabs/media_indicator_button_unittest.cc (right):

https://codereview.chromium.org/1653113002/diff/80001/chrome/browser/ui/views...
chrome/browser/ui/views/tabs/media_indicator_button_unittest.cc:28: // Do this
to force TabStrip to create the buttons.
Nit: How about "The tab strip must be added to the view hierarchy for it to
create the buttons."

https://codereview.chromium.org/1653113002/diff/80001/chrome/browser/ui/views...
chrome/browser/ui/views/tabs/media_indicator_button_unittest.cc:43:
widget_.reset();
Either omit this or comment as to why it has to explicitly happen.

https://codereview.chromium.org/1653113002/diff/80001/chrome/browser/ui/views...
chrome/browser/ui/views/tabs/media_indicator_button_unittest.cc:50: bool
ShowingMediaIndicator(Tab* tab) const {
Nit: Inline accessors like this should normally be named unix_hacker()-style

https://codereview.chromium.org/1653113002/diff/80001/chrome/browser/ui/views...
chrome/browser/ui/views/tabs/media_indicator_button_unittest.cc:53: void
StopAnimation(Tab* tab) {
Nit: Blank line above

[enne (OOO), 2016-02-03 22:40:50]

The CQ bit was unchecked by enne@chromium.org

[enne (OOO), 2016-02-03 22:46:46]

https://codereview.chromium.org/1653113002/diff/80001/chrome/browser/ui/views...
File chrome/browser/ui/views/tabs/media_indicator_button_unittest.cc (right):

https://codereview.chromium.org/1653113002/diff/80001/chrome/browser/ui/views...
chrome/browser/ui/views/tabs/media_indicator_button_unittest.cc:28: // Do this
to force TabStrip to create the buttons.
On 2016/02/03 at 22:36:39, Peter Kasting wrote:
> Nit: How about "The tab strip must be added to the view hierarchy for it to
create the buttons."

Sure.

https://codereview.chromium.org/1653113002/diff/80001/chrome/browser/ui/views...
chrome/browser/ui/views/tabs/media_indicator_button_unittest.cc:43:
widget_.reset();
On 2016/02/03 at 22:36:39, Peter Kasting wrote:
> Either omit this or comment as to why it has to explicitly happen.

Left a comment.

https://codereview.chromium.org/1653113002/diff/80001/chrome/browser/ui/views...
chrome/browser/ui/views/tabs/media_indicator_button_unittest.cc:50: bool
ShowingMediaIndicator(Tab* tab) const {
On 2016/02/03 at 22:36:39, Peter Kasting wrote:
> Nit: Inline accessors like this should normally be named unix_hacker()-style

The coding style says this is optional, but sure.

https://codereview.chromium.org/1653113002/diff/80001/chrome/browser/ui/views...
chrome/browser/ui/views/tabs/media_indicator_button_unittest.cc:53: void
StopAnimation(Tab* tab) {
On 2016/02/03 at 22:36:39, Peter Kasting wrote:
> Nit: Blank line above

Sure.

[enne (OOO), 2016-02-03 22:48:54]

The CQ bit was checked by enne@chromium.org

[enne (OOO), 2016-02-03 22:48:54]

The patchset sent to the CQ was uploaded after l-g-t-m from
pkasting@chromium.org, miu@chromium.org, sky@chromium.org
Link to the patchset: https://codereview.chromium.org/1653113002/#ps100001
(title: "pkasting comments")

[commit-bot: I haz the power, 2016-02-03 22:50:03]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1653113002/100001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1653113002/100001

[commit-bot: I haz the power, 2016-02-03 23:38:07]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2016-02-03 23:39:03]

Description was changed from

==========
Fix favicon disappearing on pinned tabs

Because of https://codereview.chromium.org/1566313002, layout no longer
happens during paint, so any operations that require a layout need to
explicitly request one, rather than just scheduling a paint.

This updates the MediaIndicatorButton to update the parent tab whenever
the media state changes, such as during an animation.

BUG=581202
==========

to

==========
Fix favicon disappearing on pinned tabs

Because of https://codereview.chromium.org/1566313002, layout no longer
happens during paint, so any operations that require a layout need to
explicitly request one, rather than just scheduling a paint.

This updates the MediaIndicatorButton to update the parent tab whenever
the media state changes, such as during an animation.

BUG=581202

Committed: https://crrev.com/db5cc6687e396fc77282e2c32c39731e20d4fb94
Cr-Commit-Position: refs/heads/master@{#373384}
==========

[commit-bot: I haz the power, 2016-02-03 23:39:04]

Patchset 6 (id:??) landed as
https://crrev.com/db5cc6687e396fc77282e2c32c39731e20d4fb94
Cr-Commit-Position: refs/heads/master@{#373384}