[Views] App Menu Icon Update

chrome/browser/ui/views/toolbar/app_menu_button.cc

Index: chrome/browser/ui/views/toolbar/app_menu_button.cc
diff --git a/chrome/browser/ui/views/toolbar/app_menu_button.cc b/chrome/browser/ui/views/toolbar/app_menu_button.cc
index 6b3b5bd66b3559e05674523ffcce1f066944ba5a..69a7056dc95b8114cb4d9f94e2470ad8a08d5962 100644
--- a/chrome/browser/ui/views/toolbar/app_menu_button.cc
+++ b/chrome/browser/ui/views/toolbar/app_menu_button.cc
@@ -47,22 +47,17 @@ AppMenuButton::AppMenuButton(ToolbarView* toolbar_view)
       severity_(AppMenuIconController::Severity::NONE),
       type_(AppMenuIconController::IconType::NONE),
       toolbar_view_(toolbar_view),
+      should_use_new_icon_(false),
       margin_trailing_(0),
       weak_factory_(this) {
   SetInkDropMode(InkDropMode::ON);
   SetFocusPainter(nullptr);
 
   base::CommandLine* command_line = base::CommandLine::ForCurrentProcess();
-  if (command_line->HasSwitch(switches::kAppMenuIcon)) {
-    std::string flag =
-        command_line->GetSwitchValueASCII(switches::kAppMenuIcon);
-    if (flag == switches::kAppMenuIconPersistentClosedState) {
-      Browser* browser = toolbar_view_->browser();
-      browser->tab_strip_model()->AddObserver(this);
-      animation_ = base::MakeUnique<AppMenuAnimation>(this, true);
-    } else if (flag == switches::kAppMenuIconPersistentOpenedState) {
-      animation_ = base::MakeUnique<AppMenuAnimation>(this, false);
-    }
+  if (command_line->HasSwitch(switches::kEnableNewAppMenuIcon)) {
+    Browser* browser = toolbar_view_->browser();

[msw, 2017/04/28 21:39:12]

nit: inline this

[spqchan, 2017/04/29 00:36:33]

Done.

+    browser->tab_strip_model()->AddObserver(this);
+    should_use_new_icon_ = true;
   }
 }
 
@@ -109,8 +104,7 @@ void AppMenuButton::ShowMenu(bool for_drop) {
                         base::TimeTicks::Now() - menu_open_time);
   }
 
-  if (animation_ && severity_ != AppMenuIconController::Severity::NONE)
-    animation_->StartAnimation();
+  AnimateIconIfPossible();
 }
 
 void AppMenuButton::CloseMenu() {
@@ -162,8 +156,7 @@ void AppMenuButton::TabInsertedAt(TabStripModel* tab_strip_model,
                                   content::WebContents* contents,
                                   int index,
                                   bool foreground) {
-  if (severity_ != AppMenuIconController::Severity::NONE)
-    animation_->StartAnimation();
+  AnimateIconIfPossible();
 }
 
 void AppMenuButton::UpdateIcon(bool should_animate) {
@@ -189,10 +182,17 @@ void AppMenuButton::UpdateIcon(bool should_animate) {
       break;
   }
 
-  if (animation_) {
-    animation_->SetIconColors(toolbar_icon_color, severity_color);
+  if (should_use_new_icon_) {
+    // |animation_| is created here instead of the constructor because the

[msw, 2017/04/28 21:39:13]

Perhaps the old pattern of setting both colors here was simpler?

[msw, 2017/04/28 21:41:02]

Or is this meant to animate from one severity color to another?

[spqchan, 2017/04/29 00:36:33]

The old pattern of setting both colors was causing an issue, since it would
reset the starting color to "toolbar button icon". This would cause an issue
where if the severity is updated from low to critical, the icon will animate
from grey to red instead of a green to red

+    // theme provider is necessary to provide the correct initial toolbar
+    // icon color.
+    if (!animation_) {

[msw, 2017/04/28 21:39:12]

nit: curlies not needed

[spqchan, 2017/04/29 00:36:33]

Done.

+      animation_ = base::MakeUnique<AppMenuAnimation>(this, toolbar_icon_color);
+    }
+    animation_->set_severity_color(severity_color);
     if (should_animate)
-      animation_->StartAnimation();
+      AnimateIconIfPossible();
+
     return;
   }
 
@@ -221,6 +221,17 @@ void AppMenuButton::SetTrailingMargin(int margin) {
   InvalidateLayout();
 }
 
+void AppMenuButton::AnimateIconIfPossible() {
+  if (!should_use_new_icon_)

[msw, 2017/04/28 21:39:12]

optional nit: combine conditionals instead of using separate early returns.

[spqchan, 2017/04/29 00:36:33]

Done.

+    return;
+
+  if (severity_ == AppMenuIconController::Severity::NONE)
+    return;
+
+  if (animation_)
+    animation_->StartAnimation();
+}
+
 void AppMenuButton::AppMenuAnimationStarted() {
   SetPaintToLayer();
   layer()->SetFillsBoundsOpaquely(false);