Move Voice Interaction shortcut to accelerator_controller.cc

chrome/browser/chromeos/arc/voice_interaction/arc_voice_interaction_framework_service.cc

Index: chrome/browser/chromeos/arc/voice_interaction/arc_voice_interaction_framework_service.cc
diff --git a/chrome/browser/chromeos/arc/voice_interaction/arc_voice_interaction_framework_service.cc b/chrome/browser/chromeos/arc/voice_interaction/arc_voice_interaction_framework_service.cc
index 1b89b06f539261b8b4d7d34093821438eeab5a17..057eca3929ee9d050f25f78531d5d1f12e836791 100644
--- a/chrome/browser/chromeos/arc/voice_interaction/arc_voice_interaction_framework_service.cc
+++ b/chrome/browser/chromeos/arc/voice_interaction/arc_voice_interaction_framework_service.cc
@@ -13,6 +13,7 @@
 #include "base/logging.h"
 #include "base/memory/ref_counted.h"
 #include "base/task_scheduler/post_task.h"
+#include "chrome/browser/ui/app_list/arc/arc_app_utils.h"

[Yusuke Sato, 2017/05/23 19:41:52]

components/arc/arc_util.h

[yueli, 2017/05/23 22:59:56]

Done.

 #include "chromeos/chromeos_switches.h"
 #include "components/arc/arc_bridge_service.h"
 #include "components/arc/instance_holder.h"
@@ -61,17 +62,10 @@ void ArcVoiceInteractionFrameworkService::OnInstanceReady() {
   DCHECK(framework_instance);
   framework_instance->Init(binding_.CreateInterfacePtrAndBind());
 
-  // TODO(updowndota): Move the dynamic shortcuts to accelerator_controller.cc
-  // to prevent several issues.
-  ash::Shell::Get()->accelerator_controller()->Register(
-      {ui::Accelerator(ui::VKEY_A, ui::EF_COMMAND_DOWN)}, this);
   // Temporary shortcut added to enable the metalayer experiment.
   ash::Shell::Get()->accelerator_controller()->Register(
       {ui::Accelerator(ui::VKEY_A, ui::EF_COMMAND_DOWN | ui::EF_SHIFT_DOWN)},
       this);
-  // Temporary shortcut added for UX/PM exploration.
-  ash::Shell::Get()->accelerator_controller()->Register(
-      {ui::Accelerator(ui::VKEY_SPACE, ui::EF_COMMAND_DOWN)}, this);
 }
 
 void ArcVoiceInteractionFrameworkService::OnInstanceClosed() {
@@ -85,23 +79,14 @@ bool ArcVoiceInteractionFrameworkService::AcceleratorPressed(
     const ui::Accelerator& accelerator) {
   DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
 
-  if (accelerator.IsShiftDown()) {
-    // Temporary, used for debugging.
-    // Does not take into account or update the palette state.
-    mojom::VoiceInteractionFrameworkInstance* framework_instance =
-        ARC_GET_INSTANCE_FOR_METHOD(
-            arc_bridge_service()->voice_interaction_framework(),
-            SetMetalayerVisibility);
-    DCHECK(framework_instance);
-    framework_instance->SetMetalayerVisibility(true);
-  } else {
-    mojom::VoiceInteractionFrameworkInstance* framework_instance =
-        ARC_GET_INSTANCE_FOR_METHOD(
-            arc_bridge_service()->voice_interaction_framework(),
-            StartVoiceInteractionSession);
-    DCHECK(framework_instance);
-    framework_instance->StartVoiceInteractionSession();
-  }
+  // Temporary, used for debugging.
+  // Does not take into account or update the palette state.
+  mojom::VoiceInteractionFrameworkInstance* framework_instance =
+      ARC_GET_INSTANCE_FOR_METHOD(
+          arc_bridge_service()->voice_interaction_framework(),
+          SetMetalayerVisibility);
+  DCHECK(framework_instance);
+  framework_instance->SetMetalayerVisibility(true);
 
   return true;
 }
@@ -178,6 +163,18 @@ void ArcVoiceInteractionFrameworkService::HideMetalayer() {
   SetMetalayerVisibility(false);
 }
 
+void ArcVoiceInteractionFrameworkService::StartVoiceInteractionSession() {
+  mojom::VoiceInteractionFrameworkInstance* framework_instance =
+      ARC_GET_INSTANCE_FOR_METHOD(
+          arc_bridge_service()->voice_interaction_framework(),
+          StartVoiceInteractionSession);
+  if (framework_instance) {
+    framework_instance->StartVoiceInteractionSession();
+  } else {
+    arc::PrioritizeArcContainerStartup();

[Yusuke Sato, 2017/05/23 19:41:51]

If the user starts an ARC app (e.g. Play Store) before the container is ready,
ash shows a spinner icon to notify the user that the app is being launched.
Shouldn't we do the same for this feature? Calling the prioritization function
is no-op from the user's perspective.

[yueli, 2017/05/23 22:59:56]

Currently we have no place to show such spinner since we don't even have an icon
for voice interaction on the shelf. And the UX side also have not come up with a
solution for this currently.

[Yusuke Sato, 2017/05/23 23:11:56]

Ok, but please make sure to inform UX team of the problem. The current behavior
is probably not optimal for users.

For now, can you add a TODO?

+  }
+}
+
 void ArcVoiceInteractionFrameworkService::SetMetalayerVisibility(bool visible) {
   DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
   mojom::VoiceInteractionFrameworkInstance* framework_instance =