Use animated vector icon for app menu notification animation.

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

 // Copyright 2013 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.
 
 #ifndef CHROME_BROWSER_UI_VIEWS_TOOLBAR_APP_MENU_BUTTON_H_
 #define CHROME_BROWSER_UI_VIEWS_TOOLBAR_APP_MENU_BUTTON_H_
 
 #include <memory>
 
 #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"
 
 class AppMenu;
 class AppMenuModel;
 
 namespace views {
 class LabelButtonBorder;
 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;
 
   void SetSeverity(AppMenuIconController::IconType type,
                    AppMenuIconController::Severity severity,
                    bool animate);
 
   // Shows the app menu. |for_drop| indicates whether the menu is opened for a
   // drag-and-drop operation.
   void ShowMenu(bool for_drop);
 
   // Closes the app menu, if it's open.
   void CloseMenu();
 
   AppMenu* app_menu_for_testing() { return menu_.get(); }
 
   // Whether the app/hotdogs menu is currently showing.
   bool IsMenuShowing() const;
 
   // Adds a listener to receive a callback when the menu opens.
   void AddMenuListener(views::MenuListener* listener);
 
   // Removes a menu listener.
   void RemoveMenuListener(views::MenuListener* listener);
 
   // 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,
                      content::WebContents* contents,
                      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);
 
   // Sets |margin_trailing_| when the browser is maximized and updates layout
   // to make the focus rectangle centered.
   void SetTrailingMargin(int margin);
 
   // Animates the icon if possible. The icon will not animate if the severity
   // level is none, |animation_| is nullptr or |should_use_new_icon_| is false.
@@ skipping 27 matching lines @@
 
   // Listeners to call when the menu opens.
   base::ObserverList<views::MenuListener> menu_listeners_;
 
   // App model and menu.
   // Note that the menu should be destroyed before the model it uses, so the
   // menu should be listed later.
   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_;
 
   DISALLOW_COPY_AND_ASSIGN(AppMenuButton);
 };
 
 #endif  // CHROME_BROWSER_UI_VIEWS_TOOLBAR_APP_MENU_BUTTON_H_