Changes pinned tab throbbing indicator to a static image

Changes pinned tab throbbing indicator to a static image

The throbbing indicator consumes too much cpu on some devices and is
too subtle. See the spec attached to the bug for details on how this
looks in action.

BUG=473898
TEST=none
R=estade@chromium.org

Committed: https://crrev.com/725e93be4ee5c6dbaa7f0ae6ae9517027f4f297c
Cr-Commit-Position: refs/heads/master@{#401946}

[sky, 2016-06-24 16:47:51]

Description was changed from

==========
Changes pinned tab throbbing indicator to a static image

The throbbing indicator consumes too much cpu on some devices and is
too subtle. See the spec attached to the bug for details on how this
looks in action.

BUG=473898
TEST=none
R=estade@chromium.org
==========

to

==========
Changes pinned tab throbbing indicator to a static image

The throbbing indicator consumes too much cpu on some devices and is
too subtle. See the spec attached to the bug for details on how this
looks in action.

BUG=473898
TEST=none
R=estade@chromium.org
==========

[Evan Stade, 2016-06-24 17:09:39]

https://codereview.chromium.org/2091343002/diff/20001/chrome/browser/ui/views...
File chrome/browser/ui/views/tabs/tab.cc (right):

https://codereview.chromium.org/2091343002/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/tabs/tab.cc:1455: const int circle_x =
base::i18n::IsRTL() ? 0 : gfx::kFaviconSize;
the spec makes me think the indicator/crop area should actually be centered at
15.5 rather than 16

https://codereview.chromium.org/2091343002/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/tabs/tab.cc:1458: favicon =
gfx::ImageSkia(icon_canvas.ExtractImageRep());
I think it would be a little simpler (slightly fewer LOC, fewer conditional
blocks) to do

if (showing_pinned[...]) {
  DrawFavicon
  ClearArea
  DrawIndicator
} else {
  DrawFavicon
}

You don't get to share DrawFavicon but that's only one line anyway.

https://codereview.chromium.org/2091343002/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/tabs/tab.cc:1469: const SkColor kIndicatorColor =
SkColorSetRGB(0x42, 0x85, 0xF4);
I suspect you want NativeTheme::kColorId_CallToActionColor (which is this value
on the normal theme but differs in incognito or in GTK theme contexts)

[sky, 2016-06-24 17:41:15]

https://codereview.chromium.org/2091343002/diff/20001/chrome/browser/ui/views...
File chrome/browser/ui/views/tabs/tab.cc (right):

https://codereview.chromium.org/2091343002/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/tabs/tab.cc:1455: const int circle_x =
base::i18n::IsRTL() ? 0 : gfx::kFaviconSize;
On 2016/06/24 17:09:39, Evan Stade wrote:
> the spec makes me think the indicator/crop area should actually be centered at
> 15.5 rather than 16

I sent Sebastien images of what this code looks like and he liked it.

https://codereview.chromium.org/2091343002/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/tabs/tab.cc:1458: favicon =
gfx::ImageSkia(icon_canvas.ExtractImageRep());
On 2016/06/24 17:09:39, Evan Stade wrote:
> I think it would be a little simpler (slightly fewer LOC, fewer conditional
> blocks) to do
> 
> if (showing_pinned[...]) {
>   DrawFavicon
>   ClearArea
>   DrawIndicator
> } else {
>   DrawFavicon
> }
> 
> You don't get to share DrawFavicon but that's only one line anyway.

Only one line, but a long one:) I moved the indicator drawing into a separate
function. Better?

https://codereview.chromium.org/2091343002/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/tabs/tab.cc:1469: const SkColor kIndicatorColor =
SkColorSetRGB(0x42, 0x85, 0xF4);
On 2016/06/24 17:09:39, Evan Stade wrote:
> I suspect you want NativeTheme::kColorId_CallToActionColor (which is this
value
> on the normal theme but differs in incognito or in GTK theme contexts)

Done.

[Evan Stade, 2016-06-24 17:58:13]

https://codereview.chromium.org/2091343002/diff/20001/chrome/browser/ui/views...
File chrome/browser/ui/views/tabs/tab.cc (right):

https://codereview.chromium.org/2091343002/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/tabs/tab.cc:1458: favicon =
gfx::ImageSkia(icon_canvas.ExtractImageRep());
On 2016/06/24 17:41:15, sky wrote:
> On 2016/06/24 17:09:39, Evan Stade wrote:
> > I think it would be a little simpler (slightly fewer LOC, fewer conditional
> > blocks) to do
> > 
> > if (showing_pinned[...]) {
> >   DrawFavicon
> >   ClearArea
> >   DrawIndicator
> > } else {
> >   DrawFavicon
> > }
> > 
> > You don't get to share DrawFavicon but that's only one line anyway.
> 
> Only one line, but a long one:) I moved the indicator drawing into a separate
> function. Better?

yes, thanks

https://codereview.chromium.org/2091343002/diff/40001/chrome/browser/ui/views...
File chrome/browser/ui/views/tabs/tab.cc (right):

https://codereview.chromium.org/2091343002/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/tabs/tab.cc:1456:
indicator_paint.setXfermodeMode(SkXfermode::kSrcOver_Mode);
Fill and SrcOver are the defaults I believe. Did you specify them just to be
more explicit?

https://codereview.chromium.org/2091343002/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/tabs/tab.cc:1457: const int indicator_x =
GetMirroredXWithWidthInView(
I'm unclear why this x/y calculation isn't the same as the clear one --- aren't
they centered at the same point?

https://codereview.chromium.org/2091343002/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/tabs/tab.cc:1458: favicon_bounds_.right() -
kIndicatorRadius, kIndicatorRadius * 2);
confusing that favicon_bounds != favicon_bounds_

[sky, 2016-06-24 18:16:31]

https://codereview.chromium.org/2091343002/diff/40001/chrome/browser/ui/views...
File chrome/browser/ui/views/tabs/tab.cc (right):

https://codereview.chromium.org/2091343002/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/tabs/tab.cc:1456:
indicator_paint.setXfermodeMode(SkXfermode::kSrcOver_Mode);
On 2016/06/24 17:58:13, Evan Stade wrote:
> Fill and SrcOver are the defaults I believe. Did you specify them just to be
> more explicit?

Mostly because knowing the defaults requires looking at the source, which is
tedious. Updated.

https://codereview.chromium.org/2091343002/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/tabs/tab.cc:1457: const int indicator_x =
GetMirroredXWithWidthInView(
On 2016/06/24 17:58:13, Evan Stade wrote:
> I'm unclear why this x/y calculation isn't the same as the clear one ---
aren't
> they centered at the same point?

They are centered on the same absolute point, but the relatives point are
different. This is because circle_x on 1439 is relative to the temporary canvas
created on 1432 (icon_canvas). indicator_x/y is relative to the tab.

https://codereview.chromium.org/2091343002/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/tabs/tab.cc:1458: favicon_bounds_.right() -
kIndicatorRadius, kIndicatorRadius * 2);
On 2016/06/24 17:58:13, Evan Stade wrote:
> confusing that favicon_bounds != favicon_bounds_

Ya, that is confusing. Renamed to favicon_draw_bounds.

[Evan Stade, 2016-06-24 18:32:31]

https://codereview.chromium.org/2091343002/diff/40001/chrome/browser/ui/views...
File chrome/browser/ui/views/tabs/tab.cc (right):

https://codereview.chromium.org/2091343002/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/tabs/tab.cc:1457: const int indicator_x =
GetMirroredXWithWidthInView(
On 2016/06/24 18:16:31, sky wrote:
> On 2016/06/24 17:58:13, Evan Stade wrote:
> > I'm unclear why this x/y calculation isn't the same as the clear one ---
> aren't
> > they centered at the same point?
> 
> They are centered on the same absolute point, but the relatives point are
> different. This is because circle_x on 1439 is relative to the temporary
canvas
> created on 1432 (icon_canvas). indicator_x/y is relative to the tab.

ah, I missed that you're still using ExtractImageRep(). I guess clearing the
canvas directly messes up the background?

It seems like an alternative would be to temporarily add an inverted clip path
when drawing the favicon; then you could do everything directly on canvas and
share coordinate calculations. That might come out clearer, might not. Either
way lgtm.

[sky, 2016-06-24 19:07:26]

On 2016/06/24 18:32:31, Evan Stade wrote:
>
https://codereview.chromium.org/2091343002/diff/40001/chrome/browser/ui/views...
> File chrome/browser/ui/views/tabs/tab.cc (right):
> 
>
https://codereview.chromium.org/2091343002/diff/40001/chrome/browser/ui/views...
> chrome/browser/ui/views/tabs/tab.cc:1457: const int indicator_x =
> GetMirroredXWithWidthInView(
> On 2016/06/24 18:16:31, sky wrote:
> > On 2016/06/24 17:58:13, Evan Stade wrote:
> > > I'm unclear why this x/y calculation isn't the same as the clear one ---
> > aren't
> > > they centered at the same point?
> > 
> > They are centered on the same absolute point, but the relatives point are
> > different. This is because circle_x on 1439 is relative to the temporary
> canvas
> > created on 1432 (icon_canvas). indicator_x/y is relative to the tab.
> 
> ah, I missed that you're still using ExtractImageRep(). I guess clearing the
> canvas directly messes up the background?
> 
> It seems like an alternative would be to temporarily add an inverted clip path
> when drawing the favicon; then you could do everything directly on canvas and
> share coordinate calculations. That might come out clearer, might not. Either
> way lgtm.

Ya, I considered installing a clip, but what I did here seems a lot more
straightforward, so I went with it.

[sky, 2016-06-24 19:07:32]

The CQ bit was checked by sky@chromium.org

[commit-bot: I haz the power, 2016-06-24 19:08:05]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-06-24 20:09:58]

Description was changed from

==========
Changes pinned tab throbbing indicator to a static image

The throbbing indicator consumes too much cpu on some devices and is
too subtle. See the spec attached to the bug for details on how this
looks in action.

BUG=473898
TEST=none
R=estade@chromium.org
==========

to

==========
Changes pinned tab throbbing indicator to a static image

The throbbing indicator consumes too much cpu on some devices and is
too subtle. See the spec attached to the bug for details on how this
looks in action.

BUG=473898
TEST=none
R=estade@chromium.org
==========

[commit-bot: I haz the power, 2016-06-24 20:09:59]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-06-24 20:13:04]

Description was changed from

==========
Changes pinned tab throbbing indicator to a static image

The throbbing indicator consumes too much cpu on some devices and is
too subtle. See the spec attached to the bug for details on how this
looks in action.

BUG=473898
TEST=none
R=estade@chromium.org
==========

to

==========
Changes pinned tab throbbing indicator to a static image

The throbbing indicator consumes too much cpu on some devices and is
too subtle. See the spec attached to the bug for details on how this
looks in action.

BUG=473898
TEST=none
R=estade@chromium.org

Committed: https://crrev.com/725e93be4ee5c6dbaa7f0ae6ae9517027f4f297c
Cr-Commit-Position: refs/heads/master@{#401946}
==========

[commit-bot: I haz the power, 2016-06-24 20:13:05]

Patchset 4 (id:??) landed as
https://crrev.com/725e93be4ee5c6dbaa7f0ae6ae9517027f4f297c
Cr-Commit-Position: refs/heads/master@{#401946}