[Views] App Menu Icon Update

[Views] App Menu Icon Update

Removed the "Persistent Open State" behavior and changed
the "app-menu-icon" flag to "enable-new-app-menu-icon".

Fixed an issue with the animation where it transitions between
different severity levels with an incorrect color.

BUG=704786
Review-Url: https://codereview.chromium.org/2843413003
Cr-Commit-Position: refs/heads/master@{#468222}
Committed: https://chromium.googlesource.com/chromium/src/+/6f9127a09a54711eed16c5e386ed02453309ed91

[spqchan, 2017-04-28 17:31:28]

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

[commit-bot: I haz the power, 2017-04-28 17:32:36]

Dry run: CQ is trying da patch.

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

[spqchan, 2017-04-28 17:33:54]

Description was changed from

==========
[Views] App Menu Icon Update

Changed

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

to

==========
[Views] App Menu Icon Update

Removed the "Persistent Open State" behavior and changed
the "app-menu-icon" flag to "enable-new-app-menu-icon".

Fixed an issue with the animation where it transitions between
different severity levels with an incorrect color.

BUG=704786
==========

[spqchan, 2017-04-28 17:51:40]

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

[commit-bot: I haz the power, 2017-04-28 17:52:33]

Dry run: CQ is trying da patch.

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

[spqchan, 2017-04-28 17:56:42]

Patchset #2 (id:20001) has been deleted

[commit-bot: I haz the power, 2017-04-28 20:04:44]

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

[commit-bot: I haz the power, 2017-04-28 20:04:45]

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

[spqchan, 2017-04-28 20:10:21]

spqchan@chromium.org changed reviewers:
+ isherman@chromium.org, msw@chromium.org

[spqchan, 2017-04-28 20:10:22]

PTAL

isherman for histogram.xml
msw for everything else

[Ilya Sherman, 2017-04-28 20:55:17]

https://codereview.chromium.org/2843413003/diff/40001/tools/metrics/histogram...
File tools/metrics/histograms/histograms.xml (left):

https://codereview.chromium.org/2843413003/diff/40001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:103763: -  <int value="324522065"
label="app-menu-icon"/>
nit: Please leave this in place, so that historical data remains easily
viewable.

[Ilya Sherman, 2017-04-28 20:55:32]

Erm, histograms.xml lgtm with the nit addressed =)

[spqchan, 2017-04-28 21:39:00]

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

[msw, 2017-04-28 21:39:13]

https://codereview.chromium.org/2843413003/diff/60001/chrome/browser/flag_des...
File chrome/browser/flag_descriptions.cc (right):

https://codereview.chromium.org/2843413003/diff/60001/chrome/browser/flag_des...
chrome/browser/flag_descriptions.cc:2786: "Changes the app menu icon into the
new animated one.";
optional nit: "Use the new app menu icon with update notification animations."

https://codereview.chromium.org/2843413003/diff/60001/chrome/browser/ui/views...
File chrome/browser/ui/views/toolbar/app_menu_animation.cc (right):

https://codereview.chromium.org/2843413003/diff/60001/chrome/browser/ui/views...
chrome/browser/ui/views/toolbar/app_menu_animation.cc:177: } else if
(!animation_->IsShowing()) {
Remove the redundant "if (!animation_->IsShowing())"

https://codereview.chromium.org/2843413003/diff/60001/chrome/browser/ui/views...
File chrome/browser/ui/views/toolbar/app_menu_animation.h (right):

https://codereview.chromium.org/2843413003/diff/60001/chrome/browser/ui/views...
chrome/browser/ui/views/toolbar/app_menu_animation.h:30: // Updates the icon
colors.
nit: s/colors/color.

https://codereview.chromium.org/2843413003/diff/60001/chrome/browser/ui/views...
File chrome/browser/ui/views/toolbar/app_menu_button.cc (right):

https://codereview.chromium.org/2843413003/diff/60001/chrome/browser/ui/views...
chrome/browser/ui/views/toolbar/app_menu_button.cc:58: Browser* browser =
toolbar_view_->browser();
nit: inline this

https://codereview.chromium.org/2843413003/diff/60001/chrome/browser/ui/views...
chrome/browser/ui/views/toolbar/app_menu_button.cc:186: // |animation_| is
created here instead of the constructor because the
Perhaps the old pattern of setting both colors here was simpler?

https://codereview.chromium.org/2843413003/diff/60001/chrome/browser/ui/views...
chrome/browser/ui/views/toolbar/app_menu_button.cc:189: if (!animation_) {
nit: curlies not needed

https://codereview.chromium.org/2843413003/diff/60001/chrome/browser/ui/views...
chrome/browser/ui/views/toolbar/app_menu_button.cc:225: if
(!should_use_new_icon_)
optional nit: combine conditionals instead of using separate early returns.

[spqchan, 2017-04-28 21:39:22]

thanks!

https://codereview.chromium.org/2843413003/diff/40001/tools/metrics/histogram...
File tools/metrics/histograms/histograms.xml (left):

https://codereview.chromium.org/2843413003/diff/40001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:103763: -  <int value="324522065"
label="app-menu-icon"/>
On 2017/04/28 20:55:17, Ilya Sherman wrote:
> nit: Please leave this in place, so that historical data remains easily
> viewable.

Done.

[commit-bot: I haz the power, 2017-04-28 21:39:31]

Dry run: CQ is trying da patch.

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

[msw, 2017-04-28 21:41:02]

https://codereview.chromium.org/2843413003/diff/60001/chrome/browser/ui/views...
File chrome/browser/ui/views/toolbar/app_menu_button.cc (right):

https://codereview.chromium.org/2843413003/diff/60001/chrome/browser/ui/views...
chrome/browser/ui/views/toolbar/app_menu_button.cc:186: // |animation_| is
created here instead of the constructor because the
On 2017/04/28 21:39:13, msw wrote:
> Perhaps the old pattern of setting both colors here was simpler?

Or is this meant to animate from one severity color to another?

[commit-bot: I haz the power, 2017-04-28 22:35:29]

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

[commit-bot: I haz the power, 2017-04-28 22:35:30]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[spqchan, 2017-04-28 22:38:36]

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

[commit-bot: I haz the power, 2017-04-28 22:39:34]

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, 2017-04-28 23:54:36]

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

[commit-bot: I haz the power, 2017-04-28 23:54:37]

Dry run: This issue passed the CQ dry run.

[spqchan, 2017-04-29 00:36:33]

PTAL

https://codereview.chromium.org/2843413003/diff/60001/chrome/browser/flag_des...
File chrome/browser/flag_descriptions.cc (right):

https://codereview.chromium.org/2843413003/diff/60001/chrome/browser/flag_des...
chrome/browser/flag_descriptions.cc:2786: "Changes the app menu icon into the
new animated one.";
On 2017/04/28 21:39:12, msw wrote:
> optional nit: "Use the new app menu icon with update notification animations."

Done.

https://codereview.chromium.org/2843413003/diff/60001/chrome/browser/ui/views...
File chrome/browser/ui/views/toolbar/app_menu_animation.cc (right):

https://codereview.chromium.org/2843413003/diff/60001/chrome/browser/ui/views...
chrome/browser/ui/views/toolbar/app_menu_animation.cc:177: } else if
(!animation_->IsShowing()) {
On 2017/04/28 21:39:12, msw wrote:
> Remove the redundant "if (!animation_->IsShowing())"

Done.

https://codereview.chromium.org/2843413003/diff/60001/chrome/browser/ui/views...
File chrome/browser/ui/views/toolbar/app_menu_animation.h (right):

https://codereview.chromium.org/2843413003/diff/60001/chrome/browser/ui/views...
chrome/browser/ui/views/toolbar/app_menu_animation.h:30: // Updates the icon
colors.
On 2017/04/28 21:39:12, msw wrote:
> nit: s/colors/color.

Removed the comment since it's a setter (and self documented). Let me know if
you want to put it back

https://codereview.chromium.org/2843413003/diff/60001/chrome/browser/ui/views...
File chrome/browser/ui/views/toolbar/app_menu_button.cc (right):

https://codereview.chromium.org/2843413003/diff/60001/chrome/browser/ui/views...
chrome/browser/ui/views/toolbar/app_menu_button.cc:58: Browser* browser =
toolbar_view_->browser();
On 2017/04/28 21:39:12, msw wrote:
> nit: inline this

Done.

https://codereview.chromium.org/2843413003/diff/60001/chrome/browser/ui/views...
chrome/browser/ui/views/toolbar/app_menu_button.cc:186: // |animation_| is
created here instead of the constructor because the
On 2017/04/28 21:41:02, msw wrote:
> On 2017/04/28 21:39:13, msw wrote:
> > Perhaps the old pattern of setting both colors here was simpler?
> 
> Or is this meant to animate from one severity color to another?

The old pattern of setting both colors was causing an issue, since it would
reset the starting color to "toolbar button icon". This would cause an issue
where if the severity is updated from low to critical, the icon will animate
from grey to red instead of a green to red

https://codereview.chromium.org/2843413003/diff/60001/chrome/browser/ui/views...
chrome/browser/ui/views/toolbar/app_menu_button.cc:189: if (!animation_) {
On 2017/04/28 21:39:12, msw wrote:
> nit: curlies not needed

Done.

https://codereview.chromium.org/2843413003/diff/60001/chrome/browser/ui/views...
chrome/browser/ui/views/toolbar/app_menu_button.cc:225: if
(!should_use_new_icon_)
On 2017/04/28 21:39:12, msw wrote:
> optional nit: combine conditionals instead of using separate early returns.

Done.

[msw, 2017-04-29 00:55:05]

lgtm with optional nits

https://codereview.chromium.org/2843413003/diff/80001/chrome/browser/ui/views...
File chrome/browser/ui/views/toolbar/app_menu_animation.h (right):

https://codereview.chromium.org/2843413003/diff/80001/chrome/browser/ui/views...
chrome/browser/ui/views/toolbar/app_menu_animation.h:30: void
set_severity_color(SkColor severity_color) {
nit: maybe rename this and the member to |target_color|?

https://codereview.chromium.org/2843413003/diff/80001/chrome/browser/ui/views...
File chrome/browser/ui/views/toolbar/app_menu_button.cc (right):

https://codereview.chromium.org/2843413003/diff/80001/chrome/browser/ui/views...
chrome/browser/ui/views/toolbar/app_menu_button.cc:185: // |animation_| is
created here instead of the constructor because the
optional nit: I think it's okay to remove this comment

https://codereview.chromium.org/2843413003/diff/80001/chrome/browser/ui/views...
chrome/browser/ui/views/toolbar/app_menu_button.cc:225: severity_ ==
AppMenuIconController::Severity::NONE || !animation_) {
optional nit: reorder to check animation earlier:
if (!should_use_new_icon_ || !animation_ ||

[spqchan, 2017-04-29 02:34:42]

Thanks!

https://codereview.chromium.org/2843413003/diff/80001/chrome/browser/ui/views...
File chrome/browser/ui/views/toolbar/app_menu_animation.h (right):

https://codereview.chromium.org/2843413003/diff/80001/chrome/browser/ui/views...
chrome/browser/ui/views/toolbar/app_menu_animation.h:30: void
set_severity_color(SkColor severity_color) {
On 2017/04/29 00:55:05, msw wrote:
> nit: maybe rename this and the member to |target_color|?

Done.

https://codereview.chromium.org/2843413003/diff/80001/chrome/browser/ui/views...
File chrome/browser/ui/views/toolbar/app_menu_button.cc (right):

https://codereview.chromium.org/2843413003/diff/80001/chrome/browser/ui/views...
chrome/browser/ui/views/toolbar/app_menu_button.cc:185: // |animation_| is
created here instead of the constructor because the
On 2017/04/29 00:55:05, msw wrote:
> optional nit: I think it's okay to remove this comment

Done.

https://codereview.chromium.org/2843413003/diff/80001/chrome/browser/ui/views...
chrome/browser/ui/views/toolbar/app_menu_button.cc:225: severity_ ==
AppMenuIconController::Severity::NONE || !animation_) {
On 2017/04/29 00:55:05, msw wrote:
> optional nit: reorder to check animation earlier:
> if (!should_use_new_icon_ || !animation_ ||

Done.

[spqchan, 2017-04-29 02:34:49]

The CQ bit was checked by spqchan@chromium.org

[spqchan, 2017-04-29 02:34:49]

The patchset sent to the CQ was uploaded after l-g-t-m from
isherman@chromium.org, msw@chromium.org
Link to the patchset: https://codereview.chromium.org/2843413003/#ps100001
(title: "Fixes for msw")

[commit-bot: I haz the power, 2017-04-29 02:35:08]

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, 2017-04-29 03:42:05]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1493433289217790,
"parent_rev": "f8e111a1107118e760f1883c8327d0c1ccd0f26a", "commit_rev":
"6f9127a09a54711eed16c5e386ed02453309ed91"}

[commit-bot: I haz the power, 2017-04-29 03:42:40]

Description was changed from

==========
[Views] App Menu Icon Update

Removed the "Persistent Open State" behavior and changed
the "app-menu-icon" flag to "enable-new-app-menu-icon".

Fixed an issue with the animation where it transitions between
different severity levels with an incorrect color.

BUG=704786
==========

to

==========
[Views] App Menu Icon Update

Removed the "Persistent Open State" behavior and changed
the "app-menu-icon" flag to "enable-new-app-menu-icon".

Fixed an issue with the animation where it transitions between
different severity levels with an incorrect color.

BUG=704786

Review-Url: https://codereview.chromium.org/2843413003
Cr-Commit-Position: refs/heads/master@{#468222}
Committed:
https://chromium.googlesource.com/chromium/src/+/6f9127a09a54711eed16c5e386ed...
==========

[commit-bot: I haz the power, 2017-04-29 03:42:42]

Committed patchset #5 (id:100001) as
https://chromium.googlesource.com/chromium/src/+/6f9127a09a54711eed16c5e386ed...