Add SharingDialog browser test for tab close.

This adds a test for when a browser tab is closed while a sharing dialog
is shown. Also includes a refactoring which allows us to record shown
dialogs to UKM on ChromeOS too, which was previously missing.

Bug: None
Change-Id: Icc6c1399e21a667673914e32dae90ff38e4e606d
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1823857
Reviewed-by: David Jacobo <djacobo@chromium.org>
Reviewed-by: Alex Chau <alexchau@chromium.org>
Commit-Queue: Alex Chau <alexchau@chromium.org>
Cr-Commit-Position: refs/heads/master@{#701239}

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

@@ -66,27 +66,16 @@ gfx::Image CreateDeviceIcon(const sync_pb::SyncEnums::DeviceType device_type) {
   return gfx::Image(gfx::CreateVectorIcon(icon, kDeviceIconSize, color));
 }
 
-// Adds remote devices to |picker_entries| and returns the new list. The devices
-// are added to the beginning of the list.
-std::vector<apps::IntentPickerAppInfo> MaybeAddDevices(
-    const GURL& url,
-    WebContents* web_contents,
+// Adds |devices| to |picker_entries| and returns the new list. The devices are
+// added to the beginning of the list.
+std::vector<apps::IntentPickerAppInfo> AddDevices(
+    const std::vector<std::unique_ptr<syncer::DeviceInfo>>& devices,
     std::vector<apps::IntentPickerAppInfo> picker_entries) {
-  if (!ShouldOfferClickToCallForURL(web_contents->GetBrowserContext(), url))
-    return picker_entries;
-
-  ClickToCallUiController* controller =
-      ClickToCallUiController::GetOrCreateFromWebContents(web_contents);
-  if (!controller)
-    return picker_entries;
-
-  controller->UpdateDevices();
-  if (controller->devices().empty())
-    return picker_entries;
+  DCHECK(!devices.empty());
 
   // First add all devices to the list.
   std::vector<apps::IntentPickerAppInfo> all_entries;
-  for (const auto& device : controller->devices()) {
+  for (const auto& device : devices) {
     all_entries.emplace_back(apps::PickerEntryType::kDevice,
                              CreateDeviceIcon(device->device_type()),
                              device->guid(), device->client_name());
@@ -113,21 +102,35 @@ bool MaybeAddDevicesAndShowPicker(
   if (!browser)
     return false;
 
-  std::vector<apps::IntentPickerAppInfo> all_apps =
-      MaybeAddDevices(url, web_contents, std::move(app_info));
+  bool has_apps = !app_info.empty();
+  bool has_devices = false;
+
+  PageActionIconType icon_type = PageActionIconType::kIntentPicker;
+  ClickToCallUiController* controller = nullptr;
+
+  if (ShouldOfferClickToCallForURL(web_contents->GetBrowserContext(), url)) {
+    icon_type = PageActionIconType::kClickToCall;
+    controller =
+        ClickToCallUiController::GetOrCreateFromWebContents(web_contents);
+    controller->UpdateDevices();
 
-  if (all_apps.empty())
+    const auto& devices = controller->devices();
+    has_devices = !devices.empty();
+    if (has_devices)
+      app_info = AddDevices(devices, std::move(app_info));
+  }
+
+  if (app_info.empty())
     return false;
 
-  PageActionIconType icon_type =
-      ShouldOfferClickToCallForURL(web_contents->GetBrowserContext(), url)
-          ? PageActionIconType::kClickToCall
-          : PageActionIconType::kIntentPicker;
   IntentPickerTabHelper::SetShouldShowIcon(
       web_contents, icon_type == PageActionIconType::kIntentPicker);
-  browser->window()->ShowIntentPickerBubble(std::move(all_apps), stay_in_chrome,
-                                            show_remember_selection, icon_type,
-                                            std::move(callback));
+  browser->window()->ShowIntentPickerBubble(
+      std::move(app_info), stay_in_chrome, show_remember_selection, icon_type,
+      base::BindOnce(std::move(callback)));
+
+  if (controller)
+    controller->OnDialogShown(has_devices, has_apps);
 
   return true;
 }

chrome/browser/sharing/click_to_call/click_to_call_browsertest.cc

@@ -16,6 +16,7 @@
 #include "chrome/app/chrome_command_ids.h"
 #include "chrome/browser/gcm/gcm_profile_service_factory.h"
 #include "chrome/browser/renderer_context_menu/render_view_context_menu_test_util.h"
+#include "chrome/browser/sharing/click_to_call/click_to_call_ui_controller.h"
 #include "chrome/browser/sharing/click_to_call/click_to_call_utils.h"
 #include "chrome/browser/sharing/click_to_call/feature.h"
 #include "chrome/browser/sharing/features.h"
@@ -26,15 +27,20 @@
 #include "chrome/browser/sharing/sharing_service_factory.h"
 #include "chrome/browser/sharing/sharing_sync_preference.h"
 #include "chrome/browser/sync/device_info_sync_service_factory.h"
+#include "chrome/browser/sync/profile_sync_service_factory.h"
 #include "chrome/browser/sync/test/integration/sessions_helper.h"
 #include "chrome/browser/sync/test/integration/sync_test.h"
 #include "chrome/browser/ui/browser.h"
 #include "components/gcm_driver/fake_gcm_profile_service.h"
 #include "components/sync/driver/profile_sync_service.h"
+#include "components/sync/test/fake_server/fake_server.h"
+#include "components/sync/test/fake_server/fake_server_network_resources.h"
 #include "components/sync_device_info/device_info_sync_service.h"
 #include "components/sync_device_info/fake_device_info_tracker.h"
 #include "components/ukm/test_ukm_recorder.h"
+#include "content/public/test/test_navigation_observer.h"
 #include "net/dns/mock_host_resolver.h"
+#include "net/test/embedded_test_server/embedded_test_server.h"
 #include "services/metrics/public/cpp/ukm_builders.h"
 #include "url/gurl.h"
 
@@ -57,17 +63,28 @@ class ClickToCallBrowserTest : public SyncTest {
 
   ~ClickToCallBrowserTest() override {}
 
-  void SetUpOnMainThread() override { SyncTest::SetUpOnMainThread(); }
+  void SetUpOnMainThread() override {
+    SyncTest::SetUpOnMainThread();
+    host_resolver()->AddRule("mock.http", "127.0.0.1");
+  }
 
   void Init(const std::vector<base::Feature>& enabled_features,
             const std::vector<base::Feature>& disabled_features) {
     scoped_feature_list_.InitWithFeatures(enabled_features, disabled_features);
 
     ASSERT_TRUE(SetupSync()) << "SetupSync() failed.";
 
-    sessions_helper::OpenTab(0, GURL("http://www.google.com/"));
+    ASSERT_TRUE(embedded_test_server()->Start());
+    GURL url = embedded_test_server()->GetURL("mock.http", "/sharing/tel.html");
+    ASSERT_TRUE(sessions_helper::OpenTab(0, url));
+
     web_contents_ = GetBrowser(0)->tab_strip_model()->GetWebContentsAt(0);
 
+    // Ensure that the test page has loaded.
+    const base::string16 title = base::ASCIIToUTF16("Tel");
+    content::TitleWatcher watcher(web_contents_, title);
+    EXPECT_EQ(title, watcher.WaitAndGetTitle());
+
     gcm_service_ = static_cast<gcm::FakeGCMProfileService*>(
         gcm::GCMProfileServiceFactory::GetForProfile(GetProfile(0)));
     gcm_service_->set_collect(true);
@@ -165,6 +182,8 @@ class ClickToCallBrowserTest : public SyncTest {
 
   SharingService* sharing_service() const { return sharing_service_; }
 
+  content::WebContents* web_contents() const { return web_contents_; }
+
  private:
   gcm::GCMProfileServiceFactory::ScopedTestingFactoryInstaller
       scoped_testing_factory_installer_;
@@ -395,3 +414,23 @@ IN_PROC_BROWSER_TEST_F(ClickToCallBrowserTest, ContextMenu_UKM) {
             *selection);
   // TODO(knollr): mock apps and verify |has_apps| here too.
 }
+
+IN_PROC_BROWSER_TEST_F(ClickToCallBrowserTest, CloseTabWithBubble) {
+  Init({kSharingDeviceRegistration, kClickToCallUI}, {});
+  SetUpDevices(/*count=*/1);
+
+  base::RunLoop run_loop;
+  ClickToCallUiController::GetOrCreateFromWebContents(web_contents())
+      ->set_on_dialog_shown_closure_for_testing(run_loop.QuitClosure());
+
+  // Click on the tel link to trigger the bubble view.
+  web_contents()->GetMainFrame()->ExecuteJavaScriptForTests(
+      base::ASCIIToUTF16("document.querySelector('a').click();"),
+      base::NullCallback());
+  // Wait until the bubble is visible.
+  run_loop.Run();
+
+  // Close the tab while the bubble is opened.
+  // Regression test for http://crbug.com/1000934.
+  sessions_helper::CloseTab(/*browser_index=*/0, /*tab_index=*/0);
+}

chrome/browser/sharing/click_to_call/click_to_call_ui_controller.cc

@@ -14,8 +14,6 @@
 #include "chrome/browser/sharing/sharing_dialog.h"
 #include "chrome/browser/shell_integration.h"
 #include "chrome/browser/ui/browser.h"
-#include "chrome/browser/ui/browser_finder.h"
-#include "chrome/browser/ui/browser_window.h"
 #include "chrome/grit/theme_resources.h"
 #include "components/sync_device_info/device_info.h"
 #include "components/vector_icons/vector_icons.h"
@@ -125,14 +123,14 @@ void ClickToCallUiController::OnAppChosen(const SharingApp& app) {
                                                          web_contents());
 }
 
-SharingDialog* ClickToCallUiController::DoShowDialog(BrowserWindow* window) {
+void ClickToCallUiController::OnDialogShown(bool has_devices, bool has_apps) {
   if (!HasSendFailed()) {
     // Only left clicks open a dialog.
     ukm_recorder_ = base::BindOnce(&LogClickToCallUKM, web_contents(),
                                    SharingClickToCallEntryPoint::kLeftClickLink,
-                                   !devices().empty(), !apps().empty());
+                                   has_devices, has_apps);
   }
-  return window->ShowSharingDialog(web_contents(), this);
+  SharingUiController::OnDialogShown(has_devices, has_apps);
 }
 
 base::string16 ClickToCallUiController::GetContentType() const {

chrome/browser/sharing/click_to_call/click_to_call_ui_controller.h

@@ -54,12 +54,12 @@ class ClickToCallUiController
   int GetHeaderImageId() const override;
   std::unique_ptr<views::StyledLabel> GetHelpTextLabel(
       views::StyledLabelListener* listener) override;
+  void OnDialogShown(bool has_devices, bool has_apps) override;
 
  protected:
   explicit ClickToCallUiController(content::WebContents* web_contents);
 
   // Overridden from SharingUiController:
-  SharingDialog* DoShowDialog(BrowserWindow* window) override;
   void DoUpdateApps(UpdateAppsCallback callback) override;
 
  private:

chrome/browser/sharing/shared_clipboard/shared_clipboard_ui_controller.cc

@@ -11,7 +11,6 @@
 #include "chrome/app/vector_icons/vector_icons.h"
 #include "chrome/browser/sharing/sharing_constants.h"
 #include "chrome/browser/sharing/sharing_dialog.h"
-#include "chrome/browser/ui/browser_window.h"
 #include "components/sync_device_info/device_info.h"
 #include "content/public/browser/web_contents.h"
 #include "ui/base/l10n/l10n_util.h"
@@ -57,12 +56,6 @@ void SharedClipboardUiController::DoUpdateApps(UpdateAppsCallback callback) {
   std::move(callback).Run(std::vector<SharingApp>());
 }
 
-// Error message dialog.
-SharingDialog* SharedClipboardUiController::DoShowDialog(
-    BrowserWindow* window) {
-  return window->ShowSharingDialog(web_contents(), this);
-}
-
 void SharedClipboardUiController::OnDeviceChosen(
     const syncer::DeviceInfo& device) {
   chrome_browser_sharing::SharingMessage sharing_message;

chrome/browser/sharing/shared_clipboard/shared_clipboard_ui_controller.h

@@ -51,7 +51,6 @@ class SharedClipboardUiController
   explicit SharedClipboardUiController(content::WebContents* web_contents);
 
   // Overridden from SharingUiController:
-  SharingDialog* DoShowDialog(BrowserWindow* window) override;
   void DoUpdateApps(UpdateAppsCallback callback) override;
 
  private:

chrome/browser/sharing/sharing_ui_controller.cc

@@ -61,8 +61,9 @@ void SharingUiController::ShowNewDialog() {
   if (!window)
     return;
 
-  dialog_ = DoShowDialog(window);
+  dialog_ = window->ShowSharingDialog(web_contents(), this);
   UpdateIcon();
+  OnDialogShown(!devices_.empty(), !apps_.empty());
 }
 
 void SharingUiController::UpdateIcon() {
@@ -205,3 +206,8 @@ void SharingUiController::OnHelpTextClicked(SharingDialogType dialog_type) {
   ShowSingletonTab(chrome::FindBrowserWithWebContents(web_contents()),
                    GURL(chrome::kSyncLearnMoreURL));
 }
+
+void SharingUiController::OnDialogShown(bool has_devices, bool has_apps) {
+  if (on_dialog_shown_closure_for_testing_)
+    std::move(on_dialog_shown_closure_for_testing_).Run();
+}

chrome/browser/sharing/sharing_ui_controller.h

@@ -24,7 +24,6 @@
 #include "ui/views/controls/styled_label.h"
 #include "ui/views/controls/styled_label_listener.h"
 
-class BrowserWindow;
 class SharingDialog;
 class SharingService;
 
@@ -116,8 +115,14 @@ class SharingUiController {
   // Called by the SharingDialogView when the help text got clicked.
   virtual void OnHelpTextClicked(SharingDialogType dialog_type);
 
+  // Called when a new dialog is shown.
+  virtual void OnDialogShown(bool has_devices, bool has_apps);
+
+  void set_on_dialog_shown_closure_for_testing(base::OnceClosure closure) {
+    on_dialog_shown_closure_for_testing_ = std::move(closure);
+  }
+
  protected:
-  virtual SharingDialog* DoShowDialog(BrowserWindow* window) = 0;
   virtual void DoUpdateApps(UpdateAppsCallback callback) = 0;
 
   void SendMessageToDevice(
@@ -155,6 +160,9 @@ class SharingUiController {
   // ID of the last shown dialog used to ignore events from old dialogs.
   int last_dialog_id_ = 0;
 
+  // Closure to call when a new dialog is shown.
+  base::OnceClosure on_dialog_shown_closure_for_testing_;
+
   base::WeakPtrFactory<SharingUiController> weak_ptr_factory_{this};
 };
 

chrome/test/data/sharing/tel.html

@@ -0,0 +1,8 @@
+<html>
+  <head>
+    <title>Tel</title>
+  </head>
+  <body style="margin=0;">
+    <a href="tel:0123456789" style="width:100%;height:100%;display:block;">Tel</a>
+  </body>
+</html>