Reusing Ok/Cancel buttons for intent picker

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 e811528ccc8e59d82d0f8423022908a6e546e19e..b7f103f2aa9813fd5dcba51d4c62afc1eac6aab6 100644
--- a/chrome/browser/ui/views/intent_picker_bubble_view.h
+++ b/chrome/browser/ui/views/intent_picker_bubble_view.h
@@ -6,6 +6,7 @@
 #define CHROME_BROWSER_UI_VIEWS_INTENT_PICKER_BUBBLE_VIEW_H_
 
 #include <memory>
+#include <string>
 #include <vector>
 
 #include "base/callback.h"
@@ -31,6 +32,8 @@ namespace ui {
 class Event;
 }  // namespace ui
 
+class IntentPickerLabelButton;
+
 // A bubble that displays a list of aplications (icons and names), after the
 // list we show a pair of buttons which allow the user to remember the selection
 // or not. This class comunicates the user's selection with a callback used by
@@ -43,47 +46,60 @@ class Event;
 //   |  ...                           |
 //   | Icon(N) Name(N)                |
 //   |                                |
-//   |           [JUST ONCE] [ALWAYS] |
+//   |           [Just once] [Always] |
 //   +--------------------------------+
 
 class IntentPickerBubbleView : public views::BubbleDialogDelegateView,
                                public views::ButtonListener,
                                public content::WebContentsObserver {
  public:
-  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
-  // 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)>;
+  using AppInfo = arc::ArcNavigationThrottle::AppInfo;
+  // This callback informs the package name of the app selected by the user,
+  // along with the reason why the Bubble was closed. The string param must have
+  // a valid package name, except when the CloseReason is ERROR or
+  // DIALOG_DEACTIVATED, for these cases we return a dummy value which won't be
+  // used at all and has no significance.
+  using IntentPickerResponse =

[sky, 2016/10/13 13:35:07]

How about moving this using to the browser_dialogs so that it's clear they are
the same?

[djacobo_, 2016/10/14 02:35:32]

Done.

+      base::Callback<void(std::string,
+                          arc::ArcNavigationThrottle::CloseReason)>;
 
   ~IntentPickerBubbleView() override;
   static void ShowBubble(content::WebContents* web_contents,
-                         const std::vector<NameAndIcon>& app_info,
-                         const ThrottleCallback& throttle_cb);
+                         const std::vector<AppInfo>& app_info,
+                         const IntentPickerResponse& intent_picker_cb);
   static std::unique_ptr<IntentPickerBubbleView> CreateBubbleView(
-      const std::vector<NameAndIcon>& app_info,
-      const ThrottleCallback& throttle_cb,
+      const std::vector<AppInfo>& app_info,
+      const IntentPickerResponse& intent_picker_cb,
       content::WebContents* web_contents);
 
+  // views::DialogDelegate overrides:
+  bool Accept() override;
+  bool Cancel() override;
+  bool Close() override;
+  int GetDefaultDialogButton() const override;
+
  protected:
   // views::BubbleDialogDelegateView overrides:
   void Init() override;
 
+  // views::WidgetDelegate overrides:

[sky, 2016/10/13 13:35:07]

optional: you subclass BubbleDialogDelegateView. I would group the
DialogDelegate, WidgetDelegate and BubbleDialogDelegateView overrides there as
that is where you get them from.

[djacobo_, 2016/10/14 02:35:32]

Done.

+  base::string16 GetWindowTitle() const override;
+  base::string16 GetDialogButtonLabel(ui::DialogButton button) const override;
+
  private:
   friend class IntentPickerBubbleViewTest;
   FRIEND_TEST_ALL_PREFIXES(IntentPickerBubbleViewTest, NullIcons);
   FRIEND_TEST_ALL_PREFIXES(IntentPickerBubbleViewTest, NonNullIcons);
   FRIEND_TEST_ALL_PREFIXES(IntentPickerBubbleViewTest, LabelsPtrVectorSize);
-  IntentPickerBubbleView(const std::vector<NameAndIcon>& app_info,
-                         ThrottleCallback throttle_cb,
+  FRIEND_TEST_ALL_PREFIXES(IntentPickerBubbleViewTest, VerifyStartingInkDrop);
+  FRIEND_TEST_ALL_PREFIXES(IntentPickerBubbleViewTest, InkDropStateTransition);
+  FRIEND_TEST_ALL_PREFIXES(IntentPickerBubbleViewTest, PressButtonTwice);
+  IntentPickerBubbleView(const std::vector<AppInfo>& app_info,
+                         IntentPickerResponse intent_picker_cb,
                          content::WebContents* web_contents);
 
   // 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;
@@ -94,26 +110,23 @@ class IntentPickerBubbleView : public views::BubbleDialogDelegateView,
   // content::WebContentsObserver overrides:
   void WebContentsDestroyed() override;
 
-  // Retrieves the LabelButton* contained at position |index| from the internal
-  // ScrollView.
-  views::LabelButton* GetLabelButtonAt(size_t index);
-  void SetLabelButtonBackgroundColor(size_t index, SkColor color);
+  // Retrieves the IntentPickerLabelButton* contained at position |index| from
+  // the internal ScrollView.
+  IntentPickerLabelButton* GetIntentPickerLabelButtonAt(size_t index);
 
-  // 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_;
+  gfx::ImageSkia GetAppImageForTesting(size_t index);
+  views::InkDropState GetInkDropStateForTesting(size_t);
+  void PressButtonForTesting(size_t index, const ui::Event& event);
 
   // Callback used to respond to ArcNavigationThrottle.
-  ThrottleCallback throttle_cb_;
+  IntentPickerResponse intent_picker_cb_;
 
-  // Keeps a invalid value unless the user explicitly makes a decision.
+  // 0 by default, meaning we pre-select the first app on the list.
   size_t selected_app_tag_;

[sky, 2016/10/13 13:35:07]

optional: = 0 means you don't need to say '0 by default'.

[djacobo_, 2016/10/14 02:35:32]

Done.

 
-  views::LabelButton* always_button_;
-  views::LabelButton* just_once_button_;
   views::ScrollView* scroll_view_;
 
-  std::vector<NameAndIcon> app_info_;
+  std::vector<AppInfo> app_info_;
 
   DISALLOW_COPY_AND_ASSIGN(IntentPickerBubbleView);
 };