Adding a ScrollView for IntentPickerBubbleView

chrome/browser/ui/views/intent_picker_bubble_view.h

 // 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.
 
 #ifndef CHROME_BROWSER_UI_VIEWS_INTENT_PICKER_BUBBLE_VIEW_H_
 #define CHROME_BROWSER_UI_VIEWS_INTENT_PICKER_BUBBLE_VIEW_H_
 
 #include <memory>
 #include <vector>
 
 #include "base/callback.h"
 #include "base/macros.h"
 #include "chrome/browser/chromeos/arc/arc_navigation_throttle.h"
 #include "content/public/browser/web_contents_observer.h"
 #include "ui/gfx/image/image.h"
 #include "ui/views/bubble/bubble_dialog_delegate.h"
 #include "ui/views/controls/button/button.h"
 
 namespace content {
 class NavigationHandle;
 }  // namespace content
 
 namespace views {
-class EventMonitor;
 class Label;
 class LabelButton;
 class View;
 class Widget;
 }  // namespace views
 
 namespace ui {
 class Event;
 }  // namespace ui
 
@@ skipping 21 matching lines @@
   // the reason why the Bubble was closed. The size_t param must have a value in
   // the range [0, app_info.size()-1], except when the CloseReason is ERROR or
   // DIALOG_DEACTIVATED, for these cases we return a dummy value
   // |kAppTagNoneSelected| which won't be used at all and has no significance.
   using ThrottleCallback =
       base::Callback<void(size_t, arc::ArcNavigationThrottle::CloseReason)>;
 
   static void ShowBubble(content::NavigationHandle* handle,
                          const std::vector<NameAndIcon>& app_info,
                          const ThrottleCallback& throttle_cb);
+  ~IntentPickerBubbleView() override;
 
  protected:
   // views::BubbleDialogDelegateView overrides:
   void Init() override;
 
  private:
+  friend class IntentPickerBubbleViewTest;

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

Another way to allow the test to access internals of this class is to add a
public function with _for_testing() or ForTesting() suffix.

 public:
  const std::vector<views::LabelButton*>& app_label_ptr_for_testing() const;

  // or
  size_t GetNumAppLablesForTesting() const;

This allows you to control what test files can see. If you call such a method in
non-test .cc file, trybot fails.

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

Done.

+  FRIEND_TEST_ALL_PREFIXES(IntentPickerBubbleViewTest, NullIcons);

[Yusuke Sato, 2016/07/20 08:05:54]

Or, if you go with friend, can you remove line 73-75? Adding one
FRIEND_TEST_ALL_PREFIXES per TEST_F seems too noisy to me.

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

Done.

+  FRIEND_TEST_ALL_PREFIXES(IntentPickerBubbleViewTest, NonNullIcons);
+  FRIEND_TEST_ALL_PREFIXES(IntentPickerBubbleViewTest, LabelsPtrVectorSize);
   IntentPickerBubbleView(const std::vector<NameAndIcon>& app_info,
                          ThrottleCallback throttle_cb,
-                         content::WebContents* web_contents);
-  ~IntentPickerBubbleView() override;
+                         content::WebContents* web_contents,
+                         bool was_callback_run);
 
   // views::BubbleDialogDelegateView overrides:
   void OnWidgetDestroying(views::Widget* widget) override;
   int GetDialogButtons() const override;
 
   // views::ButtonListener overrides:
   void ButtonPressed(views::Button* sender, const ui::Event& event) override;
 
   // views::View overrides:
   gfx::Size GetPreferredSize() const override;
 
   // content::WebContentsObserver overrides:
   void WebContentsDestroyed() override;
 
   // 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_;
 
   // Callback used to respond to ArcNavigationThrottle.
   ThrottleCallback throttle_cb_;
 
   // Keeps a invalid value unless the user explicitly makes a decision.
   size_t selected_app_tag_;
 
   views::LabelButton* always_button_;
   views::LabelButton* just_once_button_;
 
   std::vector<NameAndIcon> app_info_;
+  std::vector<views::LabelButton*> app_label_ptr_;

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

Is this really necessary? I'm not familiar with Views at all but ui/views/view.h
has set_id() and GetViewById().

Holding non-weak and non-owning pointers looks a bit scary to me.

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

Removed, opting for the GetViewByID() method you suggested, thanks!

 
   DISALLOW_COPY_AND_ASSIGN(IntentPickerBubbleView);
 };
 
 #endif  // CHROME_BROWSER_UI_VIEWS_INTENT_PICKER_BUBBLE_VIEW_H_