Wrench Icon: Only animate low severity status once

chrome/browser/ui/views/toolbar_view.cc

 // Copyright 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "chrome/browser/ui/views/toolbar_view.h"
 
 #include "base/debug/trace_event.h"
 #include "base/i18n/number_formatting.h"
 #include "base/prefs/pref_service.h"
 #include "base/utf_string_conversions.h"
@@ skipping 797 matching lines @@
 
 void ToolbarView::UpdateWrenchButtonSeverity() {
   // Keep track of whether we were showing the badge before, so we don't send
   // multiple UMA events for example when multiple Chrome windows are open.
   static bool incompatibility_badge_showing = false;
   // Save the old value before resetting it.
   bool was_showing = incompatibility_badge_showing;
   incompatibility_badge_showing = false;
 
   if (ShouldShowUpgradeRecommended()) {
-    app_menu_->SetSeverity(WrenchIconPainter::SeverityFromUpgradeLevel(
-        UpgradeDetector::GetInstance()->upgrade_notification_stage()));
+    WrenchIconPainter::Severity severity =
+        WrenchIconPainter::SeverityFromUpgradeLevel(
+            UpgradeDetector::GetInstance()->upgrade_notification_stage());
+    bool animate = true;
+    if (severity == WrenchIconPainter::SEVERITY_LOW) {

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

Could we consolidate this code in WrenchIconPainter rather than here and the mac
code?

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

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

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

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

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

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]

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

+      // Only animate low severity upgrades once.
+      static bool s_should_animate = true;
+      animate = s_should_animate;
+      s_should_animate = false;
+    }
+    app_menu_->SetSeverity(severity, animate);
     return;
   }
 
   if (ShouldShowIncompatibilityWarning()) {
     if (!was_showing)
       content::RecordAction(UserMetricsAction("ConflictBadge"));
-    app_menu_->SetSeverity(WrenchIconPainter::SEVERITY_HIGH);
+    app_menu_->SetSeverity(WrenchIconPainter::SEVERITY_HIGH, true);
     incompatibility_badge_showing = true;
     return;
   }
 
   GlobalErrorService* service =
       GlobalErrorServiceFactory::GetForProfile(browser_->profile());
   GlobalError* error =
       service->GetHighestSeverityGlobalErrorWithWrenchMenuItem();
   if (error) {
     app_menu_->SetSeverity(WrenchIconPainter::SeverityFromGlobalErrorSeverity(
-        error->GetSeverity()));
+        error->GetSeverity()), true);
     return;
   }
 
-  app_menu_->SetSeverity(WrenchIconPainter::SEVERITY_NONE);
+  app_menu_->SetSeverity(WrenchIconPainter::SEVERITY_NONE, true);
 }
 
 void ToolbarView::OnShowHomeButtonChanged() {
   Layout();
   SchedulePaint();
 }
 
 int ToolbarView::content_shadow_height() const {
   return browser_->host_desktop_type() == chrome::HOST_DESKTOP_TYPE_ASH ?
       kContentShadowHeightAsh : kContentShadowHeight;
 }