Bisect: Run the update icon instructions and see when the color changed.

Readding triage tracking label.

bettes@ Thanks for the issue.

Tested this issue on Windows 10 on Canary 72.0.3590.0 and unable to reproduce the issue.

Could see the update icon as Green on the Canary build 72.0.3590.0. Couldn't find the update icon on the latest Canary 72.0.3591.0.
Attached is the screen shot for reference.

bettes@ Request you to confirm on which chrome version the update icon is observed as Grey, which will help in further triaging.

Thanks..

Deleted: 898958.png 95.8 KB

	898958.png 95.8 KB View Download

This might be on Mac, the window decorations don't have Windows' caption buttons.

This issue is marked as a release blocker with no OS labels associated. Please add an appropriate OS label.

All release blocking issues should have OS labels associated to it, so that the issue can tracked and promptly verified, once it gets fixed.

Thanks for your time! To disable nags, add the Disable-Nags label.

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot

We haven't reproed it on Mac, and we haven't reproed it on Windows. Unmarking RBS pending investigation.

Here's my Dev at home, Win10.

The icon for BrowserAppMenuButton seems to use one severity color lower than the entry inside the menu. This reproduced in new windows as well so it's not a stale value inside BrowserAppMenuButton as far as I can tell.

Deleted: update-icon-out-of-sync.png 5.0 KB

	update-icon-out-of-sync.png 5.0 KB View Download

Not-bisect, grt@: I guess that this is r581520? "show_very_low_upgrade_level" sounds suspicious to me if it's only used by the app-menu button and not the app menu entry.

BrowserAppMenuButton::UpdateIcon() is probably out of sync with colors now (sets the Upgrade icon per original report, but uses the no-severity grey color).

PTAL :)

Yup, looks like I broke this. I'll work on a fix for M72. Thanks.

I've removed RBS since this has already shipped to stable in M70. Fix out for review here: https://chromium-review.googlesource.com/c/chromium/src/+/1312475.

 Issue 898798  has been merged into this issue.

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/a5a766d75a0eb8006212cb5cee94d704650c787b

commit a5a766d75a0eb8006212cb5cee94d704650c787b
Author: Greg Thompson <grt@chromium.org>
Date: Fri Nov 09 15:12:12 2018

Fix the icons for the app menu and the upgrade item in it when an update is available.

A regression was introduced in r581520 that caused the wrong icon and/or
the wrong coloring to be used in some circumstances. This CL fixes this
in two parts:

- AppMenuIconController may decide that the "annoyance level" from a
  pending update is too low to bother the user. One bug introduced in
  r581520 was that the controller still notified delegates that the
  UPGRADE_NOTIFICATION icon type should be used in this case. Now, the
  VERY_LOW annoyance level is ignore entirely for beta and stable
  channels.

- AppMenuModel no longer bases its decision to include the upgrade menu
  item directly on the UpgradeDetector. Rather, it now queries the
  AppMenuIconController. This ensures that the same logic is used for
  both the badging of the app menu and for the presence of the upgrade
  item in the menu.

BUG= 898958 

Change-Id: I4b761844365968a24bc69030d711122fcaf16e28
Reviewed-on: https://chromium-review.googlesource.com/c/1312475
Commit-Queue: Greg Thompson <grt@chromium.org>
Reviewed-by: Peter Kasting <pkasting@chromium.org>
Cr-Commit-Position: refs/heads/master@{#606842}
[modify] https://crrev.com/a5a766d75a0eb8006212cb5cee94d704650c787b/chrome/browser/ui/toolbar/app_menu_icon_controller.cc
[modify] https://crrev.com/a5a766d75a0eb8006212cb5cee94d704650c787b/chrome/browser/ui/toolbar/app_menu_icon_controller.h
[add] https://crrev.com/a5a766d75a0eb8006212cb5cee94d704650c787b/chrome/browser/ui/toolbar/app_menu_icon_controller_unittest.cc
[modify] https://crrev.com/a5a766d75a0eb8006212cb5cee94d704650c787b/chrome/browser/ui/toolbar/app_menu_model.cc
[modify] https://crrev.com/a5a766d75a0eb8006212cb5cee94d704650c787b/chrome/browser/ui/toolbar/app_menu_model.h
[modify] https://crrev.com/a5a766d75a0eb8006212cb5cee94d704650c787b/chrome/browser/ui/toolbar/app_menu_model_unittest.cc
[modify] https://crrev.com/a5a766d75a0eb8006212cb5cee94d704650c787b/chrome/browser/ui/views/toolbar/browser_app_menu_button.cc
[modify] https://crrev.com/a5a766d75a0eb8006212cb5cee94d704650c787b/chrome/browser/ui/views/toolbar/browser_app_menu_button.h
[modify] https://crrev.com/a5a766d75a0eb8006212cb5cee94d704650c787b/chrome/browser/ui/views/toolbar/toolbar_view.cc
[modify] https://crrev.com/a5a766d75a0eb8006212cb5cee94d704650c787b/chrome/browser/ui/views/toolbar/toolbar_view.h
[modify] https://crrev.com/a5a766d75a0eb8006212cb5cee94d704650c787b/chrome/test/BUILD.gn

***Mass UI Triage***

bettes@ could you please help in verifying the issue?

It's hard to verify manually (you have to wait for updates and what-not), I think grt@ can make the call on whether we expect this to be done or not.

I haven't noticed anything odd in canary since this landed -- the app menu changes to the green arrow as desired. I think it's safe to merge this to 71, but we wouldn't have a full update cycle to validate it on beta before it would end up on stable. It's my belief that the CL makes things *no worse* than the current situation, so I think a merge is safe if we want to go that route.

I'm marking this as fixed and tentatively requesting a merge. Anyone else care to weigh in on the tradeoff between leaving this broken in 71 vs taking the merge at the risk that it still isn't perfect? As above, I think it's an improvement and likely is the real fix.

This bug requires manual review: M71 has already been promoted to the beta branch, so this requires manual review
Please contact the milestone owner if you have questions.
Owners: benmason@(Android), kariahda@(iOS), kbleicher@(ChromeOS), govind@(Desktop)

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot

As this is regressed in M70, can this wait until M72 as  M71 is very close to stable promotion? We're trying to minimize the merges to reduce risk as M71 is going to be a holiday release.

It's okay by me to wait as long as we're okay with a UI glitch.

Approving merge to M71 branch 3578 based on comment #19. Pls merge ASAP so we can pick it up for tomororw's besta release, cutting RC soon,Thank you.

Per grt@, this is not a clean merge. So rejecting merge to M71 also this is not M71 regression and we've getting very close to M71 stable promotion. Thank you.

Since this is already in M70, we can live without the merge to M71.

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/191b339d03c9d680b43978f5e2958beae1807ab0

commit 191b339d03c9d680b43978f5e2958beae1807ab0
Author: Greg Thompson <grt@chromium.org>
Date: Wed Nov 21 10:14:45 2018

Style cleanups following r606842.

BUG= 898958 
R=pkasting@chromium.org

Change-Id: Ib5790e607c4b0d500e39cfea7dd240596dee27ca
Reviewed-on: https://chromium-review.googlesource.com/c/1333372
Commit-Queue: Greg Thompson <grt@chromium.org>
Reviewed-by: Peter Kasting <pkasting@chromium.org>
Cr-Commit-Position: refs/heads/master@{#609981}
[modify] https://crrev.com/191b339d03c9d680b43978f5e2958beae1807ab0/chrome/browser/ui/toolbar/app_menu_icon_controller.h
[modify] https://crrev.com/191b339d03c9d680b43978f5e2958beae1807ab0/chrome/browser/ui/views/toolbar/browser_app_menu_button.cc
[modify] https://crrev.com/191b339d03c9d680b43978f5e2958beae1807ab0/chrome/browser/ui/views/toolbar/browser_app_menu_button.h