Reverse error and warning icons on app menu.

Use app menu error icon for global error app menu items.

This makes sure the error icon is the circle with an exclamation point on both the app menu button and any error menu items within the menu itself.

BUG=629204
Committed: https://crrev.com/891103c65108d1b54dce9786a72f64bab86a2bfc
Cr-Commit-Position: refs/heads/master@{#406906}

[kylix_rd, 2016-07-20 16:31:35]

Description was changed from

==========
Reverse error and warning icons on app menu. Use app menu error icon for global
error menu items

BUG=
==========

to

==========
Use app menu error icon for global error app menu items.

This makes sure the error icon is the circle with an exclamation point on both
the app menu button and any error menu items within the menu itself.

BUG=629204
==========

[kylix_rd, 2016-07-20 16:31:35]

kylixrd@chromium.org changed reviewers:
+ estade@chromium.org, pkasting@chromium.org

[kylix_rd, 2016-07-20 16:39:04]

pkasting@ please see the changes to global_error.cc

estade@ the vector icons, error and warning, for the browser tools menu were
swapped. Now the warning icon is the circle-with-a-line and the error icon is
the circle-with-exclamation-point.

This is per the discussion on this slide:
https://docs.google.com/presentation/d/1UhaA_YtZ6lOqkSXyJyqnLm3sUlMGfBvwsMf0O...
and this issue comment:
https://bugs.chromium.org/p/chromium/issues/detail?id=584342#c15

Some follow-on CLs may be needed for some cleanup such as the removal of unused
vector icon and/or error/warning states.

[Peter Kasting, 2016-07-20 18:55:32]

LGTM, but IMO we should just ditch the warning icon entirely and use the error
icon in that case; see my comment on the bug.

[kylix_rd, 2016-07-20 19:11:19]

The CQ bit was checked by kylixrd@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-07-20 19:11:51]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-20 19:16:51]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-07-20 19:16:52]

Dry run: Try jobs failed on following builders:
  android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)

[kylix_rd, 2016-07-20 19:30:17]

On 2016/07/20 18:55:32, Peter Kasting wrote:
> LGTM, but IMO we should just ditch the warning icon entirely and use the error
> icon in that case; see my comment on the bug.

I *almost* did that :). So there are clearly two votes for that.

[Evan Stade, 2016-07-20 19:45:10]

https://codereview.chromium.org/2166853002/diff/1/ui/gfx/vector_icons/browser...
File ui/gfx/vector_icons/browser_tools_error.icon (right):

https://codereview.chromium.org/2166853002/diff/1/ui/gfx/vector_icons/browser...
ui/gfx/vector_icons/browser_tools_error.icon:5: CANVAS_DIMENSIONS, 32,
aren't these icons identical (modulo scale)?

[Evan Stade, 2016-07-20 19:45:32]

https://codereview.chromium.org/2166853002/diff/1/ui/gfx/vector_icons/browser...
File ui/gfx/vector_icons/browser_tools_error.icon (right):

https://codereview.chromium.org/2166853002/diff/1/ui/gfx/vector_icons/browser...
ui/gfx/vector_icons/browser_tools_error.icon:5: CANVAS_DIMENSIONS, 32,
On 2016/07/20 19:45:10, Evan Stade wrote:
> aren't these icons identical (modulo scale)?

(referring to .icon and .1x.icon)

[kylix_rd, 2016-07-20 19:58:33]

On 2016/07/20 19:45:32, Evan Stade wrote:
>
https://codereview.chromium.org/2166853002/diff/1/ui/gfx/vector_icons/browser...
> File ui/gfx/vector_icons/browser_tools_error.icon (right):
> 
>
https://codereview.chromium.org/2166853002/diff/1/ui/gfx/vector_icons/browser...
> ui/gfx/vector_icons/browser_tools_error.icon:5: CANVAS_DIMENSIONS, 32,
> On 2016/07/20 19:45:10, Evan Stade wrote:
> > aren't these icons identical (modulo scale)?
> 
> (referring to .icon and .1x.icon)

Yes. 

However, per a discussion with pkasting@, sgabriel@, the warning icon (used for
bad indicating bad DLLs) will be removed and the error icon used instead.

[Peter Kasting, 2016-07-20 19:59:11]

On 2016/07/20 19:58:33, kylix_rd wrote:
> On 2016/07/20 19:45:32, Evan Stade wrote:
> >
>
https://codereview.chromium.org/2166853002/diff/1/ui/gfx/vector_icons/browser...
> > File ui/gfx/vector_icons/browser_tools_error.icon (right):
> > 
> >
>
https://codereview.chromium.org/2166853002/diff/1/ui/gfx/vector_icons/browser...
> > ui/gfx/vector_icons/browser_tools_error.icon:5: CANVAS_DIMENSIONS, 32,
> > On 2016/07/20 19:45:10, Evan Stade wrote:
> > > aren't these icons identical (modulo scale)?
> > 
> > (referring to .icon and .1x.icon)
> 
> Yes. 
> 
> However, per a discussion with pkasting@, sgabriel@, the warning icon (used
for
> bad indicating bad DLLs) will be removed and the error icon used instead.

Wasn't he saying that the error icon does not need a .1x version?

[kylix_rd, 2016-07-20 20:41:17]

On 2016/07/20 19:59:11, Peter Kasting wrote:
> On 2016/07/20 19:58:33, kylix_rd wrote:
> > On 2016/07/20 19:45:32, Evan Stade wrote:
> > >
> >
>
https://codereview.chromium.org/2166853002/diff/1/ui/gfx/vector_icons/browser...
> > > File ui/gfx/vector_icons/browser_tools_error.icon (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/2166853002/diff/1/ui/gfx/vector_icons/browser...
> > > ui/gfx/vector_icons/browser_tools_error.icon:5: CANVAS_DIMENSIONS, 32,
> > > On 2016/07/20 19:45:10, Evan Stade wrote:
> > > > aren't these icons identical (modulo scale)?
> > > 
> > > (referring to .icon and .1x.icon)
> > 
> > Yes. 
> > 
> > However, per a discussion with pkasting@, sgabriel@, the warning icon (used
> for
> > bad indicating bad DLLs) will be removed and the error icon used instead.
> 
> Wasn't he saying that the error icon does not need a .1x version?

The 1x version is the one used for the menu and the app menu button. If that is
the only size ever used, then I could make it the only one we have.

[Evan Stade, 2016-07-20 20:47:15]

On 2016/07/20 20:41:17, kylix_rd wrote:
> On 2016/07/20 19:59:11, Peter Kasting wrote:
> > On 2016/07/20 19:58:33, kylix_rd wrote:
> > > On 2016/07/20 19:45:32, Evan Stade wrote:
> > > >
> > >
> >
>
https://codereview.chromium.org/2166853002/diff/1/ui/gfx/vector_icons/browser...
> > > > File ui/gfx/vector_icons/browser_tools_error.icon (right):
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/2166853002/diff/1/ui/gfx/vector_icons/browser...
> > > > ui/gfx/vector_icons/browser_tools_error.icon:5: CANVAS_DIMENSIONS, 32,
> > > > On 2016/07/20 19:45:10, Evan Stade wrote:
> > > > > aren't these icons identical (modulo scale)?
> > > > 
> > > > (referring to .icon and .1x.icon)
> > > 
> > > Yes. 
> > > 
> > > However, per a discussion with pkasting@, sgabriel@, the warning icon
(used
> > for
> > > bad indicating bad DLLs) will be removed and the error icon used instead.
> > 
> > Wasn't he saying that the error icon does not need a .1x version?
> 
> The 1x version is the one used for the menu and the app menu button. If that
is
> the only size ever used, then I could make it the only one we have.

no, we use 1x if your device scale factor is 1x and otherwise we use the .icon
version. You only need to specify distinct .1x.icon and .icon files if there is
actually a difference in the icons (besides just a multiplicative factor).

[kylix_rd, 2016-07-20 20:51:40]

On 2016/07/20 20:47:15, Evan Stade wrote:
> On 2016/07/20 20:41:17, kylix_rd wrote:
> > On 2016/07/20 19:59:11, Peter Kasting wrote:
> > > On 2016/07/20 19:58:33, kylix_rd wrote:
> > > > On 2016/07/20 19:45:32, Evan Stade wrote:
> > > > >
> > > >
> > >
> >
>
https://codereview.chromium.org/2166853002/diff/1/ui/gfx/vector_icons/browser...
> > > > > File ui/gfx/vector_icons/browser_tools_error.icon (right):
> > > > > 
> > > > >
> > > >
> > >
> >
>
https://codereview.chromium.org/2166853002/diff/1/ui/gfx/vector_icons/browser...
> > > > > ui/gfx/vector_icons/browser_tools_error.icon:5: CANVAS_DIMENSIONS, 32,
> > > > > On 2016/07/20 19:45:10, Evan Stade wrote:
> > > > > > aren't these icons identical (modulo scale)?
> > > > > 
> > > > > (referring to .icon and .1x.icon)
> > > > 
> > > > Yes. 
> > > > 
> > > > However, per a discussion with pkasting@, sgabriel@, the warning icon
> (used
> > > for
> > > > bad indicating bad DLLs) will be removed and the error icon used
instead.
> > > 
> > > Wasn't he saying that the error icon does not need a .1x version?
> > 
> > The 1x version is the one used for the menu and the app menu button. If that
> is
> > the only size ever used, then I could make it the only one we have.
> 
> no, we use 1x if your device scale factor is 1x and otherwise we use the .icon
> version. You only need to specify distinct .1x.icon and .icon files if there
is
> actually a difference in the icons (besides just a multiplicative factor).

Ok, my misunderstanding. It sounds like we should only have the 1x version (as
the non-1x name) and just let it scale rather than have distinctive sizes in
this instance.

[Evan Stade, 2016-07-20 20:52:06]

On 2016/07/20 20:51:40, kylix_rd wrote:
> On 2016/07/20 20:47:15, Evan Stade wrote:
> > On 2016/07/20 20:41:17, kylix_rd wrote:
> > > On 2016/07/20 19:59:11, Peter Kasting wrote:
> > > > On 2016/07/20 19:58:33, kylix_rd wrote:
> > > > > On 2016/07/20 19:45:32, Evan Stade wrote:
> > > > > >
> > > > >
> > > >
> > >
> >
>
https://codereview.chromium.org/2166853002/diff/1/ui/gfx/vector_icons/browser...
> > > > > > File ui/gfx/vector_icons/browser_tools_error.icon (right):
> > > > > > 
> > > > > >
> > > > >
> > > >
> > >
> >
>
https://codereview.chromium.org/2166853002/diff/1/ui/gfx/vector_icons/browser...
> > > > > > ui/gfx/vector_icons/browser_tools_error.icon:5: CANVAS_DIMENSIONS,
32,
> > > > > > On 2016/07/20 19:45:10, Evan Stade wrote:
> > > > > > > aren't these icons identical (modulo scale)?
> > > > > > 
> > > > > > (referring to .icon and .1x.icon)
> > > > > 
> > > > > Yes. 
> > > > > 
> > > > > However, per a discussion with pkasting@, sgabriel@, the warning icon
> > (used
> > > > for
> > > > > bad indicating bad DLLs) will be removed and the error icon used
> instead.
> > > > 
> > > > Wasn't he saying that the error icon does not need a .1x version?
> > > 
> > > The 1x version is the one used for the menu and the app menu button. If
that
> > is
> > > the only size ever used, then I could make it the only one we have.
> > 
> > no, we use 1x if your device scale factor is 1x and otherwise we use the
.icon
> > version. You only need to specify distinct .1x.icon and .icon files if there
> is
> > actually a difference in the icons (besides just a multiplicative factor).
> 
> Ok, my misunderstanding. It sounds like we should only have the 1x version (as
> the non-1x name) and just let it scale rather than have distinctive sizes in
> this instance.

yea, that sounds right.

[kylix_rd, 2016-07-21 17:05:23]

kylixrd@chromium.org changed reviewers:
+ rohitrao@chromium.org

[kylix_rd, 2016-07-21 17:07:32]

This patch removes the Warning browser tools button icon and makes all the icons
16x16 as the only size and removes the 1x versions.

rohitrao@ please check app_toolbar_button.mm, which should be congruent with the
changes to the other platforms in app_menu_button.cc

[kylix_rd, 2016-07-21 17:07:43]

The CQ bit was checked by kylixrd@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-07-21 17:08:05]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Evan Stade, 2016-07-21 17:28:43]

lgtm

https://codereview.chromium.org/2166853002/diff/20001/chrome/browser/ui/globa...
File chrome/browser/ui/global_error/global_error.cc (right):

https://codereview.chromium.org/2166853002/diff/20001/chrome/browser/ui/globa...
chrome/browser/ui/global_error/global_error.cc:31: IDR_INPUT_ALERT_MENU);
might be worth pinging shrike to see if mac should be using the same thing as
windows here (non-blocking to this cl of course)

[rohitrao (ping after 24h), 2016-07-21 17:32:02]

cocoa lgtm

[kylix_rd, 2016-07-21 17:42:31]

On 2016/07/21 17:28:43, Evan Stade wrote:
> lgtm
> 
>
https://codereview.chromium.org/2166853002/diff/20001/chrome/browser/ui/globa...
> File chrome/browser/ui/global_error/global_error.cc (right):
> 
>
https://codereview.chromium.org/2166853002/diff/20001/chrome/browser/ui/globa...
> chrome/browser/ui/global_error/global_error.cc:31: IDR_INPUT_ALERT_MENU);
> might be worth pinging shrike to see if mac should be using the same thing as
> windows here (non-blocking to this cl of course)

And Android? Sent msg to shrike@.

[Evan Stade, 2016-07-21 17:50:34]

On 2016/07/21 17:42:31, kylix_rd wrote:
> On 2016/07/21 17:28:43, Evan Stade wrote:
> > lgtm
> > 
> >
>
https://codereview.chromium.org/2166853002/diff/20001/chrome/browser/ui/globa...
> > File chrome/browser/ui/global_error/global_error.cc (right):
> > 
> >
>
https://codereview.chromium.org/2166853002/diff/20001/chrome/browser/ui/globa...
> > chrome/browser/ui/global_error/global_error.cc:31: IDR_INPUT_ALERT_MENU);
> > might be worth pinging shrike to see if mac should be using the same thing
as
> > windows here (non-blocking to this cl of course)
> 
> And Android? Sent msg to shrike@.

android java ui doesn't use vector icons :[ I'm kinda surprised that android
wants a gfx::Image in this case, as I thought they used IDRs and map them to
native java resources

[Evan Stade, 2016-07-21 17:57:55]

On 2016/07/21 17:50:34, Evan Stade wrote:
> On 2016/07/21 17:42:31, kylix_rd wrote:
> > On 2016/07/21 17:28:43, Evan Stade wrote:
> > > lgtm
> > > 
> > >
> >
>
https://codereview.chromium.org/2166853002/diff/20001/chrome/browser/ui/globa...
> > > File chrome/browser/ui/global_error/global_error.cc (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/2166853002/diff/20001/chrome/browser/ui/globa...
> > > chrome/browser/ui/global_error/global_error.cc:31: IDR_INPUT_ALERT_MENU);
> > > might be worth pinging shrike to see if mac should be using the same thing
> as
> > > windows here (non-blocking to this cl of course)
> > 
> > And Android? Sent msg to shrike@.
> 
> android java ui doesn't use vector icons :[ I'm kinda surprised that android
> wants a gfx::Image in this case, as I thought they used IDRs and map them to
> native java resources

indeed this is in the gypi list for chrome_browser_ui_non_mobile_sources. Sounds
like it isn't actually compiled on android so you can just remove android
references.

[commit-bot: I haz the power, 2016-07-21 18:03:31]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-07-21 18:03:32]

Dry run: This issue passed the CQ dry run.

[kylix_rd, 2016-07-21 18:06:11]

The CQ bit was checked by kylixrd@chromium.org

[kylix_rd, 2016-07-21 18:06:11]

The patchset sent to the CQ was uploaded after l-g-t-m from
pkasting@chromium.org
Link to the patchset: https://codereview.chromium.org/2166853002/#ps20001
(title: "Removed warning icon and made all remaining browser_tools icons 1x
(16x16) by default")

[commit-bot: I haz the power, 2016-07-21 18:06:53]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-21 18:11:33]

Description was changed from

==========
Use app menu error icon for global error app menu items.

This makes sure the error icon is the circle with an exclamation point on both
the app menu button and any error menu items within the menu itself.

BUG=629204
==========

to

==========
Use app menu error icon for global error app menu items.

This makes sure the error icon is the circle with an exclamation point on both
the app menu button and any error menu items within the menu itself.

BUG=629204
==========

[commit-bot: I haz the power, 2016-07-21 18:11:34]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-07-21 18:13:55]

Description was changed from

==========
Use app menu error icon for global error app menu items.

This makes sure the error icon is the circle with an exclamation point on both
the app menu button and any error menu items within the menu itself.

BUG=629204
==========

to

==========
Use app menu error icon for global error app menu items.

This makes sure the error icon is the circle with an exclamation point on both
the app menu button and any error menu items within the menu itself.

BUG=629204

Committed: https://crrev.com/891103c65108d1b54dce9786a72f64bab86a2bfc
Cr-Commit-Position: refs/heads/master@{#406906}
==========

[commit-bot: I haz the power, 2016-07-21 18:13:58]

Patchset 2 (id:??) landed as
https://crrev.com/891103c65108d1b54dce9786a72f64bab86a2bfc
Cr-Commit-Position: refs/heads/master@{#406906}