Refactor arc::RunArcExternalProtocolDialog to take handled callback

RunArcExternalProtocolDialog takes a callback rather than creating the
ExternalProtocolDialog directly.  This is to allow further logic to be
added consistently in the case where arc does not handle the URL.

Bug: b/168506505
Change-Id: I82e74112870cb727e46b0a3ab987e7117a4ac7d7
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2423067
Commit-Queue: Joel Hockey <joelhockey@chromium.org>
Reviewed-by: David Jacobo <djacobo@chromium.org>
Reviewed-by: Xiyuan Xia <xiyuan@chromium.org>
Auto-Submit: Joel Hockey <joelhockey@chromium.org>
Cr-Commit-Position: refs/heads/master@{#809559}

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

@@ -144,15 +144,6 @@ bool MaybeAddDevicesAndShowPicker(
   return true;
 }
 
-// Shows the Chrome OS' original external protocol dialog as a fallback.
-void ShowFallbackExternalProtocolDialog(int render_process_host_id,
-                                        int routing_id,
-                                        const GURL& url) {
-  WebContents* web_contents =
-      tab_util::GetWebContentsByID(render_process_host_id, routing_id);
-  new ExternalProtocolDialog(web_contents, url);
-}
-
 void CloseTabIfNeeded(int render_process_host_id,
                       int routing_id,
                       bool safe_to_bypass_ui) {
@@ -601,6 +592,7 @@ void OnAppIconsReceived(
     const base::Optional<url::Origin>& initiating_origin,
     bool safe_to_bypass_ui,
     std::vector<mojom::IntentHandlerInfoPtr> handlers,
+    base::OnceCallback<void(bool)> handled_cb,
     std::unique_ptr<ArcIntentHelperBridge::ActivityToIconsMap> icons) {
   DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
 
@@ -623,34 +615,34 @@ void OnAppIconsReceived(
       web_contents ? chrome::FindBrowserWithWebContents(web_contents) : nullptr;
 
   if (!web_contents || !browser)
-    return;
+    return std::move(handled_cb).Run(false);
 
   const bool stay_in_chrome = IsChromeAnAppCandidate(handlers);
-  MaybeAddDevicesAndShowPicker(
+  bool handled = MaybeAddDevicesAndShowPicker(
       url, initiating_origin, web_contents, std::move(app_info), stay_in_chrome,
       /*show_remember_selection=*/true,
       base::BindOnce(OnIntentPickerClosed, render_process_host_id, routing_id,
                      url, safe_to_bypass_ui, std::move(handlers)));
+  return std::move(handled_cb).Run(handled);
 }
 
 void ShowExternalProtocolDialogWithoutApps(
     int render_process_host_id,
     int routing_id,
     const GURL& url,
-    const base::Optional<url::Origin>& initiating_origin) {
+    const base::Optional<url::Origin>& initiating_origin,
+    base::OnceCallback<void(bool)> handled_cb) {
   // Try to show the device picker and fallback to the default dialog otherwise.
-  if (MaybeAddDevicesAndShowPicker(
-          url, initiating_origin,
-          tab_util::GetWebContentsByID(render_process_host_id, routing_id),
-          /*app_info=*/{}, /*stay_in_chrome=*/false,
-          /*show_remember_selection=*/false,
-          base::BindOnce(OnIntentPickerClosed, render_process_host_id,
-                         routing_id, url, /*safe_to_bypass_ui=*/false,
-                         std::vector<mojom::IntentHandlerInfoPtr>()))) {
-    return;
-  }
+  bool handled = MaybeAddDevicesAndShowPicker(
+      url, initiating_origin,
+      tab_util::GetWebContentsByID(render_process_host_id, routing_id),
+      /*app_info=*/{}, /*stay_in_chrome=*/false,
+      /*show_remember_selection=*/false,
+      base::BindOnce(OnIntentPickerClosed, render_process_host_id, routing_id,
+                     url, /*safe_to_bypass_ui=*/false,
+                     std::vector<mojom::IntentHandlerInfoPtr>()));
 
-  ShowFallbackExternalProtocolDialog(render_process_host_id, routing_id, url);
+  return std::move(handled_cb).Run(handled);
 }
 
 // Called when ARC returned a handler list for the |url|.
@@ -659,6 +651,7 @@ void OnUrlHandlerList(int render_process_host_id,
                       const GURL& url,
                       const base::Optional<url::Origin>& initiating_origin,
                       bool safe_to_bypass_ui,
+                      base::OnceCallback<void(bool)> handled_cb,
                       std::vector<mojom::IntentHandlerInfoPtr> handlers) {
   DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
 
@@ -666,7 +659,8 @@ void OnUrlHandlerList(int render_process_host_id,
   if (!arc_service_manager) {
     // ARC is not running anymore. Show the Chrome OS dialog.
     ShowExternalProtocolDialogWithoutApps(render_process_host_id, routing_id,
-                                          url, initiating_origin);
+                                          url, initiating_origin,
+                                          std::move(handled_cb));
     return;
   }
 
@@ -686,7 +680,8 @@ void OnUrlHandlerList(int render_process_host_id,
   if (!instance || !intent_helper_bridge || handlers.empty() ||
       IsChromeOnlyAppCandidate(handlers)) {
     ShowExternalProtocolDialogWithoutApps(render_process_host_id, routing_id,
-                                          url, initiating_origin);
+                                          url, initiating_origin,
+                                          std::move(handled_cb));
     return;
   }
 
@@ -701,7 +696,7 @@ void OnUrlHandlerList(int render_process_host_id,
           apps::Source::kExternalProtocol,
           /*should_persist=*/false);
     }
-    return;  // the |url| has been handled.
+    return std::move(handled_cb).Run(/*handled=*/true);
   }
 
   // Otherwise, retrieve icons of the activities. Since this function is for
@@ -712,20 +707,22 @@ void OnUrlHandlerList(int render_process_host_id,
     activities.emplace_back(handler->package_name, handler->activity_name);
   }
   intent_helper_bridge->GetActivityIcons(
-      activities, base::BindOnce(OnAppIconsReceived, render_process_host_id,
-                                 routing_id, url, initiating_origin,
-                                 safe_to_bypass_ui, std::move(handlers)));
+      activities,
+      base::BindOnce(OnAppIconsReceived, render_process_host_id, routing_id,
+                     url, initiating_origin, safe_to_bypass_ui,
+                     std::move(handlers), std::move(handled_cb)));
 }
 
 }  // namespace
 
-bool RunArcExternalProtocolDialog(
+void RunArcExternalProtocolDialog(
     const GURL& url,
     const base::Optional<url::Origin>& initiating_origin,
     int render_process_host_id,
     int routing_id,
     ui::PageTransition page_transition,
-    bool has_user_gesture) {
+    bool has_user_gesture,
+    base::OnceCallback<void(bool)> handled_cb) {
   // This function is for external protocols that Chrome cannot handle.
   DCHECK(!url.SchemeIsHTTPOrHTTPS()) << url;
 
@@ -738,15 +735,16 @@ bool RunArcExternalProtocolDialog(
                                    /*allow_client_redirect=*/true)) {
     LOG(WARNING) << "RunArcExternalProtocolDialog: ignoring " << url
                  << " with PageTransition=" << masked_page_transition;
-    return false;
+    return std::move(handled_cb).Run(false);
   }
 
   auto* arc_service_manager = ArcServiceManager::Get();
   if (!arc_service_manager) {
     // ARC is either not supported or not yet ready.
     ShowExternalProtocolDialogWithoutApps(render_process_host_id, routing_id,
-                                          url, initiating_origin);
-    return true;
+                                          url, initiating_origin,
+                                          std::move(handled_cb));
+    return;
   }
 
   auto* instance = ARC_GET_INSTANCE_FOR_METHOD(
@@ -755,15 +753,16 @@ bool RunArcExternalProtocolDialog(
   if (!instance) {
     // ARC is either not supported or not yet ready.
     ShowExternalProtocolDialogWithoutApps(render_process_host_id, routing_id,
-                                          url, initiating_origin);
-    return true;
+                                          url, initiating_origin,
+                                          std::move(handled_cb));
+    return;
   }
 
   WebContents* web_contents =
       tab_util::GetWebContentsByID(render_process_host_id, routing_id);
   if (!web_contents || !web_contents->GetBrowserContext() ||
       web_contents->GetBrowserContext()->IsOffTheRecord()) {
-    return false;
+    return std::move(handled_cb).Run(/*handled=*/false);
   }
 
   const bool safe_to_bypass_ui =
@@ -772,10 +771,9 @@ bool RunArcExternalProtocolDialog(
   // Show ARC version of the dialog, which is IntentPickerBubbleView. To show
   // the bubble view, we need to ask ARC for a handler list first.
   instance->RequestUrlHandlerList(
-      url.spec(),
-      base::BindOnce(OnUrlHandlerList, render_process_host_id, routing_id, url,
-                     initiating_origin, safe_to_bypass_ui));
-  return true;
+      url.spec(), base::BindOnce(OnUrlHandlerList, render_process_host_id,
+                                 routing_id, url, initiating_origin,
+                                 safe_to_bypass_ui, std::move(handled_cb)));
 }
 
 GetActionResult GetActionForTesting(

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

@@ -123,15 +123,18 @@ enum class Scheme {
   OTHER
 };
 
-// Shows ARC version of the dialog. Returns true if ARC is supported, running,
-// and in a context where it is allowed to handle external protocol.
-bool RunArcExternalProtocolDialog(
+// Checks if ARC is supported, running, and in a context where it is allowed to
+// handle external protocol, then either shows the dialog, or directly launches
+// the app if a user has previously made a choice. Invokes |handled_cb| with
+// true if the protocol has been handled by ARC.
+void RunArcExternalProtocolDialog(
     const GURL& url,
     const base::Optional<url::Origin>& initiating_origin,
     int render_process_host_id,
     int routing_id,
     ui::PageTransition page_transition,
-    bool has_user_gesture);
+    bool has_user_gesture,
+    base::OnceCallback<void(bool)> handled_cb);
 
 GetActionResult GetActionForTesting(
     const GURL& original_url,

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

@@ -1030,13 +1030,17 @@ TEST_F(ArcExternalProtocolDialogTestUtils, TestDialogWithoutAppsWithDevices) {
   ClickToCallUiController::GetOrCreateFromWebContents(web_contents())
       ->set_on_dialog_shown_closure_for_testing(run_loop.QuitClosure());
 
-  ASSERT_TRUE(arc::RunArcExternalProtocolDialog(
+  bool handled = false;
+  arc::RunArcExternalProtocolDialog(
       GURL("tel:12341234"), /*initiating_origin=*/base::nullopt,
       rvh->GetProcess()->GetID(), rvh->GetRoutingID(), ui::PAGE_TRANSITION_LINK,
-      /*has_user_gesture=*/true));
+      /*has_user_gesture=*/true,
+      base::BindOnce([](bool* handled, bool result) { *handled = result; },
+                     &handled));
 
   // Wait until the bubble is visible.
   run_loop.Run();
+  EXPECT_TRUE(handled);
 }
 
 }  // namespace arc

chrome/browser/chromeos/external_protocol_dialog.cc

@@ -27,6 +27,11 @@ namespace {
 
 const int kMessageWidth = 400;
 
+void OnArcHandled(WebContents* web_contents, const GURL& url, bool handled) {
+  if (!handled)
+    new ExternalProtocolDialog(web_contents, url);
+}
+
 }  // namespace
 
 ///////////////////////////////////////////////////////////////////////////////
@@ -47,12 +52,10 @@ void ExternalProtocolHandler::RunExternalProtocolDialog(
   int render_process_host_id =
       web_contents->GetRenderViewHost()->GetProcess()->GetID();
   int routing_id = web_contents->GetRenderViewHost()->GetRoutingID();
-  if (arc::RunArcExternalProtocolDialog(url, initiating_origin,
-                                        render_process_host_id, routing_id,
-                                        page_transition, has_user_gesture)) {
-    return;
-  }
-  new ExternalProtocolDialog(web_contents, url);
+  arc::RunArcExternalProtocolDialog(
+      url, initiating_origin, render_process_host_id, routing_id,
+      page_transition, has_user_gesture,
+      base::BindOnce(&OnArcHandled, web_contents, url));
 }
 
 ///////////////////////////////////////////////////////////////////////////////