Vectorize help icon in Ash system menu for MD

Vectorize help icon in Ash system menu for MD

Also, updated PaletteTray to use the new icon.

BUG=646533
TEST=manually run Ash with --ash-md=experimental and check the icon

Committed: https://crrev.com/093e8f031fd001b7b9a272a74ace1ccc909aee6e
Cr-Commit-Position: refs/heads/master@{#419559}

[mohsen, 2016-09-09 22:13:32]

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

[commit-bot: I haz the power, 2016-09-09 22:13:59]

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-09-10 00:59:57]

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

[commit-bot: I haz the power, 2016-09-10 00:59:58]

Dry run: This issue passed the CQ dry run.

[mohsen, 2016-09-13 15:30:27]

Description was changed from

==========
Vectorize help icon in Ash system menu for MD

BUG=606817
==========

to

==========
Vectorize help icon in Ash system menu for MD

BUG=606817
TEST=none
==========

[mohsen, 2016-09-13 15:39:40]

mohsen@chromium.org changed reviewers:
+ tdanderson@chromium.org

[mohsen, 2016-09-13 15:39:41]

Please take a look...

Also, other than the system menu, help icon is already used in the palette tray.
Should we change that to use this new icon? In this case, do we need to change
the icon name from system_menu_help to something more general (e.g. help)? In
other words, is there any guidelines/rules for naming icons?

https://codereview.chromium.org/2327943002/diff/1/ash/common/system/date/date...
File ash/common/system/date/date_default_view.cc (right):

https://codereview.chromium.org/2327943002/diff/1/ash/common/system/date/date...
ash/common/system/date/date_default_view.cc:82: if (base::i18n::IsRTL() &&
still relevant?

[tdanderson, 2016-09-14 14:56:12]

Yes, please change the gfx::VectorIconId::HELP call site to use your new icon
instead; gfx::VectorIconId::HELP looks to be on an 18x18 grid which is stretched
to 20x20 so it will not look as good as the one you're adding here.

As for naming, I'm inclined to suggest "system_menu_help" rather than just
"help" as an indicator that the icon was specifically designed to fit on the
system menu's pixel grid.

https://chromiumcodereview.appspot.com/2327943002/diff/1/ash/common/system/da...
File ash/common/system/date/date_default_view.cc (right):

https://chromiumcodereview.appspot.com/2327943002/diff/1/ash/common/system/da...
ash/common/system/date/date_default_view.cc:73:
color_utils::DeriveDefaultIconColor(kMenuIconColor));
No need for DeriveDefaultIconColor, you can just pass in kMenuIconColor directly
here.

https://chromiumcodereview.appspot.com/2327943002/diff/1/ash/common/system/da...
ash/common/system/date/date_default_view.cc:82: if (base::i18n::IsRTL() &&
On 2016/09/13 15:39:41, mohsen wrote:
> still relevant?

Not sure, but let's assume so.

[mohsen, 2016-09-14 18:45:11]

Description was changed from

==========
Vectorize help icon in Ash system menu for MD

BUG=606817
TEST=none
==========

to

==========
Vectorize help icon in Ash system menu for MD

BUG=646533
TEST=none
==========

[mohsen, 2016-09-14 18:46:28]

Description was changed from

==========
Vectorize help icon in Ash system menu for MD

BUG=646533
TEST=none
==========

to

==========
Vectorize help icon in Ash system menu for MD

BUG=646533
TEST=manually run Ash and check the icon
==========

[mohsen, 2016-09-14 18:47:29]

Description was changed from

==========
Vectorize help icon in Ash system menu for MD

BUG=646533
TEST=manually run Ash and check the icon
==========

to

==========
Vectorize help icon in Ash system menu for MD

BUG=646533
TEST=manually run Ash with --ash-md=experimental and check the icon
==========

[mohsen, 2016-09-14 21:32:58]

Description was changed from

==========
Vectorize help icon in Ash system menu for MD

BUG=646533
TEST=manually run Ash with --ash-md=experimental and check the icon
==========

to

==========
Vectorize help icon in Ash system menu for MD

Also, updated PaletteTray to use the new icon.

BUG=646533
TEST=manually run Ash with --ash-md=experimental and check the icon
==========

[mohsen, 2016-09-14 21:48:22]

Patchset #2 (id:20001) has been deleted

[mohsen, 2016-09-14 21:53:25]

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

[commit-bot: I haz the power, 2016-09-14 21:53:58]

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-09-14 21:57:02]

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

[commit-bot: I haz the power, 2016-09-14 21:57:03]

Dry run: Try jobs failed on following builders:
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[mohsen, 2016-09-14 22:04:16]

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

[commit-bot: I haz the power, 2016-09-14 22:04:48]

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

[mohsen, 2016-09-14 22:10:50]

https://codereview.chromium.org/2327943002/diff/1/ash/common/system/date/date...
File ash/common/system/date/date_default_view.cc (right):

https://codereview.chromium.org/2327943002/diff/1/ash/common/system/date/date...
ash/common/system/date/date_default_view.cc:73:
color_utils::DeriveDefaultIconColor(kMenuIconColor));
On 2016/09/14 at 14:56:12, tdanderson wrote:
> No need for DeriveDefaultIconColor, you can just pass in kMenuIconColor
directly here.

Done.

https://codereview.chromium.org/2327943002/diff/1/ash/common/system/date/date...
ash/common/system/date/date_default_view.cc:82: if (base::i18n::IsRTL() &&
On 2016/09/14 at 14:56:12, tdanderson wrote:
> On 2016/09/13 15:39:41, mohsen wrote:
> > still relevant?
> 
> Not sure, but let's assume so.

Oops. That was meant to be a note to myself ;) Sorry, for publishing!

[commit-bot: I haz the power, 2016-09-14 23:08:30]

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

[commit-bot: I haz the power, 2016-09-14 23:08:31]

Dry run: This issue passed the CQ dry run.

[tdanderson, 2016-09-15 15:21:54]

ash/ lgtm

[mohsen, 2016-09-15 15:24:57]

mohsen@chromium.org changed reviewers:
+ estade@chromium.org

[mohsen, 2016-09-15 15:24:58]

estade@: Can you please take a look at changes in ui/gfx/vector_icons/?

[Evan Stade, 2016-09-15 16:30:33]

lgtm

https://chromiumcodereview.appspot.com/2327943002/diff/60001/ash/common/syste...
File ash/common/system/chromeos/palette/palette_tray.cc (right):

https://chromiumcodereview.appspot.com/2327943002/diff/60001/ash/common/syste...
ash/common/system/chromeos/palette/palette_tray.cc:90:
gfx::VectorIconId::SETTINGS, kIconSize, gfx::kChromeIconGrey);
side note: should this be using kMenuIconColor?

https://chromiumcodereview.appspot.com/2327943002/diff/60001/ash/common/syste...
ash/common/system/chromeos/palette/palette_tray.cc:92:
CreateVectorIcon(kSystemMenuHelpIcon, gfx::kChromeIconGrey);
maybe DCHECK_EQ(kIconSize, help_icon.width())?

[mohsen, 2016-09-15 20:53:16]

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

[commit-bot: I haz the power, 2016-09-15 20:54:06]

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

[mohsen, 2016-09-15 20:54:45]

https://codereview.chromium.org/2327943002/diff/60001/ash/common/system/chrom...
File ash/common/system/chromeos/palette/palette_tray.cc (right):

https://codereview.chromium.org/2327943002/diff/60001/ash/common/system/chrom...
ash/common/system/chromeos/palette/palette_tray.cc:90:
gfx::VectorIconId::SETTINGS, kIconSize, gfx::kChromeIconGrey);
On 2016/09/15 at 16:30:32, Evan Stade wrote:
> side note: should this be using kMenuIconColor?

Yep, I changed it to kMenuIconColor; also changed the definition of
kMenuIconColor to get its value from gfx::kChromeIconGrey. WDYT?

https://codereview.chromium.org/2327943002/diff/60001/ash/common/system/chrom...
ash/common/system/chromeos/palette/palette_tray.cc:92:
CreateVectorIcon(kSystemMenuHelpIcon, gfx::kChromeIconGrey);
On 2016/09/15 at 16:30:32, Evan Stade wrote:
> maybe DCHECK_EQ(kIconSize, help_icon.width())?

Sure. Done.

[commit-bot: I haz the power, 2016-09-15 21:56:25]

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

[commit-bot: I haz the power, 2016-09-15 21:56:26]

Dry run: This issue passed the CQ dry run.

[tdanderson, 2016-09-19 19:17:44]

On 2016/09/15 20:54:45, mohsen wrote:
>
https://codereview.chromium.org/2327943002/diff/60001/ash/common/system/chrom...
> File ash/common/system/chromeos/palette/palette_tray.cc (right):
> 
>
https://codereview.chromium.org/2327943002/diff/60001/ash/common/system/chrom...
> ash/common/system/chromeos/palette/palette_tray.cc:90:
> gfx::VectorIconId::SETTINGS, kIconSize, gfx::kChromeIconGrey);
> On 2016/09/15 at 16:30:32, Evan Stade wrote:
> > side note: should this be using kMenuIconColor?
> 
> Yep, I changed it to kMenuIconColor; also changed the definition of
> kMenuIconColor to get its value from gfx::kChromeIconGrey. WDYT?

Patch Set 4 lgtm

> 
>
https://codereview.chromium.org/2327943002/diff/60001/ash/common/system/chrom...
> ash/common/system/chromeos/palette/palette_tray.cc:92:
> CreateVectorIcon(kSystemMenuHelpIcon, gfx::kChromeIconGrey);
> On 2016/09/15 at 16:30:32, Evan Stade wrote:
> > maybe DCHECK_EQ(kIconSize, help_icon.width())?
> 
> Sure. Done.

[Evan Stade, 2016-09-19 19:30:33]

https://codereview.chromium.org/2327943002/diff/60001/ash/common/system/chrom...
File ash/common/system/chromeos/palette/palette_tray.cc (right):

https://codereview.chromium.org/2327943002/diff/60001/ash/common/system/chrom...
ash/common/system/chromeos/palette/palette_tray.cc:90:
gfx::VectorIconId::SETTINGS, kIconSize, gfx::kChromeIconGrey);
On 2016/09/15 20:54:45, mohsen wrote:
> On 2016/09/15 at 16:30:32, Evan Stade wrote:
> > side note: should this be using kMenuIconColor?
> 
> Yep, I changed it to kMenuIconColor; also changed the definition of
> kMenuIconColor to get its value from gfx::kChromeIconGrey. WDYT?

thumbs up

[mohsen, 2016-09-19 19:43:25]

The CQ bit was checked by mohsen@chromium.org

[mohsen, 2016-09-19 19:43:26]

The patchset sent to the CQ was uploaded after l-g-t-m from estade@chromium.org
Link to the patchset: https://codereview.chromium.org/2327943002/#ps80001
(title: "Addressed review comments")

[commit-bot: I haz the power, 2016-09-19 19:44:33]

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-09-19 21:33:23]

Committed patchset #4 (id:80001)

[commit-bot: I haz the power, 2016-09-19 21:36:33]

Description was changed from

==========
Vectorize help icon in Ash system menu for MD

Also, updated PaletteTray to use the new icon.

BUG=646533
TEST=manually run Ash with --ash-md=experimental and check the icon
==========

to

==========
Vectorize help icon in Ash system menu for MD

Also, updated PaletteTray to use the new icon.

BUG=646533
TEST=manually run Ash with --ash-md=experimental and check the icon

Committed: https://crrev.com/093e8f031fd001b7b9a272a74ace1ccc909aee6e
Cr-Commit-Position: refs/heads/master@{#419559}
==========

[commit-bot: I haz the power, 2016-09-19 21:36:34]

Patchset 4 (id:??) landed as
https://crrev.com/093e8f031fd001b7b9a272a74ace1ccc909aee6e
Cr-Commit-Position: refs/heads/master@{#419559}