Adding a ScrollView for IntentPickerBubbleView

chrome/browser/ui/views/intent_picker_bubble_view.h

Index: chrome/browser/ui/views/intent_picker_bubble_view.h
diff --git a/chrome/browser/ui/views/intent_picker_bubble_view.h b/chrome/browser/ui/views/intent_picker_bubble_view.h
index ae516f2db1caeb38cbbdf44a6de3a17e6637f78f..6e0dcdd624dd087701763e115d2c42d2f5de2520 100644
--- a/chrome/browser/ui/views/intent_picker_bubble_view.h
+++ b/chrome/browser/ui/views/intent_picker_bubble_view.h
@@ -21,7 +21,6 @@ class NavigationHandle;
 }  // namespace content
 
 namespace views {
-class EventMonitor;
 class Label;
 class LabelButton;
 class View;
@@ -51,6 +50,11 @@ class IntentPickerBubbleView : public views::BubbleDialogDelegateView,
                                public views::ButtonListener,
                                public content::WebContentsObserver {
  public:
+  enum class ViewsId : int {

[sky, 2016/07/21 22:57:06]

Move to private section, and add a comment.

[djacobo_, 2016/07/22 01:22:07]

Done.

+    HEADER = 0,
+    SCROLL_VIEW,
+    FOOTER,
+  };
   using NameAndIcon = arc::ArcNavigationThrottle::NameAndIcon;
   // This callback informs the index of the app selected by the user, along with
   // the reason why the Bubble was closed. The size_t param must have a value in
@@ -60,9 +64,16 @@ class IntentPickerBubbleView : public views::BubbleDialogDelegateView,
   using ThrottleCallback =
       base::Callback<void(size_t, arc::ArcNavigationThrottle::CloseReason)>;
 
+  ~IntentPickerBubbleView() override;
   static void ShowBubble(content::NavigationHandle* handle,
                          const std::vector<NameAndIcon>& app_info,
                          const ThrottleCallback& throttle_cb);
+  static std::unique_ptr<IntentPickerBubbleView> CreateBubbleForTesting(

[sky, 2016/07/21 22:57:06]

You're exposing 3 methods purely for testing. How about friending the test
instead?

[djacobo_, 2016/07/22 01:22:07]

We already did that on a previous version but opted for this way (with
ForTesting() methods) in order to keep a better separation of the class to test
and the tester, let me know what you think.

[sky, 2016/07/22 16:32:54]

I prefer the friend and moving to test, otherwise you're adding a bunch of code
that may or may not get optimized out that is only for testing.

[djacobo_, 2016/07/22 22:37:24]

Ok, got rid of the ForTesting() methods, I want to keep this method as static.
Take a look at the new version.

+      const std::vector<NameAndIcon>& app_info,
+      const ThrottleCallback& throttle_cb,
+      content::WebContents* web_contents);
+  size_t GetAppInfoSizeForTesting() const;
+  views::LabelButton* GetLabelButtonAtForTesting(size_t index);
 
  protected:
   // views::BubbleDialogDelegateView overrides:
@@ -72,7 +83,6 @@ class IntentPickerBubbleView : public views::BubbleDialogDelegateView,
   IntentPickerBubbleView(const std::vector<NameAndIcon>& app_info,
                          ThrottleCallback throttle_cb,
                          content::WebContents* web_contents);
-  ~IntentPickerBubbleView() override;
 
   // views::BubbleDialogDelegateView overrides:
   void OnWidgetDestroying(views::Widget* widget) override;
@@ -87,6 +97,9 @@ class IntentPickerBubbleView : public views::BubbleDialogDelegateView,
   // content::WebContentsObserver overrides:
   void WebContentsDestroyed() override;
 
+  views::LabelButton* GetLabelButtonAt(size_t index);

[sky, 2016/07/21 22:57:06]

Add description.

[djacobo_, 2016/07/22 01:22:07]

Done.

+  void PaintLabelButton(size_t index, SkColor color);

[sky, 2016/07/21 22:57:06]

Based on the name I thought this was called during painting to paint the label.
That isn't the case. It resets the border. Please rename to make it more
obvious, perhaps SetLabelButtonBackgroundColor.

[djacobo_, 2016/07/22 01:22:07]

Much better, thanks!. Done

+
   // Flag set to true iff the callback was Run at some previous step, used to
   // ensure we only use the callback once.
   bool was_callback_run_;