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

chrome/browser/ui/cocoa/autofill/generated_credit_card_bubble_cocoa.mm

+// Copyright (c) 2013 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "chrome/browser/ui/cocoa/autofill/generated_credit_card_bubble_cocoa.h"
+
+#include "base/mac/foundation_util.h"
+#include "base/mac/scoped_block.h"
+#include "base/mac/scoped_nsobject.h"
+#include "base/strings/sys_string_conversions.h"
+#include "chrome/browser/ui/autofill/generated_credit_card_bubble_view.h"
+#include "chrome/browser/ui/autofill/generated_credit_card_bubble_controller.h"

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

order c, v.

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

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

+#import "chrome/browser/ui/cocoa/base_bubble_controller.h"
+#import "chrome/browser/ui/cocoa/hyperlink_text_view.h"
+#import "chrome/browser/ui/cocoa/info_bubble_view.h"
+#import "chrome/browser/ui/cocoa/info_bubble_window.h"
+#include "chrome/browser/ui/cocoa/location_bar/location_bar_view_mac.h"
+#import "chrome/browser/ui/cocoa/toolbar/toolbar_controller.h"
+#include "content/public/browser/web_contents_view.h"
+#include "skia/ext/skia_utils_mac.h"
+#include "ui/native_theme/native_theme.h"
+
+typedef void(^CloseObserver)(GeneratedCreditCardBubbleControllerCocoa*);
+typedef void(^ClickObserver)(int);

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

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

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

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

+
+namespace {
+
+const CGFloat kTitlePadding = 20.0;
+const CGFloat kInset = 20.0;
+
+}  // namespace
+
+// The Cocoa side of the bubble controller.
+@interface GeneratedCreditCardBubbleControllerCocoa :
+    BaseBubbleController<NSTextViewDelegate> {
+  // A block that is run when the bubble is being closed. That allows wiping
+  // out weak references. (BaseBubbleController self-destroys on close).
+  base::mac::ScopedBlock<CloseObserver> closeObserver_;
+
+  // A block that is run when the user clicks on a link.
+  base::mac::ScopedBlock<ClickObserver> clickObserver_;
+}
+
+// Designate initializer. Runs |closeObserver| when -windowWillClose: runs.
+- (id)initWithParentWindow:(NSWindow*)parentWindow
+                controller:
+                    (autofill::GeneratedCreditCardBubbleController*)controller
+             clickObserver:(ClickObserver)clickObserver
+             closeObserver:(CloseObserver)closeObserver;

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

The nice thing about a weak ptr to the bridge object is that it can cover all
three of these parameters.

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

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

+
+// Show the bubble at the given |anchor| point. Coordinates are in screen space.
+- (void)showAtAnchor:(NSPoint)anchor;
+
+// Return true if the bubble is in the process of hiding.
+- (BOOL)isHiding;
+
+// Build the window contents and lay them out.
+- (void)performLayoutWithController:
+      (autofill::GeneratedCreditCardBubbleController*)controller;
+
+@end
+
+
+@implementation GeneratedCreditCardBubbleControllerCocoa
+
+- (id)initWithParentWindow:(NSWindow*)parentWindow
+                controller:
+                    (autofill::GeneratedCreditCardBubbleController*)controller
+             clickObserver:(ClickObserver)clickObserver
+             closeObserver:(CloseObserver)closeObserver {
+  base::scoped_nsobject<InfoBubbleWindow> window(
+      [[InfoBubbleWindow alloc] initWithContentRect:NSMakeRect(0, 0, 200, 100)
+                                          styleMask:NSBorderlessWindowMask
+                                            backing:NSBackingStoreBuffered
+                                              defer:NO]);
+  if ((self = [super initWithWindow:window
+                       parentWindow:parentWindow
+                         anchoredAt:NSZeroPoint])) {
+    [window setCanBecomeKeyWindow:NO];
+    closeObserver_.reset(Block_copy(closeObserver));
+    clickObserver_.reset(Block_copy(clickObserver));
+
+    ui::NativeTheme* nativeTheme = ui::NativeTheme::instance();
+    [[self bubble] setAlignment:info_bubble::kAlignArrowToAnchor];
+    [[self bubble] setArrowLocation:info_bubble::kTopRight];
+    [[self bubble] setBackgroundColor:
+        gfx::SkColorToCalibratedNSColor(nativeTheme->GetSystemColor(
+            ui::NativeTheme::kColorId_DialogBackground))];
+    [self performLayoutWithController:controller];
+  }
+  return self;
+}
+
+- (void)windowWillClose:(NSNotification*)notification {
+  closeObserver_.get()(self);
+  [super windowWillClose:notification];
+}
+
+// Called when embedded links are clicked.
+- (BOOL)textView:(NSTextView*)textView
+    clickedOnLink:(id)link
+          atIndex:(NSUInteger)charIndex {

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

Align the : chars.

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

Done.

+  int index = [base::mac::ObjCCastStrict<NSNumber>(link) intValue];
+  clickObserver_.get()(index);
+  return YES;
+}
+
+- (void)showAtAnchor:(NSPoint)anchorPoint {
+  [self setAnchorPoint:anchorPoint];
+  [self showWindow:nil];
+}
+
+- (BOOL)isHiding {
+  InfoBubbleWindow* window =
+      base::mac::ObjCCastStrict<InfoBubbleWindow>([self window]);
+  return [window isClosing];
+}
+
+
+- (void)performLayoutWithController:
+    (autofill::GeneratedCreditCardBubbleController*)controller {
+  CGFloat bubbleWidth = autofill::GeneratedCreditCardBubbleView::kContentsWidth;
+
+  // Build the bubble title.
+  NSFont* titleFont = [NSFont systemFontOfSize:15.0];
+  NSString* title = base::SysUTF16ToNSString(controller->TitleText());
+  base::scoped_nsobject<HyperlinkTextView> titleView(
+    [[HyperlinkTextView alloc] initWithFrame:NSZeroRect]);
+  [titleView setMessage:title
+               withFont:titleFont
+           messageColor:[NSColor blackColor]];
+
+  [titleView setFrame:NSMakeRect(
+      0, 0,
+      CGFLOAT_MAX, CGFLOAT_MAX)];
+
+  // Now use the layout manager to compute layout.
+  NSLayoutManager* layoutManager = [titleView layoutManager];
+  NSTextContainer* textContainer = [titleView textContainer];
+  [layoutManager ensureLayoutForTextContainer:textContainer];
+  NSRect titleFrame = [layoutManager usedRectForTextContainer:textContainer];
+  [titleView setFrame:titleFrame];

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

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.

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

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

+
+  bubbleWidth = std::max(bubbleWidth, NSWidth([titleView frame]) + 2 * kInset);

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

Is it certain that the title won't cause windows to go off the edge of the
screen?

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

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?

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

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.

+
+  // Build the contents view.
+  base::scoped_nsobject<HyperlinkTextView> contentsView(
+      [[HyperlinkTextView alloc] initWithFrame:NSZeroRect]);
+
+  [contentsView setEditable:NO];
+  [contentsView setDelegate:self];
+
+  NSFont* font = [NSFont systemFontOfSize:[NSFont systemFontSize]];
+  NSFont* boldFont = [NSFont boldSystemFontOfSize:[NSFont systemFontSize]];

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

Could as easily store the @{} in an NSDictionary* attributes.

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

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

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

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

+  [contentsView setMessage:base::SysUTF16ToNSString(controller->ContentsText())
+                  withFont:font
+              messageColor:[NSColor blackColor]];
+
+  const std::vector<autofill::TextRange>& text_ranges =
+      controller->ContentsTextRanges();
+  for (size_t i = 0; i < text_ranges.size(); ++i) {
+    NSRange range = text_ranges[i].range.ToNSRange();
+    if (text_ranges[i].is_link) {
+      [contentsView addLinkRange:range
+                        withName:@(i)
+                       linkColor:[NSColor blueColor]];
+    } else {
+       [[contentsView textStorage]
+           addAttributes:@{ NSFontAttributeName : boldFont }
+                   range:range];
+    }
+  }
+
+  // There's no direct API to compute desired sizes - use layouting instead.
+  // Layout in a rect with fixed width and "infinite" height.
+  [contentsView setFrame:NSMakeRect(
+      0, 0,
+      bubbleWidth - 2 * kInset, CGFLOAT_MAX)];
+
+  // Use the layout manager to compute size.
+  layoutManager = [contentsView layoutManager];
+  textContainer = [contentsView textContainer];
+  [layoutManager ensureLayoutForTextContainer:textContainer];
+  NSRect newFrame = [layoutManager usedRectForTextContainer:textContainer];
+  [contentsView setFrame:newFrame];

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

Likewise here, seems like having contentsView expose a -sizeToWidth: or
something of the sort would be appropriate.

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

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?

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

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.

+
+  // Sizes are computed, now lay out the individual parts.
+  CGFloat bubbleHeight = NSHeight([titleView frame]) +
+      kTitlePadding +
+      2 * kInset +
+      NSHeight([contentsView frame]);
+  [titleView setFrameOrigin:
+      NSMakePoint(kInset, bubbleHeight - kInset - NSHeight([titleView frame]))];

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

Rather than constructing bubbleHeight and then backing out components of it,
construct this height, then construct bubbleHeight with this height as a
component.

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

Done.

+  [contentsView setFrameOrigin:NSMakePoint(kInset, kInset)];

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

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.

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

Done.

+  [[[self window] contentView] setSubviews:@[ contentsView, titleView ]];
+
+  // Update window frame.
+  NSRect windowFrame = [[self window] frame];
+  windowFrame.size = NSMakeSize(bubbleWidth, bubbleHeight);
+  [[self window] setFrame:windowFrame display:YES];
+}
+
+@end
+
+
+namespace autofill {
+
+// static
+base::WeakPtr<GeneratedCreditCardBubbleView>
+    GeneratedCreditCardBubbleView::Create(
+        const base::WeakPtr<GeneratedCreditCardBubbleController>& controller) {
+  return (new GeneratedCreditCardBubbleCocoa(controller))->weak_ptr_factory_.
+      GetWeakPtr();
+}
+
+GeneratedCreditCardBubbleCocoa::GeneratedCreditCardBubbleCocoa(
+    const base::WeakPtr<GeneratedCreditCardBubbleController>& controller)
+    : bubbleController_(NULL),
+      controller_(controller),
+      weak_ptr_factory_(this) {
+}
+
+GeneratedCreditCardBubbleCocoa::~GeneratedCreditCardBubbleCocoa() {}
+
+void GeneratedCreditCardBubbleCocoa::Show() {
+  NSView* browser_view =
+      controller_->web_contents()->GetView()->GetNativeView();

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

AFAICT, your code never checks whether the weak pointer is valid.  Why is it a
weak pointer at all?

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

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)

+  NSWindow* parent_window = [browser_view window];
+  LocationBarViewMac* location_bar =
+      [[parent_window windowController] locationBarBridge];
+
+  // |location_bar| can be NULL during initialization stages.
+  if (!location_bar)
+    return;
+
+  if (!bubbleController_) {
+    CloseObserver observer = ^(GeneratedCreditCardBubbleControllerCocoa*) {
+            bubbleController_ = nil;
+        };
+    ClickObserver clickObserver = ^(int index) {
+        if (controller_)
+          controller_->OnLinkClicked();
+    };
+    bubbleController_ = [[GeneratedCreditCardBubbleControllerCocoa alloc]
+        initWithParentWindow:parent_window
+                  controller:controller_.get()
+               clickObserver:clickObserver
+               closeObserver:observer];
+  }
+
+  NSRect frame = NSZeroRect;
+  frame.origin =  location_bar->GetGeneratedCreditCardBubblePoint();

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

Excess space.

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

Done.

+  NSPoint anchor = [parent_window convertRectToScreen:frame].origin;
+  [bubbleController_ showAtAnchor:anchor];
+}
+

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

extra line.

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

Done.

+
+void GeneratedCreditCardBubbleCocoa::Hide() {
+  [bubbleController_ close];
+}
+
+bool GeneratedCreditCardBubbleCocoa::IsHiding() const {
+  return [bubbleController_ isHiding];
+}
+

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

extra line

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

Done.

+
+}  // autofill