Integrate new MD update menu icons.

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

 // 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.
 
 #include "chrome/browser/ui/views/toolbar/app_menu_button.h"
 
 #include "base/location.h"
 #include "base/metrics/histogram_macros.h"
 #include "base/single_thread_task_runner.h"
 #include "base/thread_task_runner_handle.h"
@@ skipping 116 matching lines @@
   return ResourceBundle::GetSharedInstance().
       GetImageSkiaNamed(IDR_TOOLBAR_BEZEL_HOVER)->size();
 }
 
 void AppMenuButton::ScheduleAppMenuIconPaint() {
   SchedulePaint();
 }
 
 void AppMenuButton::UpdateIcon() {
   DCHECK(ui::MaterialDesignController::IsModeMaterial());
+  gfx::VectorIconId icon_id = gfx::VectorIconId::BROWSER_TOOLS;
   SkColor color = gfx::kPlaceholderColor;
   switch (severity_) {
     case AppMenuIconPainter::SEVERITY_NONE:
       color = GetThemeProvider()->GetColor(
           ThemeProperties::COLOR_TOOLBAR_BUTTON_ICON);
       break;
     case AppMenuIconPainter::SEVERITY_LOW:
       color = gfx::kGoogleGreen700;
+      icon_id = gfx::VectorIconId::UPDATE_MENU_SEVERITY_LOW;
       break;
     case AppMenuIconPainter::SEVERITY_MEDIUM:
       color = gfx::kGoogleYellow700;
+      icon_id = gfx::VectorIconId::UPDATE_MENU_SEVERITY_MEDIUM;
       break;
     case AppMenuIconPainter::SEVERITY_HIGH:
       color = gfx::kGoogleRed700;
+      icon_id = gfx::VectorIconId::UPDATE_MENU_SEVERITY_HIGH;
       break;
   }

[kylix_rd, 2016/05/10 14:20:10]

TODO: Determine other state information to select "update failed", "new
extension" & "severity warning" icons.

Is this the right/wrong place for this?

[Evan Stade, 2016/05/10 16:48:14]

You're assuming the menu is changing color due to an update when that isn't
necessarily the case. See AppMenuBadgeController::BadgeType

I don't think fixing this should be a TODO because showing an upgrade arrow for
a global error strikes me as a regression.

[kylix_rd, 2016/05/10 17:49:01]

The AppMenuBadgeController doesn't update the BadgeType on the AppMenuButton
instance; only the severity is updated. Should this information now be plumbed
through to this class?
It's only a TODO in term of this review, not in terms of the overall patch.
Subsequent patches should resolve these issues.

[Evan Stade, 2016/05/10 18:04:13]

I suppose it must, or the logic of mapping state to icon should move out of this
class. I would actually consider merging the two enums because it looks like
GLOBAL_ERROR and INCOMPATIBILITY_WARNING imply a certain severity (medium) so
these aren't really tangential concepts any more. All of this is made more
difficult by the need to continue supporting pre-md.

Also I think we should worry about how this looks in incognito. You might want
slightly different numerical values there (e.g. kGoogleRed300 or something).
sg

[kylix_rd, 2016/05/10 18:10:51]

I've arrived at the same conclusion. A subsequent patch is forthcoming that
should accomplish this. Right now, I'm keeping the enumerations separate for, as
you said, simplicity in supporting pre-md.
 
Is Incognito mode available as a global state?
 

[Evan Stade, 2016/05/10 18:48:39]

no. First step would be to see if designers actually do want a different color
for incognito. Second step would probably involve putting the colors in
NativeTheme.

[kylix_rd, 2016/05/10 19:17:02]

Ah, so the question was more rhetorical and speculative than an actual decision
or direction to follow.

Do we want to do this now or drop in a TODO and assume this patch is
good-enough?

 
-  SetImage(views::Button::STATE_NORMAL,
-           gfx::CreateVectorIcon(gfx::VectorIconId::BROWSER_TOOLS, color));
+  SetImage(views::Button::STATE_NORMAL, gfx::CreateVectorIcon(icon_id, color));

[kylix_rd, 2016/05/10 16:23:47]

In seems that calling this gfx::CreateVectorIcon() overload doesn't scale the
icon down the the correct size as shown here:
https://drive.google.com/a/google.com/file/d/0B6LQg8CWo5bNOGFKZ0g5OVVWVkU/vie...

However, I'm reluctant to call this overload:

GFX_EXPORT ImageSkia CreateVectorIcon(VectorIconId id,
                                      size_t dip_size,
                                      SkColor color);

with a hard-coded value. Is there a constant or some other way to know what to
pass in as the dip_size?

[Evan Stade, 2016/05/10 16:48:14]

hardcode a value (16)

[kylix_rd, 2016/05/10 17:49:01]

Done.

 }
 
 void AppMenuButton::SetTrailingMargin(int margin) {
   margin_trailing_ = margin;
 
   UpdateThemedBorder();
 
   const int inset = LabelButton::kFocusRectInset;
   SetFocusPainter(views::Painter::CreateDashedFocusPainterWithInsets(
       gfx::Insets(inset, inset, inset, inset + margin)));
@@ skipping 78 matching lines @@
   views::MenuButton::OnPaint(canvas);
   if (ui::MaterialDesignController::IsModeMaterial())
     return;
   // Use GetPreferredSize() to center the icon inside the visible bounds rather
   // than the whole size() (which may refer to hit test region extended to the
   // end of the toolbar in maximized mode).
   icon_painter_->Paint(canvas, GetThemeProvider(),
                        gfx::Rect(GetPreferredSize()),
                        AppMenuIconPainter::BEZEL_NONE);
 }