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

chrome/browser/ui/autofill/save_card_bubble_controller_impl.cc

+// 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.
+
+#include "chrome/browser/ui/autofill/save_card_bubble_controller_impl.h"
+
+#include "chrome/browser/ui/browser.h"
+#include "chrome/browser/ui/browser_finder.h"
+#include "chrome/browser/ui/browser_window.h"
+#include "chrome/browser/ui/location_bar/location_bar.h"
+#include "components/autofill/core/browser/ui/save_card_bubble_view.h"
+#include "content/public/browser/navigation_details.h"
+
+DEFINE_WEB_CONTENTS_USER_DATA_KEY(autofill::SaveCardBubbleControllerImpl);
+
+namespace {
+
+// Minimal time span the bubble should survive implicit navigations.
+// TODO(bondd): Share with ManagePasswordsUIController.
+const int kBubbleMinTime = 5;
+
+}  // namespace
+
+namespace autofill {
+
+SaveCardBubbleControllerImpl::SaveCardBubbleControllerImpl(
+    content::WebContents* web_contents)
+    : content::WebContentsObserver(web_contents),
+      web_contents_(web_contents),
+      save_card_bubble_view_(nullptr),
+      state_(INACTIVE_STATE) {
+  DCHECK(web_contents);
+}
+
+SaveCardBubbleControllerImpl::~SaveCardBubbleControllerImpl() {
+  if (save_card_bubble_view_)
+    save_card_bubble_view_->ControllerGone();
+}
+
+void SaveCardBubbleControllerImpl::CreateBubble(

[Evan Stade, 2015/10/16 01:20:44]

CreateAndShow, and Show below, since you should never call Create without an
immediate Show following it.

also I think Create is pretty deceptive because you don't actually create the
bubble view here. Maybe

ShowBubble(callback);
ReshowBubble();

[Justin Donnelly, 2015/10/19 17:39:08]

At the risk of going full bikeshed, I think you're right to question that this
doesn't create so that may be a poor name choice but I'd argue against
CreateAndShow. "And" in a method name always troubles me. I'd prefer each method
do one thing and one thing only where possible. There's nothing wrong with
making a caller create or otherwise setup an object and then invoke behavior
separately. This happens all the time and can often be useful.

I'd argue for SetCallback as the representation of what this method is actually
doing and leave the caller as-is (calling both this method and then ShowBubble).

I'm having a hard time convincing myself that this is very important, however. I
think what I'd really like to see is some guidelines established as to what the
setup and control of bubbles would look like in general. They probably can't all
conform to an exact interface given the varying circumstances of their creation.
But they could at least look similar enough that readers would find each of them
familiar.

Hector, is this something you've thought about?

[Evan Stade, 2015/10/19 17:58:31]

Sure, in fact my final suggestion did not include And, so we're in agreement.
Whether it's useful elsewhere or not, I can't see how it's useful here.
Sounds fine to me. Calling it SetCallback resolves the ambiguity of create vs
show and how or why you'd try to do one without the other.

[bondd, 2015/10/22 02:15:17]

Done.

+    const base::Closure& save_card_callback) {
+  save_card_callback_ = save_card_callback;
+  state_ = BUBBLE_AVAILABLE_STATE;
+}
+
+void SaveCardBubbleControllerImpl::ShowBubble() {
+  DCHECK_EQ(state_, BUBBLE_AVAILABLE_STATE);
+
+  // Need to create location bar icon before bubble, otherwise bubble will be

[hcarmona, 2015/10/16 21:16:35]

What happens if the location bar isn't visible? Like in fullscreen.
Does this unanchored bubble need to have an arrow?

[bondd, 2015/10/22 02:15:17]

In fullscreen the arrow points up toward where the icon would be if it was
visible. I assume the Translate bubble works the same way, as they both inherit
from ManagedFullScreenBubbleDelegateView (as Evan mentioned before, a terribly
named class).

+  // unanchored.
+  state_ = BUBBLE_VISIBLE_STATE;
+  UpdateIcon();
+
+  Browser* browser = chrome::FindBrowserWithWebContents(web_contents_);
+  save_card_bubble_view_ =
+      browser->window()->ShowSaveCreditCardBubble(web_contents_, this);
+  DCHECK(save_card_bubble_view_);
+
+  timer_.reset(new base::ElapsedTimer());
+}
+
+void SaveCardBubbleControllerImpl::OnSaveButton() {
+  save_card_callback_.Run();
+  Deactivate();
+}
+
+void SaveCardBubbleControllerImpl::OnCancelButton() {
+  Deactivate();
+}
+
+void SaveCardBubbleControllerImpl::OnBubbleClosed() {
+  // Do nothing if bubble has already been closed because of a button press.
+  if (state_ == INACTIVE_STATE)
+    return;
+
+  state_ = BUBBLE_AVAILABLE_STATE;
+  save_card_bubble_view_ = nullptr;
+  UpdateIcon();
+}
+
+bool SaveCardBubbleControllerImpl::IsIconVisible() const {
+  return state_ != INACTIVE_STATE;
+}
+
+bool SaveCardBubbleControllerImpl::IsIconToggled() const {
+  return state_ == BUBBLE_VISIBLE_STATE;
+}
+
+SaveCardBubbleView* SaveCardBubbleControllerImpl::save_card_bubble_view()
+    const {
+  return save_card_bubble_view_;
+}
+
+void SaveCardBubbleControllerImpl::UpdateIcon() {
+  Browser* browser = chrome::FindBrowserWithWebContents(web_contents_);
+  LocationBar* location_bar = browser->window()->GetLocationBar();
+  DCHECK(location_bar);
+  location_bar->UpdateSaveCreditCardIcon();
+}
+
+void SaveCardBubbleControllerImpl::Deactivate() {
+  save_card_callback_.Reset();
+  state_ = INACTIVE_STATE;
+  save_card_bubble_view_ = nullptr;
+  UpdateIcon();
+}
+
+base::TimeDelta SaveCardBubbleControllerImpl::Elapsed() const {
+  return timer_ ? timer_->Elapsed() : base::TimeDelta::Max();

[Evan Stade, 2015/10/16 01:20:44]

not a particularly useful method IMO. I expect you copy-pasted this from another
bubble and it only exists in another bubble for testing purposes.

[bondd, 2015/10/22 02:15:17]

Done.

+}
+
+void SaveCardBubbleControllerImpl::DidNavigateMainFrame(
+    const content::LoadCommittedDetails& details,
+    const content::FrameNavigateParams& params) {
+  if (state_ == INACTIVE_STATE)
+    return;
+
+  // Don't react to in-page (fragment) navigations.
+  if (details.is_in_page)
+    return;
+
+  // Don't do anything if a navigation occurs before a user could reasonably
+  // interact with the bubble.
+  if (Elapsed() < base::TimeDelta::FromSeconds(kBubbleMinTime))

[Evan Stade, 2015/10/16 01:20:43]

can just be

if (timer_ && timer_->Elapsed() < ...)

(also do you really need to check timer_?)

[bondd, 2015/10/22 02:15:17]

Done.

+    return;
+
+  // Otherwise, make the bubble and icon inactive.
+  if (save_card_bubble_view_)
+    save_card_bubble_view_->Close();
+  Deactivate();
+}
+
+}  // namespace autofill