views: don't pass null anchor view in IntentPickerBubbleView

As per the linked bug, non-null anchor views will become mandatory in
LocationBarBubbleDelegateView. This change modifies IntentPickerBubbleView to
always pass a non-null anchor view. In fact, it already always passed a non-null
anchor view in actual use, since it is only ever invoked in one place
conditional on the anchor view being non-null. As a consequence, this change
deletes the null anchor view path in IntentPickerBubbleView, which removes some
bespoke layout constants (yay!)

This change also refactors IntentPickerBubbleView a bit: IntentPickerBubbleView
itself is built on all Views platforms, but its tests were only built on
ChromeOS. This change therefore makes the tests build on all platforms alongside
the code. As of right now, I think this code is only invoked on ChromeOS, but in
a runtime way that makes it difficult to be sure.

This change also makes private a couple of test-only methods on
IntentPickerBubbleView that used to be public.

Bug: 989080
Change-Id: Id1a9d28ffb5c11e164547ce5c5aac5b26c0ef6d3
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1726431
Commit-Queue: Elly Fong-Jones <ellyjones@chromium.org>
Reviewed-by: Peter Boström <pbos@chromium.org>
Auto-Submit: Elly Fong-Jones <ellyjones@chromium.org>
Cr-Commit-Position: refs/heads/master@{#682830}

chrome/browser/chromeos/BUILD.gn

@@ -2721,7 +2721,6 @@ source_set("unit_tests") {
     "../../common/extensions/api/file_browser_handlers/file_browser_handler_manifest_unittest.cc",
     "../../common/extensions/api/file_system_provider/file_system_provider_handler_unittest.cc",
     "../ui/views/frame/immersive_mode_controller_ash_unittest.cc",
-    "../ui/views/intent_picker_bubble_view_unittest.cc",
     "../ui/views/select_file_dialog_extension_unittest.cc",
     "../ui/webui/chromeos/login/encryption_migration_screen_handler_unittest.cc",
     "../ui/webui/chromeos/login/l10n_util_test_util.cc",

chrome/browser/ui/views/intent_picker_bubble_view.cc

@@ -50,9 +50,6 @@ constexpr int kMaxIntentPickerLabelButtonWidth = 320;
 constexpr gfx::Insets kSeparatorPadding(16, 0, 16, 0);
 constexpr SkColor kSeparatorColor = SkColorSetARGB(0x1F, 0x0, 0x0, 0x0);
 
-// UI position wrt the Top Container
-constexpr int kTopContainerMerge = 3;
-
 constexpr char kInvalidLaunchName[] = "";
 
 bool IsKeyboardCodeArrow(ui::KeyboardCode key_code) {
@@ -137,28 +134,10 @@ views::Widget* IntentPickerBubbleView::ShowBubble(
              apps::IntentPickerCloseReason::ERROR_BEFORE_PICKER, false);
     return nullptr;
   }
-  BrowserView* browser_view = BrowserView::GetBrowserViewForBrowser(browser);
   intent_picker_bubble_ = new IntentPickerBubbleView(
-      std::move(app_info), std::move(intent_picker_cb), web_contents,
-      show_stay_in_chrome, show_remember_selection);
+      anchor_view, std::move(app_info), std::move(intent_picker_cb),
+      web_contents, show_stay_in_chrome, show_remember_selection);
   intent_picker_bubble_->set_margins(gfx::Insets());
-
-  if (anchor_view) {
-    intent_picker_bubble_->SetAnchorView(anchor_view);
-    intent_picker_bubble_->SetArrow(views::BubbleBorder::TOP_RIGHT);
-  } else {
-    intent_picker_bubble_->set_parent_window(
-        platform_util::GetViewForWindow(browser_view->GetNativeWindow()));
-    // Using the TopContainerBoundsInScreen Rect to specify an anchor for the
-    // the UI. Rect allow us to set the coordinates(x,y), the width and height
-    // for the new Rectangle.
-    intent_picker_bubble_->SetAnchorRect(
-        gfx::Rect(browser_view->GetTopContainerBoundsInScreen().x(),
-                  browser_view->GetTopContainerBoundsInScreen().y(),
-                  browser_view->GetTopContainerBoundsInScreen().width(),
-                  browser_view->GetTopContainerBoundsInScreen().height() -
-                      kTopContainerMerge));
-  }
   intent_picker_bubble_->Initialize();
   views::Widget* widget =
       views::BubbleDialogDelegateView::CreateBubble(intent_picker_bubble_);
@@ -183,14 +162,16 @@ views::Widget* IntentPickerBubbleView::ShowBubble(
 
 // static
 std::unique_ptr<IntentPickerBubbleView>
-IntentPickerBubbleView::CreateBubbleView(std::vector<AppInfo> app_info,
-                                         bool show_stay_in_chrome,
-                                         bool show_remember_selection,
-                                         IntentPickerResponse intent_picker_cb,
-                                         content::WebContents* web_contents) {
-  std::unique_ptr<IntentPickerBubbleView> bubble(new IntentPickerBubbleView(
-      std::move(app_info), std::move(intent_picker_cb), web_contents,
-      show_stay_in_chrome, show_remember_selection));
+IntentPickerBubbleView::CreateBubbleViewForTesting(
+    views::View* anchor_view,
+    std::vector<AppInfo> app_info,
+    bool show_stay_in_chrome,
+    bool show_remember_selection,
+    IntentPickerResponse intent_picker_cb,
+    content::WebContents* web_contents) {
+  auto bubble = std::make_unique<IntentPickerBubbleView>(
+      anchor_view, std::move(app_info), std::move(intent_picker_cb),
+      web_contents, show_stay_in_chrome, show_remember_selection);
   bubble->Initialize();
   return bubble;
 }
@@ -261,19 +242,19 @@ base::string16 IntentPickerBubbleView::GetDialogButtonLabel(
 }
 
 IntentPickerBubbleView::IntentPickerBubbleView(
+    views::View* anchor_view,
     std::vector<AppInfo> app_info,
     IntentPickerResponse intent_picker_cb,
     content::WebContents* web_contents,
     bool show_stay_in_chrome,
     bool show_remember_selection)
-    : LocationBarBubbleDelegateView(nullptr /* anchor_view */,
-                                    gfx::Point(),
-                                    web_contents),
+    : LocationBarBubbleDelegateView(anchor_view, gfx::Point(), web_contents),
       intent_picker_cb_(std::move(intent_picker_cb)),
       selected_app_tag_(0),
       app_info_(std::move(app_info)),
       show_stay_in_chrome_(show_stay_in_chrome),
       show_remember_selection_(show_remember_selection) {
+  DCHECK(anchor_view);
   chrome::RecordDialogCreation(chrome::DialogIdentifier::INTENT_PICKER);
 }
 

chrome/browser/ui/views/intent_picker_bubble_view.h

@@ -60,26 +60,25 @@ class IntentPickerBubbleView : public LocationBarBubbleDelegateView,
  public:
   using AppInfo = apps::IntentPickerAppInfo;
 
+  IntentPickerBubbleView(views::View* anchor_view,
+                         std::vector<AppInfo> app_info,
+                         IntentPickerResponse intent_picker_cb,
+                         content::WebContents* web_contents,
+                         bool show_stay_in_chrome,
+                         bool show_remember_selection);
   ~IntentPickerBubbleView() override;
+
   static views::Widget* ShowBubble(views::View* anchor_view,
                                    content::WebContents* web_contents,
                                    std::vector<AppInfo> app_info,
                                    bool show_stay_in_chrome,
                                    bool show_remember_selection,
                                    IntentPickerResponse intent_picker_cb);
-  static std::unique_ptr<IntentPickerBubbleView> CreateBubbleView(
-      std::vector<AppInfo> app_info,
-      bool show_stay_in_chrome,
-      bool show_remember_selection,
-      IntentPickerResponse intent_picker_cb,
-      content::WebContents* web_contents);
   static IntentPickerBubbleView* intent_picker_bubble() {
     return intent_picker_bubble_;
   }
   static void CloseCurrentBubble();
 
-  const std::vector<AppInfo>& GetAppInfoForTesting() const { return app_info_; }
-
   // LocationBarBubbleDelegateView overrides:
   bool Accept() override;
   bool Cancel() override;
@@ -104,11 +103,16 @@ class IntentPickerBubbleView : public LocationBarBubbleDelegateView,
   FRIEND_TEST_ALL_PREFIXES(IntentPickerBubbleViewTest, ChromeNotInCandidates);
   FRIEND_TEST_ALL_PREFIXES(IntentPickerBubbleViewTest, StayInChromeTest);
   FRIEND_TEST_ALL_PREFIXES(IntentPickerBubbleViewTest, WebContentsTiedToBubble);
-  IntentPickerBubbleView(std::vector<AppInfo> app_info,
-                         IntentPickerResponse intent_picker_cb,
-                         content::WebContents* web_contents,
-                         bool show_stay_in_chrome,
-                         bool show_remember_selection);
+
+  static std::unique_ptr<IntentPickerBubbleView> CreateBubbleViewForTesting(
+      views::View* anchor_view,
+      std::vector<AppInfo> app_info,
+      bool show_stay_in_chrome,
+      bool show_remember_selection,
+      IntentPickerResponse intent_picker_cb,
+      content::WebContents* web_contents);
+
+  const std::vector<AppInfo>& app_info_for_testing() const { return app_info_; }
 
   // views::BubbleDialogDelegateView overrides:
   void OnWidgetDestroying(views::Widget* widget) override;

chrome/browser/ui/views/intent_picker_bubble_view_unittest.cc

@@ -11,9 +11,7 @@
 #include "base/callback.h"
 #include "base/macros.h"
 #include "chrome/browser/apps/intent_helper/apps_navigation_types.h"
-#include "chrome/browser/chromeos/arc/intent_helper/arc_intent_picker_app_fetcher.h"
 #include "chrome/test/base/browser_with_test_window_test.h"
-#include "components/arc/intent_helper/arc_intent_helper_bridge.h"
 #include "content/public/browser/web_contents.h"
 #include "ui/base/resource/resource_bundle.h"
 #include "ui/events/base_event_utils.h"
@@ -23,11 +21,33 @@
 #include "ui/views/resources/grit/views_resources.h"
 #include "url/gurl.h"
 
+#if defined(OS_CHROMEOS)
+#include "chrome/browser/chromeos/arc/intent_helper/arc_intent_picker_app_fetcher.h"
+#include "components/arc/intent_helper/arc_intent_helper_bridge.h"
+#endif
+
 using AppInfo = apps::IntentPickerAppInfo;
 using content::WebContents;
 using content::OpenURLParams;
 using content::Referrer;
 
+// There is logic inside IntentPickerBubbleView that filters out the intent
+// helper by checking IsIntentHelperPackage() on them. That logic is
+// ChromeOS-only, so for this unit test to match the behavior of
+// IntentPickerBubbleView on non-ChromeOS platforms, if needs to not filter any
+// packages.
+#if defined(OS_CHROMEOS)
+const char* kArcIntentHelperPackageName =
+    arc::ArcIntentHelperBridge::kArcIntentHelperPackageName;
+bool (*IsIntentHelperPackage)(const std::string&) =
+    arc::ArcIntentHelperBridge::IsIntentHelperPackage;
+#else
+static const char kArcIntentHelperPackageName[] = "unused_intent_helper";
+bool IsIntentHelperPackage(const std::string& package_name) {
+  return false;
+}
+#endif
+
 class IntentPickerBubbleViewTest : public BrowserWithTestWindowTest {
  public:
   IntentPickerBubbleViewTest() = default;
@@ -41,6 +61,8 @@ class IntentPickerBubbleViewTest : public BrowserWithTestWindowTest {
 
  protected:
   void CreateBubbleView(bool use_icons, bool show_stay_in_chrome) {
+    anchor_view_ = std::make_unique<views::View>();
+
     // Pushing a couple of fake apps just to check they are created on the UI.
     app_info_.emplace_back(apps::mojom::AppType::kArc, gfx::Image(),
                            "package_1", "dank app 1");
@@ -48,10 +70,8 @@ class IntentPickerBubbleViewTest : public BrowserWithTestWindowTest {
                            "package_2", "dank_app_2");
     // Also adding the corresponding Chrome's package name on ARC, even if this
     // is given to the picker UI as input it should be ignored.
-    app_info_.emplace_back(
-        apps::mojom::AppType::kArc, gfx::Image(),
-        arc::ArcIntentHelperBridge::kArcIntentHelperPackageName,
-        "legit_chrome");
+    app_info_.emplace_back(apps::mojom::AppType::kArc, gfx::Image(),
+                           kArcIntentHelperPackageName, "legit_chrome");
 
     if (use_icons)
       FillAppListWithDummyIcons();
@@ -72,8 +92,8 @@ class IntentPickerBubbleViewTest : public BrowserWithTestWindowTest {
                             app.display_name);
     }
 
-    bubble_ = IntentPickerBubbleView::CreateBubbleView(
-        std::move(app_info), show_stay_in_chrome,
+    bubble_ = IntentPickerBubbleView::CreateBubbleViewForTesting(
+        anchor_view_.get(), std::move(app_info), show_stay_in_chrome,
         /*show_remember_selection=*/true,
         base::Bind(&IntentPickerBubbleViewTest::OnBubbleClosed,
                    base::Unretained(this)),
@@ -94,6 +114,7 @@ class IntentPickerBubbleViewTest : public BrowserWithTestWindowTest {
                       bool should_persist) {}
 
   std::unique_ptr<IntentPickerBubbleView> bubble_;
+  std::unique_ptr<views::View> anchor_view_;
   std::vector<AppInfo> app_info_;
 
  private:
@@ -129,7 +150,7 @@ TEST_F(IntentPickerBubbleViewTest, LabelsPtrVectorSize) {
   size_t size = app_info_.size();
   size_t chrome_package_repetitions = 0;
   for (const AppInfo& app_info : app_info_) {
-    if (arc::ArcIntentHelperBridge::IsIntentHelperPackage(app_info.launch_name))
+    if (IsIntentHelperPackage(app_info.launch_name))
       ++chrome_package_repetitions;
   }
 
@@ -183,8 +204,8 @@ TEST_F(IntentPickerBubbleViewTest, ChromeNotInCandidates) {
   CreateBubbleView(/*use_icons=*/false, /*show_stay_in_chrome=*/true);
   size_t size = bubble_->GetScrollViewSize();
   for (size_t i = 0; i < size; ++i) {
-    EXPECT_FALSE(arc::ArcIntentHelperBridge::IsIntentHelperPackage(
-        bubble_->GetAppInfoForTesting()[i].launch_name));
+    EXPECT_FALSE(
+        IsIntentHelperPackage(bubble_->app_info_for_testing()[i].launch_name));
   }
 }
 

chrome/test/BUILD.gn

@@ -4863,6 +4863,7 @@ test("unit_tests") {
       "../browser/ui/views/global_error_bubble_view_unittest.cc",
       "../browser/ui/views/hover_button_unittest.cc",
       "../browser/ui/views/infobars/infobar_view_unittest.cc",
+      "../browser/ui/views/intent_picker_bubble_view_unittest.cc",
       "../browser/ui/views/layout_provider_unittest.cc",
       "../browser/ui/views/location_bar/icon_label_bubble_view_unittest.cc",
       "../browser/ui/views/location_bar/location_icon_view_unittest.cc",