sync voice_interaction_enabled to ARC.

The flag needs to be synced since hotword does
not go through any entrance point in
ArcVoiceInteractionFrameworkService. For this
case we can only guard context but not the
invocation of voice interaction session.

BUG=b:65963302
TEST=Manual test

Change-Id: I2cddd95b8018e18d79991602834451ebbe28e698
Reviewed-on: https://chromium-review.googlesource.com/676344
Commit-Queue: Muyuan Li <muyuanli@chromium.org>
Reviewed-by: Luis Hector Chavez <lhchavez@chromium.org>
Reviewed-by: Daniel Cheng <dcheng@chromium.org>
Reviewed-by: Xiyuan Xia <xiyuan@chromium.org>
Cr-Commit-Position: refs/heads/master@{#504886}

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

@@ -289,6 +289,8 @@ void ArcVoiceInteractionArcHomeService::GetVoiceInteractionStructure(
     return;
   }
 
+  VLOG(1) << "Retrieving voice interaction context";
+
   content::WebContents* web_contents =
       browser->tab_strip_model()->GetActiveWebContents();
   // Do not process incognito tab.

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

@@ -11,7 +11,7 @@
 #include "ash/public/cpp/shell_window_ids.h"
 #include "ash/shell.h"
 #include "base/bind.h"
-#include "base/callback.h"
+#include "base/callback_helpers.h"
 #include "base/command_line.h"
 #include "base/containers/flat_set.h"
 #include "base/logging.h"
@@ -152,6 +152,9 @@ void EncodeAndReturnImage(
       std::move(callback));
 }
 
+template <typename T>
+void DoNothing(T value) {}
+
 // Singleton factory for ArcVoiceInteractionFrameworkService.
 class ArcVoiceInteractionFrameworkServiceFactory
     : public internal::ArcBrowserContextKeyedServiceFactoryBase<
@@ -199,7 +202,10 @@ KeyedServiceBaseFactory* ArcVoiceInteractionFrameworkService::GetFactory() {
 ArcVoiceInteractionFrameworkService::ArcVoiceInteractionFrameworkService(
     content::BrowserContext* context,
     ArcBridgeService* bridge_service)
-    : context_(context), arc_bridge_service_(bridge_service), binding_(this) {
+    : context_(context),
+      arc_bridge_service_(bridge_service),
+      binding_(this),
+      weak_ptr_factory_(this) {
   arc_bridge_service_->voice_interaction_framework()->AddObserver(this);
   ArcSessionManager::Get()->AddObserver(this);
   session_manager::SessionManager::Get()->AddObserver(this);
@@ -314,7 +320,8 @@ void ArcVoiceInteractionFrameworkService::SetVoiceInteractionState(
         value_prop_accepted &&
         (!prefs->GetUserPrefValue(prefs::kVoiceInteractionEnabled) ||
          prefs->GetBoolean(prefs::kVoiceInteractionEnabled));
-    SetVoiceInteractionEnabled(enable_voice_interaction);
+    SetVoiceInteractionEnabled(enable_voice_interaction,
+                               base::BindOnce(&DoNothing<bool>));
 
     SetVoiceInteractionContextEnabled(
         (enable_voice_interaction &&
@@ -356,7 +363,7 @@ void ArcVoiceInteractionFrameworkService::OnArcPlayStoreEnabledChanged(
   // TODO(xiaohuic): remove deprecated prefs::kVoiceInteractionPrefSynced.
   prefs->SetBoolean(prefs::kVoiceInteractionPrefSynced, false);
   SetVoiceInteractionSetupCompletedInternal(false);
-  SetVoiceInteractionEnabled(false);
+  SetVoiceInteractionEnabled(false, base::BindOnce(&DoNothing<bool>));
   SetVoiceInteractionContextEnabled(false);
 }
 
@@ -393,21 +400,27 @@ void ArcVoiceInteractionFrameworkService::OnHotwordTriggered(uint64_t tv_sec,
 void ArcVoiceInteractionFrameworkService::StartVoiceInteractionSetupWizard() {
   DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
 
-  arc::mojom::VoiceInteractionFrameworkInstance* framework_instance =
-      ARC_GET_INSTANCE_FOR_METHOD(
-          arc_bridge_service_->voice_interaction_framework(),
-          StartVoiceInteractionSetupWizard);
-
-  if (!framework_instance)
-    return;
-
-  if (should_start_runtime_flow_) {
-    VLOG(1) << "Starting runtime setup flow.";
-    framework_instance->StartVoiceInteractionSession(IsHomescreenActive());
-    return;
-  }
-
-  framework_instance->StartVoiceInteractionSetupWizard();
+  // This screen is shown after the Just-A-Sec screen, which blocks until
+  // application sync'd is received. At that point, framework service should
+  // already be initialized. Here we get a method that is defined in version 1
+  // to ensure the connection is established.
+  // TODO(muyuanli): This is a hack for backward compatibility and should be
+  // removed once Android side is checked in and stablized, then we should
+  // DCHECK the |setting_applied| parameter in the lambda. See
+  // crbug.com/768935.
+  DCHECK(ARC_GET_INSTANCE_FOR_METHOD(
+      arc_bridge_service_->voice_interaction_framework(),
+      StartVoiceInteractionSession));
+  SetVoiceInteractionEnabled(
+      true, base::BindOnce(
+                [](base::OnceClosure next, bool setting_applied) {
+                  if (!setting_applied)
+                    DVLOG(1) << "Not synchronizing settings: version mismatch";
+                  std::move(next).Run();
+                },
+                base::BindOnce(&ArcVoiceInteractionFrameworkService::
+                                   StartVoiceInteractionSetupWizardActivity,
+                               weak_ptr_factory_.GetWeakPtr())));
 }
 
 void ArcVoiceInteractionFrameworkService::ShowVoiceInteractionSettings() {
@@ -434,19 +447,28 @@ void ArcVoiceInteractionFrameworkService::NotifyMetalayerStatusChanged(
 }
 
 void ArcVoiceInteractionFrameworkService::SetVoiceInteractionEnabled(
-    bool enable) {
+    bool enable,
+    VoiceInteractionSettingCompleteCallback callback) {
   DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
 
   ash::Shell::Get()->NotifyVoiceInteractionEnabled(enable);
 
   PrefService* prefs = Profile::FromBrowserContext(context_)->GetPrefs();
 
-  // We assume voice interaction is always enabled on in ARC, but we guard
-  // all possible entry points on CrOS side with this flag. In this case,
-  // we only need to set CrOS side flag.
   prefs->SetBoolean(prefs::kVoiceInteractionEnabled, enable);
   if (!enable)
     prefs->SetBoolean(prefs::kVoiceInteractionContextEnabled, false);
+
+  mojom::VoiceInteractionFrameworkInstance* framework_instance =
+      ARC_GET_INSTANCE_FOR_METHOD(
+          arc_bridge_service_->voice_interaction_framework(),
+          SetVoiceInteractionEnabled);
+  if (!framework_instance) {
+    std::move(callback).Run(false);
+    return;
+  }
+  framework_instance->SetVoiceInteractionEnabled(
+      enable, base::BindOnce(std::move(callback), true));
 }
 
 void ArcVoiceInteractionFrameworkService::SetVoiceInteractionContextEnabled(
@@ -471,7 +493,7 @@ void ArcVoiceInteractionFrameworkService::SetVoiceInteractionSetupCompleted() {
   DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
 
   SetVoiceInteractionSetupCompletedInternal(true);
-  SetVoiceInteractionEnabled(true);
+  SetVoiceInteractionEnabled(true, base::BindOnce(&DoNothing<bool>));
   SetVoiceInteractionContextEnabled(true);
 }
 
@@ -603,4 +625,25 @@ bool ArcVoiceInteractionFrameworkService::IsHomescreenActive() {
   return !ash::Shell::Get()->activation_client()->GetActiveWindow();
 }
 
+void ArcVoiceInteractionFrameworkService::
+    StartVoiceInteractionSetupWizardActivity() {
+  DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
+
+  arc::mojom::VoiceInteractionFrameworkInstance* framework_instance =
+      ARC_GET_INSTANCE_FOR_METHOD(
+          arc_bridge_service_->voice_interaction_framework(),
+          StartVoiceInteractionSetupWizard);
+
+  if (!framework_instance)
+    return;
+
+  if (should_start_runtime_flow_) {
+    should_start_runtime_flow_ = false;
+    VLOG(1) << "Starting runtime setup flow.";
+    framework_instance->StartVoiceInteractionSession(IsHomescreenActive());
+    return;
+  }
+  framework_instance->StartVoiceInteractionSetupWizard();
+}
+
 }  // namespace arc

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

@@ -7,7 +7,9 @@
 
 #include <memory>
 
+#include "base/callback_forward.h"
 #include "base/macros.h"
+#include "base/memory/weak_ptr.h"
 #include "base/time/time.h"
 #include "chrome/browser/chromeos/arc/arc_session_manager.h"
 #include "chromeos/audio/cras_audio_handler.h"
@@ -93,10 +95,13 @@ class ArcVoiceInteractionFrameworkService
   // seesion if it is already running.
   void ToggleSessionFromUserInteraction();
 
-  // Turn on / off voice interaction in ARC.
-  // TODO(muyuanli): We should also check on Chrome side once CrOS side settings
-  // are ready (tracked separately at crbug.com/727873).
-  void SetVoiceInteractionEnabled(bool enable);
+  using VoiceInteractionSettingCompleteCallback =
+      base::OnceCallback<void(bool)>;
+  // Turn on / off voice interaction in ARC. |callback| will be called with
+  // |true| if setting is applied to Android side.
+  void SetVoiceInteractionEnabled(
+      bool enable,
+      VoiceInteractionSettingCompleteCallback callback);
 
   // Turn on / off voice interaction context (screenshot and structural data)
   // in ARC.
@@ -130,6 +135,8 @@ class ArcVoiceInteractionFrameworkService
 
   bool IsHomescreenActive();
 
+  void StartVoiceInteractionSetupWizardActivity();
+
   content::BrowserContext* context_;
   ArcBridgeService* const arc_bridge_service_;  // Owned by ArcServiceManager
   mojo::Binding<mojom::VoiceInteractionFrameworkHost> binding_;
@@ -159,6 +166,8 @@ class ArcVoiceInteractionFrameworkService
   // something malicious is going on.
   int32_t context_request_remaining_count_ = 0;
 
+  base::WeakPtrFactory<ArcVoiceInteractionFrameworkService> weak_ptr_factory_;
+
   DISALLOW_COPY_AND_ASSIGN(ArcVoiceInteractionFrameworkService);
 };
 

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

@@ -8,6 +8,7 @@
 
 #include "ash/shell.h"
 #include "ash/test/ash_test_base.h"
+#include "base/bind.h"
 #include "base/files/scoped_temp_dir.h"
 #include "chrome/browser/chromeos/arc/arc_session_manager.h"
 #include "chrome/test/base/testing_profile.h"
@@ -106,7 +107,8 @@ TEST_F(ArcVoiceInteractionFrameworkServiceTest, StartSession) {
 
 TEST_F(ArcVoiceInteractionFrameworkServiceTest, StartSessionWithoutFlag) {
   // Remove the voice interaction enabled flag.
-  framework_service()->SetVoiceInteractionEnabled(false);
+  framework_service()->SetVoiceInteractionEnabled(false,
+                                                  base::BindOnce([](bool) {}));
 
   framework_service()->StartSessionFromUserInteraction(gfx::Rect());
   // The signal should not be sent when voice interaction disabled.

chrome/browser/ui/webui/settings/chromeos/google_assistant_handler.cc

@@ -48,7 +48,7 @@ void GoogleAssistantHandler::HandleSetGoogleAssistantEnabled(
   auto* service =
       arc::ArcVoiceInteractionFrameworkService::GetForBrowserContext(profile_);
   if (service)
-    service->SetVoiceInteractionEnabled(enabled);
+    service->SetVoiceInteractionEnabled(enabled, base::BindOnce([](bool) {}));
 }
 
 void GoogleAssistantHandler::HandleSetGoogleAssistantContextEnabled(

components/arc/common/voice_interaction_framework.mojom

@@ -2,7 +2,7 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 //
-// Next MinVersion: 12
+// Next MinVersion: 13
 
 module arc.mojom;
 
@@ -53,7 +53,8 @@ struct VoiceInteractionStatus {
 };
 
 // Connects with Android system server.
-// Next method ID: 10
+// Next method ID: 11
+// Deprecated method ID: 4
 interface VoiceInteractionFrameworkInstance {
   Init@0(VoiceInteractionFrameworkHost host_ptr);
 
@@ -72,8 +73,8 @@ interface VoiceInteractionFrameworkInstance {
   // Shows/hides the metalayer in the container.
   [MinVersion=1] SetMetalayerVisibility@3([MinVersion=2] bool visible);
 
-  // Turns on / off voice interaction in container.
-  [MinVersion=4] SetVoiceInteractionEnabled@4(bool enable);
+  // Turns on / off voice interaction and ack the setting.
+  [MinVersion=12] SetVoiceInteractionEnabled@10(bool enable) => ();
 
   // Turns on / off context for voice interaction in container. This function
   // controls whether screenshot and view hierarchy information should be sent

components/arc/test/fake_voice_interaction_framework_instance.cc

@@ -32,7 +32,10 @@ void FakeVoiceInteractionFrameworkInstance::SetMetalayerVisibility(
     bool visible) {}
 
 void FakeVoiceInteractionFrameworkInstance::SetVoiceInteractionEnabled(
-    bool enable) {}
+    bool enable,
+    SetVoiceInteractionEnabledCallback callback) {
+  std::move(callback).Run();
+}
 
 void FakeVoiceInteractionFrameworkInstance::SetVoiceInteractionContextEnabled(
     bool enable) {}

components/arc/test/fake_voice_interaction_framework_instance.h

@@ -23,7 +23,9 @@ class FakeVoiceInteractionFrameworkInstance
   void ToggleVoiceInteractionSession(bool homescreen_is_active) override;
   void StartVoiceInteractionSessionForRegion(const gfx::Rect& region) override;
   void SetMetalayerVisibility(bool visible) override;
-  void SetVoiceInteractionEnabled(bool enable) override;
+  void SetVoiceInteractionEnabled(
+      bool enable,
+      SetVoiceInteractionEnabledCallback callback) override;
   void SetVoiceInteractionContextEnabled(bool enable) override;
   void StartVoiceInteractionSetupWizard() override;
   void ShowVoiceInteractionSettings() override;