Adding a ScrollView for IntentPickerBubbleView

chrome/browser/ui/views/intent_picker_bubble_view_unittest.cc

+// Copyright 2016 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "chrome/browser/ui/views/intent_picker_bubble_view.h"
+
+#include "base/callback.h"
+#include "base/macros.h"
+#include "build/build_config.h"

[Yusuke Sato, 2016/07/19 10:23:58]

nit: what is this for?

[djacobo_, 2016/07/20 21:18:31]

My bad, forgot to delete it from previous versions.

+#include "chrome/browser/chromeos/arc/arc_navigation_throttle.h"
+#include "chrome/test/base/browser_with_test_window_test.h"
+#include "content/public/browser/navigation_handle.h"

[Yusuke Sato, 2016/07/19 10:23:58]

Is this necessary?

[djacobo_, 2016/07/20 21:18:31]

A previous version was using it, removed. Done

+#include "content/public/browser/web_contents.h"
+#include "ui/base/resource/resource_bundle.h"
+#include "ui/gfx/image/image.h"
+#include "ui/views/controls/button/button.h"
+#include "ui/views/controls/button/label_button.h"
+#include "ui/views/resources/grit/views_resources.h"

[Yusuke Sato, 2016/07/19 10:23:58]

same

[djacobo_, 2016/07/20 21:18:31]

My understanding is that this is for the IDR_CLOSE image used for the dummy
icons.

+#include "url/gurl.h"
+
+using NameAndIcon = arc::ArcNavigationThrottle::NameAndIcon;
+using content::WebContents;
+using content::OpenURLParams;
+using content::Referrer;
+
+class IntentPickerBubbleViewTest : public BrowserWithTestWindowTest {
+ public:
+  IntentPickerBubbleViewTest() {}

[Yusuke Sato, 2016/07/19 10:23:58]

= default;

[djacobo_, 2016/07/20 21:18:31]

I will need a different constructor since I am using weak_ptr now for Bind'ing.

+
+  void TearDown() override {
+    // Make sure the bubble is destroyed before the profile to avoid a crash.
+    bubble_.reset();
+
+    BrowserWithTestWindowTest::TearDown();
+  }
+
+ protected:
+  void CreateBubbleView(bool use_custom_icons) {

[Yusuke Sato, 2016/07/19 10:23:58]

use_icons might be better? I wasn't sure what 'custom' meant here.

[djacobo_, 2016/07/20 21:18:31]

Done.

+    // Pushing a couple of fake apps just to check they are created on the UI.
+    app_info_.emplace_back("dank app 1", gfx::Image());
+    app_info_.emplace_back("dank app 2", gfx::Image());
+
+    if (use_custom_icons)
+      FillAppListWithDummyIcons();
+
+    GURL url("http://www.google.com");
+    WebContents* web_contents = browser()->OpenURL(OpenURLParams(

[Yusuke Sato, 2016/07/19 10:23:58]

Why is it necessary to open a page here? Can you add a code comment?

[djacobo_, 2016/07/20 21:18:31]

The bubble class need it since the bubble's lifetime is tied to the WebContents'
lifetime. At least that's my understanding on the WebContentsObserver.

+        url, Referrer(), CURRENT_TAB, ui::PAGE_TRANSITION_TYPED, false));
+
+    // This callback should never be used, his only purpose is to serve as a
+    // dummy callback so the changes are minimal for IntentPickerBubbleView.
+    base::Callback<void(size_t, arc::ArcNavigationThrottle::CloseReason)>
+        dummy_arc_navigation_throttle_cb;
+    bubble_.reset(new IntentPickerBubbleView(
+        app_info_, dummy_arc_navigation_throttle_cb, web_contents, true));

[Yusuke Sato, 2016/07/19 10:23:58]

true /* was_callback_run */

[djacobo_, 2016/07/20 21:18:31]

Done.

+  }
+
+  void FillAppListWithDummyIcons() {
+    ui::ResourceBundle& rb = ui::ResourceBundle::GetSharedInstance();
+    gfx::Image dummy_icon = rb.GetImageNamed(IDR_CLOSE);
+    for (auto& app : app_info_) {
+      app.second = dummy_icon;
+    }
+  }
+
+  std::unique_ptr<IntentPickerBubbleView> bubble_;
+  std::vector<NameAndIcon> app_info_;
+
+ private:
+  DISALLOW_COPY_AND_ASSIGN(IntentPickerBubbleViewTest);
+};
+
+// Verifies that we didn't set up an image for any LabelButton.
+TEST_F(IntentPickerBubbleViewTest, NullIcons) {
+  CreateBubbleView(false);
+  bubble_->Init();
+  for (auto app : bubble_->app_label_ptr_) {

[Yusuke Sato, 2016/07/19 10:23:58]

s/app/label/ probably?

[djacobo_, 2016/07/20 21:18:31]

Changing this part, now we retrieve View* with GetViewByID()

+    EXPECT_TRUE(
+        app->GetImage(views::Button::ButtonState::STATE_NORMAL).isNull());
+  }
+}
+
+// Verifies that all the icons contain a non-null icon.
+TEST_F(IntentPickerBubbleViewTest, NonNullIcons) {
+  CreateBubbleView(true);
+  bubble_->Init();
+  for (auto app : bubble_->app_label_ptr_) {

[Yusuke Sato, 2016/07/19 10:23:58]

same

[djacobo_, 2016/07/20 21:18:31]

Done.

+    EXPECT_FALSE(
+        app->GetImage(views::Button::ButtonState::STATE_NORMAL).isNull());
+  }
+}
+
+// Verifies that the bubble contains as many rows as the input. Populated the
+// bubble with an arbitrary image in every row.
+TEST_F(IntentPickerBubbleViewTest, LabelsPtrVectorSize) {
+  CreateBubbleView(true);
+  bubble_->Init();
+  EXPECT_TRUE(app_info_.size() == bubble_->app_label_ptr_.size());

[Yusuke Sato, 2016/07/19 10:23:58]

EXPECT_EQ(app_info_.size(), bubble_->app_label_ptr_.size());

This way, when the test failed, you can see both values.

[djacobo_, 2016/07/20 21:18:31]

Done.

+}