mash: Have chrome set itself as a controller interface for changing volume

ash/common/accelerators/accelerator_controller.h

Index: ash/common/accelerators/accelerator_controller.h
diff --git a/ash/common/accelerators/accelerator_controller.h b/ash/common/accelerators/accelerator_controller.h
index 7a5ae515da4a2323719d2ed466c1f35fd8e394a8..c418fccf03dbffb83a446e9331c2947f238c1110 100644
--- a/ash/common/accelerators/accelerator_controller.h
+++ b/ash/common/accelerators/accelerator_controller.h
@@ -18,6 +18,7 @@
 #include "base/compiler_specific.h"
 #include "base/gtest_prod_util.h"
 #include "base/macros.h"
+#include "mojo/public/cpp/bindings/binding_set.h"
 #include "ui/base/accelerators/accelerator.h"
 #include "ui/base/accelerators/accelerator_history.h"
 
@@ -35,8 +36,12 @@ class ImeControlDelegate;
 
 // AcceleratorController provides functions for registering or unregistering
 // global keyboard accelerators, which are handled earlier than any windows. It
-// also implements several handlers as an accelerator target.
-class ASH_EXPORT AcceleratorController : public ui::AcceleratorTarget {
+// also implements several handlers as an accelerator target. Implements
+// mojom::Volume so that Chrome can set itself as the mojom::VolumeClient,
+// which is only used by accelerator keys.
+class ASH_EXPORT AcceleratorController
+    : public ui::AcceleratorTarget,
+      NON_EXPORTED_BASE(public mojom::Volume) {

[msw, 2016/12/03 00:26:43]

Inheriting a Volume-specific interface here is somewhat odd. Consider instead
adding mojom:Accelerator[Controller] with SetVolumeClient(mojom::VolumeClientPtr
client). Renaming the interface doesn't commit us to anything more, it just
reflects the implementor. It'll be a little odd for Chrome to tell ash's
accelerator controller "here's your volume client", but it can all be scrapped
when we deprecate chrome's CrasAudioHandler/dbus use. WDYT?

[James Cook, 2016/12/06 19:23:57]

Done.

  public:
   AcceleratorController(AcceleratorControllerDelegate* delegate,
                         ui::AcceleratorManagerDelegate* manager_delegate);
@@ -121,6 +126,12 @@ class ASH_EXPORT AcceleratorController : public ui::AcceleratorTarget {
   bool AcceleratorPressed(const ui::Accelerator& accelerator) override;
   bool CanHandleAccelerators() const override;
 
+  // Binds the mojom::Volume interface to this object.
+  void BindRequest(mojom::VolumeRequest request);
+
+  // mojom::Volume:
+  void SetClient(mojom::VolumeClientPtr client) override;
+
  private:
   FRIEND_TEST_ALL_PREFIXES(AcceleratorControllerTest, GlobalAccelerators);
   FRIEND_TEST_ALL_PREFIXES(AcceleratorControllerTest,
@@ -156,10 +167,6 @@ class ASH_EXPORT AcceleratorController : public ui::AcceleratorTarget {
   AcceleratorProcessingRestriction GetAcceleratorProcessingRestriction(
       int action);
 
-  // Returns the volume controller interface raw pointer, may be null in tests.
-  mojom::VolumeController* GetVolumeController();
-  void OnVolumeControllerConnectionError();
-
   AcceleratorControllerDelegate* delegate_;
 
   std::unique_ptr<ui::AcceleratorManager> accelerator_manager_;
@@ -181,8 +188,11 @@ class ASH_EXPORT AcceleratorController : public ui::AcceleratorTarget {
       actions_with_deprecations_;
   std::set<ui::Accelerator> deprecated_accelerators_;
 
-  // The cached volume controller interface pointer.
-  mojom::VolumeControllerPtr volume_controller_;
+  // Bindings for the mojom::Volume interface.
+  mojo::BindingSet<mojom::Volume> bindings_;
+
+  // Client interface in chrome browser. May be null in tests.

[msw, 2016/12/05 20:15:09]

optional nit: Mention here that it's used for [CrasAudioHandler] volume control
functions? Changing 'controller' -> 'client' makes the purpose of this member
less clear to me (I think "Why does the accelerator controller need to access
some random client of the volume interface?").

[James Cook, 2016/12/06 19:23:57]

Done.

+  mojom::VolumeClientPtr volume_client_;
 
   // Actions allowed when the user is not signed in.
   std::set<int> actions_allowed_at_login_screen_;