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

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

Views "Save credit card" infobar is replaced with a bubble and Omnibox
icon.

Video of new bubble and icon are attached to bug: https://code.google.com/p/chromium/issues/detail?id=535784#c27

BUG=535784
Committed: https://crrev.com/052b5f8112cf52524b92e92c504206eee9723bf9
Cr-Commit-Position: refs/heads/master@{#356673}

[bondd, 2015-10-09 01:36:01]

bondd@chromium.org changed reviewers:
+ estade@chromium.org, jdonnelly@chromium.org

[bondd, 2015-10-09 01:36:01]

bondd@chromium.org changed required reviewers:
+ estade@chromium.org

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

Hi estade@ - I've uploaded this for discussion. It's not done yet but I'd like
to get your take on it before I go any further.

I'm mostly looking for input about big structural changes at this point. For
example, is there a better way to structure any of this? Is something missing?
Have I added a bunch of stuff that is completely unnecessary?

That said, I'll gladly accept any and all feedback. :)

It's structured differently than the card unmask prompt because the bubble needs
to be anchored to the Omnibox icon, and be shown/hidden by clicking the icon.

Mocks of the more complicated bubble layouts that will come later are attached
to bug. Video of bubble and icon in action is also attached to the bug.

I haven't done much yet to exclude the new code on non-Views platforms. I don't
even know if it compiles there.

https://codereview.chromium.org/1396923003/diff/1/chrome/browser/ui/autofill/...
File chrome/browser/ui/autofill/save_card_bubble_controller.cc (right):

https://codereview.chromium.org/1396923003/diff/1/chrome/browser/ui/autofill/...
chrome/browser/ui/autofill/save_card_bubble_controller.cc:26:
web_contents_(web_contents),
Is it okay to save |web_contents| off like this for the life of the tab? Do I
ever need to check if it's gone away or become invalid?

https://codereview.chromium.org/1396923003/diff/1/chrome/browser/ui/autofill/...
chrome/browser/ui/autofill/save_card_bubble_controller.cc:97: void
SaveCardBubbleController::DidNavigateMainFrame(
This function is based on the one in ManagePasswordsUIController.

https://codereview.chromium.org/1396923003/diff/1/chrome/browser/ui/views/aut...
File chrome/browser/ui/views/autofill/save_card_bubble_view.h (right):

https://codereview.chromium.org/1396923003/diff/1/chrome/browser/ui/views/aut...
chrome/browser/ui/views/autofill/save_card_bubble_view.h:60:
SaveCardBubbleController* controller_;
Should I make this a WeakPtr?

[Evan Stade, 2015-10-09 18:54:45]

https://codereview.chromium.org/1396923003/diff/1/chrome/browser/ui/autofill/...
File chrome/browser/ui/autofill/save_card_bubble_controller.h (right):

https://codereview.chromium.org/1396923003/diff/1/chrome/browser/ui/autofill/...
chrome/browser/ui/autofill/save_card_bubble_controller.h:32: void
OnSaveButton();
I like separating these out into another interface. So there's

// interface
class SaveCardBubbleController ...

and

// impl
class SaveCardBubbleControllerImpl : public SaveCardBubbleController, public
content::WebContentsObserver ...

and the view only sees SaveCardBubbleController

https://codereview.chromium.org/1396923003/diff/1/chrome/browser/ui/views/aut...
File chrome/browser/ui/views/autofill/save_card_bubble_view.h (right):

https://codereview.chromium.org/1396923003/diff/1/chrome/browser/ui/views/aut...
chrome/browser/ui/views/autofill/save_card_bubble_view.h:24: class
SaveCardBubbleView : public ManagedFullScreenBubbleDelegateView,
> ManagedFullScreenBubbleDelegateView

huh?

https://codereview.chromium.org/1396923003/diff/1/chrome/browser/ui/views/aut...
chrome/browser/ui/views/autofill/save_card_bubble_view.h:37: // Returns the
bubble view currently shown. This may return NULL.
nullptr

https://codereview.chromium.org/1396923003/diff/1/chrome/browser/ui/views/aut...
chrome/browser/ui/views/autofill/save_card_bubble_view.h:60:
SaveCardBubbleController* controller_;
On 2015/10/09 01:36:02, bondd wrote:
> Should I make this a WeakPtr?

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?

https://codereview.chromium.org/1396923003/diff/1/chrome/browser/ui/views/loc...
File chrome/browser/ui/views/location_bar/save_credit_card_icon_view.cc (right):

https://codereview.chromium.org/1396923003/diff/1/chrome/browser/ui/views/loc...
chrome/browser/ui/views/location_bar/save_credit_card_icon_view.cc:27:
SetImage(gfx::CreateVectorIcon(gfx::VectorIconId::AUTOFILL, 16,
this is slightly out of date now (after a recent change of mine)

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

Thanks Evan. Responded to a few of your questions.

https://codereview.chromium.org/1396923003/diff/1/chrome/browser/ui/autofill/...
File chrome/browser/ui/autofill/save_card_bubble_controller.h (right):

https://codereview.chromium.org/1396923003/diff/1/chrome/browser/ui/autofill/...
chrome/browser/ui/autofill/save_card_bubble_controller.h:32: void
OnSaveButton();
On 2015/10/09 18:54:44, Evan Stade wrote:
> I like separating these out into another interface. So there's
> 
> // interface
> class SaveCardBubbleController ...
> 
> and
> 
> // impl
> class SaveCardBubbleControllerImpl : public SaveCardBubbleController, public
> content::WebContentsObserver ...
> 
> and the view only sees SaveCardBubbleController

Sounds good. I'll do that.

https://codereview.chromium.org/1396923003/diff/1/chrome/browser/ui/views/aut...
File chrome/browser/ui/views/autofill/save_card_bubble_view.h (right):

https://codereview.chromium.org/1396923003/diff/1/chrome/browser/ui/views/aut...
chrome/browser/ui/views/autofill/save_card_bubble_view.h:24: class
SaveCardBubbleView : public ManagedFullScreenBubbleDelegateView,
On 2015/10/09 18:54:44, Evan Stade wrote:
> > ManagedFullScreenBubbleDelegateView
> 
> huh?

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.

https://codereview.chromium.org/1396923003/diff/1/chrome/browser/ui/views/aut...
chrome/browser/ui/views/autofill/save_card_bubble_view.h:60:
SaveCardBubbleController* controller_;
On 2015/10/09 18:54:44, Evan Stade wrote:
> On 2015/10/09 01:36:02, bondd wrote:
> > Should I make this a WeakPtr?
> 
> 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?

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]

https://codereview.chromium.org/1396923003/diff/1/chrome/browser/ui/views/aut...
File chrome/browser/ui/views/autofill/save_card_bubble_view.h (right):

https://codereview.chromium.org/1396923003/diff/1/chrome/browser/ui/views/aut...
chrome/browser/ui/views/autofill/save_card_bubble_view.h:24: class
SaveCardBubbleView : public ManagedFullScreenBubbleDelegateView,
On 2015/10/13 02:03:16, bondd wrote:
> On 2015/10/09 18:54:44, Evan Stade wrote:
> > > ManagedFullScreenBubbleDelegateView
> > 
> > huh?
> 
> 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.

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?

https://codereview.chromium.org/1396923003/diff/1/chrome/browser/ui/views/aut...
chrome/browser/ui/views/autofill/save_card_bubble_view.h:60:
SaveCardBubbleController* controller_;
On 2015/10/13 02:03:16, bondd wrote:
> On 2015/10/09 18:54:44, Evan Stade wrote:
> > On 2015/10/09 01:36:02, bondd wrote:
> > > Should I make this a WeakPtr?
> > 
> > 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?
> 
> 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?

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|.

[Justin Donnelly, 2015-10-13 16:25:53]

Looks pretty good for the most part. I have a few little requests below but the
main issue is (as you and Evan discussed) the design of the lifecycle of the
controller and view.

Also, have you shown this to Hector? I haven't yet figured out how the bubble
manager is meant to fit into these various interactions between the browser
view, location bar view and bubble views but it'd be good to confirm that what
you're doing here will fit into their long-term plans. Or at least understand
what their plan is if they are changing the way this works.

https://codereview.chromium.org/1396923003/diff/1/chrome/browser/ui/autofill/...
File chrome/browser/ui/autofill/save_card_bubble_controller.cc (right):

https://codereview.chromium.org/1396923003/diff/1/chrome/browser/ui/autofill/...
chrome/browser/ui/autofill/save_card_bubble_controller.cc:37: ReshowBubble();
This seems a little awkward. This API feels more like this method should be
called CreateBubble() and ReshowBubble() should be named ShowBubble(). Assuming
you follow Evan's suggestion to have the controller create and own the view,
this is what you'd be doing anyhow (I think).

https://codereview.chromium.org/1396923003/diff/1/chrome/browser/ui/autofill/...
chrome/browser/ui/autofill/save_card_bubble_controller.cc:97: void
SaveCardBubbleController::DidNavigateMainFrame(
On 2015/10/09 01:36:02, bondd wrote:
> This function is based on the one in ManagePasswordsUIController.

Is there a motivation for not resetting the timer at the end like
ManagePasswordsUIController does?

https://codereview.chromium.org/1396923003/diff/1/chrome/browser/ui/views/loc...
File chrome/browser/ui/views/location_bar/location_bar_view.cc (right):

https://codereview.chromium.org/1396923003/diff/1/chrome/browser/ui/views/loc...
chrome/browser/ui/views/location_bar/location_bar_view.cc:998: bool enabled =
autofill_client->WantSaveCreditCardIconVisible();
I'm not in love with the names of these Want methods. It seems to me that it
puts us in a state where neither side of this relationship really knows what
that means. The location bar just knows that there's some undefined desire while
ChromeAutofillClient doesn't have a good grasp on what toggled means. (It's
clear in the code here but when I first read ChromeAutofillClient it seemed like
a poor naming choice.)

I'd prefer the same approach used by the translate icon code below, which deals
in concrete states. In other words, IsBubbleActive() and IsBubbleVisible(). From
ChromeAutofillClient's point of view it makes sense that other views would be
interested in these states. From here, it's clear that we want the icon visible
when the bubble is available and the icon on ("toggled") when the bubble is
visible.

https://codereview.chromium.org/1396923003/diff/1/chrome/browser/ui/views/loc...
chrome/browser/ui/views/location_bar/location_bar_view.cc:1005:
autofill::SaveCardBubbleView::CloseBubble();
AutofillClient already has the ShowSaveCreditCardBubble, how about adding a
CloseSaveCreditCardBubble to go with it and then there's one less interface this
class needs to know about.

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

bondd@chromium.org changed reviewers:
+ hcarmona@chromium.org

[bondd, 2015-10-15 22:27:08]

I split the controller and the view into interface and implementation classes,
and changed their ownership and lifecycle management.

+hcarmona@: Does this look like it will have any issues integrating with the new
Bubble API? From what I saw the Bubble API is pretty flexible, but it would be
good to get your take on this.

Left to do in this CL:
- Make sure it works on non-Views platforms. ifdef out unused code.

File bugs for later CLs:
- Add "Learn more" link to bubble.
- Rename ManagedFullScreenBubbleDelegateView -> LocationBarBubbleDelegateView
- Use updated location bar icon.
- Use Bubble API.

https://codereview.chromium.org/1396923003/diff/1/chrome/browser/ui/autofill/...
File chrome/browser/ui/autofill/save_card_bubble_controller.cc (right):

https://codereview.chromium.org/1396923003/diff/1/chrome/browser/ui/autofill/...
chrome/browser/ui/autofill/save_card_bubble_controller.cc:37: ReshowBubble();
On 2015/10/13 16:25:53, Justin Donnelly wrote:
> This seems a little awkward. This API feels more like this method should be
> called CreateBubble() and ReshowBubble() should be named ShowBubble().
Assuming
> you follow Evan's suggestion to have the controller create and own the view,
> this is what you'd be doing anyhow (I think).

Done. I flip-flopped a couple of times between these exact two combos,
ShowBubble+ReshowBubble vs CreateBubble+ShowBubble. Thanks for casting the
deciding vote!

https://codereview.chromium.org/1396923003/diff/1/chrome/browser/ui/autofill/...
chrome/browser/ui/autofill/save_card_bubble_controller.cc:97: void
SaveCardBubbleController::DidNavigateMainFrame(
On 2015/10/13 16:25:53, Justin Donnelly wrote:
> On 2015/10/09 01:36:02, bondd wrote:
> > This function is based on the one in ManagePasswordsUIController.
> 
> Is there a motivation for not resetting the timer at the end like
> ManagePasswordsUIController does? 

Yes, I considered resetting it at the end like ManagePasswordsUIController does,
but I can't figure out what problem that is solving. Perhaps it is solving
something that is an issue for passwords but not for SaveCardBubble, or maybe I
am missing something. AFAICT it doesn't add anything good to SaveCardBubble, and
SaveCardBubble works fine without it, so I left it out.

https://codereview.chromium.org/1396923003/diff/1/chrome/browser/ui/views/aut...
File chrome/browser/ui/views/autofill/save_card_bubble_view.h (right):

https://codereview.chromium.org/1396923003/diff/1/chrome/browser/ui/views/aut...
chrome/browser/ui/views/autofill/save_card_bubble_view.h:24: class
SaveCardBubbleView : public ManagedFullScreenBubbleDelegateView,
On 2015/10/13 02:21:55, Evan Stade wrote:
> On 2015/10/13 02:03:16, bondd wrote:
> > On 2015/10/09 18:54:44, Evan Stade wrote:
> > > > ManagedFullScreenBubbleDelegateView
> > > 
> > > huh?
> > 
> > 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.
> 
> 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?

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.

https://codereview.chromium.org/1396923003/diff/1/chrome/browser/ui/views/aut...
chrome/browser/ui/views/autofill/save_card_bubble_view.h:37: // Returns the
bubble view currently shown. This may return NULL.
On 2015/10/09 18:54:44, Evan Stade wrote:
> nullptr

Done. GetCurrentBubble() no longer exists.

https://codereview.chromium.org/1396923003/diff/1/chrome/browser/ui/views/aut...
chrome/browser/ui/views/autofill/save_card_bubble_view.h:60:
SaveCardBubbleController* controller_;
On 2015/10/13 02:21:55, Evan Stade wrote:
> On 2015/10/13 02:03:16, bondd wrote:
> > On 2015/10/09 18:54:44, Evan Stade wrote:
> > > On 2015/10/09 01:36:02, bondd wrote:
> > > > Should I make this a WeakPtr?
> > > 
> > > 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?
> > 
> > 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?
> 
> 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|.

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.

https://codereview.chromium.org/1396923003/diff/1/chrome/browser/ui/views/loc...
File chrome/browser/ui/views/location_bar/location_bar_view.cc (right):

https://codereview.chromium.org/1396923003/diff/1/chrome/browser/ui/views/loc...
chrome/browser/ui/views/location_bar/location_bar_view.cc:998: bool enabled =
autofill_client->WantSaveCreditCardIconVisible();
On 2015/10/13 16:25:53, Justin Donnelly wrote:
> I'm not in love with the names of these Want methods. It seems to me that it
> puts us in a state where neither side of this relationship really knows what
> that means. The location bar just knows that there's some undefined desire
while
> ChromeAutofillClient doesn't have a good grasp on what toggled means. (It's
> clear in the code here but when I first read ChromeAutofillClient it seemed
like
> a poor naming choice.)
> 
> I'd prefer the same approach used by the translate icon code below, which
deals
> in concrete states. In other words, IsBubbleActive() and IsBubbleVisible().
From
> ChromeAutofillClient's point of view it makes sense that other views would be
> interested in these states. From here, it's clear that we want the icon
visible
> when the bubble is available and the icon on ("toggled") when the bubble is
> visible.

Done. Thanks. This is another case where I was going around and around and
figured I could really use someone else's opinion.

https://codereview.chromium.org/1396923003/diff/1/chrome/browser/ui/views/loc...
chrome/browser/ui/views/location_bar/location_bar_view.cc:1005:
autofill::SaveCardBubbleView::CloseBubble();
On 2015/10/13 16:25:53, Justin Donnelly wrote:
> AutofillClient already has the ShowSaveCreditCardBubble, how about adding a
> CloseSaveCreditCardBubble to go with it and then there's one less interface
this
> class needs to know about.

Moved this Close() call to the controller (in DidNavigateMainFrame()), so
doesn't need to be done here anymore.

https://codereview.chromium.org/1396923003/diff/1/chrome/browser/ui/views/loc...
File chrome/browser/ui/views/location_bar/save_credit_card_icon_view.cc (right):

https://codereview.chromium.org/1396923003/diff/1/chrome/browser/ui/views/loc...
chrome/browser/ui/views/location_bar/save_credit_card_icon_view.cc:27:
SetImage(gfx::CreateVectorIcon(gfx::VectorIconId::AUTOFILL, 16,
On 2015/10/09 18:54:45, Evan Stade wrote:
> this is slightly out of date now (after a recent change of mine)

Acknowledged.

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

https://codereview.chromium.org/1396923003/diff/20001/chrome/browser/ui/autof...
File chrome/browser/ui/autofill/save_card_bubble_controller_impl.cc (right):

https://codereview.chromium.org/1396923003/diff/20001/chrome/browser/ui/autof...
chrome/browser/ui/autofill/save_card_bubble_controller_impl.cc:40: void
SaveCardBubbleControllerImpl::CreateBubble(
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();

https://codereview.chromium.org/1396923003/diff/20001/chrome/browser/ui/autof...
chrome/browser/ui/autofill/save_card_bubble_controller_impl.cc:109: return
timer_ ? timer_->Elapsed() : base::TimeDelta::Max();
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.

https://codereview.chromium.org/1396923003/diff/20001/chrome/browser/ui/autof...
chrome/browser/ui/autofill/save_card_bubble_controller_impl.cc:124: if
(Elapsed() < base::TimeDelta::FromSeconds(kBubbleMinTime))
can just be

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

(also do you really need to check timer_?)

https://codereview.chromium.org/1396923003/diff/20001/chrome/browser/ui/autof...
File chrome/browser/ui/autofill/save_card_bubble_controller_impl.h (right):

https://codereview.chromium.org/1396923003/diff/20001/chrome/browser/ui/autof...
chrome/browser/ui/autofill/save_card_bubble_controller_impl.h:85: State state_;
probably don't need this var, can just check other members to divine the state
(e.g. save_card_bubble_view_ == nullptr)

https://codereview.chromium.org/1396923003/diff/20001/chrome/browser/ui/autof...
chrome/browser/ui/autofill/save_card_bubble_controller_impl.h:85: State state_;
instead of having this redundant information, check state of other member vars
to divine the status of things. e.g. instead of checking state is
AVAILABLE_STATE, check !save_card_callback_.is_null(). Instead of checking
INACTIVE, ask save_card_bubble_view_ == nullptr?

https://codereview.chromium.org/1396923003/diff/20001/chrome/browser/ui/brows...
File chrome/browser/ui/browser_window.h (right):

https://codereview.chromium.org/1396923003/diff/20001/chrome/browser/ui/brows...
chrome/browser/ui/browser_window.h:254: virtual autofill::SaveCardBubbleView*
ShowSaveCreditCardBubble(
need to add this to cocoa (and maybe a test/mock class or two)

https://codereview.chromium.org/1396923003/diff/20001/chrome/browser/ui/views...
File chrome/browser/ui/views/autofill/save_card_bubble_views.h (right):

https://codereview.chromium.org/1396923003/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/autofill/save_card_bubble_views.h:33:
~SaveCardBubbleViews() override;
make private

https://codereview.chromium.org/1396923003/diff/20001/chrome/browser/ui/views...
File chrome/browser/ui/views/location_bar/location_bar_view.cc (right):

https://codereview.chromium.org/1396923003/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/location_bar/location_bar_view.cc:994: return false;
\n

https://codereview.chromium.org/1396923003/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/location_bar/location_bar_view.cc:1004:
save_credit_card_icon_view_->SetToggled(controller->IsIconToggled());
\n

https://codereview.chromium.org/1396923003/diff/20001/components/autofill/cor...
File components/autofill/core/browser/ui/save_card_bubble_controller.h (right):

https://codereview.chromium.org/1396923003/diff/20001/components/autofill/cor...
components/autofill/core/browser/ui/save_card_bubble_controller.h:6: #define
COMPONENTS_AUTOFILL_CORE_BROWSER_UI_SAVE_CARD_BUBBLE_CONTROLLER_H_
I don't think this belongs in components as it won't be used by ios

https://codereview.chromium.org/1396923003/diff/20001/components/autofill/cor...
components/autofill/core/browser/ui/save_card_bubble_controller.h:16: class
SaveCardBubbleController {
only the methods visible/used by the view should be in this interface. Other
stuff can be moved to public on the impl class.

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

A few things I noticed.

https://codereview.chromium.org/1396923003/diff/20001/chrome/browser/ui/autof...
File chrome/browser/ui/autofill/save_card_bubble_controller_impl.cc (right):

https://codereview.chromium.org/1396923003/diff/20001/chrome/browser/ui/autof...
chrome/browser/ui/autofill/save_card_bubble_controller_impl.cc:49: // Need to
create location bar icon before bubble, otherwise bubble will be
What happens if the location bar isn't visible? Like in fullscreen.
Does this unanchored bubble need to have an arrow?

https://codereview.chromium.org/1396923003/diff/20001/chrome/browser/ui/brows...
File chrome/browser/ui/browser_window.h (right):

https://codereview.chromium.org/1396923003/diff/20001/chrome/browser/ui/brows...
chrome/browser/ui/browser_window.h:254: virtual autofill::SaveCardBubbleView*
ShowSaveCreditCardBubble(
This is only called by SaveCardBubbleControllerImpl::ShowBubble()
Will cocoa use this the same way? Maybe this function should move to the
SaveCardBubbleControllerImpl?

https://codereview.chromium.org/1396923003/diff/20001/chrome/browser/ui/views...
File chrome/browser/ui/views/autofill/save_card_bubble_views.cc (right):

https://codereview.chromium.org/1396923003/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/autofill/save_card_bubble_views.cc:75: DCHECK(sender ==
cancel_button_);
DCHECK_EQ

https://codereview.chromium.org/1396923003/diff/20001/chrome/browser/ui/views...
File chrome/browser/ui/views/location_bar/save_credit_card_icon_view.cc (right):

https://codereview.chromium.org/1396923003/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/location_bar/save_credit_card_icon_view.cc:39: if
(!browser_)
Why allow browser to be nullptr if that means you can't get the bubble?

https://codereview.chromium.org/1396923003/diff/40001/components/autofill/cor...
File components/autofill/core/browser/ui/save_card_bubble_view.h (right):

https://codereview.chromium.org/1396923003/diff/40001/components/autofill/cor...
components/autofill/core/browser/ui/save_card_bubble_view.h:10: class
SaveCardBubbleController;
Not used here.

[Justin Donnelly, 2015-10-19 17:39:09]

https://codereview.chromium.org/1396923003/diff/20001/chrome/browser/ui/autof...
File chrome/browser/ui/autofill/save_card_bubble_controller_impl.cc (right):

https://codereview.chromium.org/1396923003/diff/20001/chrome/browser/ui/autof...
chrome/browser/ui/autofill/save_card_bubble_controller_impl.cc:40: void
SaveCardBubbleControllerImpl::CreateBubble(
On 2015/10/16 01:20:44, Evan Stade wrote:
> 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();

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?

https://codereview.chromium.org/1396923003/diff/20001/components/autofill/cor...
File components/autofill/core/browser/ui/save_card_bubble_controller.h (right):

https://codereview.chromium.org/1396923003/diff/20001/components/autofill/cor...
components/autofill/core/browser/ui/save_card_bubble_controller.h:6: #define
COMPONENTS_AUTOFILL_CORE_BROWSER_UI_SAVE_CARD_BUBBLE_CONTROLLER_H_
On 2015/10/16 01:20:44, Evan Stade wrote:
> I don't think this belongs in components as it won't be used by ios

It won't? Do you mean the save card UI would have a different interface or that
there won't be a save card UI on iOS? I don't think either of those are true but
I could be missing something.

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

https://codereview.chromium.org/1396923003/diff/20001/chrome/browser/ui/autof...
File chrome/browser/ui/autofill/save_card_bubble_controller_impl.cc (right):

https://codereview.chromium.org/1396923003/diff/20001/chrome/browser/ui/autof...
chrome/browser/ui/autofill/save_card_bubble_controller_impl.cc:40: void
SaveCardBubbleControllerImpl::CreateBubble(
On 2015/10/19 17:39:08, Justin Donnelly wrote:
> On 2015/10/16 01:20:44, Evan Stade wrote:
> > 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();
> 
> 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.

Sure, in fact my final suggestion did not include And, so we're in agreement.

> 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.

Whether it's useful elsewhere or not, I can't see how it's useful here.

> 
> 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).

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-20 02:13:56]

Hey estade@, trybots look to be all good now except for two tests:
AutofillTest.WhitespacesAndSeparatorCharsStrippedForValidCCNums
AutofillTest.CCInfoNotStoredWhenAutocompleteOff (I'll remove the "Not" from this
one.)

These hang because they are waiting for the old infobar to show, so that they
can call ConfirmInfoBarDelegate::Accept() and save the data. This is in test
class WindowedPersonalDataManagerObserver.

This wait + accept strategy relies on InfoBarManager::Observer. The save credit
card bubble doesn't have an analogous observer, because it's not necessary in
the non-test environment.

I could:
1. Add SaveCardBubbleObserver, with it only used for testing.
OR
2. Create SaveCardBubbleControllerImpl using a factory, and for the tests in
question create TestSaveCardBubbleControllerImpl that calls save_card_callback
immediately (well, via PostTask) without actually creating the bubble. (I've
hacked a test of this together and it works.)
OR
3. ??? <-- your better idea here

#2 seems okay to me, because it doesn't add any test-only code to the production
build, and the two tests aren't testing the infobar / bubble directly - they are
just clicking through it to get to what they really want to test.

What do you think: 1, 2, or 3?

[Evan Stade, 2015-10-20 15:28:14]

On 2015/10/20 02:13:56, bondd wrote:
> Hey estade@, trybots look to be all good now except for two tests:
> AutofillTest.WhitespacesAndSeparatorCharsStrippedForValidCCNums
> AutofillTest.CCInfoNotStoredWhenAutocompleteOff (I'll remove the "Not" from
this
> one.)
> 
> These hang because they are waiting for the old infobar to show, so that they
> can call ConfirmInfoBarDelegate::Accept() and save the data. This is in test
> class WindowedPersonalDataManagerObserver.
> 
> This wait + accept strategy relies on InfoBarManager::Observer. The save
credit
> card bubble doesn't have an analogous observer, because it's not necessary in
> the non-test environment.
> 
> I could:
> 1. Add SaveCardBubbleObserver, with it only used for testing.
> OR
> 2. Create SaveCardBubbleControllerImpl using a factory, and for the tests in
> question create TestSaveCardBubbleControllerImpl that calls save_card_callback
> immediately (well, via PostTask) without actually creating the bubble. (I've
> hacked a test of this together and it works.)
> OR
> 3. ??? <-- your better idea here
> 
> #2 seems okay to me, because it doesn't add any test-only code to the
production
> build, and the two tests aren't testing the infobar / bubble directly - they
are
> just clicking through it to get to what they really want to test.
> 
> What do you think: 1, 2, or 3?

Why is CCInfoNotStoredWhenAutocompleteOff currently passing? I think I'd change
that one into a unit test and don't rely on UI state at all. Basically, verify
whether the AutofillClient was told to ConfirmSaveCreditCard.

I see some other tests that need updating as well.
InvalidCreditCardNumberIsNotAggregated may pass but no longer has any meaning
because ASSERT_EQ(0u, infobar_service->infobar_count()); is trivially true.
Since we have to update these, might as well make them unit tests, which they
probably should have been in the first place (along the lines of
autofill_popup_controller_unittest.cc perhaps).

[bondd, 2015-10-22 01:53:41]

On 2015/10/20 15:28:14, Evan Stade wrote:
> On 2015/10/20 02:13:56, bondd wrote:
> > Hey estade@, trybots look to be all good now except for two tests:
> > AutofillTest.WhitespacesAndSeparatorCharsStrippedForValidCCNums
> > AutofillTest.CCInfoNotStoredWhenAutocompleteOff (I'll remove the "Not" from
> this
> > one.)
> > 
> > These hang because they are waiting for the old infobar to show, so that
they
> > can call ConfirmInfoBarDelegate::Accept() and save the data. This is in test
> > class WindowedPersonalDataManagerObserver.
> > 
> > This wait + accept strategy relies on InfoBarManager::Observer. The save
> credit
> > card bubble doesn't have an analogous observer, because it's not necessary
in
> > the non-test environment.
> > 
> > I could:
> > 1. Add SaveCardBubbleObserver, with it only used for testing.
> > OR
> > 2. Create SaveCardBubbleControllerImpl using a factory, and for the tests in
> > question create TestSaveCardBubbleControllerImpl that calls
save_card_callback
> > immediately (well, via PostTask) without actually creating the bubble. (I've
> > hacked a test of this together and it works.)
> > OR
> > 3. ??? <-- your better idea here
> > 
> > #2 seems okay to me, because it doesn't add any test-only code to the
> production
> > build, and the two tests aren't testing the infobar / bubble directly - they
> are
> > just clicking through it to get to what they really want to test.
> > 
> > What do you think: 1, 2, or 3?
> 
> Why is CCInfoNotStoredWhenAutocompleteOff currently passing? I think I'd
change
> that one into a unit test and don't rely on UI state at all. Basically, verify
> whether the AutofillClient was told to ConfirmSaveCreditCard.

I believe it's because FillFormAndSubmit() waits on
WindowedPersonalDataManagerObserver, which waits for personal data changes but
also calls Accept() on infobars. So when the card data is saved, the observer
accepts the dialog, resulting in infobar_service->infobar_count() == 0.

> I see some other tests that need updating as well.
> InvalidCreditCardNumberIsNotAggregated may pass but no longer has any meaning
> because ASSERT_EQ(0u, infobar_service->infobar_count()); is trivially true.
> Since we have to update these, might as well make them unit tests, which they
> probably should have been in the first place (along the lines of
> autofill_popup_controller_unittest.cc perhaps).

I replaced the three browser tests that depended on the infobar with unit tests.
Additionally, I removed infobar dependencies from
WindowedPersonalDataManagerObserver and PdmChangeWaiter classes to be sure that
no existing Autofill tests are erroneously dependent on infobar.

[bondd, 2015-10-22 02:15:18]

The big change in this latest patch set is I replaced three browser tests that
depended on infobar with unit tests. See my comment to Evan on this CL from
about 20 minutes ago for a bit more info.

AFAIK I've addressed all the outstanding reviewer comments now, and the trybots
are all good (well, almost all good...looks like I broke a couple of trivial
things just now). If I missed anything please let me know.

https://codereview.chromium.org/1396923003/diff/20001/chrome/browser/ui/autof...
File chrome/browser/ui/autofill/save_card_bubble_controller_impl.cc (right):

https://codereview.chromium.org/1396923003/diff/20001/chrome/browser/ui/autof...
chrome/browser/ui/autofill/save_card_bubble_controller_impl.cc:40: void
SaveCardBubbleControllerImpl::CreateBubble(
On 2015/10/19 17:58:31, Evan Stade wrote:
> On 2015/10/19 17:39:08, Justin Donnelly wrote:
> > On 2015/10/16 01:20:44, Evan Stade wrote:
> > > 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();
> > 
> > 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.
> 
> Sure, in fact my final suggestion did not include And, so we're in agreement.
> 
> > 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.
> 
> Whether it's useful elsewhere or not, I can't see how it's useful here.
> 
> > 
> > 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).
> 
> 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.
> 

Done.

https://codereview.chromium.org/1396923003/diff/20001/chrome/browser/ui/autof...
chrome/browser/ui/autofill/save_card_bubble_controller_impl.cc:49: // Need to
create location bar icon before bubble, otherwise bubble will be
On 2015/10/16 21:16:35, Hector Carmona wrote:
> What happens if the location bar isn't visible? Like in fullscreen.
> Does this unanchored bubble need to have an arrow?

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).

https://codereview.chromium.org/1396923003/diff/20001/chrome/browser/ui/autof...
chrome/browser/ui/autofill/save_card_bubble_controller_impl.cc:109: return
timer_ ? timer_->Elapsed() : base::TimeDelta::Max();
On 2015/10/16 01:20:44, Evan Stade wrote:
> 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.

Done.

https://codereview.chromium.org/1396923003/diff/20001/chrome/browser/ui/autof...
chrome/browser/ui/autofill/save_card_bubble_controller_impl.cc:124: if
(Elapsed() < base::TimeDelta::FromSeconds(kBubbleMinTime))
On 2015/10/16 01:20:43, Evan Stade wrote:
> can just be
> 
> if (timer_ && timer_->Elapsed() < ...)
> 
> (also do you really need to check timer_?)

Done.

https://codereview.chromium.org/1396923003/diff/20001/chrome/browser/ui/autof...
File chrome/browser/ui/autofill/save_card_bubble_controller_impl.h (right):

https://codereview.chromium.org/1396923003/diff/20001/chrome/browser/ui/autof...
chrome/browser/ui/autofill/save_card_bubble_controller_impl.h:85: State state_;
On 2015/10/16 01:20:44, Evan Stade wrote:
> instead of having this redundant information, check state of other member vars
> to divine the status of things. e.g. instead of checking state is
> AVAILABLE_STATE, check !save_card_callback_.is_null(). Instead of checking
> INACTIVE, ask save_card_bubble_view_ == nullptr?

Done.

https://codereview.chromium.org/1396923003/diff/20001/chrome/browser/ui/brows...
File chrome/browser/ui/browser_window.h (right):

https://codereview.chromium.org/1396923003/diff/20001/chrome/browser/ui/brows...
chrome/browser/ui/browser_window.h:254: virtual autofill::SaveCardBubbleView*
ShowSaveCreditCardBubble(
On 2015/10/16 01:20:44, Evan Stade wrote:
> need to add this to cocoa (and maybe a test/mock class or two)

Done.

https://codereview.chromium.org/1396923003/diff/20001/chrome/browser/ui/brows...
chrome/browser/ui/browser_window.h:254: virtual autofill::SaveCardBubbleView*
ShowSaveCreditCardBubble(
On 2015/10/16 21:16:35, Hector Carmona wrote:
> This is only called by SaveCardBubbleControllerImpl::ShowBubble()
> Will cocoa use this the same way? Maybe this function should move to the
> SaveCardBubbleControllerImpl?

Yes, the plan is for Cocoa to use it the same way. AFAIK it needs to be in
BrowserWindow so that subclass BrowserView can call
GetToolbarView()->GetSaveCreditCardBubbleAnchor() to get the anchor to pass in
to the SaveCardBubbleViews constructor.

https://codereview.chromium.org/1396923003/diff/20001/chrome/browser/ui/views...
File chrome/browser/ui/views/autofill/save_card_bubble_views.cc (right):

https://codereview.chromium.org/1396923003/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/autofill/save_card_bubble_views.cc:75: DCHECK(sender ==
cancel_button_);
On 2015/10/16 21:16:35, Hector Carmona wrote:
> DCHECK_EQ

Done.

https://codereview.chromium.org/1396923003/diff/20001/chrome/browser/ui/views...
File chrome/browser/ui/views/autofill/save_card_bubble_views.h (right):

https://codereview.chromium.org/1396923003/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/autofill/save_card_bubble_views.h:33:
~SaveCardBubbleViews() override;
On 2015/10/16 01:20:44, Evan Stade wrote:
> make private

Done.

https://codereview.chromium.org/1396923003/diff/20001/chrome/browser/ui/views...
File chrome/browser/ui/views/location_bar/location_bar_view.cc (right):

https://codereview.chromium.org/1396923003/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/location_bar/location_bar_view.cc:994: return false;
On 2015/10/16 01:20:44, Evan Stade wrote:
> \n

Done.

https://codereview.chromium.org/1396923003/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/location_bar/location_bar_view.cc:1004:
save_credit_card_icon_view_->SetToggled(controller->IsIconToggled());
On 2015/10/16 01:20:44, Evan Stade wrote:
> \n

Done.

https://codereview.chromium.org/1396923003/diff/20001/chrome/browser/ui/views...
File chrome/browser/ui/views/location_bar/save_credit_card_icon_view.cc (right):

https://codereview.chromium.org/1396923003/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/location_bar/save_credit_card_icon_view.cc:39: if
(!browser_)
On 2015/10/16 21:16:35, Hector Carmona wrote:
> Why allow browser to be nullptr if that means you can't get the bubble?

SaveCreditCardIconView is instantiated in the LocationBarView constructor with
the rest of the location bar icons. It's possible for browser to be null there
so I have to handle it here. I suppose I could just not create the icon if
browser is null, but none of the other icons do it that way and I'd like to keep
things as consistent as possible.

https://codereview.chromium.org/1396923003/diff/20001/components/autofill/cor...
File components/autofill/core/browser/ui/save_card_bubble_controller.h (right):

https://codereview.chromium.org/1396923003/diff/20001/components/autofill/cor...
components/autofill/core/browser/ui/save_card_bubble_controller.h:6: #define
COMPONENTS_AUTOFILL_CORE_BROWSER_UI_SAVE_CARD_BUBBLE_CONTROLLER_H_
On 2015/10/19 17:39:08, Justin Donnelly wrote:
> On 2015/10/16 01:20:44, Evan Stade wrote:
> > I don't think this belongs in components as it won't be used by ios
> 
> It won't? Do you mean the save card UI would have a different interface or
that
> there won't be a save card UI on iOS? I don't think either of those are true
but
> I could be missing something.

Yes, it seems to me we will use this class on iOS. Don't we do a similar thing
for the CardUnmaskPrompt on iOS?

https://codereview.chromium.org/1396923003/diff/20001/components/autofill/cor...
components/autofill/core/browser/ui/save_card_bubble_controller.h:16: class
SaveCardBubbleController {
On 2015/10/16 01:20:44, Evan Stade wrote:
> only the methods visible/used by the view should be in this interface. Other
> stuff can be moved to public on the impl class.

Done.

https://codereview.chromium.org/1396923003/diff/40001/components/autofill/cor...
File components/autofill/core/browser/ui/save_card_bubble_view.h (right):

https://codereview.chromium.org/1396923003/diff/40001/components/autofill/cor...
components/autofill/core/browser/ui/save_card_bubble_view.h:10: class
SaveCardBubbleController;
On 2015/10/16 21:16:35, Hector Carmona wrote:
> Not used here.

Done. Thanks.

[bondd, 2015-10-22 02:23:01]

bondd@chromium.org changed reviewers:
+ sky@chromium.org

[bondd, 2015-10-22 02:23:01]

bondd@chromium.org changed required reviewers:
+ sky@chromium.org

[bondd, 2015-10-22 02:23:02]

+sky@ for files not in an autofill-specific directory:
    chrome/app/chrome_command_ids.h
    chrome/browser/ui/browser_command_controller.cc
    chrome/browser/ui/browser_commands.cc
    chrome/browser/ui/browser_commands.h
    chrome/browser/ui/browser_window.h
    chrome/browser/ui/cocoa/browser_window_cocoa.h
    chrome/browser/ui/cocoa/browser_window_cocoa.mm
    chrome/browser/ui/cocoa/location_bar/location_bar_view_mac.h
    chrome/browser/ui/cocoa/location_bar/location_bar_view_mac.mm
    chrome/browser/ui/location_bar/location_bar.h
    chrome/browser/ui/view_ids.h
    chrome/browser/ui/views/frame/browser_view.cc
    chrome/browser/ui/views/frame/browser_view.h
    chrome/browser/ui/views/location_bar/location_bar_view.cc
    chrome/browser/ui/views/location_bar/location_bar_view.h
    chrome/browser/ui/views/location_bar/save_credit_card_icon_view.cc
    chrome/browser/ui/views/location_bar/save_credit_card_icon_view.h
    chrome/browser/ui/views/toolbar/toolbar_view.cc
    chrome/browser/ui/views/toolbar/toolbar_view.h
    chrome/test/base/test_browser_window.cc
    chrome/test/base/test_browser_window.h

[bondd, 2015-10-22 17:10:02]

Added a few minor review comments that I forgot last night.

Should I disable autofill_cc_infobar_delegate_unittest.cc for Views? All the
tests still pass, but this infobar is no longer used on Views.

https://codereview.chromium.org/1396923003/diff/100001/chrome/browser/ui/coco...
File chrome/browser/ui/cocoa/location_bar/location_bar_view_mac.mm (right):

https://codereview.chromium.org/1396923003/diff/100001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/location_bar/location_bar_view_mac.mm:190: // Not
implemented on Mac.
Using comment instead of NOTIMPLEMENTED to keep with precedent elsewhere in this
file.

https://codereview.chromium.org/1396923003/diff/100001/components/autofill/co...
File components/autofill/core/browser/autofill_manager_unittest.cc (right):

https://codereview.chromium.org/1396923003/diff/100001/components/autofill/co...
components/autofill/core/browser/autofill_manager_unittest.cc:212:
"4234567890123456",  // Visa
Removed spaces because they're tested explicitly elsewhere now.

[Evan Stade, 2015-10-22 22:41:38]

> Should I disable autofill_cc_infobar_delegate_unittest.cc for Views? All the
> tests still pass, but this infobar is no longer used on Views.

sure, move it to an android only block in chrome_tests_unit.gypi

[Evan Stade, 2015-10-22 23:02:43]

overall looking pretty good

https://codereview.chromium.org/1396923003/diff/100001/chrome/browser/ui/auto...
File chrome/browser/ui/autofill/chrome_autofill_client.cc (right):

https://codereview.chromium.org/1396923003/diff/100001/chrome/browser/ui/auto...
chrome/browser/ui/autofill/chrome_autofill_client.cc:158: #if
defined(OS_CHROMEOS) || defined(OS_LINUX) || defined(OS_WIN)
probably can just do TOOLKIT_VIEWS?

https://codereview.chromium.org/1396923003/diff/100001/chrome/browser/ui/auto...
File chrome/browser/ui/autofill/save_card_bubble_controller_impl.cc (right):

https://codereview.chromium.org/1396923003/diff/100001/chrome/browser/ui/auto...
chrome/browser/ui/autofill/save_card_bubble_controller_impl.cc:18: // Minimal
time span the bubble should survive implicit navigations.
you should note what unit this is in, particularly since it says "Min" which
could be interpreted as "minutes"

also I object to using just "bubble" here (in the comment and the var name)
because the period of invincibility also applies to the icon.

https://codereview.chromium.org/1396923003/diff/100001/chrome/browser/ui/auto...
chrome/browser/ui/autofill/save_card_bubble_controller_impl.cc:45:
DCHECK_EQ(save_card_callback_.is_null(), false);
DCHECK(!bla.is_null());

https://codereview.chromium.org/1396923003/diff/100001/chrome/browser/ui/auto...
chrome/browser/ui/autofill/save_card_bubble_controller_impl.cc:68: return
save_card_bubble_view_;
I don't like this implicit cast, I'm used to seeing !!pointer_value_ instead.
Dunno what the style guide has to say about this issue.

https://codereview.chromium.org/1396923003/diff/100001/chrome/browser/ui/auto...
chrome/browser/ui/autofill/save_card_bubble_controller_impl.cc:93:
DCHECK(location_bar);
no need to DCHECK since you're derefing on the next line anyway

https://codereview.chromium.org/1396923003/diff/100001/chrome/browser/ui/auto...
File chrome/browser/ui/autofill/save_card_bubble_controller_impl.h (right):

https://codereview.chromium.org/1396923003/diff/100001/chrome/browser/ui/auto...
chrome/browser/ui/autofill/save_card_bubble_controller_impl.h:65:
SaveCardBubbleView* save_card_bubble_view_;
also note that it's a weak ref

https://codereview.chromium.org/1396923003/diff/100001/chrome/browser/ui/view...
File chrome/browser/ui/views/autofill/save_card_bubble_views.cc (right):

https://codereview.chromium.org/1396923003/diff/100001/chrome/browser/ui/view...
chrome/browser/ui/views/autofill/save_card_bubble_views.cc:35:
views::BubbleDelegateView::CreateBubble(this);
DCHECK controller?

https://codereview.chromium.org/1396923003/diff/100001/components/autofill/co...
File components/autofill/core/browser/autofill_manager_unittest.cc (right):

https://codereview.chromium.org/1396923003/diff/100001/components/autofill/co...
components/autofill/core/browser/autofill_manager_unittest.cc:750: bool
cc_autocomplete_off,
am I missing something? When is this ever true?

also, the callsites which were already hard to read are now even harder to read
(wth does "true false false" mean?)

https://codereview.chromium.org/1396923003/diff/100001/components/autofill/co...
File components/autofill/core/browser/ui/save_card_bubble_controller.h (right):

https://codereview.chromium.org/1396923003/diff/100001/components/autofill/co...
components/autofill/core/browser/ui/save_card_bubble_controller.h:6: #define
COMPONENTS_AUTOFILL_CORE_BROWSER_UI_SAVE_CARD_BUBBLE_CONTROLLER_H_
why is this in this directory?

https://codereview.chromium.org/1396923003/diff/100001/components/autofill/co...
components/autofill/core/browser/ui/save_card_bubble_controller.h:15: // Omnibox
icon.
this comment should be something like

"Interface that exposes controller functionality to SaveCardBubbleView."

The comment above the functions is unnecessary

https://codereview.chromium.org/1396923003/diff/100001/components/autofill/co...
File components/autofill/core/browser/ui/save_card_bubble_view.h (right):

https://codereview.chromium.org/1396923003/diff/100001/components/autofill/co...
components/autofill/core/browser/ui/save_card_bubble_view.h:6: #define
COMPONENTS_AUTOFILL_CORE_BROWSER_UI_SAVE_CARD_BUBBLE_VIEW_H_
this file is also in the wrong directory. It's never going to be used for ios
and all references to it are from within chrome/browser/

[Evan Stade, 2015-10-23 01:39:41]

https://codereview.chromium.org/1396923003/diff/100001/chrome/browser/ui/view...
File chrome/browser/ui/views/autofill/save_card_bubble_views.h (right):

https://codereview.chromium.org/1396923003/diff/100001/chrome/browser/ui/view...
chrome/browser/ui/views/autofill/save_card_bubble_views.h:25: // This class
displays the "Save credit card?" bubble.
probably helps to be a bit more descriptive in terms of how one might
demonstrate this bubble in action. How do I get this bubble?

https://codereview.chromium.org/1396923003/diff/100001/chrome/browser/ui/view...
File chrome/browser/ui/views/location_bar/save_credit_card_icon_view.cc (right):

https://codereview.chromium.org/1396923003/diff/100001/chrome/browser/ui/view...
chrome/browser/ui/views/location_bar/save_credit_card_icon_view.cc:15: #include
"ui/gfx/color_palette.h"
you don't use this include

https://codereview.chromium.org/1396923003/diff/100001/chrome/browser/ui/view...
chrome/browser/ui/views/location_bar/save_credit_card_icon_view.cc:16: #include
"ui/gfx/paint_vector_icon.h"
you don't use this include

[bondd, 2015-10-23 03:32:35]

Addressed all outstanding comments and kicked off the trybots.
+sky@: Do you have time to review the non-autofill files?
Thanks
-Dave

https://codereview.chromium.org/1396923003/diff/100001/chrome/browser/ui/auto...
File chrome/browser/ui/autofill/chrome_autofill_client.cc (right):

https://codereview.chromium.org/1396923003/diff/100001/chrome/browser/ui/auto...
chrome/browser/ui/autofill/chrome_autofill_client.cc:158: #if
defined(OS_CHROMEOS) || defined(OS_LINUX) || defined(OS_WIN)
On 2015/10/22 23:02:43, Evan Stade wrote:
> probably can just do TOOLKIT_VIEWS?

Done.

https://codereview.chromium.org/1396923003/diff/100001/chrome/browser/ui/auto...
File chrome/browser/ui/autofill/save_card_bubble_controller_impl.cc (right):

https://codereview.chromium.org/1396923003/diff/100001/chrome/browser/ui/auto...
chrome/browser/ui/autofill/save_card_bubble_controller_impl.cc:18: // Minimal
time span the bubble should survive implicit navigations.
On 2015/10/22 23:02:43, Evan Stade wrote:
> you should note what unit this is in, particularly since it says "Min" which
> could be interpreted as "minutes"
> 
> also I object to using just "bubble" here (in the comment and the var name)
> because the period of invincibility also applies to the icon.

Done.

https://codereview.chromium.org/1396923003/diff/100001/chrome/browser/ui/auto...
chrome/browser/ui/autofill/save_card_bubble_controller_impl.cc:45:
DCHECK_EQ(save_card_callback_.is_null(), false);
On 2015/10/22 23:02:43, Evan Stade wrote:
> DCHECK(!bla.is_null());

Done.

https://codereview.chromium.org/1396923003/diff/100001/chrome/browser/ui/auto...
chrome/browser/ui/autofill/save_card_bubble_controller_impl.cc:68: return
save_card_bubble_view_;
On 2015/10/22 23:02:43, Evan Stade wrote:
> I don't like this implicit cast, I'm used to seeing !!pointer_value_ instead.
> Dunno what the style guide has to say about this issue.

Done. Windows compile didn't like the implicit cast either. :)

https://codereview.chromium.org/1396923003/diff/100001/chrome/browser/ui/auto...
chrome/browser/ui/autofill/save_card_bubble_controller_impl.cc:93:
DCHECK(location_bar);
On 2015/10/22 23:02:43, Evan Stade wrote:
> no need to DCHECK since you're derefing on the next line anyway

Done.

https://codereview.chromium.org/1396923003/diff/100001/chrome/browser/ui/auto...
File chrome/browser/ui/autofill/save_card_bubble_controller_impl.h (right):

https://codereview.chromium.org/1396923003/diff/100001/chrome/browser/ui/auto...
chrome/browser/ui/autofill/save_card_bubble_controller_impl.h:65:
SaveCardBubbleView* save_card_bubble_view_;
On 2015/10/22 23:02:43, Evan Stade wrote:
> also note that it's a weak ref

Done.

https://codereview.chromium.org/1396923003/diff/100001/chrome/browser/ui/view...
File chrome/browser/ui/views/autofill/save_card_bubble_views.cc (right):

https://codereview.chromium.org/1396923003/diff/100001/chrome/browser/ui/view...
chrome/browser/ui/views/autofill/save_card_bubble_views.cc:35:
views::BubbleDelegateView::CreateBubble(this);
On 2015/10/22 23:02:43, Evan Stade wrote:
> DCHECK controller?

Done.

https://codereview.chromium.org/1396923003/diff/100001/chrome/browser/ui/view...
File chrome/browser/ui/views/autofill/save_card_bubble_views.h (right):

https://codereview.chromium.org/1396923003/diff/100001/chrome/browser/ui/view...
chrome/browser/ui/views/autofill/save_card_bubble_views.h:25: // This class
displays the "Save credit card?" bubble.
On 2015/10/23 01:39:41, Evan Stade wrote:
> probably helps to be a bit more descriptive in terms of how one might
> demonstrate this bubble in action. How do I get this bubble?

Done.

https://codereview.chromium.org/1396923003/diff/100001/chrome/browser/ui/view...
File chrome/browser/ui/views/location_bar/save_credit_card_icon_view.cc (right):

https://codereview.chromium.org/1396923003/diff/100001/chrome/browser/ui/view...
chrome/browser/ui/views/location_bar/save_credit_card_icon_view.cc:15: #include
"ui/gfx/color_palette.h"
On 2015/10/23 01:39:41, Evan Stade wrote:
> you don't use this include

Done.

https://codereview.chromium.org/1396923003/diff/100001/chrome/browser/ui/view...
chrome/browser/ui/views/location_bar/save_credit_card_icon_view.cc:16: #include
"ui/gfx/paint_vector_icon.h"
On 2015/10/23 01:39:41, Evan Stade wrote:
> you don't use this include

Done.

https://codereview.chromium.org/1396923003/diff/100001/components/autofill/co...
File components/autofill/core/browser/autofill_manager_unittest.cc (right):

https://codereview.chromium.org/1396923003/diff/100001/components/autofill/co...
components/autofill/core/browser/autofill_manager_unittest.cc:750: bool
cc_autocomplete_off,
On 2015/10/22 23:02:43, Evan Stade wrote:
> am I missing something? When is this ever true?
> 
> also, the callsites which were already hard to read are now even harder to
read
> (wth does "true false false" mean?)

It was called via TestSaveCreditCards, but you are right it was becoming a mess.
Got rid of the extra parameter and instead set "should_autocomplete=false" in
the one test that requires it.

https://codereview.chromium.org/1396923003/diff/100001/components/autofill/co...
File components/autofill/core/browser/ui/save_card_bubble_controller.h (right):

https://codereview.chromium.org/1396923003/diff/100001/components/autofill/co...
components/autofill/core/browser/ui/save_card_bubble_controller.h:6: #define
COMPONENTS_AUTOFILL_CORE_BROWSER_UI_SAVE_CARD_BUBBLE_CONTROLLER_H_
On 2015/10/22 23:02:43, Evan Stade wrote:
> why is this in this directory?

Done. Moved to chrome/browser/ui/autofill/

https://codereview.chromium.org/1396923003/diff/100001/components/autofill/co...
components/autofill/core/browser/ui/save_card_bubble_controller.h:15: // Omnibox
icon.
On 2015/10/22 23:02:43, Evan Stade wrote:
> this comment should be something like
> 
> "Interface that exposes controller functionality to SaveCardBubbleView."
> 
> The comment above the functions is unnecessary

Done.

https://codereview.chromium.org/1396923003/diff/100001/components/autofill/co...
File components/autofill/core/browser/ui/save_card_bubble_view.h (right):

https://codereview.chromium.org/1396923003/diff/100001/components/autofill/co...
components/autofill/core/browser/ui/save_card_bubble_view.h:6: #define
COMPONENTS_AUTOFILL_CORE_BROWSER_UI_SAVE_CARD_BUBBLE_VIEW_H_
On 2015/10/22 23:02:43, Evan Stade wrote:
> this file is also in the wrong directory. It's never going to be used for ios
> and all references to it are from within chrome/browser/

Done.

[Justin Donnelly, 2015-10-23 13:53:09]

https://codereview.chromium.org/1396923003/diff/20001/components/autofill/cor...
File components/autofill/core/browser/ui/save_card_bubble_controller.h (right):

https://codereview.chromium.org/1396923003/diff/20001/components/autofill/cor...
components/autofill/core/browser/ui/save_card_bubble_controller.h:6: #define
COMPONENTS_AUTOFILL_CORE_BROWSER_UI_SAVE_CARD_BUBBLE_CONTROLLER_H_
On 2015/10/22 02:15:17, bondd wrote:
> On 2015/10/19 17:39:08, Justin Donnelly wrote:
> > On 2015/10/16 01:20:44, Evan Stade wrote:
> > > I don't think this belongs in components as it won't be used by ios
> > 
> > It won't? Do you mean the save card UI would have a different interface or
> that
> > there won't be a save card UI on iOS? I don't think either of those are true
> but
> > I could be missing something.
> 
> Yes, it seems to me we will use this class on iOS. Don't we do a similar thing
> for the CardUnmaskPrompt on iOS?

Evan, you never answered this question. I'm pretty sure I'm going to be moving
this and save_card_bubble_view.h right back to components as soon as we
implement upload on iOS. But again, I may be missing something, so I'm hoping
you can clarify what you have in mind.

[Evan Stade, 2015-10-23 17:36:14]

https://codereview.chromium.org/1396923003/diff/20001/components/autofill/cor...
File components/autofill/core/browser/ui/save_card_bubble_controller.h (right):

https://codereview.chromium.org/1396923003/diff/20001/components/autofill/cor...
components/autofill/core/browser/ui/save_card_bubble_controller.h:6: #define
COMPONENTS_AUTOFILL_CORE_BROWSER_UI_SAVE_CARD_BUBBLE_CONTROLLER_H_
On 2015/10/23 13:53:09, Justin Donnelly wrote:
> On 2015/10/22 02:15:17, bondd wrote:
> > On 2015/10/19 17:39:08, Justin Donnelly wrote:
> > > On 2015/10/16 01:20:44, Evan Stade wrote:
> > > > I don't think this belongs in components as it won't be used by ios
> > > 
> > > It won't? Do you mean the save card UI would have a different interface or
> > that
> > > there won't be a save card UI on iOS? I don't think either of those are
true
> > but
> > > I could be missing something.
> > 
> > Yes, it seems to me we will use this class on iOS. Don't we do a similar
thing
> > for the CardUnmaskPrompt on iOS?
> 
> Evan, you never answered this question. I'm pretty sure I'm going to be moving
> this and save_card_bubble_view.h right back to components as soon as we
> implement upload on iOS. But again, I may be missing something, so I'm hoping
> you can clarify what you have in mind.

ios plans to have a bubble? Does ios Chrome have the concept of a bubble or an
icon in the location bar?

It's easier to see in more recent patchsets, but this "controller" interface is
really really thin, so we're not even gaining anything by sharing it. It's
basically just some thunks for passing through to the model, and is tightly
coupled with the presentation (a bubble). I don't see anything substantive that
could/should be shared with ios here. That said, you'll have the opportunity to
refactor later if it does make sense. But I'd rather have the code make sense
relative to the present than make sense relative to a hypothetical future world.

[Justin Donnelly, 2015-10-23 17:41:09]

lgtm

https://codereview.chromium.org/1396923003/diff/20001/components/autofill/cor...
File components/autofill/core/browser/ui/save_card_bubble_controller.h (right):

https://codereview.chromium.org/1396923003/diff/20001/components/autofill/cor...
components/autofill/core/browser/ui/save_card_bubble_controller.h:6: #define
COMPONENTS_AUTOFILL_CORE_BROWSER_UI_SAVE_CARD_BUBBLE_CONTROLLER_H_
On 2015/10/23 17:36:14, Evan Stade wrote:
> But I'd rather have the code make sense
> relative to the present than make sense relative to a hypothetical future
world.

Fair enough.

> ios plans to have a bubble? Does ios Chrome have the concept of a bubble or an
> icon in the location bar?

It'll be an infobar, not a bubble. I suppose that strengthens your argument,
since a common interface would probably need to be renamed from "Bubble" to
"Prompt" or somesuch.

[Evan Stade, 2015-10-23 21:56:30]

https://codereview.chromium.org/1396923003/diff/160001/chrome/browser/ui/auto...
File chrome/browser/ui/autofill/save_card_bubble_controller_impl.cc (right):

https://codereview.chromium.org/1396923003/diff/160001/chrome/browser/ui/auto...
chrome/browser/ui/autofill/save_card_bubble_controller_impl.cc:86:
save_card_bubble_view_ = nullptr;
reset the callback here instead of above in save and cancel?

(also then you don't need OnCancelButton)

https://codereview.chromium.org/1396923003/diff/160001/chrome/browser/ui/auto...
chrome/browser/ui/autofill/save_card_bubble_controller_impl.cc:116:
save_card_bubble_view_ = nullptr;
won't the above Close() call already null this out and update the icon. I'd make
this something like

if (save_card_bubble_view_)
  save_card_bubble_view_->Close();
else
  OnBubbleClosed();

[bondd, 2015-10-23 22:58:28]

https://codereview.chromium.org/1396923003/diff/160001/chrome/browser/ui/auto...
File chrome/browser/ui/autofill/save_card_bubble_controller_impl.cc (right):

https://codereview.chromium.org/1396923003/diff/160001/chrome/browser/ui/auto...
chrome/browser/ui/autofill/save_card_bubble_controller_impl.cc:86:
save_card_bubble_view_ = nullptr;
On 2015/10/23 21:56:30, Evan Stade wrote:
> reset the callback here instead of above in save and cancel?
> 
> (also then you don't need OnCancelButton)

Pressing the cancel button causes different behavior than if the dialog closes
for a different reason (e.g. the user clicking outside of the bubble, or the
Chrome window losing focus). When the cancel button is pressed the bubble and
icon go away and don't come back (unless user submits another credit card
number). In the other OnBubbleClosed() cases the bubble has gone away, but can
be brought back by clicking the icon (which is still visible).

save_card_callback_.is_null() indicates whether the bubble and icon are
available (see save_card_callback_ comment in header file), so it can't be reset
in OnBubbleClosed().

https://codereview.chromium.org/1396923003/diff/160001/chrome/browser/ui/auto...
chrome/browser/ui/autofill/save_card_bubble_controller_impl.cc:116:
save_card_bubble_view_ = nullptr;
On 2015/10/23 21:56:30, Evan Stade wrote:
> won't the above Close() call already null this out and update the icon. I'd
make
> this something like
> 
> if (save_card_bubble_view_)
>   save_card_bubble_view_->Close();
> else
>   OnBubbleClosed();

You are correct about "save_card_bubble_view_ = nullptr" being unnecessary. I
removed it. Shuffled the other stuff around a bit so that UpdateIcon() isn't
called twice when save_card_bubble_view_ != nullptr. In both cases
save_card_callback_.Reset() needs to be called before UpdateIcon() so I moved it
to the top.

Tested both cases and they still work properly.

[Evan Stade, 2015-10-23 23:24:02]

lgtm

[sky, 2015-10-26 20:56:26]

https://codereview.chromium.org/1396923003/diff/180001/chrome/browser/ui/brow...
File chrome/browser/ui/browser_command_controller.cc (right):

https://codereview.chromium.org/1396923003/diff/180001/chrome/browser/ui/brow...
chrome/browser/ui/browser_command_controller.cc:560: SaveCreditCard(browser_);
Shouldn't you only enable this if it actually does something? For example, don't
enable on mac yet.

https://codereview.chromium.org/1396923003/diff/180001/chrome/browser/ui/coco...
File chrome/browser/ui/cocoa/location_bar/location_bar_view_mac.mm (right):

https://codereview.chromium.org/1396923003/diff/180001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/location_bar/location_bar_view_mac.mm:190: // Not
implemented on Mac.
NOTIMPLEMENTED

https://codereview.chromium.org/1396923003/diff/180001/chrome/browser/ui/view...
File chrome/browser/ui/views/location_bar/location_bar_view.cc (right):

https://codereview.chromium.org/1396923003/diff/180001/chrome/browser/ui/view...
chrome/browser/ui/views/location_bar/location_bar_view.cc:148:
save_credit_card_icon_view_(NULL),
nullptr

https://codereview.chromium.org/1396923003/diff/180001/chrome/browser/ui/view...
File chrome/browser/ui/views/location_bar/save_credit_card_icon_view.cc (right):

https://codereview.chromium.org/1396923003/diff/180001/chrome/browser/ui/view...
chrome/browser/ui/views/location_bar/save_credit_card_icon_view.cc:43:
autofill::SaveCardBubbleControllerImpl::FromWebContents(web_contents);
Can this ever be null?

https://codereview.chromium.org/1396923003/diff/180001/chrome/browser/ui/view...
File chrome/browser/ui/views/location_bar/save_credit_card_icon_view.h (right):

https://codereview.chromium.org/1396923003/diff/180001/chrome/browser/ui/view...
chrome/browser/ui/views/location_bar/save_credit_card_icon_view.h:31: // May be
nullptr.
AFAICT browser_ is always non-null. Am I missing something? If I'm right remove
the check for non-null browser_ in GetBubble().

[bondd, 2015-10-27 01:06:39]

Addressed sky@ comments.

https://codereview.chromium.org/1396923003/diff/180001/chrome/browser/ui/brow...
File chrome/browser/ui/browser_command_controller.cc (right):

https://codereview.chromium.org/1396923003/diff/180001/chrome/browser/ui/brow...
chrome/browser/ui/browser_command_controller.cc:560: SaveCreditCard(browser_);
On 2015/10/26 20:56:26, sky wrote:
> Shouldn't you only enable this if it actually does something? For example,
don't
> enable on mac yet.

Done. I also ifdef'd SaveCreditCard() out of browser_commands.h and
browser_commands.cc. Please let me know if I misunderstood and you'd like me to
undo that.

In other news I found out an hour ago that "TOOLKIT_VIEWS" seems to be defined
for Mac builds too, even when Views is not enabled (???). If that's really the
case I'll change things from "#if defined (TOOLKIT_VIEWS)" to "#if
defined(OS_CHROMEOS) || defined(OS_LINUX) || defined(OS_WIN)" everywhere in this
CL before committing.

https://codereview.chromium.org/1396923003/diff/180001/chrome/browser/ui/coco...
File chrome/browser/ui/cocoa/location_bar/location_bar_view_mac.mm (right):

https://codereview.chromium.org/1396923003/diff/180001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/location_bar/location_bar_view_mac.mm:190: // Not
implemented on Mac.
On 2015/10/26 20:56:26, sky wrote:
> NOTIMPLEMENTED

Done. I had originally used a comment to match the surrounding code in
UpdateLocationBarVisibility() and UpdateOpenPDFInReaderPrompt() below. But you
are right, NOTIMPLEMENTED() is better because it actually enforces that the
function is not called. I was about to update the other "not implemented"
locations in this file to use NOTIMPLEMENTED() instead of just a comment, but I
realized that I'm not 100% sure that they are never called. Better to leave them
alone for now, since they're completely unrelated to the rest of this CL.

https://codereview.chromium.org/1396923003/diff/180001/chrome/browser/ui/view...
File chrome/browser/ui/views/location_bar/location_bar_view.cc (right):

https://codereview.chromium.org/1396923003/diff/180001/chrome/browser/ui/view...
chrome/browser/ui/views/location_bar/location_bar_view.cc:148:
save_credit_card_icon_view_(NULL),
On 2015/10/26 20:56:26, sky wrote:
> nullptr

Done. I had originally used NULL to match the surrounding code.

https://codereview.chromium.org/1396923003/diff/180001/chrome/browser/ui/view...
File chrome/browser/ui/views/location_bar/save_credit_card_icon_view.cc (right):

https://codereview.chromium.org/1396923003/diff/180001/chrome/browser/ui/view...
chrome/browser/ui/views/location_bar/save_credit_card_icon_view.cc:43:
autofill::SaveCardBubbleControllerImpl::FromWebContents(web_contents);
On 2015/10/26 20:56:26, sky wrote:
> Can this ever be null?

controller is created lazily in chrome_autofill_client.cc, so if
ConfirmSaveCreditCard() hasn't been called yet then controller will be null
here. GetBubble() is overriding superclass public member function
BubbleIconView::GetBubble(), so I don't think we have any guarantees that the
bubble will always be shown before this GetBubble() is called.

https://codereview.chromium.org/1396923003/diff/180001/chrome/browser/ui/view...
File chrome/browser/ui/views/location_bar/save_credit_card_icon_view.h (right):

https://codereview.chromium.org/1396923003/diff/180001/chrome/browser/ui/view...
chrome/browser/ui/views/location_bar/save_credit_card_icon_view.h:31: // May be
nullptr.
On 2015/10/26 20:56:26, sky wrote:
> AFAICT browser_ is always non-null. Am I missing something? If I'm right
remove
> the check for non-null browser_ in GetBubble().

browser_ is null if LocationBarView is created through
SimpleWebViewDialog::Init(). From the comment above SimpleWebViewDialog class in
simple_web_view_dialog.h it appears that SimpleWebViewDialog is only used on
ChromeOS for login screens, when Browser isn't running. It might make sense to
just not allocate any icon views that are not used by SimpleWebViewDialog, but
I'm wary of doing that, because I don't see any existing icons dealing with it
that way. There are a few other locations in LocationBarView that check for a
null browser_, and star_view.cc checks for null browser_. Thoughts?

[sky, 2015-10-27 20:58:25]

LGTM

https://codereview.chromium.org/1396923003/diff/180001/chrome/browser/ui/view...
File chrome/browser/ui/views/location_bar/save_credit_card_icon_view.cc (right):

https://codereview.chromium.org/1396923003/diff/180001/chrome/browser/ui/view...
chrome/browser/ui/views/location_bar/save_credit_card_icon_view.cc:43:
autofill::SaveCardBubbleControllerImpl::FromWebContents(web_contents);
On 2015/10/27 01:06:39, bondd wrote:
> On 2015/10/26 20:56:26, sky wrote:
> > Can this ever be null?
> 
> controller is created lazily in chrome_autofill_client.cc, so if
> ConfirmSaveCreditCard() hasn't been called yet then controller will be null
> here. GetBubble() is overriding superclass public member function
> BubbleIconView::GetBubble(), so I don't think we have any guarantees that the
> bubble will always be shown before this GetBubble() is called.

Acknowledged.

https://codereview.chromium.org/1396923003/diff/180001/chrome/browser/ui/view...
File chrome/browser/ui/views/location_bar/save_credit_card_icon_view.h (right):

https://codereview.chromium.org/1396923003/diff/180001/chrome/browser/ui/view...
chrome/browser/ui/views/location_bar/save_credit_card_icon_view.h:31: // May be
nullptr.
On 2015/10/27 01:06:39, bondd wrote:
> On 2015/10/26 20:56:26, sky wrote:
> > AFAICT browser_ is always non-null. Am I missing something? If I'm right
> remove
> > the check for non-null browser_ in GetBubble().
> 
> browser_ is null if LocationBarView is created through
> SimpleWebViewDialog::Init(). From the comment above SimpleWebViewDialog class
in
> simple_web_view_dialog.h it appears that SimpleWebViewDialog is only used on
> ChromeOS for login screens, when Browser isn't running. It might make sense to
> just not allocate any icon views that are not used by SimpleWebViewDialog, but
> I'm wary of doing that, because I don't see any existing icons dealing with it
> that way. There are a few other locations in LocationBarView that check for a
> null browser_, and star_view.cc checks for null browser_. Thoughts?

Acknowledged.

[bondd, 2015-10-28 21:17:31]

The CQ bit was checked by bondd@chromium.org

[bondd, 2015-10-28 21:17:32]

The patchset sent to the CQ was uploaded after l-g-t-m from
jdonnelly@chromium.org, estade@chromium.org, sky@chromium.org
Link to the patchset: https://codereview.chromium.org/1396923003/#ps240001
(title: "defined(TOOLKIT_VIEWS) -> defined(TOOLKIT_VIEWS) &&
!defined(OS_MACOSX).")

[commit-bot: I haz the power, 2015-10-28 21:18:58]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1396923003/240001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1396923003/240001

[commit-bot: I haz the power, 2015-10-28 22:39:40]

Committed patchset #13 (id:240001)

[commit-bot: I haz the power, 2015-10-28 22:40:29]

Patchset 13 (id:??) landed as
https://crrev.com/052b5f8112cf52524b92e92c504206eee9723bf9
Cr-Commit-Position: refs/heads/master@{#356673}