Use mojo volume interfaces for mash and classic ash.

Use mojo volume interfaces for mash and classic ash.

Add mojom::VolumeController; remove VolumeControlDelegate.
Make AccelleratorController connect to VolumeController.

Move UMA recording to ash, instead of Chrome.
Update unit tests; check UMA and accelerator history.
Move extension code to ExtensionSystemEventObserver.

BUG=657126
TEST=Automated; no Chrome OS volume button regressions.
R=jamescook@chromium.org,sky@chromium.org,dcheng@chromium.org

Committed: https://crrev.com/67f43b078ce358d8b81ef120bfd13a07be8c6eed
Cr-Commit-Position: refs/heads/master@{#426622}

[msw, 2016-10-18 20:46:52]

Description was changed from

==========
Use mojo volume interfaces for mash and classic ash.

BUG=657126
TEST=Automated; No Chrome OS volume button regressions.
R=jamescook@chromium.org
==========

to

==========
Use mojo volume interfaces for mash and classic ash.

Add mojom::VolumeController; remove VolumeControlDelegate.
Make AccelleratorController connect to VolumeController.

Move UMA recording to ash, instead of Chrome.
Update unit tests; check UMA and accelerator history.

BUG=657126
TEST=Automated; no Chrome OS volume button regressions.
R=jamescook@chromium.org
==========

[msw, 2016-10-18 20:47:03]

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

[commit-bot: I haz the power, 2016-10-18 20:47:33]

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

[msw, 2016-10-18 20:50:01]

Patchset #2 (id:20001) has been deleted

[msw, 2016-10-18 20:50:07]

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

[commit-bot: I haz the power, 2016-10-18 20:50:50]

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-10-18 21:33:11]

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

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

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

[msw, 2016-10-18 23:16:35]

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

[commit-bot: I haz the power, 2016-10-18 23:17:31]

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

[msw, 2016-10-18 23:24:13]

Hey James, please take a look; thanks!

[James Cook, 2016-10-18 23:53:40]

LGTM. Nice that this patch is net-negative lines of code.

https://codereview.chromium.org/2427913003/diff/60001/ash/accelerators/accele...
File ash/accelerators/accelerator_controller_unittest.cc (right):

https://codereview.chromium.org/2427913003/diff/60001/ash/accelerators/accele...
ash/accelerators/accelerator_controller_unittest.cc:841: base::UserActionTester
user_action_tester;
Clever. I didn't know we had this.

https://codereview.chromium.org/2427913003/diff/60001/ash/common/accelerators...
File ash/common/accelerators/accelerator_controller.h (right):

https://codereview.chromium.org/2427913003/diff/60001/ash/common/accelerators...
ash/common/accelerators/accelerator_controller.h:184: // The cached volume
controller interface pointer, implemented in Chrome.
super nit: I might leave off the "implemented in Chrome" bit. The interface
itself describes who implements it, and I was a bit confused at first (I thought
it pointed to an object whose implementation was in chrome :-)

https://codereview.chromium.org/2427913003/diff/60001/ash/public/interfaces/v...
File ash/public/interfaces/volume.mojom (right):

https://codereview.chromium.org/2427913003/diff/60001/ash/public/interfaces/v...
ash/public/interfaces/volume.mojom:7: // Implemented by Chrome and used by ash
to request audio volume changes.
super nit: I might add a quick note about why this has to be delegated to chrome
(chrome controls media, also volume changes need to be propagated to
extensions/apps, like videoconferencing apps).

https://codereview.chromium.org/2427913003/diff/60001/chrome/browser/ui/ash/v...
File chrome/browser/ui/ash/volume_controller_chromeos.cc (right):

https://codereview.chromium.org/2427913003/diff/60001/chrome/browser/ui/ash/v...
chrome/browser/ui/ash/volume_controller_chromeos.cc:1: // Copyright 2016 The
Chromium Authors. All rights reserved.
nit: I don't think you need to update this.

https://codereview.chromium.org/2427913003/diff/60001/chrome/browser/ui/ash/v...
chrome/browser/ui/ash/volume_controller_chromeos.cc:14: #include
"content/public/browser/user_metrics.h"
nit: remove?

[commit-bot: I haz the power, 2016-10-19 01:35:17]

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

[commit-bot: I haz the power, 2016-10-19 01:35:18]

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

[msw, 2016-10-19 15:19:08]

Patchset #4 (id:80001) has been deleted

[msw, 2016-10-19 15:21:29]

Description was changed from

==========
Use mojo volume interfaces for mash and classic ash.

Add mojom::VolumeController; remove VolumeControlDelegate.
Make AccelleratorController connect to VolumeController.

Move UMA recording to ash, instead of Chrome.
Update unit tests; check UMA and accelerator history.

BUG=657126
TEST=Automated; no Chrome OS volume button regressions.
R=jamescook@chromium.org
==========

to

==========
Use mojo volume interfaces for mash and classic ash.

Add mojom::VolumeController; remove VolumeControlDelegate.
Make AccelleratorController connect to VolumeController.

Move UMA recording to ash, instead of Chrome.
Update unit tests; check UMA and accelerator history.

BUG=657126
TEST=Automated; no Chrome OS volume button regressions.
R=jamescook@chromium.org,sky@chromium.org,dcheng@chromium.org
==========

[msw, 2016-10-19 15:21:30]

msw@chromium.org changed reviewers:
+ dcheng@chromium.org, sky@chromium.org

[msw, 2016-10-19 15:24:11]

Hey Scott, please take a look at chrome (minus browser/ui).
Hey Daniel, please take a look at volume.mojom.
Thanks!

https://codereview.chromium.org/2427913003/diff/60001/ash/accelerators/accele...
File ash/accelerators/accelerator_controller_unittest.cc (right):

https://codereview.chromium.org/2427913003/diff/60001/ash/accelerators/accele...
ash/accelerators/accelerator_controller_unittest.cc:841: base::UserActionTester
user_action_tester;
On 2016/10/18 23:53:39, James Cook wrote:
> Clever. I didn't know we had this.

Acknowledged.

https://codereview.chromium.org/2427913003/diff/60001/ash/common/accelerators...
File ash/common/accelerators/accelerator_controller.h (right):

https://codereview.chromium.org/2427913003/diff/60001/ash/common/accelerators...
ash/common/accelerators/accelerator_controller.h:184: // The cached volume
controller interface pointer, implemented in Chrome.
On 2016/10/18 23:53:39, James Cook wrote:
> super nit: I might leave off the "implemented in Chrome" bit. The interface
> itself describes who implements it, and I was a bit confused at first (I
thought
> it pointed to an object whose implementation was in chrome :-)

Done.

https://codereview.chromium.org/2427913003/diff/60001/ash/public/interfaces/v...
File ash/public/interfaces/volume.mojom (right):

https://codereview.chromium.org/2427913003/diff/60001/ash/public/interfaces/v...
ash/public/interfaces/volume.mojom:7: // Implemented by Chrome and used by ash
to request audio volume changes.
On 2016/10/18 23:53:40, James Cook wrote:
> super nit: I might add a quick note about why this has to be delegated to
chrome
> (chrome controls media, also volume changes need to be propagated to
> extensions/apps, like videoconferencing apps).

I added a note; let me know if it should be changed.

https://codereview.chromium.org/2427913003/diff/60001/chrome/browser/ui/ash/v...
File chrome/browser/ui/ash/volume_controller_chromeos.cc (right):

https://codereview.chromium.org/2427913003/diff/60001/chrome/browser/ui/ash/v...
chrome/browser/ui/ash/volume_controller_chromeos.cc:1: // Copyright 2016 The
Chromium Authors. All rights reserved.
On 2016/10/18 23:53:40, James Cook wrote:
> nit: I don't think you need to update this.

Done.

https://codereview.chromium.org/2427913003/diff/60001/chrome/browser/ui/ash/v...
chrome/browser/ui/ash/volume_controller_chromeos.cc:14: #include
"content/public/browser/user_metrics.h"
On 2016/10/18 23:53:40, James Cook wrote:
> nit: remove?

Done.

[msw, 2016-10-19 15:24:18]

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

[commit-bot: I haz the power, 2016-10-19 15:24:54]

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

[sky, 2016-10-19 15:26:39]

LGTM

https://codereview.chromium.org/2427913003/diff/100001/chrome/browser/chromeo...
File chrome/browser/chromeos/chrome_interface_factory.cc (right):

https://codereview.chromium.org/2427913003/diff/100001/chrome/browser/chromeo...
chrome/browser/chromeos/chrome_interface_factory.cc:133:
volume_controller_.reset(new VolumeController);
MakeUnique if possible.

[commit-bot: I haz the power, 2016-10-19 15:27:23]

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

[commit-bot: I haz the power, 2016-10-19 15:27:25]

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

[msw, 2016-10-19 16:00:10]

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

[commit-bot: I haz the power, 2016-10-19 16:00:36]

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-10-19 17:16:50]

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

[commit-bot: I haz the power, 2016-10-19 17:16:51]

Dry run: This issue passed the CQ dry run.

[dcheng, 2016-10-19 22:24:08]

mojo lgtm

[msw, 2016-10-19 22:36:25]

https://codereview.chromium.org/2427913003/diff/100001/chrome/browser/chromeo...
File chrome/browser/chromeos/chrome_interface_factory.cc (right):

https://codereview.chromium.org/2427913003/diff/100001/chrome/browser/chromeo...
chrome/browser/chromeos/chrome_interface_factory.cc:133:
volume_controller_.reset(new VolumeController);
On 2016/10/19 15:26:39, sky wrote:
> MakeUnique if possible.

Done (here and elsewhere).

[msw, 2016-10-19 22:36:33]

The CQ bit was checked by msw@chromium.org

[msw, 2016-10-19 22:36:33]

The patchset sent to the CQ was uploaded after l-g-t-m from
jamescook@chromium.org, sky@chromium.org
Link to the patchset: https://codereview.chromium.org/2427913003/#ps140001
(title: "Sync and rebase.")

[commit-bot: I haz the power, 2016-10-19 22:37:29]

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-10-19 23:14:43]

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

[commit-bot: I haz the power, 2016-10-19 23:14:45]

Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[msw, 2016-10-20 03:04:19]

Description was changed from

==========
Use mojo volume interfaces for mash and classic ash.

Add mojom::VolumeController; remove VolumeControlDelegate.
Make AccelleratorController connect to VolumeController.

Move UMA recording to ash, instead of Chrome.
Update unit tests; check UMA and accelerator history.

BUG=657126
TEST=Automated; no Chrome OS volume button regressions.
R=jamescook@chromium.org,sky@chromium.org,dcheng@chromium.org
==========

to

==========
Use mojo volume interfaces for mash and classic ash.

Add mojom::VolumeController; remove VolumeControlDelegate.
Make AccelleratorController connect to VolumeController.

Move UMA recording to ash, instead of Chrome.
Update unit tests; check UMA and accelerator history.
Move extension code to ExtensionSystemEventObserver.

BUG=657126
TEST=Automated; no Chrome OS volume button regressions.
R=jamescook@chromium.org,sky@chromium.org,dcheng@chromium.org
==========

[msw, 2016-10-20 03:08:55]

Scott, please take another look at the latest Chrome OS changes.
I moved volume change event observation for extensions around.
(we want notifications outside the accelerator helper's lifetime)

Dan and Rahul, please feel free to weigh in on that part of the CL.
(you respectively added and removed [use of] that class, etc.)

[msw, 2016-10-20 03:09:08]

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

[commit-bot: I haz the power, 2016-10-20 03:09:38]

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-10-20 04:29:40]

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

[commit-bot: I haz the power, 2016-10-20 04:29:41]

Dry run: This issue passed the CQ dry run.

[sky, 2016-10-20 15:41:07]

LGTM

https://codereview.chromium.org/2427913003/diff/140002/chrome/browser/chromeo...
File chrome/browser/chromeos/chrome_browser_main_chromeos.cc (right):

https://codereview.chromium.org/2427913003/diff/140002/chrome/browser/chromeo...
chrome/browser/chromeos/chrome_browser_main_chromeos.cc:675:
extension_system_event_observer_.reset(new ExtensionSystemEventObserver());
MakeUnique if you can.

https://codereview.chromium.org/2427913003/diff/140002/chrome/browser/chromeo...
File chrome/browser/chromeos/extensions/extension_system_event_observer.cc
(right):

https://codereview.chromium.org/2427913003/diff/140002/chrome/browser/chromeo...
chrome/browser/chromeos/extensions/extension_system_event_observer.cc:50:
CrasAudioHandler* audio_handler = CrasAudioHandler::Get();
This is the same as OnOutputMuteChanged. Move to common function, perhaps
DispatchVolumeChangedEvent()?

[Daniel Erat, 2016-10-20 15:46:59]

derat@chromium.org changed reviewers:
+ derat@chromium.org

[Daniel Erat, 2016-10-20 15:47:00]

replied over email, but given that ExtensionSystemEventObserver seems to have
been uninstantiated for more than a year, i'm wondering if it can be deleted and
the volume stuff related to it removed.

[msw, 2016-10-20 16:15:32]

+CC tbarzic@ and zelidrag@, regarding this cros extension question.

On 2016/10/20 15:47:00, Daniel Erat wrote:
> given that ExtensionSystemEventObserver seems to have
> been uninstantiated for more than a year, i'm wondering if it can be deleted
and
> the volume stuff related to it removed.

I'm not familiar enough with extension apis to make that call. Hopefully someone
can tell me which of these notifications are actually desired, if any. Please
loop in anyone else knowledgable or opinionated here.

https://codereview.chromium.org/2427913003/diff/140002/chrome/browser/chromeo...
File chrome/browser/chromeos/chrome_browser_main_chromeos.cc (right):

https://codereview.chromium.org/2427913003/diff/140002/chrome/browser/chromeo...
chrome/browser/chromeos/chrome_browser_main_chromeos.cc:675:
extension_system_event_observer_.reset(new ExtensionSystemEventObserver());
On 2016/10/20 15:41:07, sky wrote:
> MakeUnique if you can.

Done here and elsewhere in this function.

https://codereview.chromium.org/2427913003/diff/140002/chrome/browser/chromeo...
File chrome/browser/chromeos/extensions/extension_system_event_observer.cc
(right):

https://codereview.chromium.org/2427913003/diff/140002/chrome/browser/chromeo...
chrome/browser/chromeos/extensions/extension_system_event_observer.cc:50:
CrasAudioHandler* audio_handler = CrasAudioHandler::Get();
On 2016/10/20 15:41:07, sky wrote:
> This is the same as OnOutputMuteChanged. Move to common function, perhaps
> DispatchVolumeChangedEvent()?

Done.

[msw, 2016-10-20 16:15:55]

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

[commit-bot: I haz the power, 2016-10-20 16:16:20]

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-10-20 16:19:22]

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

[commit-bot: I haz the power, 2016-10-20 16:19:23]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Rahul Chaturvedi, 2016-10-20 16:56:34]

steel@chromium.org changed reviewers:
+ steel@chromium.org

[Rahul Chaturvedi, 2016-10-20 16:56:35]

It seems that it is unused in the code and no one seems to know what it is.
These seem pretty good reasons to just delete it.

[tbarzic, 2016-10-20 17:36:51]

tbarzic@chromium.org changed reviewers:
+ tbarzic@chromium.org

[tbarzic, 2016-10-20 17:36:52]

Yeah, if it isn't actually used (and it hasn't been for a while), it would make
sense to remove the class.
It looks like this was supposed to fire chrome.systemPrivate API events (they
seem to be needed by spoken feedback, at least used to be needed). We should
probably remove those as well (not in this cl though)
+dmazzoni, the owner of system_private API

[msw, 2016-10-20 19:58:16]

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

[commit-bot: I haz the power, 2016-10-20 19:58:41]

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

[dmazzoni, 2016-10-20 20:33:37]

On 2016/10/20 17:36:52, tbarzic wrote:
> Yeah, if it isn't actually used (and it hasn't been for a while), it would
make
> sense to remove the class.
> It looks like this was supposed to fire chrome.systemPrivate API events (they
> seem to be needed by spoken feedback, at least used to be needed). We should
> probably remove those as well (not in this cl though)
> +dmazzoni, the owner of system_private API

Yes, we used to use the systemPrivate API for accessibility but I believe that a
year or two ago we eliminated any dependence on it.

I see a few component extensions that declare systemPrivate permission,
should we audit those and see if they need it anymore.

[msw, 2016-10-20 21:05:28]

I moved the volume observer and notifications to a separate class to avoid
restoring the other extension system event notifications. I filed
http://crbug.com/658006 to audit unused extension system event notification
code.

Checking the cq bit, but any more review/feedback is welcome.

[msw, 2016-10-20 21:05:36]

The CQ bit was checked by msw@chromium.org

[msw, 2016-10-20 21:05:36]

The patchset sent to the CQ was uploaded after l-g-t-m from
jamescook@chromium.org, dcheng@chromium.org, sky@chromium.org
Link to the patchset: https://codereview.chromium.org/2427913003/#ps230001
(title: "Split volume and system events observers; only hook up volume.")

[commit-bot: I haz the power, 2016-10-20 21:06:18]

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-10-20 22:43:03]

Description was changed from

==========
Use mojo volume interfaces for mash and classic ash.

Add mojom::VolumeController; remove VolumeControlDelegate.
Make AccelleratorController connect to VolumeController.

Move UMA recording to ash, instead of Chrome.
Update unit tests; check UMA and accelerator history.
Move extension code to ExtensionSystemEventObserver.

BUG=657126
TEST=Automated; no Chrome OS volume button regressions.
R=jamescook@chromium.org,sky@chromium.org,dcheng@chromium.org
==========

to

==========
Use mojo volume interfaces for mash and classic ash.

Add mojom::VolumeController; remove VolumeControlDelegate.
Make AccelleratorController connect to VolumeController.

Move UMA recording to ash, instead of Chrome.
Update unit tests; check UMA and accelerator history.
Move extension code to ExtensionSystemEventObserver.

BUG=657126
TEST=Automated; no Chrome OS volume button regressions.
R=jamescook@chromium.org,sky@chromium.org,dcheng@chromium.org
==========

[commit-bot: I haz the power, 2016-10-20 22:43:07]

Committed patchset #11 (id:230001)

[commit-bot: I haz the power, 2016-10-21 13:23:36]

Description was changed from

==========
Use mojo volume interfaces for mash and classic ash.

Add mojom::VolumeController; remove VolumeControlDelegate.
Make AccelleratorController connect to VolumeController.

Move UMA recording to ash, instead of Chrome.
Update unit tests; check UMA and accelerator history.
Move extension code to ExtensionSystemEventObserver.

BUG=657126
TEST=Automated; no Chrome OS volume button regressions.
R=jamescook@chromium.org,sky@chromium.org,dcheng@chromium.org
==========

to

==========
Use mojo volume interfaces for mash and classic ash.

Add mojom::VolumeController; remove VolumeControlDelegate.
Make AccelleratorController connect to VolumeController.

Move UMA recording to ash, instead of Chrome.
Update unit tests; check UMA and accelerator history.
Move extension code to ExtensionSystemEventObserver.

BUG=657126
TEST=Automated; no Chrome OS volume button regressions.
R=jamescook@chromium.org,sky@chromium.org,dcheng@chromium.org

Committed: https://crrev.com/67f43b078ce358d8b81ef120bfd13a07be8c6eed
Cr-Commit-Position: refs/heads/master@{#426622}
==========

[commit-bot: I haz the power, 2016-10-21 13:23:37]

Patchset 11 (id:??) landed as
https://crrev.com/67f43b078ce358d8b81ef120bfd13a07be8c6eed
Cr-Commit-Position: refs/heads/master@{#426622}