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=[email protected] > > Bug: 870093 > Change-Id: I3dd5bcd9c8e3946e1ae42a5db69f2bb7ed3ea586 > Reviewed-on: https://chromium-review.googlesource.com/1185498 > Commit-Queue: Jeremy Klein <[email protected]> > Reviewed-by: Tommy Li <[email protected]> > Reviewed-by: Kyle Horimoto <[email protected]> > Cr-Commit-Position: refs/heads/master@{#589196} [email protected],[email protected],[email protected] 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 <[email protected]> Commit-Queue: Justin Donnelly <[email protected]> Cr-Commit-Position: refs/heads/master@{#589218}
feiyue1206 · Sep 6, 2018 · 54d36f8 · 54d36f8
1 parent d694dd9
commit 54d36f8
Show file tree

Hide file tree

Showing 13 changed files with 7 additions and 163 deletions.
diff --git a/chrome/browser/chromeos/multidevice_setup/android_sms_app_helper_delegate_impl.cc b/chrome/browser/chromeos/multidevice_setup/android_sms_app_helper_delegate_impl.cc
@@ -9,15 +9,10 @@
 #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 {
 
@@ -27,7 +22,6 @@ AndroidSmsAppHelperDelegateImpl::AndroidSmsAppHelperDelegateImpl(
     Profile* profile)
     : pending_app_manager_(
           &web_app::WebAppProvider::Get(profile)->pending_app_manager()),
-      profile_(profile),
       weak_ptr_factory_(this) {}
 
 AndroidSmsAppHelperDelegateImpl::AndroidSmsAppHelperDelegateImpl(
@@ -48,24 +42,6 @@ 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) {

diff --git a/chrome/browser/chromeos/multidevice_setup/android_sms_app_helper_delegate_impl.h b/chrome/browser/chromeos/multidevice_setup/android_sms_app_helper_delegate_impl.h
@@ -30,22 +30,16 @@ 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);

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

diff --git a/chrome/browser/resources/settings/multidevice_page/multidevice_subpage.js b/chrome/browser/resources/settings/multidevice_page/multidevice_subpage.js
@@ -56,17 +56,9 @@ Polymer({
     },
   },
 
-  /** @private {?settings.MultiDeviceBrowserProxy} */
-  browserProxy_: null,
-
-  /** @override */
-  created: function() {
-    this.browserProxy_ = settings.MultiDeviceBrowserProxyImpl.getInstance();
-  },
-
   /** @private */
   handleAndroidMessagesButtonClick_: function() {
-    this.browserProxy_.setUpAndroidSms();
+    this.androidMessagesRequiresSetup_ = false;
   },
 
   listeners: {

diff --git a/chrome/browser/ui/webui/settings/chromeos/multidevice_handler.cc b/chrome/browser/ui/webui/settings/chromeos/multidevice_handler.cc
@@ -7,7 +7,6 @@
 #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"
@@ -36,11 +35,8 @@ void OnRetrySetHostNowResult(bool success) {
 }  // namespace
 
 MultideviceHandler::MultideviceHandler(
-    multidevice_setup::MultiDeviceSetupClient* multidevice_setup_client,
-    std::unique_ptr<multidevice_setup::AndroidSmsAppHelperDelegate>
-        android_sms_app_helper)
+    multidevice_setup::MultiDeviceSetupClient* multidevice_setup_client)
     : 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) {}
 
@@ -67,10 +63,6 @@ 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() {
@@ -166,12 +158,6 @@ 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) {

diff --git a/chrome/browser/ui/webui/settings/chromeos/multidevice_handler.h b/chrome/browser/ui/webui/settings/chromeos/multidevice_handler.h
@@ -15,10 +15,6 @@
 
 namespace chromeos {
 
-namespace multidevice_setup {
-class AndroidSmsAppHelperDelegate;
-}  // namespace multidevice_setup
-
 namespace settings {
 
 // Chrome "Multidevice" (a.k.a. "Connected Devices") settings page UI handler.
@@ -27,9 +23,7 @@ class MultideviceHandler
       public multidevice_setup::MultiDeviceSetupClient::Observer {
  public:
   explicit MultideviceHandler(
-      multidevice_setup::MultiDeviceSetupClient* multidevice_setup_client,
-      std::unique_ptr<multidevice_setup::AndroidSmsAppHelperDelegate>
-          android_sms_app_helper);
+      multidevice_setup::MultiDeviceSetupClient* multidevice_setup_client);
   ~MultideviceHandler() override;
 
  protected:
@@ -58,7 +52,6 @@ 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);
@@ -69,8 +62,6 @@ 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>

diff --git a/chrome/browser/ui/webui/settings/chromeos/multidevice_handler_unittest.cc b/chrome/browser/ui/webui/settings/chromeos/multidevice_handler_unittest.cc
@@ -7,7 +7,6 @@
 #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"
@@ -22,11 +21,8 @@ namespace {
 class TestMultideviceHandler : public MultideviceHandler {
  public:
   TestMultideviceHandler(
-      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)) {}
+      multidevice_setup::MultiDeviceSetupClient* multidevice_setup_client)
+      : MultideviceHandler(multidevice_setup_client) {}
   ~TestMultideviceHandler() override = default;
 
   // Make public for testing.
@@ -111,14 +107,9 @@ 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(),
-        std::move(fake_android_sms_app_helper_delegate));
+        fake_multidevice_setup_client_.get());
     handler_->set_web_ui(test_web_ui_.get());
     handler_->RegisterMessages();
     handler_->AllowJavascript();
@@ -193,11 +184,6 @@ 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,
@@ -239,11 +225,6 @@ 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:
@@ -263,8 +244,6 @@ 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);
 };
@@ -301,12 +280,6 @@ 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,

diff --git a/chrome/browser/ui/webui/settings/md_settings_ui.cc b/chrome/browser/ui/webui/settings/md_settings_ui.cc
@@ -77,7 +77,6 @@
 #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"
@@ -227,10 +226,7 @@ MdSettingsUI::MdSettingsUI(content::WebUI* web_ui)
     AddSettingsPageUIHandler(
         std::make_unique<chromeos::settings::MultideviceHandler>(
             chromeos::multidevice_setup::MultiDeviceSetupClientFactory::
-                GetForProfile(profile),
-            std::make_unique<
-                chromeos::multidevice_setup::AndroidSmsAppHelperDelegateImpl>(
-                profile)));
+                GetForProfile(profile)));
   }
   AddSettingsPageUIHandler(
       std::make_unique<chromeos::settings::PointerHandler>());

diff --git a/chrome/test/data/webui/settings/cr_settings_browsertest.js b/chrome/test/data/webui/settings/cr_settings_browsertest.js
@@ -1992,7 +1992,6 @@ CrSettingsMultideviceSubpageTest.prototype = {
 
   /** @override */
   extraLibraries: CrSettingsBrowserTest.prototype.extraLibraries.concat([
-    '../test_browser_proxy.js',
     'multidevice_subpage_tests.js',
   ]),
 };

diff --git a/chrome/test/data/webui/settings/multidevice_subpage_tests.js b/chrome/test/data/webui/settings/multidevice_subpage_tests.js
@@ -2,25 +2,8 @@
 // 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.
@@ -77,10 +60,6 @@ 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);
@@ -154,20 +133,4 @@ 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');
-      });
 });
diff --git a/chromeos/services/multidevice_setup/public/cpp/android_sms_app_helper_delegate.h b/chromeos/services/multidevice_setup/public/cpp/android_sms_app_helper_delegate.h
@@ -17,9 +17,6 @@ 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;

diff --git a/chromeos/services/multidevice_setup/public/cpp/fake_android_sms_app_helper_delegate.cc b/chromeos/services/multidevice_setup/public/cpp/fake_android_sms_app_helper_delegate.cc
@@ -23,15 +23,6 @@ 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