Add devices to intent picker list if no apps found.

There is an edge case where ARC returns an empty list of handlers for
tel links. Usually there is at least the Contacts app installed but in
case there was an error we still want to append devices to select from.

With this change we always call through
ShowExternalProtocolDialogWithoutApps which then calls
ShowFallbackExternalProtocolDialog if there are no devices.

Test: Added a test case that verifies that for an empty list of apps we
correctly show the external protocol dialog with devices.

Bug: 1020155
Change-Id: Idb55d8548d8c428c6864a3f6cdf50baa8ffff663
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1890440Reviewed-by: Hidehiko Abe <hidehiko@chromium.org>
Reviewed-by: David Jacobo <djacobo@chromium.org>
Commit-Queue: Richard Knoll <knollr@chromium.org>
Cr-Commit-Position: refs/heads/master@{#712728}

chrome/browser/chromeos/arc/intent_helper/arc_external_protocol_dialog.cc

@@ -302,10 +302,7 @@ GetActionResult GetAction(
     GurlAndActivityInfo* out_url_and_activity_name,
     bool* in_out_safe_to_bypass_ui) {
   DCHECK(out_url_and_activity_name);
-  if (!handlers.size()) {
-    *in_out_safe_to_bypass_ui = false;
-    return GetActionResult::SHOW_CHROME_OS_DIALOG;  // no apps found.
-  }
+  DCHECK(!handlers.empty());
 
   if (selected_app_index == handlers.size()) {
     // The user hasn't made the selection yet.
@@ -419,10 +416,6 @@ bool HandleUrl(int render_process_host_id,
     *out_result = result;
 
   switch (result) {
-    case GetActionResult::SHOW_CHROME_OS_DIALOG:
-      ShowFallbackExternalProtocolDialog(render_process_host_id, routing_id,
-                                         url);
-      return true;
     case GetActionResult::OPEN_URL_IN_CHROME:
       OpenUrlInChrome(render_process_host_id, routing_id,
                       url_and_activity_name.first);
@@ -692,7 +685,7 @@ void OnUrlHandlerList(int render_process_host_id,
   // We only reach here if Chrome doesn't think it can handle the URL. If ARC is
   // not running anymore, or Chrome is the only candidate returned, show the
   // usual Chrome OS dialog that says we cannot handle the URL.
-  if (!instance || !intent_helper_bridge ||
+  if (!instance || !intent_helper_bridge || handlers.empty() ||
       IsChromeOnlyAppCandidate(handlers)) {
     ShowExternalProtocolDialogWithoutApps(render_process_host_id, routing_id,
                                           url, initiating_origin);

chrome/browser/chromeos/arc/intent_helper/arc_external_protocol_dialog.h

@@ -34,10 +34,6 @@ using GurlAndActivityInfo =
 
 // An enum returned from GetAction function. This is visible for testing.
 enum class GetActionResult {
-  // ARC cannot handle the |original_url|, and the URL does not have a fallback
-  // http(s) URL. Chrome should show the "Google Chrome OS can't open the page"
-  // dialog now.
-  SHOW_CHROME_OS_DIALOG,
   // ARC cannot handle the |original_url|, but the URL did have a fallback URL
   // which Chrome can handle. Chrome should show the fallback URL now.
   OPEN_URL_IN_CHROME,

chrome/browser/chromeos/arc/intent_helper/arc_external_protocol_dialog_unittest.cc

@@ -7,10 +7,18 @@
 #include "base/macros.h"
 #include "base/time/time.h"
 #include "chrome/browser/chromeos/arc/arc_web_contents_data.h"
+#include "chrome/browser/sharing/click_to_call/click_to_call_ui_controller.h"
 #include "chrome/browser/sharing/mock_sharing_service.h"
 #include "chrome/browser/sharing/sharing_service_factory.h"
+#include "chrome/browser/ui/app_list/arc/arc_app_test.h"
 #include "chrome/test/base/browser_with_test_window_test.h"
+#include "chromeos/dbus/session_manager/fake_session_manager_client.h"
+#include "components/arc/arc_prefs.h"
+#include "components/arc/arc_service_manager.h"
 #include "components/arc/intent_helper/arc_intent_helper_bridge.h"
+#include "components/arc/session/arc_bridge_service.h"
+#include "components/arc/test/connection_holder_util.h"
+#include "components/arc/test/fake_intent_helper_instance.h"
 #include "content/public/browser/render_process_host.h"
 #include "content/public/browser/render_view_host.h"
 #include "testing/gmock/include/gmock/gmock.h"
@@ -20,11 +28,42 @@ namespace arc {
 
 namespace {
 
-// Helper class to run tests that need a dummy WebContents.
+// Helper class to run tests that need a dummy WebContents and arc bridge.
 class ArcExternalProtocolDialogTestUtils : public BrowserWithTestWindowTest {
  public:
   ArcExternalProtocolDialogTestUtils() = default;
 
+  void SetUp() override {
+    chromeos::SessionManagerClient::InitializeFakeInMemory();
+    chromeos::FakeSessionManagerClient::Get()->set_arc_available(true);
+    BrowserWithTestWindowTest::SetUp();
+
+    profile()->GetPrefs()->SetBoolean(arc::prefs::kArcEnabled, true);
+    arc_test_.set_wait_default_apps(true);
+    arc_test_.SetUp(profile());
+    // Set up ArcIntentHelperBridge to emulate full-duplex IntentHelper
+    // connection.
+    intent_helper_bridge_ = std::make_unique<arc::ArcIntentHelperBridge>(
+        profile(), arc::ArcServiceManager::Get()->arc_bridge_service());
+    arc::ArcServiceManager::Get()
+        ->arc_bridge_service()
+        ->intent_helper()
+        ->SetInstance(&intent_helper_);
+    WaitForInstanceReady(
+        arc::ArcServiceManager::Get()->arc_bridge_service()->intent_helper());
+  }
+
+  void TearDown() override {
+    arc::ArcServiceManager::Get()
+        ->arc_bridge_service()
+        ->intent_helper()
+        ->CloseInstance(&intent_helper_);
+    intent_helper_bridge_.reset();
+    arc_test_.TearDown();
+    BrowserWithTestWindowTest::TearDown();
+    chromeos::SessionManagerClient::Shutdown();
+  }
+
  protected:
   void CreateTab(bool started_from_arc) {
     AddTab(browser(), GURL("http://www.tests.com"));
@@ -41,11 +80,35 @@ class ArcExternalProtocolDialogTestUtils : public BrowserWithTestWindowTest {
         web_contents_);
   }
 
+  std::unique_ptr<syncer::DeviceInfo> CreateSharingDevice(
+      const std::string& device_guid) {
+    return std::make_unique<syncer::DeviceInfo>(
+        device_guid, "device_name", "chrome_version", "user_agent",
+        sync_pb::SyncEnums_DeviceType_TYPE_PHONE, "device_id",
+        base::SysInfo::HardwareInfo(),
+        /*last_updated_timestamp=*/base::Time::Now(),
+        /*send_tab_to_self_receiving_enabled=*/false,
+        /*sharing_info=*/base::nullopt);
+  }
+
+  MockSharingService* CreateSharingService() {
+    return static_cast<MockSharingService*>(
+        SharingServiceFactory::GetInstance()->SetTestingFactoryAndUse(
+            profile(),
+            base::BindRepeating([](content::BrowserContext* context) {
+              return static_cast<std::unique_ptr<KeyedService>>(
+                  std::make_unique<MockSharingService>());
+            })));
+  }
+
   content::WebContents* web_contents() { return web_contents_; }
 
  private:
   // Keep only one |WebContents| at a time.
   content::WebContents* web_contents_;
+  arc::FakeIntentHelperInstance intent_helper_;
+  ArcAppTest arc_test_;
+  std::unique_ptr<arc::ArcIntentHelperBridge> intent_helper_bridge_;
 
   DISALLOW_COPY_AND_ASSIGN(ArcExternalProtocolDialogTestUtils);
 };
@@ -78,29 +141,6 @@ mojom::IntentHandlerInfoPtr Create(const std::string& name,
 
 }  // namespace
 
-// Tests that when no apps are returned from ARC, GetAction returns
-// SHOW_CHROME_OS_DIALOG.
-TEST(ArcExternalProtocolDialogTest, TestGetActionWithNoApp) {
-  std::vector<mojom::IntentHandlerInfoPtr> handlers;
-  GurlAndActivityInfo url_and_activity_name = CreateEmptyGurlAndActivityInfo();
-
-  // Marking this as safe to bypass or not makes no difference since there are
-  // no handlers.
-  bool in_out_safe_to_bypass_ui = false;
-  EXPECT_EQ(GetActionResult::SHOW_CHROME_OS_DIALOG,
-            GetActionForTesting(GURL("external-protocol:foo"), handlers,
-                                handlers.size(), &url_and_activity_name,
-                                &in_out_safe_to_bypass_ui));
-  EXPECT_FALSE(in_out_safe_to_bypass_ui);
-
-  in_out_safe_to_bypass_ui = true;
-  EXPECT_EQ(GetActionResult::SHOW_CHROME_OS_DIALOG,
-            GetActionForTesting(GURL("external-protocol:foo"), handlers,
-                                handlers.size(), &url_and_activity_name,
-                                &in_out_safe_to_bypass_ui));
-  EXPECT_FALSE(in_out_safe_to_bypass_ui);
-}
-
 // Tests that when one app is passed to GetAction but the user hasn't selected
 // it and |in_out_safe_to_bypass_ui| is true, the function returns
 // HANDLE_URL_IN_ARC.
@@ -962,32 +1002,18 @@ TEST(ArcExternalProtocolDialogTest,
 
 // Tests that clicking on a device calls through to SharingService.
 TEST_F(ArcExternalProtocolDialogTestUtils, TestSelectDeviceForTelLink) {
-  std::string device_guid = "device_guid";
-
-  MockSharingService* sharing_service = static_cast<MockSharingService*>(
-      SharingServiceFactory::GetInstance()->SetTestingFactoryAndUse(
-          profile(), base::BindRepeating([](content::BrowserContext* context) {
-            return static_cast<std::unique_ptr<KeyedService>>(
-                std::make_unique<MockSharingService>());
-          })));
-
-  EXPECT_CALL(*sharing_service,
-              SendMessageToDevice(testing::Eq(device_guid), testing::_,
-                                  testing::_, testing::_));
-
   CreateTab(/*started_from_arc=*/false);
 
+  std::string device_guid = "device_guid";
+  MockSharingService* sharing_service = CreateSharingService();
   content::RenderViewHost* rvh = web_contents()->GetRenderViewHost();
   std::vector<mojom::IntentHandlerInfoPtr> handlers;
   std::vector<std::unique_ptr<syncer::DeviceInfo>> devices;
+  devices.push_back(CreateSharingDevice(device_guid));
 
-  devices.push_back(std::make_unique<syncer::DeviceInfo>(
-      device_guid, "device_name", "chrome_version", "user_agent",
-      sync_pb::SyncEnums_DeviceType_TYPE_PHONE, "device_id",
-      base::SysInfo::HardwareInfo(),
-      /*last_updated_timestamp=*/base::Time::Now(),
-      /*send_tab_to_self_receiving_enabled=*/false,
-      /*sharing_info=*/base::nullopt));
+  EXPECT_CALL(*sharing_service,
+              SendMessageToDevice(testing::Eq(device_guid), testing::_,
+                                  testing::_, testing::_));
 
   OnIntentPickerClosedForTesting(
       rvh->GetProcess()->GetID(), rvh->GetRoutingID(), GURL("tel:0123456789"),
@@ -996,4 +1022,28 @@ TEST_F(ArcExternalProtocolDialogTestUtils, TestSelectDeviceForTelLink) {
       apps::IntentPickerCloseReason::OPEN_APP, /*should_persist=*/false);
 }
 
+TEST_F(ArcExternalProtocolDialogTestUtils, TestDialogWithoutAppsWithDevices) {
+  CreateTab(/*started_from_arc=*/false);
+
+  MockSharingService* sharing_service = CreateSharingService();
+  content::RenderViewHost* rvh = web_contents()->GetRenderViewHost();
+  std::vector<std::unique_ptr<syncer::DeviceInfo>> devices;
+  devices.push_back(CreateSharingDevice("device_guid"));
+
+  EXPECT_CALL(*sharing_service, GetDeviceCandidates(testing::_))
+      .WillOnce(testing::Return(testing::ByMove(std::move(devices))));
+
+  base::RunLoop run_loop;
+  ClickToCallUiController::GetOrCreateFromWebContents(web_contents())
+      ->set_on_dialog_shown_closure_for_testing(run_loop.QuitClosure());
+
+  ASSERT_TRUE(arc::RunArcExternalProtocolDialog(
+      GURL("tel:12341234"), /*initiating_origin=*/base::nullopt,
+      rvh->GetProcess()->GetID(), rvh->GetRoutingID(), ui::PAGE_TRANSITION_LINK,
+      /*has_user_gesture=*/true));
+
+  // Wait until the bubble is visible.
+  run_loop.Run();
+}
+
 }  // namespace arc

components/arc/test/fake_intent_helper_instance.cc

@@ -114,7 +114,12 @@ void FakeIntentHelperInstance::RequestIntentHandlerList(
 
 void FakeIntentHelperInstance::RequestUrlHandlerList(
     const std::string& url,
-    RequestUrlHandlerListCallback callback) {}
+    RequestUrlHandlerListCallback callback) {
+  std::vector<mojom::IntentHandlerInfoPtr> handlers;
+  // Post the reply to run asynchronously to match the real implementation.
+  base::ThreadTaskRunnerHandle::Get()->PostTask(
+      FROM_HERE, base::BindOnce(std::move(callback), std::move(handlers)));
+}
 
 void FakeIntentHelperInstance::RequestUrlListHandlerList(
     std::vector<mojom::UrlWithMimeTypePtr> urls,