Reland "Add the ability for AndroidSmsAppHelperDelegate to launch the…

… PWA."

This reverts commit 54d36f8.

Reason for revert: The original change was in fact not the cause of the builder breakage. Confirmed by jdonnelley.

Original change's description:
> Revert "Add the ability for AndroidSmsAppHelperDelegate to launch the PWA."
> 
> This reverts commit 6d6b617.
> 
> Reason for revert: Suspected of breaking the Mac builder:
> https://ci.chromium.org/p/chromium/builders/luci.chromium.ci/Mac%20Builder/89748
> 
> This code doesn't seem obviously related to the webui failure there, but there are no other webui CLs in the blame list.
> 
> Original change's description:
> > Add the ability for AndroidSmsAppHelperDelegate to launch the PWA.
> > 
> > Also actually use this ability when the "Set Up" button in multidevice
> > settings is clicked.
> > 
> > R=​khorimoto@chromium.org
> > 
> > Bug: 870093
> > Change-Id: I3dd5bcd9c8e3946e1ae42a5db69f2bb7ed3ea586
> > Reviewed-on: https://chromium-review.googlesource.com/1185498
> > Commit-Queue: Jeremy Klein <jlklein@chromium.org>
> > Reviewed-by: Tommy Li <tommycli@chromium.org>
> > Reviewed-by: Kyle Horimoto <khorimoto@chromium.org>
> > Cr-Commit-Position: refs/heads/master@{#589196}
> 
> TBR=khorimoto@chromium.org,tommycli@chromium.org,jlklein@chromium.org
> 
> Change-Id: I855abd7c530600362082c83ee155aaa3d713415f
> No-Presubmit: true
> No-Tree-Checks: true
> No-Try: true
> Bug: 870093
> Reviewed-on: https://chromium-review.googlesource.com/1211382
> Reviewed-by: Justin Donnelly <jdonnelly@chromium.org>
> Commit-Queue: Justin Donnelly <jdonnelly@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#589218}

TBR=khorimoto@chromium.org,tommycli@chromium.org,jdonnelly@chromium.org,jlklein@chromium.org

Change-Id: I906bbc0862ca17834fd7e009fdd23af4735d4d49
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug: 870093
Reviewed-on: https://chromium-review.googlesource.com/1211430
Reviewed-by: Jeremy Klein <jlklein@chromium.org>
Commit-Queue: Jeremy Klein <jlklein@chromium.org>
Cr-Commit-Position: refs/heads/master@{#589263}

chrome/browser/chromeos/multidevice_setup/android_sms_app_helper_delegate_impl.cc

@@ -9,10 +9,15 @@
 #include "base/bind.h"
 #include "base/callback.h"
 #include "chrome/browser/chromeos/android_sms/android_sms_urls.h"
+#include "chrome/browser/extensions/extension_util.h"
 #include "chrome/browser/profiles/profile.h"
+#include "chrome/browser/ui/extensions/app_launch_params.h"
+#include "chrome/browser/ui/extensions/application_launch.h"
 #include "chrome/browser/web_applications/components/pending_app_manager.h"
 #include "chrome/browser/web_applications/web_app_provider.h"
 #include "chromeos/components/proximity_auth/logging/logging.h"
+#include "extensions/common/constants.h"
+#include "ui/base/window_open_disposition.h"
 
 namespace chromeos {
 
@@ -22,6 +27,7 @@ AndroidSmsAppHelperDelegateImpl::AndroidSmsAppHelperDelegateImpl(
     Profile* profile)
     : pending_app_manager_(
           &web_app::WebAppProvider::Get(profile)->pending_app_manager()),
+      profile_(profile),
       weak_ptr_factory_(this) {}
 
 AndroidSmsAppHelperDelegateImpl::AndroidSmsAppHelperDelegateImpl(
@@ -42,6 +48,24 @@ void AndroidSmsAppHelperDelegateImpl::InstallAndroidSmsApp() {
                      weak_ptr_factory_.GetWeakPtr()));
 }
 
+bool AndroidSmsAppHelperDelegateImpl::LaunchAndroidSmsApp() {
+  const extensions::Extension* android_sms_pwa =
+      extensions::util::GetInstalledPwaForUrl(
+          profile_, chromeos::android_sms::GetAndroidMessagesURL());
+  if (!android_sms_pwa) {
+    PA_LOG(ERROR) << "No Messages app found.";
+    return false;
+  }
+
+  PA_LOG(INFO) << "Messages app Launching...";
+  AppLaunchParams params(
+      profile_, android_sms_pwa, extensions::LAUNCH_CONTAINER_WINDOW,
+      WindowOpenDisposition::NEW_WINDOW, extensions::SOURCE_CHROME_INTERNAL);
+  // OpenApplications() is defined in application_launch.h.
+  OpenApplication(params);
+  return true;
+}
+
 void AndroidSmsAppHelperDelegateImpl::OnAppInstalled(
     const GURL& app_url,
     const base::Optional<std::string>& app_id) {

chrome/browser/chromeos/multidevice_setup/android_sms_app_helper_delegate_impl.h

@@ -30,16 +30,22 @@ class AndroidSmsAppHelperDelegateImpl : public AndroidSmsAppHelperDelegate {
  private:
   friend class AndroidSmsAppHelperDelegateImplTest;
 
+  // Note: This constructor should only be used in testing. Right now, objects
+  // built using this constructor will segfault on profile_ if
+  // LaunchAndroidSmsApp is called. We'll need to fix this once tests for that
+  // function are added. See https://crbug.com/876972.
   explicit AndroidSmsAppHelperDelegateImpl(
       web_app::PendingAppManager* pending_app_manager);
   void OnAppInstalled(const GURL& app_url,
                       const base::Optional<std::string>& app_id);
 
   // AndroidSmsAppHelperDelegate:
   void InstallAndroidSmsApp() override;
+  bool LaunchAndroidSmsApp() override;
 
   static const char kMessagesWebAppUrl[];
   web_app::PendingAppManager* pending_app_manager_;
+  Profile* profile_;
   base::WeakPtrFactory<AndroidSmsAppHelperDelegateImpl> weak_ptr_factory_;
 
   DISALLOW_COPY_AND_ASSIGN(AndroidSmsAppHelperDelegateImpl);

chrome/browser/resources/settings/multidevice_page/multidevice_browser_proxy.js

@@ -24,6 +24,12 @@ cr.define('settings', function() {
     removeHostDevice() {}
 
     retryPendingHostSetup() {}
+
+    /**
+     * Called when the "Set Up" button is clicked to open the Android Messages
+     * PWA.
+     */
+    setUpAndroidSms() {}
   }
 
   /**
@@ -55,6 +61,11 @@ cr.define('settings', function() {
     retryPendingHostSetup() {
       chrome.send('retryPendingHostSetup');
     }
+
+    /** @override */
+    setUpAndroidSms() {
+      chrome.send('setUpAndroidSms');
+    }
   }
 
   cr.addSingletonGetter(MultiDeviceBrowserProxyImpl);

chrome/browser/resources/settings/multidevice_page/multidevice_subpage.js

@@ -56,9 +56,17 @@ Polymer({
     },
   },
 
+  /** @private {?settings.MultiDeviceBrowserProxy} */
+  browserProxy_: null,
+
+  /** @override */
+  created: function() {
+    this.browserProxy_ = settings.MultiDeviceBrowserProxyImpl.getInstance();
+  },
+
   /** @private */
   handleAndroidMessagesButtonClick_: function() {
-    this.androidMessagesRequiresSetup_ = false;
+    this.browserProxy_.setUpAndroidSms();
   },
 
   listeners: {

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

@@ -7,6 +7,7 @@
 #include "base/bind.h"
 #include "base/bind_helpers.h"
 #include "base/logging.h"
+#include "chrome/browser/chromeos/multidevice_setup/android_sms_app_helper_delegate_impl.h"
 #include "chrome/browser/ui/webui/chromeos/multidevice_setup/multidevice_setup_dialog.h"
 #include "chromeos/components/proximity_auth/logging/logging.h"
 #include "content/public/browser/web_ui.h"
@@ -35,8 +36,11 @@ void OnRetrySetHostNowResult(bool success) {
 }  // namespace
 
 MultideviceHandler::MultideviceHandler(
-    multidevice_setup::MultiDeviceSetupClient* multidevice_setup_client)
+    multidevice_setup::MultiDeviceSetupClient* multidevice_setup_client,
+    std::unique_ptr<multidevice_setup::AndroidSmsAppHelperDelegate>
+        android_sms_app_helper)
     : multidevice_setup_client_(multidevice_setup_client),
+      android_sms_app_helper_(std::move(android_sms_app_helper)),
       multidevice_setup_observer_(this),
       callback_weak_ptr_factory_(this) {}
 
@@ -63,6 +67,10 @@ void MultideviceHandler::RegisterMessages() {
       "retryPendingHostSetup",
       base::BindRepeating(&MultideviceHandler::HandleRetryPendingHostSetup,
                           base::Unretained(this)));
+  web_ui()->RegisterMessageCallback(
+      "setUpAndroidSms",
+      base::BindRepeating(&MultideviceHandler::HandleSetUpAndroidSms,
+                          base::Unretained(this)));
 }
 
 void MultideviceHandler::OnJavascriptAllowed() {
@@ -158,6 +166,12 @@ void MultideviceHandler::HandleRetryPendingHostSetup(
       base::BindOnce(&OnRetrySetHostNowResult));
 }
 
+void MultideviceHandler::HandleSetUpAndroidSms(const base::ListValue* args) {
+  PA_LOG(WARNING) << "HandlingSetupSms";
+  DCHECK(args->empty());
+  android_sms_app_helper_->LaunchAndroidSmsApp();
+}
+
 void MultideviceHandler::OnSetFeatureStateEnabledResult(
     const std::string& js_callback_id,
     bool success) {

chrome/browser/ui/webui/settings/chromeos/multidevice_handler.h

@@ -15,6 +15,10 @@
 
 namespace chromeos {
 
+namespace multidevice_setup {
+class AndroidSmsAppHelperDelegate;
+}  // namespace multidevice_setup
+
 namespace settings {
 
 // Chrome "Multidevice" (a.k.a. "Connected Devices") settings page UI handler.
@@ -23,7 +27,9 @@ class MultideviceHandler
       public multidevice_setup::MultiDeviceSetupClient::Observer {
  public:
   explicit MultideviceHandler(
-      multidevice_setup::MultiDeviceSetupClient* multidevice_setup_client);
+      multidevice_setup::MultiDeviceSetupClient* multidevice_setup_client,
+      std::unique_ptr<multidevice_setup::AndroidSmsAppHelperDelegate>
+          android_sms_app_helper);
   ~MultideviceHandler() override;
 
  protected:
@@ -52,6 +58,7 @@ class MultideviceHandler
   void HandleSetFeatureEnabledState(const base::ListValue* args);
   void HandleRemoveHostDevice(const base::ListValue* args);
   void HandleRetryPendingHostSetup(const base::ListValue* args);
+  void HandleSetUpAndroidSms(const base::ListValue* args);
 
   void OnSetFeatureStateEnabledResult(const std::string& js_callback_id,
                                       bool success);
@@ -62,6 +69,8 @@ class MultideviceHandler
   std::unique_ptr<base::DictionaryValue> GeneratePageContentDataDictionary();
 
   multidevice_setup::MultiDeviceSetupClient* multidevice_setup_client_;
+  std::unique_ptr<multidevice_setup::AndroidSmsAppHelperDelegate>
+      android_sms_app_helper_;
 
   ScopedObserver<multidevice_setup::MultiDeviceSetupClient,
                  multidevice_setup::MultiDeviceSetupClient::Observer>

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

@@ -7,6 +7,7 @@
 #include <memory>
 
 #include "base/macros.h"
+#include "chromeos/services/multidevice_setup/public/cpp/fake_android_sms_app_helper_delegate.h"
 #include "chromeos/services/multidevice_setup/public/cpp/fake_multidevice_setup_client.h"
 #include "components/cryptauth/remote_device_test_util.h"
 #include "content/public/test/test_web_ui.h"
@@ -21,8 +22,11 @@ namespace {
 class TestMultideviceHandler : public MultideviceHandler {
  public:
   TestMultideviceHandler(
-      multidevice_setup::MultiDeviceSetupClient* multidevice_setup_client)
-      : MultideviceHandler(multidevice_setup_client) {}
+      multidevice_setup::MultiDeviceSetupClient* multidevice_setup_client,
+      std::unique_ptr<multidevice_setup::AndroidSmsAppHelperDelegate>
+          android_sms_app_helper)
+      : MultideviceHandler(multidevice_setup_client,
+                           std::move(android_sms_app_helper)) {}
   ~TestMultideviceHandler() override = default;
 
   // Make public for testing.
@@ -107,9 +111,14 @@ class MultideviceHandlerTest : public testing::Test {
 
     fake_multidevice_setup_client_ =
         std::make_unique<multidevice_setup::FakeMultiDeviceSetupClient>();
+    auto fake_android_sms_app_helper_delegate =
+        std::make_unique<multidevice_setup::FakeAndroidSmsAppHelperDelegate>();
+    fake_android_sms_app_helper_delegate_ =
+        fake_android_sms_app_helper_delegate.get();
 
     handler_ = std::make_unique<TestMultideviceHandler>(
-        fake_multidevice_setup_client_.get());
+        fake_multidevice_setup_client_.get(),
+        std::move(fake_android_sms_app_helper_delegate));
     handler_->set_web_ui(test_web_ui_.get());
     handler_->RegisterMessages();
     handler_->AllowJavascript();
@@ -184,6 +193,11 @@ class MultideviceHandlerTest : public testing::Test {
         success);
   }
 
+  void CallSetUpAndroidSms() {
+    base::ListValue empty_args;
+    test_web_ui()->HandleReceivedMessage("setUpAndroidSms", &empty_args);
+  }
+
   void CallSetFeatureEnabledState(multidevice_setup::mojom::Feature feature,
                                   bool enabled,
                                   const base::Optional<std::string>& auth_token,
@@ -225,6 +239,11 @@ class MultideviceHandlerTest : public testing::Test {
     return fake_multidevice_setup_client_.get();
   }
 
+  multidevice_setup::FakeAndroidSmsAppHelperDelegate*
+  fake_android_sms_app_helper_delegate() {
+    return fake_android_sms_app_helper_delegate_;
+  }
+
   const cryptauth::RemoteDeviceRef test_device_;
 
  private:
@@ -244,6 +263,8 @@ class MultideviceHandlerTest : public testing::Test {
       host_status_with_device_;
   multidevice_setup::MultiDeviceSetupClient::FeatureStatesMap
       feature_states_map_;
+  multidevice_setup::FakeAndroidSmsAppHelperDelegate*
+      fake_android_sms_app_helper_delegate_;
 
   DISALLOW_COPY_AND_ASSIGN(MultideviceHandlerTest);
 };
@@ -280,6 +301,12 @@ TEST_F(MultideviceHandlerTest, RetryPendingHostSetup) {
   CallRetryPendingHostSetup(false /* success */);
 }
 
+TEST_F(MultideviceHandlerTest, SetUpAndroidSms) {
+  EXPECT_FALSE(fake_android_sms_app_helper_delegate()->HasLaunchedApp());
+  CallSetUpAndroidSms();
+  EXPECT_TRUE(fake_android_sms_app_helper_delegate()->HasLaunchedApp());
+}
+
 TEST_F(MultideviceHandlerTest, SetFeatureEnabledState) {
   CallSetFeatureEnabledState(
       multidevice_setup::mojom::Feature::kBetterTogetherSuite,

chrome/browser/ui/webui/settings/md_settings_ui.cc

@@ -77,6 +77,7 @@
 #include "chrome/browser/chromeos/crostini/crostini_util.h"
 #include "chrome/browser/chromeos/login/demo_mode/demo_session.h"
 #include "chrome/browser/chromeos/login/quick_unlock/quick_unlock_utils.h"
+#include "chrome/browser/chromeos/multidevice_setup/android_sms_app_helper_delegate_impl.h"
 #include "chrome/browser/chromeos/multidevice_setup/multidevice_setup_client_factory.h"
 #include "chrome/browser/signin/account_tracker_service_factory.h"
 #include "chrome/browser/ui/webui/settings/chromeos/accessibility_handler.h"
@@ -226,7 +227,10 @@ MdSettingsUI::MdSettingsUI(content::WebUI* web_ui)
     AddSettingsPageUIHandler(
         std::make_unique<chromeos::settings::MultideviceHandler>(
             chromeos::multidevice_setup::MultiDeviceSetupClientFactory::
-                GetForProfile(profile)));
+                GetForProfile(profile),
+            std::make_unique<
+                chromeos::multidevice_setup::AndroidSmsAppHelperDelegateImpl>(
+                profile)));
   }
   AddSettingsPageUIHandler(
       std::make_unique<chromeos::settings::PointerHandler>());

chrome/test/data/webui/settings/cr_settings_browsertest.js

@@ -1992,6 +1992,7 @@ CrSettingsMultideviceSubpageTest.prototype = {
 
   /** @override */
   extraLibraries: CrSettingsBrowserTest.prototype.extraLibraries.concat([
+    '../test_browser_proxy.js',
     'multidevice_subpage_tests.js',
   ]),
 };

chrome/test/data/webui/settings/multidevice_subpage_tests.js

@@ -2,8 +2,25 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
+/**
+ * @implements {settings.MultideviceBrowserProxy}
+ */
+class TestMultideviceBrowserProxy extends TestBrowserProxy {
+  constructor() {
+    super([
+      'setUpAndroidSms',
+    ]);
+  }
+
+  /** @override */
+  setUpAndroidSms() {
+    this.methodCalled('setUpAndroidSms');
+  }
+}
+
 suite('Multidevice', function() {
   let multideviceSubpage = null;
+  let browserProxy = null;
   // Although HOST_SET_MODES is effectively a constant, it cannot reference the
   // enum settings.MultiDeviceSettingsMode from here so its initialization is
   // deferred to the suiteSetup function.
@@ -60,6 +77,10 @@ suite('Multidevice', function() {
   });
 
   setup(function() {
+    browserProxy = new TestMultideviceBrowserProxy();
+    settings.MultiDeviceBrowserProxyImpl.instance_ = browserProxy;
+
+    PolymerTest.clearBody();
     multideviceSubpage = document.createElement('settings-multidevice-subpage');
     multideviceSubpage.pageContentData = {hostDeviceName: 'Pixel XL'};
     setMode(settings.MultiDeviceSettingsMode.HOST_SET_VERIFIED);
@@ -133,4 +154,20 @@ suite('Multidevice', function() {
 
     assertFalse(!!multideviceSubpage.$$(controllerSelector));
   });
+
+  test(
+      'AndroidMessages set up button calls browser proxy function', function() {
+        const messagesItem = multideviceSubpage.$$('#messagesItem');
+
+        multideviceSubpage.androidMessagesRequiresSetup_ = true;
+        Polymer.dom.flush();
+
+        const setUpButton =
+            multideviceSubpage.$$('#messagesItem > [slot=feature-controller]');
+        assertTrue(!!setUpButton);
+
+        setUpButton.click();
+
+        return browserProxy.whenCalled('setUpAndroidSms');
+      });
 });

chromeos/services/multidevice_setup/public/cpp/android_sms_app_helper_delegate.h

@@ -17,6 +17,9 @@ class AndroidSmsAppHelperDelegate {
 
   // Installs the Messages for Web PWA. Handles retries and errors internally.
   virtual void InstallAndroidSmsApp() = 0;
+  // Launches the Messages for Web PWA if it's installed. Returns true if the
+  // app was launched successfully, false otherwise.
+  virtual bool LaunchAndroidSmsApp() = 0;
 
  protected:
   AndroidSmsAppHelperDelegate() = default;

chromeos/services/multidevice_setup/public/cpp/fake_android_sms_app_helper_delegate.cc

@@ -23,6 +23,15 @@ bool FakeAndroidSmsAppHelperDelegate::HasInstalledApp() {
   return has_installed_;
 }
 
+bool FakeAndroidSmsAppHelperDelegate::LaunchAndroidSmsApp() {
+  has_launched_ = true;
+  return true;
+}
+
+bool FakeAndroidSmsAppHelperDelegate::HasLaunchedApp() {
+  return has_launched_;
+}
+
 }  // namespace multidevice_setup
 
 }  // namespace chromeos