Mac: Display a native bubble (instead of the JS one) after the web signin flow. (branched from http…

chrome/browser/ui/cocoa/one_click_signin_view_controller.mm

 // Copyright (c) 2012 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.
 
 #import "chrome/browser/ui/cocoa/one_click_signin_view_controller.h"
 
 #include "base/callback_helpers.h"
 #include "base/logging.h"
 #include "base/mac/bundle_locations.h"
+#include "base/strings/sys_string_conversions.h"
 #import "chrome/browser/ui/chrome_style.h"
 #import "chrome/browser/ui/cocoa/hyperlink_text_view.h"
 #include "chrome/common/url_constants.h"
 #include "content/public/browser/web_contents.h"
 #include "grit/generated_resources.h"
 #include "skia/ext/skia_utils_mac.h"
 #import "third_party/GTM/AppKit/GTMUILocalizerAndLayoutTweaker.h"
 #include "ui/base/l10n/l10n_util_mac.h"
 
 namespace {
 
 // Shift the origin of |view|'s frame by the given amount in the
 // positive y direction (up).
 void ShiftOriginY(NSView* view, CGFloat amount) {
   NSPoint origin = [view frame].origin;
   origin.y += amount;
   [view setFrameOrigin:origin];
 }
 
 }  // namespace
 
 @interface OneClickSigninViewController ()
 - (CGFloat)initializeInformativeTextView;
 - (void)close;
 @end
 
 @implementation OneClickSigninViewController
 
+
 - (id)initWithNibName:(NSString*)nibName
           webContents:(content::WebContents*)webContents
          syncCallback:(const BrowserWindow::StartSyncCallback&)syncCallback
-        closeCallback:(const base::Closure&)closeCallback {
+        closeCallback:(const base::Closure&)closeCallback
+         isSyncDialog:(bool)isSyncDialog
+         errorMessage:(const string16&)errorMessage {
   if ((self = [super initWithNibName:nibName
                               bundle:base::mac::FrameworkBundle()])) {
     webContents_ = webContents;
     startSyncCallback_ = syncCallback;
     closeCallback_ = closeCallback;
-    DCHECK(!startSyncCallback_.is_null());
+    isSyncDialog_ = isSyncDialog;
+    errorMessage_ = base::SysUTF16ToNSString(errorMessage);

[Alexei Svitkine (slow), 2013/04/19 19:54:15]

Make the ctor param type NSString* and pass in nil instead of std::string() when
you want to not have it, then you can change the [errorMessage_ length] != 0
check to just errorMessage_.

[noms (inactive), 2013/04/22 15:28:07]

I have changed the parameter to NSString* as it's more Objective-C-ish, but
sadly, I have to leave in the length check, as the string can be not null but
still empty.

On 2013/04/19 19:54:15, Alexei Svitkine wrote:

+
+    if (isSyncDialog_)
+      DCHECK(!startSyncCallback_.is_null());
   }
   return self;
 }
 
 - (void)viewWillClose {
-  if (!startSyncCallback_.is_null()) {
+  if (isSyncDialog_ && !startSyncCallback_.is_null()) {

[Alexei Svitkine (slow), 2013/04/19 19:54:15]

If |isSyncDialog_| is YES, then you're already DCHECK'ing that the callback
isn't NULL, so you don't need the second condition in the if.

[noms (inactive), 2013/04/22 15:28:07]

Hmm, but you only DCHECK in the constructor, and this is called much later,
where I worry about it having been NULL'ed randomly...Too much worry?

On 2013/04/19 19:54:15, Alexei Svitkine wrote:

[Alexei Svitkine (slow), 2013/04/22 18:16:51]

I guess you can add a DCHECK() here too, if you want - it's silly to have an if
guard here where you DCHECK on this earlier.

It's also strange to just do it here and not in one of the other places you use
the callback.

[noms (inactive), 2013/04/23 18:52:48]

Ok, did some analysis: viewWillClose is called after the actions for ok/undo are
executed. These latter start sync, which also resets the syncCallback upon
completion. So by the time viewWillClose is called, if the syncCallback has
executed, it's now null. However, there could be an action where the view will
close without pressing ok/undo (at the moment this seems unlikely), in which
case viewWillClose needs to start sync. Since we can't remove the null check,
we'd have to remove the entire sync bit from viewWillClose, but I don't want
this to leave open an edge case where sync doesn't start correctly...Is this ok?

On 2013/04/22 18:16:51, Alexei Svitkine wrote:

[Alexei Svitkine (slow), 2013/04/23 19:43:24]

Okay, I understand the need for the NULL check given your explanation.

Please add a comment to explain this. Also, it would be good to find out what
this case is where viewWillClose will be called without buttons being clicked.
Is it when the window is just closed, for example? It would be good to mention
the specific cases explicitly in the comment.

     base::ResetAndReturn(&startSyncCallback_).Run(
         OneClickSigninSyncStarter::SYNC_WITH_DEFAULT_SETTINGS);
   }
 }
 
 - (IBAction)ok:(id)sender {
-  base::ResetAndReturn(&startSyncCallback_).Run(
+  if (isSyncDialog_) {
+    base::ResetAndReturn(&startSyncCallback_).Run(
       OneClickSigninSyncStarter::SYNC_WITH_DEFAULT_SETTINGS);
+  }
   [self close];
 }
 
 - (IBAction)onClickUndo:(id)sender {
-  base::ResetAndReturn(&startSyncCallback_).Run(
-      OneClickSigninSyncStarter::UNDO_SYNC);
+  if (isSyncDialog_) {
+    base::ResetAndReturn(&startSyncCallback_).Run(
+        OneClickSigninSyncStarter::UNDO_SYNC);
+  }
   [self close];
 }
 
 - (IBAction)onClickAdvancedLink:(id)sender {
-  base::ResetAndReturn(&startSyncCallback_).Run(
-      OneClickSigninSyncStarter::CONFIGURE_SYNC_FIRST);
+  if (isSyncDialog_) {
+    base::ResetAndReturn(&startSyncCallback_).Run(
+        OneClickSigninSyncStarter::CONFIGURE_SYNC_FIRST);
+  } else {
+    content::OpenURLParams params(GURL(chrome::kChromeUISettingsURL),
+                                content::Referrer(), CURRENT_TAB,
+                                content::PAGE_TRANSITION_LINK, false);
+    webContents_->OpenURL(params);
+  }
   [self close];
 }
 
 - (void)awakeFromNib {
   // Lay out the text controls from the bottom up.
   CGFloat totalYOffset = 0.0;
 
   totalYOffset +=
       [GTMUILocalizerAndLayoutTweaker sizeToFitView:advancedLink_].height;
   [[advancedLink_ cell] setTextColor:
       gfx::SkColorToCalibratedNSColor(chrome_style::GetLinkColor())];
 
   if (informativePlaceholderTextField_) {
     ShiftOriginY(informativePlaceholderTextField_, totalYOffset);
     totalYOffset += [self initializeInformativeTextView];
   }
 
   ShiftOriginY(messageTextField_, totalYOffset);
   totalYOffset +=
       [GTMUILocalizerAndLayoutTweaker
           sizeToFitFixedWidthTextField:messageTextField_];
 
   if (closeButton_)
     ShiftOriginY(closeButton_, totalYOffset);
 
   NSSize delta = NSMakeSize(0.0, totalYOffset);
 
+  if (!isSyncDialog_ && [errorMessage_ length] != 0)
+    [messageTextField_ setStringValue:errorMessage_];
+
   // Resize bubble and window to hold the controls.
   [GTMUILocalizerAndLayoutTweaker
       resizeViewWithoutAutoResizingSubViews:[self view]
                                       delta:delta];
 }
 
 - (CGFloat)initializeInformativeTextView {
   NSRect oldFrame = [informativePlaceholderTextField_ frame];
 
   // Replace the placeholder NSTextField with the real label NSTextView. The
   // former doesn't show links in a nice way, but the latter can't be added in
   // a xib without a containing scroll view, so create the NSTextView
   // programmatically.
   informativeTextView_.reset(
       [[HyperlinkTextView alloc] initWithFrame:oldFrame]);
   [informativeTextView_.get() setAutoresizingMask:
       [informativePlaceholderTextField_ autoresizingMask]];
   [informativeTextView_.get() setDelegate:self];
 
   // Set the text.
   NSString* learnMoreText = l10n_util::GetNSStringWithFixup(IDS_LEARN_MORE);
-  NSString* messageText =
-      l10n_util::GetNSStringWithFixup(IDS_ONE_CLICK_SIGNIN_DIALOG_MESSAGE);
-  messageText = [messageText stringByAppendingString:@" "];
-  NSFont* font = ui::ResourceBundle::GetSharedInstance().GetFont(
+  NSString* messageText;
+
+  NSFont* font;
+
+  // the non-modal bubble already has a text content

[Alexei Svitkine (slow), 2013/04/19 19:54:15]

Comments should be full sentences.

[noms (inactive), 2013/04/22 15:28:07]

Done.

+  // and only needs the Learn More link (in a smaller font)
+  if (isSyncDialog_) {
+    messageText = l10n_util::GetNSStringWithFixup(IDS_ONE_CLICK_SIGNIN_DIALOG_MESSAGE);

[Alexei Svitkine (slow), 2013/04/19 19:54:15]

Wrap this so it doesn't excede 80 chars.

[noms (inactive), 2013/04/22 15:28:07]

Done.

+    messageText = [messageText stringByAppendingString:@" "];
+    font = ui::ResourceBundle::GetSharedInstance().GetFont(
       chrome_style::kTextFontStyle).GetNativeFont();
+  } else {
+    messageText = @"";
+    font = ui::ResourceBundle::GetSharedInstance().GetFont(
+      ui::ResourceBundle::SmallFont).GetNativeFont();

[Alexei Svitkine (slow), 2013/04/19 19:54:15]

I suggest just having a fontSize local variable that's assigned differently
based on the if - that way you don't have to repeat the wordy ResourceBundle
call (which should also be good for binary size).

[noms (inactive), 2013/04/22 15:28:07]

Done.

+  }
+
   NSColor* linkColor =
       gfx::SkColorToCalibratedNSColor(chrome_style::GetLinkColor());
   [informativeTextView_ setMessageAndLink:messageText
                                  withLink:learnMoreText
                                  atOffset:[messageText length]
                                      font:font
                              messageColor:[NSColor blackColor]
                                 linkColor:linkColor];
 
   // Size to fit.
   [[informativePlaceholderTextField_ cell] setAttributedStringValue:
       [informativeTextView_ attributedString]];
   [GTMUILocalizerAndLayoutTweaker
         sizeToFitFixedWidthTextField:informativePlaceholderTextField_];
   NSRect newFrame = [informativePlaceholderTextField_ frame];
   [informativeTextView_ setFrame:newFrame];
 
   // Swap placeholder.
   [[informativePlaceholderTextField_ superview]
      replaceSubview:informativePlaceholderTextField_
                with:informativeTextView_.get()];
   informativePlaceholderTextField_ = nil;  // Now released.
 
   return NSHeight(newFrame) - NSHeight(oldFrame);
 }
 
 - (BOOL)textView:(NSTextView*)textView
    clickedOnLink:(id)link
          atIndex:(NSUInteger)charIndex {
+  WindowOpenDisposition location = isSyncDialog_ ?
+                                   NEW_WINDOW : NEW_FOREGROUND_TAB;
+
   content::OpenURLParams params(GURL(chrome::kChromeSyncLearnMoreURL),
-                                content::Referrer(), NEW_WINDOW,
+                                content::Referrer(), location,
                                 content::PAGE_TRANSITION_LINK, false);
   webContents_->OpenURL(params);
   return YES;
 }
 
 - (void)close {
   base::ResetAndReturn(&closeCallback_).Run();
 }
 
 @end