Wrench Icon: Only animate low severity status once

Wrench Icon: Only animate low severity status once

Currently we animate icon status every time the user opens a new
window.

This CL changes the low severity update status to only animate for the
first window.

BUG=180107
TEST=Ran Chrome with --simulate-upgrade. Verified that the low
severity status was not animated when a new window was opened.

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=194714

[sky, 2013-04-15 13:50:28]

https://codereview.chromium.org/14245003/diff/1/chrome/browser/ui/views/toolb...
File chrome/browser/ui/views/toolbar_view.cc (right):

https://codereview.chromium.org/14245003/diff/1/chrome/browser/ui/views/toolb...
chrome/browser/ui/views/toolbar_view.cc:822: if (severity ==
WrenchIconPainter::SEVERITY_LOW) {
Could we consolidate this code in WrenchIconPainter rather than here and the mac
code?

[sail, 2013-04-15 15:58:49]

https://codereview.chromium.org/14245003/diff/1/chrome/browser/ui/views/toolb...
File chrome/browser/ui/views/toolbar_view.cc (right):

https://codereview.chromium.org/14245003/diff/1/chrome/browser/ui/views/toolb...
chrome/browser/ui/views/toolbar_view.cc:822: if (severity ==
WrenchIconPainter::SEVERITY_LOW) {
On 2013/04/15 13:50:28, sky wrote:
> Could we consolidate this code in WrenchIconPainter rather than here and the
mac
> code?

It's difficult to do because WrenchIconPainter can't distinguish between update
notifications and other notifications.

[sky, 2013-04-15 16:54:55]

https://codereview.chromium.org/14245003/diff/1/chrome/browser/ui/views/toolb...
File chrome/browser/ui/views/toolbar_view.cc (right):

https://codereview.chromium.org/14245003/diff/1/chrome/browser/ui/views/toolb...
chrome/browser/ui/views/toolbar_view.cc:822: if (severity ==
WrenchIconPainter::SEVERITY_LOW) {
On 2013/04/15 15:58:49, sail wrote:
> On 2013/04/15 13:50:28, sky wrote:
> > Could we consolidate this code in WrenchIconPainter rather than here and the
> mac
> > code?
> 
> It's difficult to do because WrenchIconPainter can't distinguish between
update
> notifications and other notifications.

Isn't this the only place that calls into WrenchIconPainter?

[sail, 2013-04-15 17:00:40]

https://codereview.chromium.org/14245003/diff/1/chrome/browser/ui/views/toolb...
File chrome/browser/ui/views/toolbar_view.cc (right):

https://codereview.chromium.org/14245003/diff/1/chrome/browser/ui/views/toolb...
chrome/browser/ui/views/toolbar_view.cc:822: if (severity ==
WrenchIconPainter::SEVERITY_LOW) {
On 2013/04/15 16:54:55, sky wrote:
> On 2013/04/15 15:58:49, sail wrote:
> > On 2013/04/15 13:50:28, sky wrote:
> > > Could we consolidate this code in WrenchIconPainter rather than here and
the
> > mac
> > > code?
> > 
> > It's difficult to do because WrenchIconPainter can't distinguish between
> update
> > notifications and other notifications.
> 
> Isn't this the only place that calls into WrenchIconPainter?

Right. We would still need to plumb extra information to it so that it would
that the notification is for an update.

So for example, we could do this:
  WrenchIconPainter::SetSeverity(Severity severity, Reason reason) {
    bool should_animate = true;
    if (reason == APP_UPDATE && severity == LOW_SERVERITY) {
      // only animate once
      ...
    }
    ...
  }
But I'd like to avoid pushing knowledge of app update into WrenchIconPainter.

[sail, 2013-04-15 21:38:43]

https://codereview.chromium.org/14245003/diff/1/chrome/browser/ui/views/toolb...
File chrome/browser/ui/views/toolbar_view.cc (right):

https://codereview.chromium.org/14245003/diff/1/chrome/browser/ui/views/toolb...
chrome/browser/ui/views/toolbar_view.cc:822: if (severity ==
WrenchIconPainter::SEVERITY_LOW) {
On 2013/04/15 17:00:40, sail wrote:
> On 2013/04/15 16:54:55, sky wrote:
> > On 2013/04/15 15:58:49, sail wrote:
> > > On 2013/04/15 13:50:28, sky wrote:
> > > > Could we consolidate this code in WrenchIconPainter rather than here and
> the
> > > mac
> > > > code?
> > > 
> > > It's difficult to do because WrenchIconPainter can't distinguish between
> > update
> > > notifications and other notifications.
> > 
> > Isn't this the only place that calls into WrenchIconPainter?
> 
> Right. We would still need to plumb extra information to it so that it would
> that the notification is for an update.
> 
> So for example, we could do this:
>   WrenchIconPainter::SetSeverity(Severity severity, Reason reason) {
>     bool should_animate = true;
>     if (reason == APP_UPDATE && severity == LOW_SERVERITY) {
>       // only animate once
>       ...
>     }
>     ...
>   }
> But I'd like to avoid pushing knowledge of app update into WrenchIconPainter.

Actually, moving this to a static function in WrenchIconPainter wasn't too bad!

[sky, 2013-04-15 21:42:00]

LGTM with the following change

https://codereview.chromium.org/14245003/diff/9001/chrome/browser/ui/toolbar/...
File chrome/browser/ui/toolbar/wrench_icon_painter.cc (right):

https://codereview.chromium.org/14245003/diff/9001/chrome/browser/ui/toolbar/...
chrome/browser/ui/toolbar/wrench_icon_painter.cc:60: static bool
s_should_animate = true;
s_should_animate -> should_animate

[sail, 2013-04-15 21:43:53]

https://codereview.chromium.org/14245003/diff/9001/chrome/browser/ui/toolbar/...
File chrome/browser/ui/toolbar/wrench_icon_painter.cc (right):

https://codereview.chromium.org/14245003/diff/9001/chrome/browser/ui/toolbar/...
chrome/browser/ui/toolbar/wrench_icon_painter.cc:60: static bool
s_should_animate = true;
On 2013/04/15 21:42:00, sky wrote:
> s_should_animate -> should_animate

Done.

[sail, 2013-04-17 23:11:50]

Committed patchset #3 manually as r194714 (presubmit successful).