ash: Use system tray mojo interface to show system update tray icon

ash: Use system tray mojo interface to show system update tray icon

Change the flow so that Chrome explicitly asks ash to show the icon
rather than having ash ask Chrome whether there is an update available.

* Add ShowUpdateIcon to system_tray.mojom
* Introduce update.mojom
* Migrate ash to using ash::mojom::UpdateSeverity internally
* Migrate update methods from SystemTrayDelegate to SystemTrayClient
* Add a new SystemTrayClientTest and move existing Flash test there

Also add docs for SystemTrayItem (used by TrayUpdate) since tray view
vs. default view vs. detailed view always confuses me.

BUG=647412
TEST=chrome browser_tests, ash_unittests

Committed: https://crrev.com/196ee3b66b3700733cecb8472d63d872d99e3783
Cr-Commit-Position: refs/heads/master@{#438051}

[James Cook, 2016-12-09 22:26:00]

Description was changed from

==========
ash: Use system tray mojo interface to show system update tray icon

* Add SetUpdateRecommended to system_tray.mojom
* Migrate ash to using ash::mojom::UpdateInfo directly
* Migrate update methods from SystemTrayDelegate to SystemTrayClient
* Add SystemTrayClientTest and move existing Flash test there
* Add docs for SystemTrayItem (used by TrayUpdate)

BUG=647412
TEST=chrome browser_tests, ash_unittests
==========

to

==========
ash: Use system tray mojo interface to show system update tray icon

Change the flow so that Chrome explicitly asks ash to show the icon
rather than having ash ask Chrome whether there is an update available.

* Add ShowUpdateIcon to system_tray.mojom
* Introduce update.mojom
* Migrate ash to using ash::mojom::UpdateSeverity internally
* Migrate update methods from SystemTrayDelegate to SystemTrayClient
* Add a new SystemTrayClientTest and move existing Flash test there

Also add docs for SystemTrayItem (used by TrayUpdate) since tray view
vs. default view vs. detailed view always confuses me.

BUG=647412
TEST=chrome browser_tests, ash_unittests
==========

[James Cook, 2016-12-09 22:32:45]

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

[commit-bot: I haz the power, 2016-12-09 22:33:58]

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

[James Cook, 2016-12-09 22:34:12]

jamescook@chromium.org changed reviewers:
+ kerrnel@chromium.org, msw@chromium.org

[James Cook, 2016-12-09 22:34:13]

msw, please take a look at everything. Look closely, since we don't have great
end-to-end test coverage here, and it would be pretty bad if I screw up update
notifications. :-)

kerrnel, please take a look at the Flash update bits in chrome/browser.

https://codereview.chromium.org/2558043006/diff/40001/ash/common/system/updat...
File ash/common/system/update/tray_update_unittest.cc (right):

https://codereview.chromium.org/2558043006/diff/40001/ash/common/system/updat...
ash/common/system/update/tray_update_unittest.cc:26:
WmShell::Get()->system_tray_controller()->ShowUpdateIcon(
I decided to test this at the SystemTrayController level instead of
SystemTrayNotifier because I think we should test as close as possible to the
exposed mojo interfaces.

[msw, 2016-12-09 23:41:52]

This is a great Cl; lgtm with minor comments/nits.
I'll gladly take another look if you nix the observer.

https://codereview.chromium.org/2558043006/diff/40001/ash/common/system/tray/...
File ash/common/system/tray/system_tray_item.h (right):

https://codereview.chromium.org/2558043006/diff/40001/ash/common/system/tray/...
ash/common/system/tray/system_tray_item.h:28: // Controller for an item in the
system tray. Each item can create these views:
Nice; thanks for the comment!

https://codereview.chromium.org/2558043006/diff/40001/ash/common/system/updat...
File ash/common/system/update/tray_update.cc (right):

https://codereview.chromium.org/2558043006/diff/40001/ash/common/system/updat...
ash/common/system/update/tray_update.cc:167: return false;
Should this just return update_required_?

https://codereview.chromium.org/2558043006/diff/40001/ash/common/system/updat...
File ash/common/system/update/tray_update_unittest.cc (right):

https://codereview.chromium.org/2558043006/diff/40001/ash/common/system/updat...
ash/common/system/update/tray_update_unittest.cc:26:
WmShell::Get()->system_tray_controller()->ShowUpdateIcon(
On 2016/12/09 22:34:13, James Cook wrote:
> I decided to test this at the SystemTrayController level instead of
> SystemTrayNotifier because I think we should test as close as possible to the
> exposed mojo interfaces.

Acknowledged.

https://codereview.chromium.org/2558043006/diff/40001/ash/common/system/updat...
File ash/common/system/update/update_observer.h (right):

https://codereview.chromium.org/2558043006/diff/40001/ash/common/system/updat...
ash/common/system/update/update_observer.h:18: virtual void
ShowUpdateIcon(mojom::UpdateSeverity severity,
It's somewhat odd that we have an observer pattern for the
SystemTray[Controller|Notifier] to reach TrayUpdate's implementation, especially
since that's the only observer, and the observer function is more of an
imperative ("Show it") than a heads up ("An Update is recommended"). Is it
possible and desirable to simplify this (perhaps in a followup)? It seems like
SystemTrayController could use something like SystemTray::GetTrayUpdate() to
replace the observation and avoid the notifier altogether.

https://codereview.chromium.org/2558043006/diff/40001/ash/public/interfaces/s...
File ash/public/interfaces/system_tray.mojom (right):

https://codereview.chromium.org/2558043006/diff/40001/ash/public/interfaces/s...
ash/public/interfaces/system_tray.mojom:21: ShowUpdateIcon(UpdateSeverity
severity, bool factory_reset_required);
Nice that you were able to remove |update_required|, and good comment.

https://codereview.chromium.org/2558043006/diff/40001/chrome/browser/ui/ash/s...
File chrome/browser/ui/ash/system_tray_client.cc (right):

https://codereview.chromium.org/2558043006/diff/40001/chrome/browser/ui/ash/s...
chrome/browser/ui/ash/system_tray_client.cc:354: // Show update icon for Chrome
updates and Flash component updates.
nit: "Show an update icon"

https://codereview.chromium.org/2558043006/diff/40001/chrome/browser/ui/ash/s...
File chrome/browser/ui/ash/system_tray_client_browsertest.cc (right):

https://codereview.chromium.org/2558043006/diff/40001/chrome/browser/ui/ash/s...
chrome/browser/ui/ash/system_tray_client_browsertest.cc:46: // Simualte a Flash
update. This sends a mojo message to ash.
nit: Simulate

[commit-bot: I haz the power, 2016-12-09 23:49:43]

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

[commit-bot: I haz the power, 2016-12-09 23:49:44]

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

[James Cook, 2016-12-12 18:15:11]

jamescook@chromium.org changed reviewers:
+ tsepez@chromium.org

[James Cook, 2016-12-12 18:15:12]

msw / kerrnel - please take another look. I nixed the observer.

tsepez, please take a look at ash/public/interfaces mojom

https://codereview.chromium.org/2558043006/diff/40001/ash/common/system/updat...
File ash/common/system/update/tray_update.cc (right):

https://codereview.chromium.org/2558043006/diff/40001/ash/common/system/updat...
ash/common/system/update/tray_update.cc:167: return false;
On 2016/12/09 23:41:51, msw wrote:
> Should this just return update_required_?

Yeah, I think so.

https://codereview.chromium.org/2558043006/diff/40001/ash/common/system/updat...
File ash/common/system/update/update_observer.h (right):

https://codereview.chromium.org/2558043006/diff/40001/ash/common/system/updat...
ash/common/system/update/update_observer.h:18: virtual void
ShowUpdateIcon(mojom::UpdateSeverity severity,
On 2016/12/09 23:41:51, msw wrote:
> It's somewhat odd that we have an observer pattern for the
> SystemTray[Controller|Notifier] to reach TrayUpdate's implementation,
especially
> since that's the only observer, and the observer function is more of an
> imperative ("Show it") than a heads up ("An Update is recommended"). Is it
> possible and desirable to simplify this (perhaps in a followup)? It seems like
> SystemTrayController could use something like SystemTray::GetTrayUpdate() to
> replace the observation and avoid the notifier altogether.

I removed the observer. This revealed a problem -- if an external monitor was
connected mid-session then its TrayUpdate would have stale info about the update
state. I fixed that by making the cached update info in TrayUpdate static.

https://codereview.chromium.org/2558043006/diff/40001/chrome/browser/ui/ash/s...
File chrome/browser/ui/ash/system_tray_client.cc (right):

https://codereview.chromium.org/2558043006/diff/40001/chrome/browser/ui/ash/s...
chrome/browser/ui/ash/system_tray_client.cc:354: // Show update icon for Chrome
updates and Flash component updates.
On 2016/12/09 23:41:51, msw wrote:
> nit: "Show an update icon"

Done.

https://codereview.chromium.org/2558043006/diff/40001/chrome/browser/ui/ash/s...
File chrome/browser/ui/ash/system_tray_client_browsertest.cc (right):

https://codereview.chromium.org/2558043006/diff/40001/chrome/browser/ui/ash/s...
chrome/browser/ui/ash/system_tray_client_browsertest.cc:46: // Simualte a Flash
update. This sends a mojo message to ash.
On 2016/12/09 23:41:51, msw wrote:
> nit: Simulate

Done.

[James Cook, 2016-12-12 18:35:46]

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

[commit-bot: I haz the power, 2016-12-12 18:36:27]

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

[Greg K, 2016-12-12 18:57:19]

On 2016/12/12 18:15:12, James Cook wrote:
> msw / kerrnel - please take another look. I nixed the observer.
> 
> tsepez, please take a look at ash/public/interfaces mojom
> 
>
https://codereview.chromium.org/2558043006/diff/40001/ash/common/system/updat...
> File ash/common/system/update/tray_update.cc (right):
> 
>
https://codereview.chromium.org/2558043006/diff/40001/ash/common/system/updat...
> ash/common/system/update/tray_update.cc:167: return false;
> On 2016/12/09 23:41:51, msw wrote:
> > Should this just return update_required_?
> 
> Yeah, I think so.
> 
>
https://codereview.chromium.org/2558043006/diff/40001/ash/common/system/updat...
> File ash/common/system/update/update_observer.h (right):
> 
>
https://codereview.chromium.org/2558043006/diff/40001/ash/common/system/updat...
> ash/common/system/update/update_observer.h:18: virtual void
> ShowUpdateIcon(mojom::UpdateSeverity severity,
> On 2016/12/09 23:41:51, msw wrote:
> > It's somewhat odd that we have an observer pattern for the
> > SystemTray[Controller|Notifier] to reach TrayUpdate's implementation,
> especially
> > since that's the only observer, and the observer function is more of an
> > imperative ("Show it") than a heads up ("An Update is recommended"). Is it
> > possible and desirable to simplify this (perhaps in a followup)? It seems
like
> > SystemTrayController could use something like SystemTray::GetTrayUpdate() to
> > replace the observation and avoid the notifier altogether.
> 
> I removed the observer. This revealed a problem -- if an external monitor was
> connected mid-session then its TrayUpdate would have stale info about the
update
> state. I fixed that by making the cached update info in TrayUpdate static.
> 
>
https://codereview.chromium.org/2558043006/diff/40001/chrome/browser/ui/ash/s...
> File chrome/browser/ui/ash/system_tray_client.cc (right):
> 
>
https://codereview.chromium.org/2558043006/diff/40001/chrome/browser/ui/ash/s...
> chrome/browser/ui/ash/system_tray_client.cc:354: // Show update icon for
Chrome
> updates and Flash component updates.
> On 2016/12/09 23:41:51, msw wrote:
> > nit: "Show an update icon"
> 
> Done.
> 
>
https://codereview.chromium.org/2558043006/diff/40001/chrome/browser/ui/ash/s...
> File chrome/browser/ui/ash/system_tray_client_browsertest.cc (right):
> 
>
https://codereview.chromium.org/2558043006/diff/40001/chrome/browser/ui/ash/s...
> chrome/browser/ui/ash/system_tray_client_browsertest.cc:46: // Simualte a
Flash
> update. This sends a mojo message to ash.
> On 2016/12/09 23:41:51, msw wrote:
> > nit: Simulate
> 
> Done.

LGTM for flash, thanks.

[Tom Sepez, 2016-12-12 19:35:58]

lgtm

[msw, 2016-12-12 20:43:35]

lgtm

[James Cook, 2016-12-12 20:52:48]

jamescook@chromium.org changed reviewers:
+ waffles@chromium.org

[James Cook, 2016-12-12 20:52:49]

waffles, can I get an OWNERS stamp for
chrome/browser/component_updater/pepper_flash_component_installer.cc ?

[waffles, 2016-12-12 20:55:54]

component_updater lgtm

[commit-bot: I haz the power, 2016-12-12 21:52:31]

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

[commit-bot: I haz the power, 2016-12-12 21:52:32]

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

[James Cook, 2016-12-12 22:27:15]

jamescook@chromium.org changed reviewers:
+ sky@chromium.org

[James Cook, 2016-12-12 22:27:16]

sky, can I get a rubber stamp for a FRIEND_TEST in
chrome/browser/upgrade_detector.h ?

[James Cook, 2016-12-12 22:27:21]

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

[commit-bot: I haz the power, 2016-12-12 22:33:21]

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

[sky, 2016-12-13 01:44:11]

LGTM

[commit-bot: I haz the power, 2016-12-13 01:53:27]

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

[commit-bot: I haz the power, 2016-12-13 01:53:28]

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...)

[James Cook, 2016-12-13 03:25:33]

The CQ bit was checked by jamescook@chromium.org

[James Cook, 2016-12-13 03:25:34]

The patchset sent to the CQ was uploaded after l-g-t-m from tsepez@chromium.org,
msw@chromium.org, kerrnel@chromium.org, waffles@chromium.org
Link to the patchset: https://codereview.chromium.org/2558043006/#ps100001
(title: "fix gcc")

[commit-bot: I haz the power, 2016-12-13 03:26:10]

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-12-13 04:05:09]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1481599533504750,
"parent_rev": "0772dbe03285b7db3cb5ef4205c695a69e101800", "commit_rev":
"bbee9db634491902eb7fe2d28c65db9be4f2ede8"}

[commit-bot: I haz the power, 2016-12-13 04:09:48]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1481599533504750,
"parent_rev": "d7015dcf48cbb3ea805871737af2169c1dd10f32", "commit_rev":
"a6c34805d914be6629bab2d553be67133b60773c"}

[commit-bot: I haz the power, 2016-12-13 04:11:25]

Description was changed from

==========
ash: Use system tray mojo interface to show system update tray icon

Change the flow so that Chrome explicitly asks ash to show the icon
rather than having ash ask Chrome whether there is an update available.

* Add ShowUpdateIcon to system_tray.mojom
* Introduce update.mojom
* Migrate ash to using ash::mojom::UpdateSeverity internally
* Migrate update methods from SystemTrayDelegate to SystemTrayClient
* Add a new SystemTrayClientTest and move existing Flash test there

Also add docs for SystemTrayItem (used by TrayUpdate) since tray view
vs. default view vs. detailed view always confuses me.

BUG=647412
TEST=chrome browser_tests, ash_unittests
==========

to

==========
ash: Use system tray mojo interface to show system update tray icon

Change the flow so that Chrome explicitly asks ash to show the icon
rather than having ash ask Chrome whether there is an update available.

* Add ShowUpdateIcon to system_tray.mojom
* Introduce update.mojom
* Migrate ash to using ash::mojom::UpdateSeverity internally
* Migrate update methods from SystemTrayDelegate to SystemTrayClient
* Add a new SystemTrayClientTest and move existing Flash test there

Also add docs for SystemTrayItem (used by TrayUpdate) since tray view
vs. default view vs. detailed view always confuses me.

BUG=647412
TEST=chrome browser_tests, ash_unittests

Review-Url: https://codereview.chromium.org/2558043006
==========

[commit-bot: I haz the power, 2016-12-13 04:11:26]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2016-12-13 04:13:52]

Description was changed from

==========
ash: Use system tray mojo interface to show system update tray icon

Change the flow so that Chrome explicitly asks ash to show the icon
rather than having ash ask Chrome whether there is an update available.

* Add ShowUpdateIcon to system_tray.mojom
* Introduce update.mojom
* Migrate ash to using ash::mojom::UpdateSeverity internally
* Migrate update methods from SystemTrayDelegate to SystemTrayClient
* Add a new SystemTrayClientTest and move existing Flash test there

Also add docs for SystemTrayItem (used by TrayUpdate) since tray view
vs. default view vs. detailed view always confuses me.

BUG=647412
TEST=chrome browser_tests, ash_unittests

Review-Url: https://codereview.chromium.org/2558043006
==========

to

==========
ash: Use system tray mojo interface to show system update tray icon

Change the flow so that Chrome explicitly asks ash to show the icon
rather than having ash ask Chrome whether there is an update available.

* Add ShowUpdateIcon to system_tray.mojom
* Introduce update.mojom
* Migrate ash to using ash::mojom::UpdateSeverity internally
* Migrate update methods from SystemTrayDelegate to SystemTrayClient
* Add a new SystemTrayClientTest and move existing Flash test there

Also add docs for SystemTrayItem (used by TrayUpdate) since tray view
vs. default view vs. detailed view always confuses me.

BUG=647412
TEST=chrome browser_tests, ash_unittests

Committed: https://crrev.com/196ee3b66b3700733cecb8472d63d872d99e3783
Cr-Commit-Position: refs/heads/master@{#438051}
==========

[commit-bot: I haz the power, 2016-12-13 04:13:53]

Patchset 6 (id:??) landed as
https://crrev.com/196ee3b66b3700733cecb8472d63d872d99e3783
Cr-Commit-Position: refs/heads/master@{#438051}

[Yuta Kitamura, 2016-12-13 05:36:28]

A revert of this CL (patchset #6 id:100001) has been created in
https://codereview.chromium.org/2568413002/ by yutak@chromium.org.

The reason for reverting is: Broke compile on Google Chrome ChromeOS:

https://build.chromium.org/p/chromium.chrome/builders/Google%20Chrome%20Chrom...

../../chrome/browser/component_updater/pepper_flash_component_installer.cc:94:3:
error: no type named 'SystemTrayClient' in namespace 'chromeos'; did you mean
simply 'SystemTrayClient'?
  chromeos::SystemTrayClient* tray = chromeos::SystemTrayClient::Get();
  ^~~~~~~~~~~~~~~~~~~~~~~~~~
  SystemTrayClient
../../chrome/browser/ui/ash/system_tray_client.h:27:7: note: 'SystemTrayClient'
declared here
class SystemTrayClient : public ash::mojom::SystemTrayClient,
      ^
../../chrome/browser/component_updater/pepper_flash_component_installer.cc:94:38:
error: no member named 'SystemTrayClient' in namespace 'chromeos'; did you mean
simply 'SystemTrayClient'?
  chromeos::SystemTrayClient* tray = chromeos::SystemTrayClient::Get();
                                     ^~~~~~~~~~~~~~~~~~~~~~~~~~
                                     SystemTrayClient
../../chrome/browser/ui/ash/system_tray_client.h:27:7: note: 'SystemTrayClient'
declared here
class SystemTrayClient : public ash::mojom::SystemTrayClient,
      ^
2 errors generated.
.

[James Cook, 2016-12-13 17:04:47]

On 2016/12/13 05:36:28, Yuta Kitamura wrote:
> A revert of this CL (patchset #6 id:100001) has been created in
> https://codereview.chromium.org/2568413002/ by mailto:yutak@chromium.org.
> 
> The reason for reverting is: Broke compile on Google Chrome ChromeOS:
> 
>
https://build.chromium.org/p/chromium.chrome/builders/Google%20Chrome%20Chrom...
> 
>
../../chrome/browser/component_updater/pepper_flash_component_installer.cc:94:3:
> error: no type named 'SystemTrayClient' in namespace 'chromeos'; did you mean
> simply 'SystemTrayClient'?
>   chromeos::SystemTrayClient* tray = chromeos::SystemTrayClient::Get();
>   ^~~~~~~~~~~~~~~~~~~~~~~~~~
>   SystemTrayClient
> ../../chrome/browser/ui/ash/system_tray_client.h:27:7: note:
'SystemTrayClient'
> declared here
> class SystemTrayClient : public ash::mojom::SystemTrayClient,
>       ^
>
../../chrome/browser/component_updater/pepper_flash_component_installer.cc:94:38:
> error: no member named 'SystemTrayClient' in namespace 'chromeos'; did you
mean
> simply 'SystemTrayClient'?
>   chromeos::SystemTrayClient* tray = chromeos::SystemTrayClient::Get();
>                                      ^~~~~~~~~~~~~~~~~~~~~~~~~~
>                                      SystemTrayClient
> ../../chrome/browser/ui/ash/system_tray_client.h:27:7: note:
'SystemTrayClient'
> declared here
> class SystemTrayClient : public ash::mojom::SystemTrayClient,
>       ^
> 2 errors generated.
> .

Thanks for the revert. It looks like the section of component updater code I
touched is only compiled in the official build. I'll reland a separate CL.