Autofill: Replace "save credit card" infobar with a bubble (Views only).

chrome/browser/ui/views/autofill/save_card_bubble_view.h

+// Copyright 2015 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_AUTOFILL_SAVE_CARD_BUBBLE_VIEW_H_
+#define CHROME_BROWSER_UI_VIEWS_AUTOFILL_SAVE_CARD_BUBBLE_VIEW_H_
+
+#include "chrome/browser/ui/views/managed_full_screen_bubble_delegate_view.h"
+#include "ui/views/controls/button/button.h"
+
+namespace content {
+class WebContents;
+}
+
+namespace views {
+class LabelButton;
+}
+
+namespace autofill {
+
+class SaveCardBubbleController;
+
+// This class displays the "Save credit card?" bubble.
+class SaveCardBubbleView : public ManagedFullScreenBubbleDelegateView,

[Evan Stade, 2015/10/09 18:54:44]

huh?

[bondd, 2015/10/13 02:03:16]

Are you wondering why I'm using ManagedFullScreenBubbleDelegateView? If so, it's
because the bubbles that act most like what I'm trying to achieve seem to be
Translate and ManagePasswords. They both use
ManagedFullScreenBubbleDelegateView.

[Evan Stade, 2015/10/13 02:21:55]

yes, i was wondering that. It seems like the right thing to do, but I also think
that's a terrifically poor name for that class. How about renaming it
LocationBarBubbleDelegateView?

[bondd, 2015/10/15 22:27:07]

If it's okay with you I'd like to land this CL first, and then file a bug and do
standalone CL afterward to rename ManagedFullScreenBubbleDelegateView ->
LocationBarBubbleDelegateView.

+                           public views::ButtonListener {
+ public:
+  ~SaveCardBubbleView() override;
+
+  // Shows the "Save credit card?" bubble anchored to |anchor_view|.
+  static void ShowBubble(views::View* anchor_view,
+                         content::WebContents* web_contents,
+                         SaveCardBubbleController* controller);
+
+  // Closes the current bubble if it exists.
+  static void CloseBubble();
+
+  // Returns the bubble view currently shown. This may return NULL.

[Evan Stade, 2015/10/09 18:54:44]

nullptr

[bondd, 2015/10/15 22:27:07]

Done. GetCurrentBubble() no longer exists.

+  static SaveCardBubbleView* GetCurrentBubble();
+
+  // views::WidgetDelegate
+  views::View* GetInitiallyFocusedView() override;
+  base::string16 GetWindowTitle() const override;
+  bool ShouldShowWindowTitle() const override;
+  void WindowClosing() override;
+
+  // views::ButtonListener
+  void ButtonPressed(views::Button* sender, const ui::Event& event) override;
+
+ private:
+  SaveCardBubbleView(views::View* anchor_view,
+                     content::WebContents* web_contents,
+                     SaveCardBubbleController* controller);
+
+  // views::BubbleDelegateView
+  void Init() override;
+
+  static SaveCardBubbleView* save_card_bubble_view_;
+
+  // Controller must outlive this object.
+  SaveCardBubbleController* controller_;

[bondd, 2015/10/09 01:36:02]

Should I make this a WeakPtr?

[Evan Stade, 2015/10/09 18:54:44]

not if it must outlive |this|. Only make it a WeakPtr if it can die first.

can the location bar or the bubble view own the controller? Then you don't need
these pass throughs in ChromeAutofillClient. Why does ChromeAutofillClient own
the controller in the first place?

[bondd, 2015/10/13 02:03:16]

The bubbles that seem closest to what I'm trying to achieve are Translate and
ManagePasswords. ManagePasswordsUIController and ChromeTranslateClient both
inherit from WebContentsUserData, so they are attached and scoped to a
WebContents. The views for both of those are singletons: see static variables
ManagePasswordsBubbleView::manage_passwords_bubble_ and
TranslateBubbleView::translate_bubble_view_.

It makes sense to me that I could use the same architecture as Translate and
ManagePasswords for the save card bubble. SaveCardBubbleController would inherit
from WebContentsUserData and no longer be owned by ChromeAutofillClient, and
SaveCardBubbleView would remain a singleton.

Is this solution acceptable to you?

[Evan Stade, 2015/10/13 02:21:55]

I'm wary of agreeing to this just on the basis of precedent (for all we know
ManagePasswords just copied Translate even though it wasn't the best thing to
do). This comment is pretty confusing:

  // Singleton instance of the Password bubble. The Password bubble can only be
  // shown on the active browser window, so there is no case in which it will be
  // shown twice at the same time. The instance is owned by the Bubble and will
  // be deleted when the bubble closes.

"The instance is owned by the Bubble" doesn't make any sense to me. What it
means to say is that the bubble view owns itself.

I'm happy with the controller being a webcontentsuserdata. I don't like using a
singleton for no particular reason. I like this quote from wikipedia: "There are
some who are critical of the singleton pattern and consider it to be an
anti-pattern in that it is frequently used in scenarios where it is not
beneficial, introduces unnecessary restrictions in situations where a sole
instance of a class is not actually required, and introduces global state into
an application." I think you can/should have the SavedCardBubbleController own
the view (the view can be deleted and recreated for each showing). Then this
|controller_| is indeed guaranteed to outlive |this|.

[bondd, 2015/10/15 22:27:07]

I made the controller a WebContentsUserData.
I made the view's lifecycle follow the CardUnmaskPromptView example: view is
responsible for its own lifetime, and has a ControllerGone() member function so
that view is guaranteed to go away when controller does and never accesses a
stale controller pointer.

+
+  // Button for the user to confirm saving the credit card info.
+  views::LabelButton* save_button_;
+
+  views::LabelButton* cancel_button_;
+
+  DISALLOW_COPY_AND_ASSIGN(SaveCardBubbleView);
+};
+
+}  // namespace autofill
+
+#endif  // CHROME_BROWSER_UI_VIEWS_AUTOFILL_SAVE_CARD_BUBBLE_VIEW_H_