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

Index: chrome/browser/ui/cocoa/one_click_signin_view_controller.mm
diff --git a/chrome/browser/ui/cocoa/one_click_signin_view_controller.mm b/chrome/browser/ui/cocoa/one_click_signin_view_controller.mm
index 05a43e94d20ad86bfb515f2a455a9e5c7b5ad165..5081c2bc9fcf59d761d883292cc0411630e63c40 100644
--- a/chrome/browser/ui/cocoa/one_click_signin_view_controller.mm
+++ b/chrome/browser/ui/cocoa/one_click_signin_view_controller.mm
@@ -7,6 +7,7 @@
 #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"
@@ -35,42 +36,60 @@ void ShiftOriginY(NSView* view, CGFloat amount) {
 
 @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];
 }
 
@@ -98,6 +117,9 @@ void ShiftOriginY(NSView* view, CGFloat amount) {
 
   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]
@@ -119,11 +141,23 @@ void ShiftOriginY(NSView* view, CGFloat amount) {
 
   // 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
@@ -153,8 +187,11 @@ void ShiftOriginY(NSView* view, CGFloat amount) {
 - (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;