[rAC, OSX] Add "generated CC" info bubble.

[rAC, OSX] Add "generated CC" info bubble.

When requestAutocomplete uses Wallet, it generates a card for use for that specific transaction. This code displays an educational bubble, highlighting that it did that and why.

It is implemented as a InfoBubble/BaseBubbleController combo, anchored to a decoration in the location bar. The decoration stays in the bar until the user navigates away, and clicking on it shows/hides the explanation.

This is the cocoa version of it - for the original views code, see http://crbug.com/243574  & https://chromiumcodereview.appspot.com/16467005)
BUG=286527
R=shess@chromium.org, thestig@chromium.org

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

[groby-ooo-7-16, 2013-10-24 20:03:14]

Uploading this step-by-step so I can see what I did there.

(I'm annoyed by the cross-platform duplication around location bar icons/bubbles
and want to be able to see a bit more clearly what happens, to potentially rip
out cross-platform code)

[groby-ooo-7-16, 2013-10-28 18:20:21]

shess: PTAL for all things cocoa
thestig: *.gypi


Screenshot at http://imgur.com/ce4LmQq

[groby-ooo-7-16, 2013-10-28 18:40:03]

Now with actual reviewers...

shess: PTAL for all things Cocoa
thestig: I can haz gypi stamped?

[Lei Zhang, 2013-10-28 18:58:05]

You can haz gypi lgtm stamp

[Scott Hess - ex-Googler, 2013-10-31 20:23:18]

sorry for delay.

https://chromiumcodereview.appspot.com/40483003/diff/150001/chrome/browser/ui...
File chrome/browser/ui/cocoa/autofill/generated_credit_card_bubble_cocoa.h
(right):

https://chromiumcodereview.appspot.com/40483003/diff/150001/chrome/browser/ui...
chrome/browser/ui/cocoa/autofill/generated_credit_card_bubble_cocoa.h:15: }
} seems wrong.

https://chromiumcodereview.appspot.com/40483003/diff/150001/chrome/browser/ui...
File chrome/browser/ui/cocoa/autofill/generated_credit_card_bubble_cocoa.mm
(right):

https://chromiumcodereview.appspot.com/40483003/diff/150001/chrome/browser/ui...
chrome/browser/ui/cocoa/autofill/generated_credit_card_bubble_cocoa.mm:12:
#include "chrome/browser/ui/autofill/generated_credit_card_bubble_controller.h"
order c, v.

https://chromiumcodereview.appspot.com/40483003/diff/150001/chrome/browser/ui...
chrome/browser/ui/cocoa/autofill/generated_credit_card_bubble_cocoa.mm:24:
typedef void(^ClickObserver)(int);
AFAICT, neither of these actually references their parameters.

More generally, I'm a little nervous about using blocks if Chromium-style
base::Bind() tasks could be used instead.  Blocks are perfectly fine for
OSX-specific stuff, but they'll make things more opaque if a Chromium generalist
finds themselves in here making changes.

And even more generally, since this is implementation detail rather than API,
just having the Objective-C object hold a weak ref to the C++ bridge object and
call OnThis and OnThat would be less heavyweight.  [Weak ref to deal with
lifetime differences.  I'm pretty sure your current blocks don't have such
issues because they should only fire before the internal close machinery has
kicked in, so both objects should still be live.]

https://chromiumcodereview.appspot.com/40483003/diff/150001/chrome/browser/ui...
chrome/browser/ui/cocoa/autofill/generated_credit_card_bubble_cocoa.mm:49:
closeObserver:(CloseObserver)closeObserver;
The nice thing about a weak ptr to the bridge object is that it can cover all
three of these parameters.

https://chromiumcodereview.appspot.com/40483003/diff/150001/chrome/browser/ui...
chrome/browser/ui/cocoa/autofill/generated_credit_card_bubble_cocoa.mm:102:
atIndex:(NSUInteger)charIndex {
Align the : chars.

https://chromiumcodereview.appspot.com/40483003/diff/150001/chrome/browser/ui...
chrome/browser/ui/cocoa/autofill/generated_credit_card_bubble_cocoa.mm:142:
[titleView setFrame:titleFrame];
This section of code seems to be copy/paste inherited in a few places using
HyperlinkTextView.  Seems subtle enough that it should probably be a helper in
HyperlinkTextView's API, like -sizeToLine or something.

I somewhat suspect that the convolution to force a wide view to get around
wrapping is missing something.  NSTextField uses a text view, and has no
problems keeping it to a single line.  I haven't been in it lately, but I'd
guess it's the NSParagraphStyle.

https://chromiumcodereview.appspot.com/40483003/diff/150001/chrome/browser/ui...
chrome/browser/ui/cocoa/autofill/generated_credit_card_bubble_cocoa.mm:144:
bubbleWidth = std::max(bubbleWidth, NSWidth([titleView frame]) + 2 * kInset);
Is it certain that the title won't cause windows to go off the edge of the
screen?

https://chromiumcodereview.appspot.com/40483003/diff/150001/chrome/browser/ui...
chrome/browser/ui/cocoa/autofill/generated_credit_card_bubble_cocoa.mm:154:
NSFont* boldFont = [NSFont boldSystemFontOfSize:[NSFont systemFontSize]];
Could as easily store the @{} in an NSDictionary* attributes.

https://chromiumcodereview.appspot.com/40483003/diff/150001/chrome/browser/ui...
chrome/browser/ui/cocoa/autofill/generated_credit_card_bubble_cocoa.mm:185:
[contentsView setFrame:newFrame];
Likewise here, seems like having contentsView expose a -sizeToWidth: or
something of the sort would be appropriate.

https://chromiumcodereview.appspot.com/40483003/diff/150001/chrome/browser/ui...
chrome/browser/ui/cocoa/autofill/generated_credit_card_bubble_cocoa.mm:193:
NSMakePoint(kInset, bubbleHeight - kInset - NSHeight([titleView frame]))];
Rather than constructing bubbleHeight and then backing out components of it,
construct this height, then construct bubbleHeight with this height as a
component.

https://chromiumcodereview.appspot.com/40483003/diff/150001/chrome/browser/ui...
chrome/browser/ui/cocoa/autofill/generated_credit_card_bubble_cocoa.mm:194:
[contentsView setFrameOrigin:NSMakePoint(kInset, kInset)];
Though another option would be to construct additively.  Place this view first,
then place the titleView at NSMaxY([contentsView frame]) + kTitlePadding, and
bubbleHeight is NSMaxY([titleView frame]) + kInset.

https://chromiumcodereview.appspot.com/40483003/diff/150001/chrome/browser/ui...
chrome/browser/ui/cocoa/autofill/generated_credit_card_bubble_cocoa.mm:227:
controller_->web_contents()->GetView()->GetNativeView();
AFAICT, your code never checks whether the weak pointer is valid.  Why is it a
weak pointer at all?

https://chromiumcodereview.appspot.com/40483003/diff/150001/chrome/browser/ui...
chrome/browser/ui/cocoa/autofill/generated_credit_card_bubble_cocoa.mm:252:
frame.origin =  location_bar->GetGeneratedCreditCardBubblePoint();
Excess space.

https://chromiumcodereview.appspot.com/40483003/diff/150001/chrome/browser/ui...
chrome/browser/ui/cocoa/autofill/generated_credit_card_bubble_cocoa.mm:256: 
extra line.

https://chromiumcodereview.appspot.com/40483003/diff/150001/chrome/browser/ui...
chrome/browser/ui/cocoa/autofill/generated_credit_card_bubble_cocoa.mm:265: 
extra line

https://chromiumcodereview.appspot.com/40483003/diff/150001/chrome/browser/ui...
File chrome/browser/ui/cocoa/location_bar/generated_credit_card_decoration.mm
(right):

https://chromiumcodereview.appspot.com/40483003/diff/150001/chrome/browser/ui...
chrome/browser/ui/cocoa/location_bar/generated_credit_card_decoration.mm:25:
SetImage(NULL);
nil rather than NULL for objc objects.

https://chromiumcodereview.appspot.com/40483003/diff/150001/chrome/browser/ui...
chrome/browser/ui/cocoa/location_bar/generated_credit_card_decoration.mm:32:
NSMaxY(draw_frame));
Alignment, and probably can have NSMakePoint() as a single line.

https://chromiumcodereview.appspot.com/40483003/diff/150001/chrome/browser/ui...
chrome/browser/ui/cocoa/location_bar/generated_credit_card_decoration.mm:41:
autofill::GeneratedCreditCardBubbleController* controller = GetController();
Alignment.

https://chromiumcodereview.appspot.com/40483003/diff/150001/chrome/browser/ui...
chrome/browser/ui/cocoa/location_bar/generated_credit_card_decoration.mm:46:
return false;
if (!controller)
  return false;

https://chromiumcodereview.appspot.com/40483003/diff/150001/chrome/browser/ui...
chrome/browser/ui/cocoa/location_bar/generated_credit_card_decoration.mm:51:
using autofill::GeneratedCreditCardBubbleController;
Maybe pull out to file scope so that other fns can share the love?

[I'm assuming that using is in our style, here, I don't really know :-).]

https://chromiumcodereview.appspot.com/40483003/diff/150001/chrome/browser/ui...
chrome/browser/ui/cocoa/location_bar/generated_credit_card_decoration.mm:67: wc,
base::UTF8ToUTF16("foo"), base::UTF8ToUTF16("bar"));
Is the TODO actually about this line?

https://chromiumcodereview.appspot.com/40483003/diff/150001/chrome/browser/ui...
chrome/browser/ui/cocoa/location_bar/generated_credit_card_decoration.mm:72:
return autofill::GeneratedCreditCardBubbleController::FromWebContents(wc);
Is this different from controller?

https://chromiumcodereview.appspot.com/40483003/diff/150001/chrome/browser/ui...
File chrome/browser/ui/cocoa/location_bar/location_bar_view_mac.h (right):

https://chromiumcodereview.appspot.com/40483003/diff/150001/chrome/browser/ui...
chrome/browser/ui/cocoa/location_bar/location_bar_view_mac.h:239:
generated_credit_card_decoration_;
Sure this can't fit on one line?  Looks like it would be shorter than the
comment on GetGeneratedCreditCardBubblePoint(), anyhow.

[groby-ooo-7-16, 2013-10-31 22:55:46]

No worries - and PTAL with the new fixes?

https://codereview.chromium.org/40483003/diff/150001/chrome/browser/ui/cocoa/...
File chrome/browser/ui/cocoa/autofill/generated_credit_card_bubble_cocoa.h
(right):

https://codereview.chromium.org/40483003/diff/150001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/autofill/generated_credit_card_bubble_cocoa.h:15: }
On 2013/10/31 20:23:19, shess wrote:
> } seems wrong.

Blatantly wrong indeed - fixed.

https://codereview.chromium.org/40483003/diff/150001/chrome/browser/ui/cocoa/...
File chrome/browser/ui/cocoa/autofill/generated_credit_card_bubble_cocoa.mm
(right):

https://codereview.chromium.org/40483003/diff/150001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/autofill/generated_credit_card_bubble_cocoa.mm:12:
#include "chrome/browser/ui/autofill/generated_credit_card_bubble_controller.h"
On 2013/10/31 20:23:19, shess wrote:
> order c, v.

Done. (Odd that presubmit didn't catch it, thanks for flagging!)

https://codereview.chromium.org/40483003/diff/150001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/autofill/generated_credit_card_bubble_cocoa.mm:24:
typedef void(^ClickObserver)(int);
On 2013/10/31 20:23:19, shess wrote:
> AFAICT, neither of these actually references their parameters.
> 
> More generally, I'm a little nervous about using blocks if Chromium-style
> base::Bind() tasks could be used instead.  Blocks are perfectly fine for
> OSX-specific stuff, but they'll make things more opaque if a Chromium
generalist
> finds themselves in here making changes.
> 
> And even more generally, since this is implementation detail rather than API,
> just having the Objective-C object hold a weak ref to the C++ bridge object
and
> call OnThis and OnThat would be less heavyweight.  [Weak ref to deal with
> lifetime differences.  I'm pretty sure your current blocks don't have such
> issues because they should only fire before the internal close machinery has
> kicked in, so both objects should still be live.]

Getting rid of blocks or base::Bind, instead having the controller own the
bridge.

https://codereview.chromium.org/40483003/diff/150001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/autofill/generated_credit_card_bubble_cocoa.mm:49:
closeObserver:(CloseObserver)closeObserver;
On 2013/10/31 20:23:19, shess wrote:
> The nice thing about a weak ptr to the bridge object is that it can cover all
> three of these parameters.

No weak ptr needed - the bridge must live on until the controller goes away, so
I'll have the controller own it.

https://codereview.chromium.org/40483003/diff/150001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/autofill/generated_credit_card_bubble_cocoa.mm:102:
atIndex:(NSUInteger)charIndex {
On 2013/10/31 20:23:19, shess wrote:
> Align the : chars.

Done.

https://codereview.chromium.org/40483003/diff/150001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/autofill/generated_credit_card_bubble_cocoa.mm:142:
[titleView setFrame:titleFrame];
On 2013/10/31 20:23:19, shess wrote:
> This section of code seems to be copy/paste inherited in a few places using
> HyperlinkTextView.  Seems subtle enough that it should probably be a helper in
> HyperlinkTextView's API, like -sizeToLine or something.
> 
> I somewhat suspect that the convolution to force a wide view to get around
> wrapping is missing something.  NSTextField uses a text view, and has no
> problems keeping it to a single line.  I haven't been in it lately, but I'd
> guess it's the NSParagraphStyle.

I can skip this since it's always going to be single-line, without links.
NSTextField and -sizeToFit do the job.

https://codereview.chromium.org/40483003/diff/150001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/autofill/generated_credit_card_bubble_cocoa.mm:144:
bubbleWidth = std::max(bubbleWidth, NSWidth([titleView frame]) + 2 * kInset);
On 2013/10/31 20:23:19, shess wrote:
> Is it certain that the title won't cause windows to go off the edge of the
> screen?

That depends - if the browser is almost completely off screen, then yes, the
window will be off-screen too. (The whole title text is "Google Wallet is
protecting your card" - it's unlikely to go off-screen even in German.)

What would I be doing differently if the width can go past screen boundaries,
though?

https://codereview.chromium.org/40483003/diff/150001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/autofill/generated_credit_card_bubble_cocoa.mm:154:
NSFont* boldFont = [NSFont boldSystemFontOfSize:[NSFont systemFontSize]];
On 2013/10/31 20:23:19, shess wrote:
> Could as easily store the @{} in an NSDictionary* attributes.

You mean the font? Yes, but HyperlinkTextView doesn't take dictionaries and
insists on a font.

https://codereview.chromium.org/40483003/diff/150001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/autofill/generated_credit_card_bubble_cocoa.mm:185:
[contentsView setFrame:newFrame];
On 2013/10/31 20:23:19, shess wrote:
> Likewise here, seems like having contentsView expose a -sizeToWidth: or
> something of the sort would be appropriate.

Technically, it belongs on NSTextView, but I'm reluctant to add a category.
Adding it onto the HyperlinkTextView for now, unless you have a better
suggestion?

https://codereview.chromium.org/40483003/diff/150001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/autofill/generated_credit_card_bubble_cocoa.mm:193:
NSMakePoint(kInset, bubbleHeight - kInset - NSHeight([titleView frame]))];
On 2013/10/31 20:23:19, shess wrote:
> Rather than constructing bubbleHeight and then backing out components of it,
> construct this height, then construct bubbleHeight with this height as a
> component.

Done.

https://codereview.chromium.org/40483003/diff/150001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/autofill/generated_credit_card_bubble_cocoa.mm:194:
[contentsView setFrameOrigin:NSMakePoint(kInset, kInset)];
On 2013/10/31 20:23:19, shess wrote:
> Though another option would be to construct additively.  Place this view
first,
> then place the titleView at NSMaxY([contentsView frame]) + kTitlePadding, and
> bubbleHeight is NSMaxY([titleView frame]) + kInset.

Done.

https://codereview.chromium.org/40483003/diff/150001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/autofill/generated_credit_card_bubble_cocoa.mm:227:
controller_->web_contents()->GetView()->GetNativeView();
On 2013/10/31 20:23:19, shess wrote:
> AFAICT, your code never checks whether the weak pointer is valid.  Why is it a
> weak pointer at all?

It does check in the observer blocks. Show is indeed guaranteed to have a
working controller, since it's _called_ from the controller :) (Added a DCHECK
to document that)

https://codereview.chromium.org/40483003/diff/150001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/autofill/generated_credit_card_bubble_cocoa.mm:252:
frame.origin =  location_bar->GetGeneratedCreditCardBubblePoint();
On 2013/10/31 20:23:19, shess wrote:
> Excess space.

Done.

https://codereview.chromium.org/40483003/diff/150001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/autofill/generated_credit_card_bubble_cocoa.mm:256: 
On 2013/10/31 20:23:19, shess wrote:
> extra line.

Done.

https://codereview.chromium.org/40483003/diff/150001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/autofill/generated_credit_card_bubble_cocoa.mm:265: 
On 2013/10/31 20:23:19, shess wrote:
> extra line

Done.

https://codereview.chromium.org/40483003/diff/150001/chrome/browser/ui/cocoa/...
File chrome/browser/ui/cocoa/location_bar/generated_credit_card_decoration.mm
(right):

https://codereview.chromium.org/40483003/diff/150001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/location_bar/generated_credit_card_decoration.mm:25:
SetImage(NULL);
On 2013/10/31 20:23:19, shess wrote:
> nil rather than NULL for objc objects.

Done.

https://codereview.chromium.org/40483003/diff/150001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/location_bar/generated_credit_card_decoration.mm:32:
NSMaxY(draw_frame));
On 2013/10/31 20:23:19, shess wrote:
> Alignment, and probably can have NSMakePoint() as a single line.

Done.

https://codereview.chromium.org/40483003/diff/150001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/location_bar/generated_credit_card_decoration.mm:41:
autofill::GeneratedCreditCardBubbleController* controller = GetController();
On 2013/10/31 20:23:19, shess wrote:
> Alignment.

Done.

https://codereview.chromium.org/40483003/diff/150001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/location_bar/generated_credit_card_decoration.mm:46:
return false;
On 2013/10/31 20:23:19, shess wrote:
> if (!controller)
>   return false;

Done.

https://codereview.chromium.org/40483003/diff/150001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/location_bar/generated_credit_card_decoration.mm:51:
using autofill::GeneratedCreditCardBubbleController;
On 2013/10/31 20:23:19, shess wrote:
> Maybe pull out to file scope so that other fns can share the love?
> 
> [I'm assuming that using is in our style, here, I don't really know :-).]

It is. I thought we were function level only, but turns out not so. Buried
deeply in
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Namespaces if you
want to read up :)

Done.

https://codereview.chromium.org/40483003/diff/150001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/location_bar/generated_credit_card_decoration.mm:67: wc,
base::UTF8ToUTF16("foo"), base::UTF8ToUTF16("bar"));
On 2013/10/31 20:23:19, shess wrote:
> Is the TODO actually about this line?
Yes. And they both can die - this code was for testing only...

https://codereview.chromium.org/40483003/diff/150001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/location_bar/generated_credit_card_decoration.mm:72:
return autofill::GeneratedCreditCardBubbleController::FromWebContents(wc);
On 2013/10/31 20:23:19, shess wrote:
> Is this different from controller?

Nope. Done.

https://codereview.chromium.org/40483003/diff/150001/chrome/browser/ui/cocoa/...
File chrome/browser/ui/cocoa/location_bar/location_bar_view_mac.h (right):

https://codereview.chromium.org/40483003/diff/150001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/location_bar/location_bar_view_mac.h:239:
generated_credit_card_decoration_;
On 2013/10/31 20:23:19, shess wrote:
> Sure this can't fit on one line?  Looks like it would be shorter than the
> comment on GetGeneratedCreditCardBubblePoint(), anyhow.

Just so - done.

[Scott Hess - ex-Googler, 2013-10-31 23:33:38]

I'm generally happy with things.  The reason I'm focussing on the text-layout
bit is because digging into the NSLayoutManager level for something like this
feels like something that shouldn't be this hard.  Since we're trying to force
specific results, I'm willing to grant that it might be this hard, but it's
possible there are options which weren't obvious, and it would be nice to ferret
them out while someone is looking at it.  Whenever I run across things like
this, the first problem is even figuring out how to get that bubble to pop up in
the first place :-).

https://codereview.chromium.org/40483003/diff/150001/chrome/browser/ui/cocoa/...
File chrome/browser/ui/cocoa/autofill/generated_credit_card_bubble_cocoa.mm
(right):

https://codereview.chromium.org/40483003/diff/150001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/autofill/generated_credit_card_bubble_cocoa.mm:144:
bubbleWidth = std::max(bubbleWidth, NSWidth([titleView frame]) + 2 * kInset);
On 2013/10/31 22:55:47, groby wrote:
> On 2013/10/31 20:23:19, shess wrote:
> > Is it certain that the title won't cause windows to go off the edge of the
> > screen?
> 
> That depends - if the browser is almost completely off screen, then yes, the
> window will be off-screen too. (The whole title text is "Google Wallet is
> protecting your card" - it's unlikely to go off-screen even in German.)
> 
> What would I be doing differently if the width can go past screen boundaries,
> though?

I agree that it would be a conundrum, but just because nothing different is
obvious doesn't mean that we should periodically do something really gross!  But
it sounds like it can never get excessive, so there's not a problem.

https://codereview.chromium.org/40483003/diff/150001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/autofill/generated_credit_card_bubble_cocoa.mm:154:
NSFont* boldFont = [NSFont boldSystemFontOfSize:[NSFont systemFontSize]];
On 2013/10/31 22:55:47, groby wrote:
> On 2013/10/31 20:23:19, shess wrote:
> > Could as easily store the @{} in an NSDictionary* attributes.
> 
> You mean the font? Yes, but HyperlinkTextView doesn't take dictionaries and
> insists on a font.

|boldFont| is being applied to non-link sections the old-fashioned way, and ends
wrapping because of it.

https://codereview.chromium.org/40483003/diff/150001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/autofill/generated_credit_card_bubble_cocoa.mm:185:
[contentsView setFrame:newFrame];
On 2013/10/31 22:55:47, groby wrote:
> On 2013/10/31 20:23:19, shess wrote:
> > Likewise here, seems like having contentsView expose a -sizeToWidth: or
> > something of the sort would be appropriate.
> 
> Technically, it belongs on NSTextView, but I'm reluctant to add a category.
> Adding it onto the HyperlinkTextView for now, unless you have a better
> suggestion?

I find two existing cases where we're doing this basic thing, in:
  chrome/browser/ui/cocoa/autofill/autofill_main_container.mm
  chrome/browser/ui/cocoa/fullscreen_exit_bubble_controller.mm
In both cases, the object in question is a HyperlinkTextView, and your code is
adding one or two new cases.  I think abstracting it into HyperlinkTextView is
the way to go, if someone finds they need to depend on it and cannot use that
class, they can lift it to a new interstitial class.

https://codereview.chromium.org/40483003/diff/400001/chrome/browser/ui/cocoa/...
File chrome/browser/ui/cocoa/autofill/generated_credit_card_bubble_cocoa.mm
(right):

https://codereview.chromium.org/40483003/diff/400001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/autofill/generated_credit_card_bubble_cocoa.mm:37: // A
block that is run when the user clicks on a link.
Can probably get rid of the comments, now :-).

https://codereview.chromium.org/40483003/diff/400001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/autofill/generated_credit_card_bubble_cocoa.mm:40:
scoped_ptr<GeneratedCreditCardBubbleCocoa> bridge_;
I'm not sure this works, as currently written.  Nobody owns the object until
Show(), and even Show() may not pass ownership down, and there are code paths in
the initializer which do not end up with ownership in bridge_ (I'm not sure
failing super init is plausible in this case, but the code path exists!).

https://codereview.chromium.org/40483003/diff/400001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/autofill/generated_credit_card_bubble_cocoa.mm:43: //
Designate initializer. Runs |closeObserver| when -windowWillClose: runs.
Can probably drop the comment entirely, since it's the only initializer.

https://codereview.chromium.org/40483003/diff/400001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/autofill/generated_credit_card_bubble_cocoa.mm:127:
[titleView sizeToFit];
That'll accomplish it, too :-).

https://codereview.chromium.org/40483003/diff/400001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/autofill/generated_credit_card_bubble_cocoa.mm:188: :
bubbleController_(NULL),
nil for objective-c object.

https://codereview.chromium.org/40483003/diff/400001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/autofill/generated_credit_card_bubble_cocoa.mm:210:
bridge:this];
: align.

https://codereview.chromium.org/40483003/diff/400001/chrome/browser/ui/cocoa/...
File chrome/browser/ui/cocoa/hyperlink_text_view.mm (right):

https://codereview.chromium.org/40483003/diff/400001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/hyperlink_text_view.mm:147: -
(void)setFrameSizeForWidth:(CGFloat)width {
This is like -sizeToFit, except with a fixed width, so I think -sizeToFitWidth:
or -sizeToWidth would be appropriate.

Since you're in the midst of poking this, would you mind testing how
-setMinSize: and -setMaxSize: impact this?  It seems plausible/likely that if
the higher-level code did:
  [textView setMinSize:NSMakeSize(width, 0.0)];
  [textView setMaxSize:NSMakeSize(width, CGFLOAT_MAX)];
  [textView sizeToFit];
then it would get the appropriate results, without the shenanigans.  Does that
make sense?

[Sorry I didn't suggest this earlier - every time I review NSTextView and
NSText, I see some new thing which has only been around since OSX 10.0 or
something.]

[groby-ooo-7-16, 2013-11-01 00:28:58]

PTAL - I think sizeToFitWidth looks nice now.

https://codereview.chromium.org/40483003/diff/400001/chrome/browser/ui/cocoa/...
File chrome/browser/ui/cocoa/autofill/generated_credit_card_bubble_cocoa.mm
(right):

https://codereview.chromium.org/40483003/diff/400001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/autofill/generated_credit_card_bubble_cocoa.mm:37: // A
block that is run when the user clicks on a link.
On 2013/10/31 23:33:38, shess wrote:
> Can probably get rid of the comments, now :-).

Yep. Kept them to copy them over to declarations, wrote new ones instead and
forgot. NEED MOAR COFFEE!

https://codereview.chromium.org/40483003/diff/400001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/autofill/generated_credit_card_bubble_cocoa.mm:40:
scoped_ptr<GeneratedCreditCardBubbleCocoa> bridge_;
On 2013/10/31 23:33:38, shess wrote:
> I'm not sure this works, as currently written.  Nobody owns the object until
> Show(), and even Show() may not pass ownership down, and there are code paths
in
> the initializer which do not end up with ownership in bridge_ (I'm not sure
> failing super init is plausible in this case, but the code path exists!).

Let's put it that way - if super init fails, we have bigger problems. But I can
apply some fixes.

The one thing I can't fix is "nobody owns the object until show". It's
self-deletion mania upstream forcing a bad API. And it's cross-platform, so I
can't easily change it.

https://codereview.chromium.org/40483003/diff/400001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/autofill/generated_credit_card_bubble_cocoa.mm:43: //
Designate initializer. Runs |closeObserver| when -windowWillClose: runs.
On 2013/10/31 23:33:38, shess wrote:
> Can probably drop the comment entirely, since it's the only initializer.

Done.

https://codereview.chromium.org/40483003/diff/400001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/autofill/generated_credit_card_bubble_cocoa.mm:127:
[titleView sizeToFit];
On 2013/10/31 23:33:38, shess wrote:
> That'll accomplish it, too :-).

And much nicer, too :)

https://codereview.chromium.org/40483003/diff/400001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/autofill/generated_credit_card_bubble_cocoa.mm:188: :
bubbleController_(NULL),
On 2013/10/31 23:33:38, shess wrote:
> nil for objective-c object.

Done.

https://codereview.chromium.org/40483003/diff/400001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/autofill/generated_credit_card_bubble_cocoa.mm:210:
bridge:this];
On 2013/10/31 23:33:38, shess wrote:
> : align.

Done.

https://codereview.chromium.org/40483003/diff/400001/chrome/browser/ui/cocoa/...
File chrome/browser/ui/cocoa/hyperlink_text_view.mm (right):

https://codereview.chromium.org/40483003/diff/400001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/hyperlink_text_view.mm:147: -
(void)setFrameSizeForWidth:(CGFloat)width {
Turns out minsize/maxsize don't bother sizeToFit that much - but setting a
frameSize of width/CGFLOAT_MAX works fine. 

On 2013/10/31 23:33:38, shess wrote:
> This is like -sizeToFit, except with a fixed width, so I think
-sizeToFitWidth:
> or -sizeToWidth would be appropriate.
> 
> Since you're in the midst of poking this, would you mind testing how
> -setMinSize: and -setMaxSize: impact this?  It seems plausible/likely that if
> the higher-level code did:
>   [textView setMinSize:NSMakeSize(width, 0.0)];
>   [textView setMaxSize:NSMakeSize(width, CGFLOAT_MAX)];
>   [textView sizeToFit];
> then it would get the appropriate results, without the shenanigans.  Does that
> make sense?
> 
> [Sorry I didn't suggest this earlier - every time I review NSTextView and
> NSText, I see some new thing which has only been around since OSX 10.0 or
> something.]

[groby-ooo-7-16, 2013-11-01 01:10:06]

Hopefully this is it.

[Scott Hess - ex-Googler, 2013-11-04 21:20:54]

lgtm, minor nits.

https://codereview.chromium.org/40483003/diff/510001/chrome/browser/ui/cocoa/...
File chrome/browser/ui/cocoa/autofill/generated_credit_card_bubble_cocoa.mm
(right):

https://codereview.chromium.org/40483003/diff/510001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/autofill/generated_credit_card_bubble_cocoa.mm:36: //
Bridge to C++ side. Owned.
Owned is implicit in the scoped_ptr<>, I think.

https://codereview.chromium.org/40483003/diff/510001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/autofill/generated_credit_card_bubble_cocoa.mm:158:
[contentsView sizeToFit];
Looks much more sensible.

https://codereview.chromium.org/40483003/diff/510001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/autofill/generated_credit_card_bubble_cocoa.mm:205:
return;
Does this case just leak?

[groby-ooo-7-16, 2013-11-04 22:02:04]

https://codereview.chromium.org/40483003/diff/510001/chrome/browser/ui/cocoa/...
File chrome/browser/ui/cocoa/autofill/generated_credit_card_bubble_cocoa.mm
(right):

https://codereview.chromium.org/40483003/diff/510001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/autofill/generated_credit_card_bubble_cocoa.mm:36: //
Bridge to C++ side. Owned.
On 2013/11/04 21:20:55, shess wrote:
> Owned is implicit in the scoped_ptr<>, I think.

Done.

https://codereview.chromium.org/40483003/diff/510001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/autofill/generated_credit_card_bubble_cocoa.mm:205:
return;
On 2013/11/04 21:20:55, shess wrote:
> Does this case just leak?

Crap. It probably does. It should never happen (Show shouldn't be called during
initialization), but yes, it would be a leak. Amending with 'delete this' and
'NOTREACHED'

[Scott Hess - ex-Googler, 2013-11-04 22:03:39]

lgtm

[commit-bot: I haz the power, 2013-11-04 22:10:27]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/groby@chromium.org/40483003/700001

[groby-ooo-7-16, 2013-11-04 23:27:30]

Just FYI - small fix to use convertBaseToScreen instead of convertRectToScreen,
so 10.6 compiles.

[Scott Hess - ex-Googler, 2013-11-04 23:49:30]

lgtm

[commit-bot: I haz the power, 2013-11-05 00:42:19]

Sorry for I got bad news for ya.
Compile failed with a clobber build on mac.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac&number...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2013-11-05 00:42:55]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/groby@chromium.org/40483003/900001

[commit-bot: I haz the power, 2013-11-05 03:02:15]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/groby@chromium.org/40483003/900001

[commit-bot: I haz the power, 2013-11-05 04:36:39]

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

[commit-bot: I haz the power, 2013-11-06 02:05:49]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/groby@chromium.org/40483003/900001

[commit-bot: I haz the power, 2013-11-06 04:02:21]

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

[commit-bot: I haz the power, 2013-11-06 21:46:56]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/groby@chromium.org/40483003/900001

[commit-bot: I haz the power, 2013-11-07 00:18:26]

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

[groby-ooo-7-16, 2013-11-07 00:27:39]

Sigh. Looks like interactive_ui_tests doesn't like this at all. Fix coming
soon.....

[groby-ooo-7-16, 2013-11-07 16:28:24]

https://codereview.chromium.org/40483003/diff/1350001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/location_bar/generated_credit_card_decoration.mm
(right):

https://codereview.chromium.org/40483003/diff/1350001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/location_bar/generated_credit_card_decoration.mm:59:
DCHECK(controller);
This DCHECK is actively bad - it is possible to have a NULL value returned.
Which is handled by the only consumer of GetController(), Update() - see line 22
above. 

Removed

[commit-bot: I haz the power, 2013-11-07 21:28:28]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/groby@chromium.org/40483003/1370001

[commit-bot: I haz the power, 2013-11-08 00:21:23]

Change committed as 233719