Implement newly saved card bubble for realz and update generated card bubble to

Implement newly saved card bubble for realz and update generated card bubble to match mocks and updated product requirements.

Because these bubbles are not particularly similar any more, this CL splits them into separate interfaces (and hence separate controllers, files, test, etc.).

Functional changes to generated card bubble:
- removed link from bottom
- added link to end of header text

Functional changes to new card bubble:
- removed bubble/window title
- added card description (icon + title + label)
- made link and header text real translations

R=estade@chromium.org
BUG=263156, 263158
TBR=sky@chromium.org (for 1 char change to ui/views/window/non_client_view.h)
NOTRY=True

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=216880

[Evan Stade, 2013-08-02 16:29:45]

given the differences in behavior and appearance, I'm not convinced that both
these bubbles should share a controller (or view).

https://codereview.chromium.org/21668003/diff/7001/chrome/browser/ui/autofill...
File chrome/browser/ui/autofill/autofill_credit_card_bubble_controller.cc
(right):

https://codereview.chromium.org/21668003/diff/7001/chrome/browser/ui/autofill...
chrome/browser/ui/autofill/autofill_credit_card_bubble_controller.cc:300: if
(!IsGeneratedCardBubble()) {
use else?

https://codereview.chromium.org/21668003/diff/7001/chrome/browser/ui/autofill...
chrome/browser/ui/autofill/autofill_credit_card_bubble_controller.cc:309:
card_desc_->title = card_number.substr(0, 4) + ASCIIToUTF16(" - ") +
I think this belongs alongside the other "ForDisplay" methods in CreditCard.
Also that way you can add unit testing for it (in particular different card
lengths might be worth testing).

https://codereview.chromium.org/21668003/diff/7001/chrome/browser/ui/autofill...
chrome/browser/ui/autofill/autofill_credit_card_bubble_controller.cc:321: if
(!IsGeneratedCardBubble()) {
nit: put this in the above block that's guarded by the same condition

https://codereview.chromium.org/21668003/diff/7001/chrome/browser/ui/autofill...
File chrome/browser/ui/autofill/autofill_credit_card_bubble_controller.h
(right):

https://codereview.chromium.org/21668003/diff/7001/chrome/browser/ui/autofill...
chrome/browser/ui/autofill/autofill_credit_card_bubble_controller.h:73: //  | 
Header text that will probably span  |
I object to this being called "header" text.

https://codereview.chromium.org/21668003/diff/7001/chrome/browser/ui/views/au...
File chrome/browser/ui/views/autofill/autofill_credit_card_bubble_views.cc
(right):

https://codereview.chromium.org/21668003/diff/7001/chrome/browser/ui/views/au...
chrome/browser/ui/views/autofill/autofill_credit_card_bubble_views.cc:89:
views::View* title_row = new views::View();
not really a title... the title is blank. Calling this a title is confusing
since it doesn't have the same styling as a title.

https://codereview.chromium.org/21668003/diff/7001/chrome/browser/ui/views/au...
chrome/browser/ui/views/autofill/autofill_credit_card_bubble_views.cc:97:
views::Label* title = new views::Label(card_desc->title);
ditto on CardDescription::title

https://codereview.chromium.org/21668003/diff/7001/chrome/browser/ui/views/au...
chrome/browser/ui/views/autofill/autofill_credit_card_bubble_views.cc:151: bool
title_empty = controller_->TitleText().empty();
yea, controller_->TitleText() vs. controller_->Description()->title --- too
confusing.

[Dan Beam, 2013-08-02 17:24:48]

On 2013/08/02 16:29:45, Evan Stade wrote:
> given the differences in behavior and appearance, I'm not convinced that both
> these bubbles should share a controller (or view).

I started to think that as well while implementing this.  They share much less
now.  Do you think the mocks are finalized enough that much else wont change in
the near future?  If so, I can break into multiple classes I suppose.

> 
>
https://codereview.chromium.org/21668003/diff/7001/chrome/browser/ui/autofill...
> File chrome/browser/ui/autofill/autofill_credit_card_bubble_controller.cc
> (right):
> 
>
https://codereview.chromium.org/21668003/diff/7001/chrome/browser/ui/autofill...
> chrome/browser/ui/autofill/autofill_credit_card_bubble_controller.cc:300: if
> (!IsGeneratedCardBubble()) {
> use else?
> 
>
https://codereview.chromium.org/21668003/diff/7001/chrome/browser/ui/autofill...
> chrome/browser/ui/autofill/autofill_credit_card_bubble_controller.cc:309:
> card_desc_->title = card_number.substr(0, 4) + ASCIIToUTF16(" - ") +
> I think this belongs alongside the other "ForDisplay" methods in CreditCard.
> Also that way you can add unit testing for it (in particular different card
> lengths might be worth testing).
> 
>
https://codereview.chromium.org/21668003/diff/7001/chrome/browser/ui/autofill...
> chrome/browser/ui/autofill/autofill_credit_card_bubble_controller.cc:321: if
> (!IsGeneratedCardBubble()) {
> nit: put this in the above block that's guarded by the same condition
> 
>
https://codereview.chromium.org/21668003/diff/7001/chrome/browser/ui/autofill...
> File chrome/browser/ui/autofill/autofill_credit_card_bubble_controller.h
> (right):
> 
>
https://codereview.chromium.org/21668003/diff/7001/chrome/browser/ui/autofill...
> chrome/browser/ui/autofill/autofill_credit_card_bubble_controller.h:73: //  | 
> Header text that will probably span  |
> I object to this being called "header" text.
> 
>
https://codereview.chromium.org/21668003/diff/7001/chrome/browser/ui/views/au...
> File chrome/browser/ui/views/autofill/autofill_credit_card_bubble_views.cc
> (right):
> 
>
https://codereview.chromium.org/21668003/diff/7001/chrome/browser/ui/views/au...
> chrome/browser/ui/views/autofill/autofill_credit_card_bubble_views.cc:89:
> views::View* title_row = new views::View();
> not really a title... the title is blank. Calling this a title is confusing
> since it doesn't have the same styling as a title.
> 
>
https://codereview.chromium.org/21668003/diff/7001/chrome/browser/ui/views/au...
> chrome/browser/ui/views/autofill/autofill_credit_card_bubble_views.cc:97:
> views::Label* title = new views::Label(card_desc->title);
> ditto on CardDescription::title
> 
>
https://codereview.chromium.org/21668003/diff/7001/chrome/browser/ui/views/au...
> chrome/browser/ui/views/autofill/autofill_credit_card_bubble_views.cc:151:
bool
> title_empty = controller_->TitleText().empty();
> yea, controller_->TitleText() vs. controller_->Description()->title --- too
> confusing.

[Dan Beam, 2013-08-06 02:41:34]

https://codereview.chromium.org/21668003/diff/7001/chrome/browser/ui/autofill...
File chrome/browser/ui/autofill/autofill_credit_card_bubble_controller.cc
(right):

https://codereview.chromium.org/21668003/diff/7001/chrome/browser/ui/autofill...
chrome/browser/ui/autofill/autofill_credit_card_bubble_controller.cc:300: if
(!IsGeneratedCardBubble()) {
On 2013/08/02 16:29:45, Evan Stade wrote:
> use else?

doesn't exist any more

https://codereview.chromium.org/21668003/diff/7001/chrome/browser/ui/autofill...
chrome/browser/ui/autofill/autofill_credit_card_bubble_controller.cc:309:
card_desc_->title = card_number.substr(0, 4) + ASCIIToUTF16(" - ") +
On 2013/08/02 16:29:45, Evan Stade wrote:
> I think this belongs alongside the other "ForDisplay" methods in CreditCard.
> Also that way you can add unit testing for it (in particular different card
> lengths might be worth testing).

changed to TypeAndLastFourDigits() and started thread on bug

https://codereview.chromium.org/21668003/diff/7001/chrome/browser/ui/autofill...
chrome/browser/ui/autofill/autofill_credit_card_bubble_controller.cc:321: if
(!IsGeneratedCardBubble()) {
On 2013/08/02 16:29:45, Evan Stade wrote:
> nit: put this in the above block that's guarded by the same condition

doesn't exist any more

https://codereview.chromium.org/21668003/diff/7001/chrome/browser/ui/autofill...
File chrome/browser/ui/autofill/autofill_credit_card_bubble_controller.h
(right):

https://codereview.chromium.org/21668003/diff/7001/chrome/browser/ui/autofill...
chrome/browser/ui/autofill/autofill_credit_card_bubble_controller.h:73: //  | 
Header text that will probably span  |
On 2013/08/02 16:29:45, Evan Stade wrote:
> I object to this being called "header" text.

Done.

https://codereview.chromium.org/21668003/diff/7001/chrome/browser/ui/views/au...
File chrome/browser/ui/views/autofill/autofill_credit_card_bubble_views.cc
(right):

https://codereview.chromium.org/21668003/diff/7001/chrome/browser/ui/views/au...
chrome/browser/ui/views/autofill/autofill_credit_card_bubble_views.cc:89:
views::View* title_row = new views::View();
On 2013/08/02 16:29:45, Evan Stade wrote:
> not really a title... the title is blank. Calling this a title is confusing
> since it doesn't have the same styling as a title.

Done.

https://codereview.chromium.org/21668003/diff/7001/chrome/browser/ui/views/au...
chrome/browser/ui/views/autofill/autofill_credit_card_bubble_views.cc:97:
views::Label* title = new views::Label(card_desc->title);
On 2013/08/02 16:29:45, Evan Stade wrote:
> ditto on CardDescription::title

Done.

https://codereview.chromium.org/21668003/diff/7001/chrome/browser/ui/views/au...
chrome/browser/ui/views/autofill/autofill_credit_card_bubble_views.cc:151: bool
title_empty = controller_->TitleText().empty();
On 2013/08/02 16:29:45, Evan Stade wrote:
> yea, controller_->TitleText() vs. controller_->Description()->title --- too
> confusing.

Done.

[Evan Stade, 2013-08-06 22:06:24]

https://codereview.chromium.org/21668003/diff/42001/chrome/browser/ui/autofil...
File chrome/browser/ui/autofill/generated_credit_card_bubble.cc (right):

https://codereview.chromium.org/21668003/diff/42001/chrome/browser/ui/autofil...
chrome/browser/ui/autofill/generated_credit_card_bubble.cc:1: // Copyright 2013
The Chromium Authors. All rights reserved.
any differences to this file (and related files) besides the file rename?

https://codereview.chromium.org/21668003/diff/42001/chrome/browser/ui/autofil...
File chrome/browser/ui/autofill/new_credit_card_bubble_controller.cc (right):

https://codereview.chromium.org/21668003/diff/42001/chrome/browser/ui/autofil...
chrome/browser/ui/autofill/new_credit_card_bubble_controller.cc:71: #if
!defined(OS_ANDROID)
can you ping aruslan to see what plans if any android has to implement this
bubble? I'd just as soon keep the controller out of android completely for the
time being.

https://codereview.chromium.org/21668003/diff/42001/chrome/browser/ui/autofil...
chrome/browser/ui/autofill/new_credit_card_bubble_controller.cc:125:
std::vector<AutofillFieldType> fields;
what you want is to create a DataModelWrapper for |billing_profile_| and use
GetDisplayText.

https://codereview.chromium.org/21668003/diff/42001/chrome/browser/ui/autofil...
File chrome/browser/ui/autofill/new_credit_card_bubble_controller.h (right):

https://codereview.chromium.org/21668003/diff/42001/chrome/browser/ui/autofil...
chrome/browser/ui/autofill/new_credit_card_bubble_controller.h:44: //  | 
Contents text that may span multiple  |
I think this is obsolete already :)

https://codereview.chromium.org/21668003/diff/42001/chrome/browser/ui/views/a...
File chrome/browser/ui/views/autofill/generated_credit_card_bubble_views.cc
(right):

https://codereview.chromium.org/21668003/diff/42001/chrome/browser/ui/views/a...
chrome/browser/ui/views/autofill/generated_credit_card_bubble_views.cc:1: //
Copyright 2013 The Chromium Authors. All rights reserved.
any differences to this file (and related files) besides the file rename?

https://codereview.chromium.org/21668003/diff/42001/chrome/browser/ui/views/a...
File chrome/browser/ui/views/autofill/new_credit_card_bubble_views.cc (right):

https://codereview.chromium.org/21668003/diff/42001/chrome/browser/ui/views/a...
chrome/browser/ui/views/autofill/new_credit_card_bubble_views.cc:29:
chrome::GetActiveDesktop());
I feel that |browser| should be sorted out based on the web contents that
initiated the dialog. This seems to leave open the possibility that the wrong
browser window will be used.

https://codereview.chromium.org/21668003/diff/42001/chrome/browser/ui/views/a...
chrome/browser/ui/views/autofill/new_credit_card_bubble_views.cc:119:
set_margins(gfx::Insets(18, 19, 18, 18));
heinous. Perhaps you can create these insets via an EmptyBorder which is hidden
behind some static function call like CreateBorderForBubble() so you don't have
to repeat the constants over and over.

[Dan Beam, 2013-08-07 02:30:22]

https://codereview.chromium.org/21668003/diff/42001/chrome/browser/ui/autofil...
File chrome/browser/ui/autofill/generated_credit_card_bubble.cc (right):

https://codereview.chromium.org/21668003/diff/42001/chrome/browser/ui/autofil...
chrome/browser/ui/autofill/generated_credit_card_bubble.cc:1: // Copyright 2013
The Chromium Authors. All rights reserved.
On 2013/08/06 22:06:25, Evan Stade wrote:
> any differences to this file (and related files) besides the file rename?

not really, no IsHiding() or kContentWidth in NewCreditCardBubble

https://codereview.chromium.org/21668003/diff/42001/chrome/browser/ui/autofil...
File chrome/browser/ui/autofill/new_credit_card_bubble_controller.cc (right):

https://codereview.chromium.org/21668003/diff/42001/chrome/browser/ui/autofil...
chrome/browser/ui/autofill/new_credit_card_bubble_controller.cc:71: #if
!defined(OS_ANDROID)
On 2013/08/06 22:06:25, Evan Stade wrote:
> can you ping aruslan to see what plans if any android has to implement this
> bubble? I'd just as soon keep the controller out of android completely for the
> time being.

the plan is toasts, which seem to need much less code (and definitely not these
controllers), so I've excluded some code on Android

https://codereview.chromium.org/21668003/diff/42001/chrome/browser/ui/autofil...
chrome/browser/ui/autofill/new_credit_card_bubble_controller.cc:125:
std::vector<AutofillFieldType> fields;
On 2013/08/06 22:06:25, Evan Stade wrote:
> what you want is to create a DataModelWrapper for |billing_profile_| and use
> GetDisplayText.

Done. (but there's no email...)

https://codereview.chromium.org/21668003/diff/42001/chrome/browser/ui/autofil...
File chrome/browser/ui/autofill/new_credit_card_bubble_controller.h (right):

https://codereview.chromium.org/21668003/diff/42001/chrome/browser/ui/autofil...
chrome/browser/ui/autofill/new_credit_card_bubble_controller.h:44: //  | 
Contents text that may span multiple  |
On 2013/08/06 22:06:25, Evan Stade wrote:
> I think this is obsolete already :)

Updated.

https://codereview.chromium.org/21668003/diff/42001/chrome/browser/ui/views/a...
File chrome/browser/ui/views/autofill/generated_credit_card_bubble_views.cc
(right):

https://codereview.chromium.org/21668003/diff/42001/chrome/browser/ui/views/a...
chrome/browser/ui/views/autofill/generated_credit_card_bubble_views.cc:1: //
Copyright 2013 The Chromium Authors. All rights reserved.
On 2013/08/06 22:06:25, Evan Stade wrote:
> any differences to this file (and related files) besides the file rename?

Init() + link listening is split and slightly different in both

https://codereview.chromium.org/21668003/diff/42001/chrome/browser/ui/views/a...
File chrome/browser/ui/views/autofill/new_credit_card_bubble_views.cc (right):

https://codereview.chromium.org/21668003/diff/42001/chrome/browser/ui/views/a...
chrome/browser/ui/views/autofill/new_credit_card_bubble_views.cc:29:
chrome::GetActiveDesktop());
On 2013/08/06 22:06:25, Evan Stade wrote:
> I feel that |browser| should be sorted out based on the web contents that
> initiated the dialog. This seems to leave open the possibility that the wrong
> browser window will be used.

popups don't have a wrench menu, which is why I used FindTabbedBrowser(). also
this bubble knows little about the web contents that initiated it now that it's
only passed a Profile. 99% of the time, the browser holding the initiating web
contents will be the same as the last activated tabbed browser.

https://codereview.chromium.org/21668003/diff/42001/chrome/browser/ui/views/a...
chrome/browser/ui/views/autofill/new_credit_card_bubble_views.cc:119:
set_margins(gfx::Insets(18, 19, 18, 18));
On 2013/08/06 22:06:25, Evan Stade wrote:
> heinous. Perhaps you can create these insets via an EmptyBorder which is
hidden
> behind some static function call like CreateBorderForBubble() so you don't
have
> to repeat the constants over and over.

check what I did, see if you like

[Dan Beam, 2013-08-07 22:11:09]

ping

[Evan Stade, 2013-08-07 23:31:20]

+msw for bubble changes (Mike, are you the right person for bubble interface
changes? if not, could you punt to whoever is)

https://chromiumcodereview.appspot.com/21668003/diff/42001/chrome/browser/ui/...
File chrome/browser/ui/views/autofill/new_credit_card_bubble_views.cc (right):

https://chromiumcodereview.appspot.com/21668003/diff/42001/chrome/browser/ui/...
chrome/browser/ui/views/autofill/new_credit_card_bubble_views.cc:29:
chrome::GetActiveDesktop());
On 2013/08/07 02:30:22, Dan Beam wrote:
> On 2013/08/06 22:06:25, Evan Stade wrote:
> > I feel that |browser| should be sorted out based on the web contents that
> > initiated the dialog. This seems to leave open the possibility that the
wrong
> > browser window will be used.
> 
> popups don't have a wrench menu, which is why I used FindTabbedBrowser(). also
> this bubble knows little about the web contents that initiated it now that
it's
> only passed a Profile. 99% of the time, the browser holding the initiating web
> contents will be the same as the last activated tabbed browser.

that doesn't seem right to me, we should still show the popup somewhere in the
current (popup) browser window because the last active tabbed browser might be
minimized, on another desktop, behind some other window, or otherwise not
noticeable. What happens with web autofill when we want to save a cc in a popup
window?

It also seems crappy that a website could show a popup window, trigger the
dialog, then close the popup window, thereby hiding this bubble before the user
has a chance to see it.

[Dan Beam, 2013-08-08 05:23:48]

https://chromiumcodereview.appspot.com/21668003/diff/42001/chrome/browser/ui/...
File chrome/browser/ui/views/autofill/new_credit_card_bubble_views.cc (right):

https://chromiumcodereview.appspot.com/21668003/diff/42001/chrome/browser/ui/...
chrome/browser/ui/views/autofill/new_credit_card_bubble_views.cc:29:
chrome::GetActiveDesktop());
On 2013/08/07 23:31:21, Evan Stade wrote:
> On 2013/08/07 02:30:22, Dan Beam wrote:
> > On 2013/08/06 22:06:25, Evan Stade wrote:
> > > I feel that |browser| should be sorted out based on the web contents that
> > > initiated the dialog. This seems to leave open the possibility that the
> wrong
> > > browser window will be used.
> > 
> > popups don't have a wrench menu, which is why I used FindTabbedBrowser().
also
> > this bubble knows little about the web contents that initiated it now that
> it's
> > only passed a Profile. 99% of the time, the browser holding the initiating
web
> > contents will be the same as the last activated tabbed browser.
> 
> that doesn't seem right to me, we should still show the popup somewhere in the
> current (popup) browser window because the last active tabbed browser might be
> minimized, on another desktop, behind some other window, or otherwise not
> noticeable. What happens with web autofill when we want to save a cc in a
popup
> window?

asked jstark@ on the bug

> 
> It also seems crappy that a website could show a popup window, trigger the
> dialog, then close the popup window, thereby hiding this bubble before the
user
> has a chance to see it.

^ don't know a good solution here unless we want to get into heuristics (i.e.
the bubble has to show for more than 2s before closing else it latches on to
another window, etc.)

[Dan Beam, 2013-08-08 05:39:52]

On 2013/08/08 05:23:48, Dan Beam wrote:
>
https://chromiumcodereview.appspot.com/21668003/diff/42001/chrome/browser/ui/...
> File chrome/browser/ui/views/autofill/new_credit_card_bubble_views.cc (right):
> 
>
https://chromiumcodereview.appspot.com/21668003/diff/42001/chrome/browser/ui/...
> chrome/browser/ui/views/autofill/new_credit_card_bubble_views.cc:29:
> chrome::GetActiveDesktop());
> On 2013/08/07 23:31:21, Evan Stade wrote:
> > On 2013/08/07 02:30:22, Dan Beam wrote:
> > > On 2013/08/06 22:06:25, Evan Stade wrote:
> > > > I feel that |browser| should be sorted out based on the web contents
that
> > > > initiated the dialog. This seems to leave open the possibility that the
> > wrong
> > > > browser window will be used.
> > > 
> > > popups don't have a wrench menu, which is why I used FindTabbedBrowser().
> also
> > > this bubble knows little about the web contents that initiated it now that
> > it's
> > > only passed a Profile. 99% of the time, the browser holding the initiating
> web
> > > contents will be the same as the last activated tabbed browser.
> > 
> > that doesn't seem right to me, we should still show the popup somewhere in
the
> > current (popup) browser window because the last active tabbed browser might
be
> > minimized, on another desktop, behind some other window, or otherwise not
> > noticeable. What happens with web autofill when we want to save a cc in a
> popup
> > window?
> 
> asked jstark@ on the bug

note: I don't think this ^ decision should block this CL, as it's pretty easy to
change

> 
> > 
> > It also seems crappy that a website could show a popup window, trigger the
> > dialog, then close the popup window, thereby hiding this bubble before the
> user
> > has a chance to see it.
> 
> ^ don't know a good solution here unless we want to get into heuristics (i.e.
> the bubble has to show for more than 2s before closing else it latches on to
> another window, etc.)

[msw, 2013-08-08 18:34:25]

Yep, I'm the right OWNER for Views bubbles.

https://chromiumcodereview.appspot.com/21668003/diff/54002/ui/views/bubble/bu...
File ui/views/bubble/bubble_delegate.cc (right):

https://chromiumcodereview.appspot.com/21668003/diff/54002/ui/views/bubble/bu...
ui/views/bubble/bubble_delegate.cc:213: BubbleFrameView* frame = new
BubbleFrameView(match_title_margins_ ?
Could individual bubbles instead call BubbleDelegateView::set_margins with
values obtained from a static BubbleFrameView::GetTitleInsets similar to the
function you've already added there? I don't like the flag clobbering the
|margins()| value and manipulating the preferred size internally.

[Dan Beam, 2013-08-08 21:15:58]

https://chromiumcodereview.appspot.com/21668003/diff/54002/ui/views/bubble/bu...
File ui/views/bubble/bubble_delegate.cc (right):

https://chromiumcodereview.appspot.com/21668003/diff/54002/ui/views/bubble/bu...
ui/views/bubble/bubble_delegate.cc:213: BubbleFrameView* frame = new
BubbleFrameView(match_title_margins_ ?
On 2013/08/08 18:34:26, msw wrote:
> Could individual bubbles instead call BubbleDelegateView::set_margins with
> values obtained from a static BubbleFrameView::GetTitleInsets similar to the
> function you've already added there? I don't like the flag clobbering the
> |margins()| value and manipulating the preferred size internally.

Done.

[msw, 2013-08-08 21:56:51]

Bubble LGTM with nits and a q.

https://chromiumcodereview.appspot.com/21668003/diff/124001/ui/views/bubble/b...
File ui/views/bubble/bubble_delegate.cc (right):

https://chromiumcodereview.appspot.com/21668003/diff/124001/ui/views/bubble/b...
ui/views/bubble/bubble_delegate.cc:385: NonClientFrameView* fv =
GetWidget()->non_client_view()->frame_view();
nit: use GetBubbleFrameView(), avoid acronym identifiers.

https://chromiumcodereview.appspot.com/21668003/diff/124001/ui/views/bubble/b...
File ui/views/bubble/bubble_delegate.h (right):

https://chromiumcodereview.appspot.com/21668003/diff/124001/ui/views/bubble/b...
ui/views/bubble/bubble_delegate.h:129: // Get the preferred size of a bubble
that matches it's title's width.
nit: s/it's/its/

https://chromiumcodereview.appspot.com/21668003/diff/124001/ui/views/bubble/b...
File ui/views/bubble/bubble_frame_view.cc (right):

https://chromiumcodereview.appspot.com/21668003/diff/124001/ui/views/bubble/b...
ui/views/bubble/bubble_frame_view.cc:59: return gfx::Insets(0, kTitleLeftInset,
kTitleTopInset, kTitleLeftInset);
q: is kTitleLeftInset actually desired for the right side?

[Evan Stade, 2013-08-08 22:43:20]

mostly nits

https://codereview.chromium.org/21668003/diff/124001/chrome/browser/ui/autofi...
File chrome/browser/ui/autofill/generated_credit_card_bubble.h (right):

https://codereview.chromium.org/21668003/diff/124001/chrome/browser/ui/autofi...
chrome/browser/ui/autofill/generated_credit_card_bubble.h:23: class
GeneratedCreditCardBubble {
I think this should be called |GeneratedCreditCardBubbleView|. i.e. this is the
interface to the view, implemented by Views, Cocoa, etc. in
GeneratedCreditCardBubbleViews, GeneratedCreditCardBubbleCocoa, etc.

https://codereview.chromium.org/21668003/diff/124001/chrome/browser/ui/autofi...
File chrome/browser/ui/autofill/generated_credit_card_bubble_controller.cc
(right):

https://codereview.chromium.org/21668003/diff/124001/chrome/browser/ui/autofi...
chrome/browser/ui/autofill/generated_credit_card_bubble_controller.cc:212:
contents_text_.append(ASCIIToUTF16(" ") + learn_more);
does this not work: string16(" ")

https://codereview.chromium.org/21668003/diff/124001/chrome/browser/ui/autofi...
chrome/browser/ui/autofill/generated_credit_card_bubble_controller.cc:255:
weak_ptr_factory_.InvalidateWeakPtrs();
I don't see why this is necessary. Calling Hide() should be enough. If the
bubble does try to call back into controller_ after Hide(), it shouldn't do any
harm (unless |this| has been destroyed, but in that case the factory will
already have invalidated all the pointers it created)

https://codereview.chromium.org/21668003/diff/124001/chrome/browser/ui/autofi...
File chrome/browser/ui/autofill/new_credit_card_bubble.h (right):

https://codereview.chromium.org/21668003/diff/124001/chrome/browser/ui/autofi...
chrome/browser/ui/autofill/new_credit_card_bubble.h:14:
////////////////////////////////////////////////////////////////////////////////
This comment block is overkill, why not just

// class level comment lorem ipsum ...
// ...
class Foo {

};

https://codereview.chromium.org/21668003/diff/124001/chrome/browser/ui/autofi...
chrome/browser/ui/autofill/new_credit_card_bubble.h:23: class
NewCreditCardBubble {
ditto on naming convention.

https://codereview.chromium.org/21668003/diff/124001/chrome/browser/ui/views/...
File chrome/browser/ui/views/autofill/generated_credit_card_bubble_views.cc
(right):

https://codereview.chromium.org/21668003/diff/124001/chrome/browser/ui/views/...
chrome/browser/ui/views/autofill/generated_credit_card_bubble_views.cc:26: const
base::WeakPtr<GeneratedCreditCardBubbleController>& controller) {
this doesn't need to be a weak pointer.

https://codereview.chromium.org/21668003/diff/124001/chrome/browser/ui/views/...
chrome/browser/ui/views/autofill/generated_credit_card_bubble_views.cc:58:
return controller_->TitleText();
check |controller_|

[Dan Beam, 2013-08-09 01:47:57]

https://chromiumcodereview.appspot.com/21668003/diff/124001/chrome/browser/ui...
File chrome/browser/ui/autofill/generated_credit_card_bubble.h (right):

https://chromiumcodereview.appspot.com/21668003/diff/124001/chrome/browser/ui...
chrome/browser/ui/autofill/generated_credit_card_bubble.h:23: class
GeneratedCreditCardBubble {
On 2013/08/08 22:43:20, Evan Stade wrote:
> I think this should be called |GeneratedCreditCardBubbleView|. i.e. this is
the
> interface to the view, implemented by Views, Cocoa, etc. in
> GeneratedCreditCardBubbleViews, GeneratedCreditCardBubbleCocoa, etc.

Done.

https://chromiumcodereview.appspot.com/21668003/diff/124001/chrome/browser/ui...
File chrome/browser/ui/autofill/generated_credit_card_bubble_controller.cc
(right):

https://chromiumcodereview.appspot.com/21668003/diff/124001/chrome/browser/ui...
chrome/browser/ui/autofill/generated_credit_card_bubble_controller.cc:212:
contents_text_.append(ASCIIToUTF16(" ") + learn_more);
On 2013/08/08 22:43:20, Evan Stade wrote:
> does this not work: string16(" ")

Nope.

https://chromiumcodereview.appspot.com/21668003/diff/124001/chrome/browser/ui...
chrome/browser/ui/autofill/generated_credit_card_bubble_controller.cc:255:
weak_ptr_factory_.InvalidateWeakPtrs();
On 2013/08/08 22:43:20, Evan Stade wrote:
> I don't see why this is necessary. Calling Hide() should be enough. If the
> bubble does try to call back into controller_ after Hide(), it shouldn't do
any
> harm (unless |this| has been destroyed, but in that case the factory will
> already have invalidated all the pointers it created)

Done.

https://chromiumcodereview.appspot.com/21668003/diff/124001/chrome/browser/ui...
File chrome/browser/ui/autofill/new_credit_card_bubble.h (right):

https://chromiumcodereview.appspot.com/21668003/diff/124001/chrome/browser/ui...
chrome/browser/ui/autofill/new_credit_card_bubble.h:14:
////////////////////////////////////////////////////////////////////////////////
On 2013/08/08 22:43:20, Evan Stade wrote:
> This comment block is overkill, why not just
> 
> // class level comment lorem ipsum ...
> // ...
> class Foo {
> 
> };

Done.

https://chromiumcodereview.appspot.com/21668003/diff/124001/chrome/browser/ui...
chrome/browser/ui/autofill/new_credit_card_bubble.h:23: class
NewCreditCardBubble {
On 2013/08/08 22:43:20, Evan Stade wrote:
> ditto on naming convention.

Done.

https://chromiumcodereview.appspot.com/21668003/diff/124001/chrome/browser/ui...
File chrome/browser/ui/views/autofill/generated_credit_card_bubble_views.cc
(right):

https://chromiumcodereview.appspot.com/21668003/diff/124001/chrome/browser/ui...
chrome/browser/ui/views/autofill/generated_credit_card_bubble_views.cc:26: const
base::WeakPtr<GeneratedCreditCardBubbleController>& controller) {
On 2013/08/08 22:43:20, Evan Stade wrote:
> this doesn't need to be a weak pointer.

Done.

https://chromiumcodereview.appspot.com/21668003/diff/124001/chrome/browser/ui...
chrome/browser/ui/views/autofill/generated_credit_card_bubble_views.cc:58:
return controller_->TitleText();
On 2013/08/08 22:43:20, Evan Stade wrote:
> check |controller_|

Done.

https://chromiumcodereview.appspot.com/21668003/diff/124001/ui/views/bubble/b...
File ui/views/bubble/bubble_delegate.cc (right):

https://chromiumcodereview.appspot.com/21668003/diff/124001/ui/views/bubble/b...
ui/views/bubble/bubble_delegate.cc:385: NonClientFrameView* fv =
GetWidget()->non_client_view()->frame_view();
On 2013/08/08 21:56:52, msw wrote:
> nit: use GetBubbleFrameView(), avoid acronym identifiers.

Done.

https://chromiumcodereview.appspot.com/21668003/diff/124001/ui/views/bubble/b...
File ui/views/bubble/bubble_delegate.h (right):

https://chromiumcodereview.appspot.com/21668003/diff/124001/ui/views/bubble/b...
ui/views/bubble/bubble_delegate.h:129: // Get the preferred size of a bubble
that matches it's title's width.
On 2013/08/08 21:56:52, msw wrote:
> nit: s/it's/its/

Done.

https://chromiumcodereview.appspot.com/21668003/diff/124001/ui/views/bubble/b...
File ui/views/bubble/bubble_frame_view.cc (right):

https://chromiumcodereview.appspot.com/21668003/diff/124001/ui/views/bubble/b...
ui/views/bubble/bubble_frame_view.cc:59: return gfx::Insets(0, kTitleLeftInset,
kTitleTopInset, kTitleLeftInset);
On 2013/08/08 21:56:52, msw wrote:
> q: is kTitleLeftInset actually desired for the right side?

I've updated this to be more clear by returning the title's insets here and
using them in an easier to understand way (I think) in each bubble's ctor.

[Dan Beam, 2013-08-09 02:11:25]

+pkasting@ for *location_bar*
+thestig@ for chrome/*.gypi, chrome/test/, chrome/browser/prefs/

msw@: FYI, I didn't end up needing to make that many changes in ui/views/bubble/

[Lei Zhang, 2013-08-09 04:51:56]

https://chromiumcodereview.appspot.com/21668003/diff/151001/chrome/browser/pr...
File chrome/browser/prefs/browser_prefs.cc (right):

https://chromiumcodereview.appspot.com/21668003/diff/151001/chrome/browser/pr...
chrome/browser/prefs/browser_prefs.cc:76: #include
"chrome/browser/ui/autofill/generated_credit_card_bubble_controller.h"
Do this down near line 157 instead.

https://chromiumcodereview.appspot.com/21668003/diff/151001/chrome/chrome_bro...
File chrome/chrome_browser_ui.gypi (right):

https://chromiumcodereview.appspot.com/21668003/diff/151001/chrome/chrome_bro...
chrome/chrome_browser_ui.gypi:2907:
'browser/ui/autofill/generated_credit_card_bubble_view.cc',
Same as the test gypi file.

https://chromiumcodereview.appspot.com/21668003/diff/151001/chrome/chrome_tes...
File chrome/chrome_tests_unit.gypi (right):

https://chromiumcodereview.appspot.com/21668003/diff/151001/chrome/chrome_tes...
chrome/chrome_tests_unit.gypi:2555:
'browser/ui/autofill/autofill_dialog_controller_unittest.cc',
Rather than adding the os!=android section here, just stick with the existing
paradigm - list all the files, and exclude the ones you don't use in the
OS==android section.

[Peter Kasting, 2013-08-09 18:15:27]

LGTM

https://chromiumcodereview.appspot.com/21668003/diff/151001/chrome/browser/ui...
File chrome/browser/ui/omnibox/location_bar.h (right):

https://chromiumcodereview.appspot.com/21668003/diff/151001/chrome/browser/ui...
chrome/browser/ui/omnibox/location_bar.h:67: // Updates the generated credit
card view. This views serves as an anchor for
Nit: views -> view

https://chromiumcodereview.appspot.com/21668003/diff/151001/chrome/browser/ui...
File chrome/browser/ui/views/location_bar/generated_credit_card_view.h (right):

https://chromiumcodereview.appspot.com/21668003/diff/151001/chrome/browser/ui...
chrome/browser/ui/views/location_bar/generated_credit_card_view.h:25: //  An
icon that shows up in the omnibox after successfully generating credit
Nit: credit -> a credit

[Dan Beam, 2013-08-09 18:56:54]

https://chromiumcodereview.appspot.com/21668003/diff/151001/chrome/browser/pr...
File chrome/browser/prefs/browser_prefs.cc (right):

https://chromiumcodereview.appspot.com/21668003/diff/151001/chrome/browser/pr...
chrome/browser/prefs/browser_prefs.cc:76: #include
"chrome/browser/ui/autofill/generated_credit_card_bubble_controller.h"
On 2013/08/09 04:51:57, Lei Zhang wrote:
> Do this down near line 157 instead.

Done.

https://chromiumcodereview.appspot.com/21668003/diff/151001/chrome/browser/ui...
File chrome/browser/ui/omnibox/location_bar.h (right):

https://chromiumcodereview.appspot.com/21668003/diff/151001/chrome/browser/ui...
chrome/browser/ui/omnibox/location_bar.h:67: // Updates the generated credit
card view. This views serves as an anchor for
On 2013/08/09 18:15:28, Peter Kasting wrote:
> Nit: views -> view

Done.

https://chromiumcodereview.appspot.com/21668003/diff/151001/chrome/browser/ui...
File chrome/browser/ui/views/location_bar/generated_credit_card_view.h (right):

https://chromiumcodereview.appspot.com/21668003/diff/151001/chrome/browser/ui...
chrome/browser/ui/views/location_bar/generated_credit_card_view.h:25: //  An
icon that shows up in the omnibox after successfully generating credit
On 2013/08/09 18:15:28, Peter Kasting wrote:
> Nit: credit -> a credit

Done.

https://chromiumcodereview.appspot.com/21668003/diff/151001/chrome/chrome_bro...
File chrome/chrome_browser_ui.gypi (right):

https://chromiumcodereview.appspot.com/21668003/diff/151001/chrome/chrome_bro...
chrome/chrome_browser_ui.gypi:2907:
'browser/ui/autofill/generated_credit_card_bubble_view.cc',
On 2013/08/09 04:51:57, Lei Zhang wrote:
> Same as the test gypi file.

Done.

https://chromiumcodereview.appspot.com/21668003/diff/151001/chrome/chrome_tes...
File chrome/chrome_tests_unit.gypi (right):

https://chromiumcodereview.appspot.com/21668003/diff/151001/chrome/chrome_tes...
chrome/chrome_tests_unit.gypi:2555:
'browser/ui/autofill/autofill_dialog_controller_unittest.cc',
On 2013/08/09 04:51:57, Lei Zhang wrote:
> Rather than adding the os!=android section here, just stick with the existing
> paradigm - list all the files, and exclude the ones you don't use in the
> OS==android section.

Done.

[Evan Stade, 2013-08-09 23:29:37]

lgtm

[Dan Beam, 2013-08-09 23:30:22]

https://chromiumcodereview.appspot.com/21668003/diff/169001/chrome/chrome_bro...
File chrome/chrome_browser_ui.gypi (right):

https://chromiumcodereview.appspot.com/21668003/diff/169001/chrome/chrome_bro...
chrome/chrome_browser_ui.gypi:2889: ['exclude',
'^browser/ui/autofill/.*credit_card_bubble.*'],
thestig@: ^ this fixed, thanks for your help

[Lei Zhang, 2013-08-09 23:41:19]

lgtm

[commit-bot: I haz the power, 2013-08-10 00:05:17]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dbeam@chromium.org/21668003/178001

[commit-bot: I haz the power, 2013-08-10 00:28:10]

Step "update" is always a major failure.
Look at the try server FAQ for more details.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win&number...

[commit-bot: I haz the power, 2013-08-10 00:33:56]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dbeam@chromium.org/21668003/178001

[commit-bot: I haz the power, 2013-08-10 08:57:02]

Retried try job too often on win_rel for step(s) browser_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[commit-bot: I haz the power, 2013-08-10 10:39:21]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dbeam@chromium.org/21668003/178001

[commit-bot: I haz the power, 2013-08-10 10:39:38]

Failed to apply patch for chrome/browser/prefs/browser_prefs.cc:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file chrome/browser/prefs/browser_prefs.cc
  Hunk #1 FAILED at 71.
  Hunk #2 succeeded at 156 (offset 3 lines).
  Hunk #3 FAILED at 289.
  2 out of 3 hunks FAILED -- saving rejects to file
chrome/browser/prefs/browser_prefs.cc.rej

Patch:       chrome/browser/prefs/browser_prefs.cc
Index: chrome/browser/prefs/browser_prefs.cc
diff --git a/chrome/browser/prefs/browser_prefs.cc
b/chrome/browser/prefs/browser_prefs.cc
index
f954807629df35883c88fcefd550948e42e9bc13..4bb4c05d8609136d8a5963d675e741c78b66b2e0
100644
--- a/chrome/browser/prefs/browser_prefs.cc
+++ b/chrome/browser/prefs/browser_prefs.cc
@@ -71,7 +71,6 @@
 #include "chrome/browser/translate/translate_prefs.h"
 #include "chrome/browser/ui/alternate_error_tab_observer.h"
 #include "chrome/browser/ui/app_list/app_list_service.h"
-#include "chrome/browser/ui/autofill/autofill_credit_card_bubble_controller.h"
 #include "chrome/browser/ui/autofill/autofill_dialog_controller_impl.h"
 #include "chrome/browser/ui/browser_ui_prefs.h"
 #include "chrome/browser/ui/network_profile_bubble.h"
@@ -153,6 +152,8 @@
 
 #if defined(OS_ANDROID)
 #include "chrome/browser/ui/webui/ntp/android/promo_handler.h"
+#else
+#include "chrome/browser/ui/autofill/generated_credit_card_bubble_controller.h"
 #endif
 
 #if defined(ENABLE_PLUGIN_INSTALLATION)
@@ -287,9 +288,11 @@ void RegisterProfilePrefs(user_prefs::PrefRegistrySyncable*
registry) {
   // User prefs. Please keep this list alphabetized.
   AlternateErrorPageTabObserver::RegisterProfilePrefs(registry);
   apps::RegisterProfilePrefs(registry);
-  autofill::AutofillCreditCardBubbleController::RegisterUserPrefs(registry);
   autofill::AutofillDialogControllerImpl::RegisterProfilePrefs(registry);
   autofill::AutofillManager::RegisterProfilePrefs(registry);
+#if !defined(OS_ANDROID)
+  autofill::GeneratedCreditCardBubbleController::RegisterUserPrefs(registry);
+#endif
   BookmarkPromptPrefs::RegisterProfilePrefs(registry);
   bookmark_utils::RegisterProfilePrefs(registry);
   browser_sync::SyncPrefs::RegisterProfilePrefs(registry);

[commit-bot: I haz the power, 2013-08-10 17:41:06]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dbeam@chromium.org/21668003/196001

[commit-bot: I haz the power, 2013-08-10 18:13:18]

Change committed as 216880