Integrate new MD update menu icons.

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 9db62bc1d38cccf88dee9237922739e7215db784..6cc19841f1ee1832cd9e6386fa581841ac2c020f 100644
--- a/chrome/browser/ui/views/toolbar/app_menu_button.cc
+++ b/chrome/browser/ui/views/toolbar/app_menu_button.cc
@@ -11,6 +11,7 @@
 #include "base/time/time.h"
 #include "chrome/browser/themes/theme_properties.h"
 #include "chrome/browser/ui/browser.h"
+#include "chrome/browser/ui/browser_otr_state.h"
 #include "chrome/browser/ui/layout_constants.h"
 #include "chrome/browser/ui/toolbar/app_menu_model.h"
 #include "chrome/browser/ui/views/extensions/browser_action_drag_data.h"
@@ -50,10 +51,12 @@ AppMenuButton::AppMenuButton(ToolbarView* toolbar_view)
 
 AppMenuButton::~AppMenuButton() {}
 
-void AppMenuButton::SetSeverity(AppMenuIconPainter::Severity severity,
+void AppMenuButton::SetSeverity(AppMenuBadgeController::BadgeType type,
+                                AppMenuIconPainter::Severity severity,
                                 bool animate) {
   if (ui::MaterialDesignController::IsModeMaterial()) {
     severity_ = severity;
+    type_ = type;
     UpdateIcon();
     return;
   }
@@ -151,8 +154,25 @@ void AppMenuButton::UpdateIcon() {
       break;
   }
 
+  gfx::VectorIconId icon_id = gfx::VectorIconId::VECTOR_ICON_NONE;
+  switch (type_) {
+    case AppMenuBadgeController::BadgeType::NONE:
+      icon_id = gfx::VectorIconId::BROWSER_TOOLS;
+      DCHECK(severity_ == AppMenuIconPainter::SEVERITY_NONE);

[msw, 2016/06/03 22:34:37]

nit: DCHECK_EQ?

[kylix_rd, 2016/06/03 22:42:45]

I was completely unaware such a beast existed. Will Change.

+      break;
+    case AppMenuBadgeController::BadgeType::UPGRADE_NOTIFICATION:
+      icon_id = gfx::VectorIconId::UPDATE_MENU_SEVERITY_LOW_MEDIUM_HIGH;

[msw, 2016/06/03 22:34:37]

nit: these names aren't terribly straightforward. How does LOW_MEDIUM_HIGH
compare to WARNING? Should they use be prefixed with BROWSER_TOOLS to match the
un-'badged' icon? Should they map directly to the BadgeType enum names? Should
UPDATE_MENU_FAILED instead be UPDATE_MENU_SEVERITY_FAILED, to match the
UPDATE_MENU_SEVERITY prefix of the others? Sorry I don't quite understand the
naming pattern and there are no comments on what the look like or really mean.

[kylix_rd, 2016/06/03 22:42:45]

I used the names (or derivatives thereof) straight from the designer as per the
discussion in this bug:
https://bugs.chromium.org/p/chromium/issues/detail?id=584342

I'm otherwise perfectly willing to make naming changes... I just wasn't sure the
protocol was and if such things should be done outside the designer's purview.

[msw, 2016/06/03 23:01:17]

Hmm, that might be okay, but it adds another layer of indirection between badge
types and their visual implementations. I'd suggest re-using the existing
BadgeType enum value names or re-naming the BadgeType enum values to match the
new icons names, or picking some new set of names that uses the best of both
worlds. Maybe prefix with APP_MENU_ (or similar) instead of UPDATE_MENU_? Evan
might have better advice, but I'd suggest something like this:

APP_MENU_NORMAL,    // The un-badged three-dots chrome app menu icon.
APP_MENU_UPDATE,    // The arrow shown when an update is available.
APP_MENU_EXTENSION, // The puzzle piece shown for extension messages.
APP_MENU_WARNING,   // The exclamation shown for incompatibility warnings.
APP_MENU_ERROR,     // The horizontal dash shown for global errors.

+      break;
+    case AppMenuBadgeController::BadgeType::GLOBAL_ERROR:
+      icon_id = gfx::VectorIconId::UPDATE_MENU_FAILED;
+      break;
+    case AppMenuBadgeController::BadgeType::INCOMPATIBILITY_WARNING:
+      icon_id = gfx::VectorIconId::UPDATE_MENU_SEVERITY_WARNING;
+      break;
+  }
+
   SetImage(views::Button::STATE_NORMAL,
-           gfx::CreateVectorIcon(gfx::VectorIconId::BROWSER_TOOLS, color));
+           gfx::CreateVectorIcon(icon_id, 16, color));

[msw, 2016/06/03 22:34:37]

Hmm, sad to see most related callers using magic numbers like this; maybe at
least do something like |const int kButtonIconSize = 16;|?

[kylix_rd, 2016/06/03 22:42:45]

Agreed. I think I asked at some point about whether there was a constant already
defined... IMO, it should be more universally available and used rather than
merely defined only within this module.

[Evan Stade, 2016/06/03 22:53:23]

Well most top chrome icons are specified in both 1x and 2x variants and when you
do that, you can just take the size from the .icon file. That's why we haven't
needed a kToolbarIconSize. There is kFaviconSize but it's merely a coincidence
that that's also 16.

[kylix_rd, 2016/06/03 23:00:47]

So should I look into creating 1x and 2x variants of these new icons then? These
files all have CANVAS_DIMENSIONS, 32, which I presume is a 2x icon.

[Evan Stade, 2016/06/03 23:04:40]

You might double check, but I assume Sebastien didn't think these needed a 1x/2x
variant. So we're stuck in this slightly annoying situation. That said, whether
we choose to hardcode one 16 here or a bunch of 16s in a bunch of .icon files
doesn't seem like a huge win either way.

One other option would be to change the .icons to size 16. You could do that by
adding a scaling transform to the .svg before running through svgo.

 }
 
 void AppMenuButton::SetTrailingMargin(int margin) {