Use animated vector icon for app menu notification animation.

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

Index: chrome/browser/ui/views/toolbar/app_menu_button.h
diff --git a/chrome/browser/ui/views/toolbar/app_menu_button.h b/chrome/browser/ui/views/toolbar/app_menu_button.h
index b8fc2befaa068c6da278befeef99c5518773fea4..8166c1f9e8abae2fdee1ced215a3bb5aa9a93190 100644
--- a/chrome/browser/ui/views/toolbar/app_menu_button.h
+++ b/chrome/browser/ui/views/toolbar/app_menu_button.h
@@ -10,8 +10,8 @@
 #include "base/macros.h"
 #include "base/memory/weak_ptr.h"
 #include "chrome/browser/ui/tabs/tab_strip_model_observer.h"
-#include "chrome/browser/ui/toolbar/app_menu_animation.h"
 #include "chrome/browser/ui/toolbar/app_menu_icon_controller.h"
+#include "ui/views/controls/animated_icon_view.h"
 #include "ui/views/controls/button/menu_button.h"
 #include "ui/views/controls/button/menu_button_listener.h"
 #include "ui/views/view.h"
@@ -26,9 +26,7 @@ class MenuListener;
 
 class ToolbarView;
 
-class AppMenuButton : public views::MenuButton,
-                      public TabStripModelObserver,
-                      public AppMenuAnimationDelegate {
+class AppMenuButton : public views::MenuButton, public TabStripModelObserver {
  public:
   explicit AppMenuButton(ToolbarView* toolbar_view);
   ~AppMenuButton() override;
@@ -58,7 +56,7 @@ class AppMenuButton : public views::MenuButton,
   // views::MenuButton:
   gfx::Size GetPreferredSize() const override;
   void Layout() override;
-  void OnPaint(gfx::Canvas* canvas) override;
+  void OnThemeChanged() override;
 
   // TabStripObserver:
   void TabInsertedAt(TabStripModel* tab_strip_model,
@@ -66,11 +64,6 @@ class AppMenuButton : public views::MenuButton,
                      int index,
                      bool foreground) override;
 
-  // AppMenuAnimationDelegate:
-  void AppMenuAnimationStarted() override;
-  void AppMenuAnimationEnded() override;
-  void InvalidateIcon() override;
-
   // Updates the presentation according to |severity_| and the theme provider.
   // If |should_animate| is true, the icon should animate.
   void UpdateIcon(bool should_animate);
@@ -118,15 +111,18 @@ class AppMenuButton : public views::MenuButton,
   std::unique_ptr<AppMenuModel> menu_model_;
   std::unique_ptr<AppMenu> menu_;
 
-  // Used for animating and drawing the app menu icon.
-  std::unique_ptr<AppMenuAnimation> animation_;
+  // The view which displays three dots and animates to a hamburger-ish icon
+  // when there's a need to alert the user. TODO(estade): rename to

[Peter Kasting, 2017/05/31 02:08:29]

Nit: Can this first sentence just be "The menu icon." to avoid the future
maintenance hazard if we change the appearance without tweaking the verbiage
here to match?

[Evan Stade, 2017/05/31 17:08:16]

I understand the concern but I think specificity is useful for people who are
less familiar with the code. When I'm reading through UI code I'm unfamiliar
with, I find concrete descriptions are very useful. Hopefully we won't let this
get out of date.

[Peter Kasting, 2017/05/31 17:24:31]

I think there's value in describing what this is, but I think that makes sense
to do in a comment above the top of the class.  I wouldn't describe the visual
look on the member down here, just because it's a lot less likely that's where
someone is looking or would update if things change.

[Evan Stade, 2017/05/31 20:19:13]

Seems like it could go either way. If you're changing the appearance
dramatically you might remove this view completely and naturally delete or
change this comment, whereas you might never look at or update the class-level
comment. In any case I've promoted the comment to be class-level.

+  // |animated_icon_| when |should_use_new_icon_| defaults to true and is
+  // removed.
+  views::AnimatedIconView* new_icon_ = nullptr;

[Peter Kasting, 2017/05/31 02:08:29]

Nit: I really like initing members in the declaration like this.  But I'm not so
big a fan of doing it for some members and not others (e.g. |severity_|,
|type_|, |toolbar_view_|), because it's inconsistent and still sort of
error-prone.  I suggest (either in this CL or another) moving all the other
initializations here as well.  (Note that even |weak_factory_| can be inited
here using {}; see
https://cs.chromium.org/chromium/src/chrome/browser/ui/views/chrome_views_del...
.)

[Evan Stade, 2017/05/31 17:08:16]

good call. Updated all except toolbar_view_, which gets inited to the parameter
passed into the ctor. In that case, it doesn't seem like inline initing adds
much except the false notion that the member starts out as null.

 
   // True if the app menu should use the new animated icon.
-  bool should_use_new_icon_;
+  bool should_use_new_icon_ = false;
 
   // Any trailing margin to be applied. Used when the browser is in
   // a maximized state to extend to the full window width.
-  int margin_trailing_;
+  int margin_trailing_ = 0;
 
   // Used to spawn weak pointers for delayed tasks to open the overflow menu.
   base::WeakPtrFactory<AppMenuButton> weak_factory_;