Win: Display a native bubble (instead of the JS one) after the web signin flow.

Win: Display a native bubble (instead of the JS one) after the web signin flow.

Part of a 3 CL Voltron, along with:
[GTK] https://codereview.chromium.org/14258007
[Mac] https://codereview.chromium.org/13845022/

BUG=119728
TEST=Open Chrome. Delete all your profiles, so that you have a fresh one. Open a new tab, and press the "sign in" link in the top right corner. Sign in. Upon completion, this should show a native popup bubble, anchored off the wrench menu. If you try to log in with the same email in another profile, the bubble should display a sad error message. Check out the issue description for screenshots.

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

[bcwhite, 2013-04-16 15:21:59]

Nicely done.  A few minor comments.  Also, be sure to check the "? errors" link
for lint comments.

https://codereview.chromium.org/13979003/diff/18001/chrome/browser/ui/cocoa/b...
File chrome/browser/ui/cocoa/browser_window_cocoa.mm (right):

https://codereview.chromium.org/13979003/diff/18001/chrome/browser/ui/cocoa/b...
chrome/browser/ui/cocoa/browser_window_cocoa.mm:485: ) {
Move up on previous line.

https://codereview.chromium.org/13979003/diff/18001/chrome/browser/ui/views/s...
File chrome/browser/ui/views/sync/one_click_signin_bubble_view.cc (right):

https://codereview.chromium.org/13979003/diff/18001/chrome/browser/ui/views/s...
chrome/browser/ui/views/sync/one_click_signin_bubble_view.cc:95: :
OneClickSigninBubbleView(web_content, anchor_view, ""),
std::string() is better than "", here and elsewhere.

https://codereview.chromium.org/13979003/diff/18001/chrome/browser/ui/views/s...
File chrome/browser/ui/views/sync/one_click_signin_bubble_view.h (right):

https://codereview.chromium.org/13979003/diff/18001/chrome/browser/ui/views/s...
chrome/browser/ui/views/sync/one_click_signin_bubble_view.h:123: // By default,
the Undo button is not implemented
Not implemented?  Meaning it can return NULL?

https://codereview.chromium.org/13979003/diff/18001/chrome/browser/ui/views/s...
File chrome/browser/ui/views/sync/one_click_signin_bubble_view_browsertest.cc
(right):

https://codereview.chromium.org/13979003/diff/18001/chrome/browser/ui/views/s...
chrome/browser/ui/views/sync/one_click_signin_bubble_view_browsertest.cc:36:
//content::RunAllPendingInMessageLoop();
Remove.

[noms (inactive), 2013-04-16 17:15:37]

https://codereview.chromium.org/13979003/diff/18001/chrome/browser/ui/cocoa/b...
File chrome/browser/ui/cocoa/browser_window_cocoa.mm (right):

https://codereview.chromium.org/13979003/diff/18001/chrome/browser/ui/cocoa/b...
chrome/browser/ui/cocoa/browser_window_cocoa.mm:485: ) {
On 2013/04/16 15:22:00, bcwhite wrote:
> Move up on previous line.

Done.

https://codereview.chromium.org/13979003/diff/18001/chrome/browser/ui/views/s...
File chrome/browser/ui/views/sync/one_click_signin_bubble_view.cc (right):

https://codereview.chromium.org/13979003/diff/18001/chrome/browser/ui/views/s...
chrome/browser/ui/views/sync/one_click_signin_bubble_view.cc:95: :
OneClickSigninBubbleView(web_content, anchor_view, ""),
On 2013/04/16 15:22:00, bcwhite wrote:
> std::string() is better than "", here and elsewhere.

Done.

https://codereview.chromium.org/13979003/diff/18001/chrome/browser/ui/views/s...
File chrome/browser/ui/views/sync/one_click_signin_bubble_view.h (right):

https://codereview.chromium.org/13979003/diff/18001/chrome/browser/ui/views/s...
chrome/browser/ui/views/sync/one_click_signin_bubble_view.h:123: // By default,
the Undo button is not implemented
Oops. Just meant it's not initially displayed. Have updated the comment.

On 2013/04/16 15:22:00, bcwhite wrote:
> Not implemented?  Meaning it can return NULL?

https://codereview.chromium.org/13979003/diff/18001/chrome/browser/ui/views/s...
File chrome/browser/ui/views/sync/one_click_signin_bubble_view_browsertest.cc
(right):

https://codereview.chromium.org/13979003/diff/18001/chrome/browser/ui/views/s...
chrome/browser/ui/views/sync/one_click_signin_bubble_view_browsertest.cc:36:
//content::RunAllPendingInMessageLoop();
On 2013/04/16 15:22:00, bcwhite wrote:
> Remove.

Done.

[tfarina, 2013-04-16 17:25:28]

Please, make sure to ask for OWNERS of c/b/ui/gtk, c/b/ui/cocoa and c/b/ui/views
when ready!

My suggestion would be for: erg, thakis and sky.

Thanks!

https://codereview.chromium.org/13979003/diff/18001/chrome/browser/ui/views/s...
File chrome/browser/ui/views/sync/one_click_signin_bubble_view.cc (right):

https://codereview.chromium.org/13979003/diff/18001/chrome/browser/ui/views/s...
chrome/browser/ui/views/sync/one_click_signin_bubble_view.cc:72: // views::View
method:
be consistent with the file you are modifying. All the current above are using
"Overridden from ..." so this should too.

https://codereview.chromium.org/13979003/diff/18001/chrome/browser/ui/views/s...
chrome/browser/ui/views/sync/one_click_signin_bubble_view.cc:95: :
OneClickSigninBubbleView(web_content, anchor_view, ""),
On 2013/04/16 15:22:00, bcwhite wrote:
> std::string() is better than "", here and elsewhere.

It isn't just better:
https://codereview.chromium.org/12746010/
https://codereview.chromium.org/13145003/

[Roger Tawa OOO till Jul 10th, 2013-04-17 17:53:25]

Looking good Monica.  My main question is about the web contents member
variable.  Please see below.

https://codereview.chromium.org/13979003/diff/23003/chrome/browser/ui/browser...
File chrome/browser/ui/browser_window.h (right):

https://codereview.chromium.org/13979003/diff/23003/chrome/browser/ui/browser...
chrome/browser/ui/browser_window.h:235: const std::string& error_message) = 0;
nit: is it better to put the error_message arg before the callback ?

https://codereview.chromium.org/13979003/diff/23003/chrome/browser/ui/sync/on...
File chrome/browser/ui/sync/one_click_signin_helper.cc (right):

https://codereview.chromium.org/13979003/diff/23003/chrome/browser/ui/sync/on...
chrome/browser/ui/sync/one_click_signin_helper.cc:982: Browser* browser =
chrome::FindBrowserWithWebContents(contents);
Move the var declaration closer to where its used (i.e. inside the if block).

https://codereview.chromium.org/13979003/diff/23003/chrome/browser/ui/sync/on...
chrome/browser/ui/sync/one_click_signin_helper.cc:1245: error_message_);
why not std::string() like line 1284 below?  Will there ever be an message in
this case?

https://codereview.chromium.org/13979003/diff/23003/chrome/browser/ui/sync/on...
File chrome/browser/ui/sync/one_click_signin_helper_unittest.cc (left):

https://codereview.chromium.org/13979003/diff/23003/chrome/browser/ui/sync/on...
chrome/browser/ui/sync/one_click_signin_helper_unittest.cc:744: 
Instead of removing this tests, can they be rewritten to test that the new
bubble is showing?  Is there a platform independent way to test for that?

https://codereview.chromium.org/13979003/diff/23003/chrome/browser/ui/views/s...
File chrome/browser/ui/views/sync/one_click_signin_bubble_view.cc (right):

https://codereview.chromium.org/13979003/diff/23003/chrome/browser/ui/views/s...
chrome/browser/ui/views/sync/one_click_signin_bubble_view.cc:212:
web_content_->OpenURL(params);
This is duplicated in the base class.  I wonder if this can be refactored
somehow.  What if there was a method in the base class like HandleLinkClicked(),
which returned something to say if the learn more was clicked or not?  Then this
code could still invoke the callback only when needed.

https://codereview.chromium.org/13979003/diff/23003/chrome/browser/ui/views/s...
chrome/browser/ui/views/sync/one_click_signin_bubble_view.cc:224: bool
isAcceleratorPressed =
variable naming should be:

is_accelerator_pressed

https://codereview.chromium.org/13979003/diff/23003/chrome/browser/ui/views/s...
chrome/browser/ui/views/sync/one_click_signin_bubble_view.cc:265:
toolbar_view->GetWebContents(),
I'm concerned about storing a contents pointer as a member of the bubble view
class.  I suspect it could be possible for the bubble to outlive the contents. 
This does not concern me too much with the dialogs since they are modal, but it
*may* be a problem there too.

The contents is used to show the a tab when the user clicks on a link in the
bubble or dialog.  I wonder if it would be better to save a pointer the Browser
object instead (which will certainly outlive the bubble or dialog) and then if a
link is clicked the code opens a new tab instead via Browser::OpenURL() ?

https://codereview.chromium.org/13979003/diff/23003/chrome/browser/ui/views/s...
chrome/browser/ui/views/sync/one_click_signin_bubble_view.cc:266:
toolbar_view->app_menu(), error_message);
ident lines 265 and 266 by 2 more.

https://codereview.chromium.org/13979003/diff/23003/chrome/browser/ui/views/s...
chrome/browser/ui/views/sync/one_click_signin_bubble_view.cc:295:
content::WebContents* web_content,
indent 2 more.

https://codereview.chromium.org/13979003/diff/23003/chrome/browser/ui/views/s...
chrome/browser/ui/views/sync/one_click_signin_bubble_view.cc:359:
l10n_util::GetStringUTF16(IDS_ONE_CLICK_SIGNIN_BUBBLE_MESSAGE));
indent 2 more.

https://codereview.chromium.org/13979003/diff/23003/chrome/browser/ui/views/s...
chrome/browser/ui/views/sync/one_click_signin_bubble_view.cc:376:
layout->AddView(ok_button_);
I'm not sure InitButtons() needs to be virtual or reimplemented/overridden in
the derived class.  If its implemented only in the base class, and it checks the
returned pointers for null before adding them to the layout, I think it can be
made generic.

Also, since the bubble no longer needs an undo button (only the dialog does)
then probably need to be creating it anyway.  Might be able to remove a lot of
code in OneClickSigninBubbleView::GetButtons().

https://codereview.chromium.org/13979003/diff/23003/chrome/browser/ui/views/s...
chrome/browser/ui/views/sync/one_click_signin_bubble_view.cc:395:
l10n_util::GetStringUTF16(IDS_ONE_CLICK_SIGNIN_DIALOG_UNDO_BUTTON);
Just want to double check this is the correct string.  The IDS contains the word
"DIALOG", but this is not the dialog implementation.  Same for line 405 below.

https://codereview.chromium.org/13979003/diff/23003/chrome/browser/ui/views/s...
chrome/browser/ui/views/sync/one_click_signin_bubble_view.cc:413:
l10n_util::GetStringUTF16(IDS_LEARN_MORE));
indent 2 less.

https://codereview.chromium.org/13979003/diff/23003/chrome/browser/ui/views/s...
chrome/browser/ui/views/sync/one_click_signin_bubble_view.cc:417: return
learn_more_link_;
Local var should not have a trailing _ in name.

However, if this method were renamed to something like CreateLearnMoreLink(), it
could the member variable as well as return the pointer.  Then you can make this
member variable private.

https://codereview.chromium.org/13979003/diff/23003/chrome/browser/ui/views/s...
chrome/browser/ui/views/sync/one_click_signin_bubble_view.cc:424:
OneClickSigninBubbleView::Hide();
I don't think you need the class name here.  Same at line 448 below.

https://codereview.chromium.org/13979003/diff/23003/chrome/browser/ui/views/s...
File chrome/browser/ui/views/sync/one_click_signin_bubble_view.h (right):

https://codereview.chromium.org/13979003/diff/23003/chrome/browser/ui/views/s...
chrome/browser/ui/views/sync/one_click_signin_bubble_view.h:81: virtual
views::Link* GetAdvancedLink();
The GetXXXLink() methods should probably moved below all the base class
overrides.  Why are these methods protected?

https://codereview.chromium.org/13979003/diff/23003/chrome/browser/ui/views/s...
chrome/browser/ui/views/sync/one_click_signin_bubble_view.h:96:
content::WebContents* web_content_;
Member variables should always be private.  Might be better to name
web_contents_ ?

[noms (inactive), 2013-04-17 20:32:37]

Merged the Bubble/Dialog classes as discussed, and hopefully fixed all of the
other comments. Please review when free :)

https://codereview.chromium.org/13979003/diff/23003/chrome/browser/ui/browser...
File chrome/browser/ui/browser_window.h (right):

https://codereview.chromium.org/13979003/diff/23003/chrome/browser/ui/browser...
chrome/browser/ui/browser_window.h:235: const std::string& error_message) = 0;
On 2013/04/17 17:53:25, Roger Tawa wrote:
> nit: is it better to put the error_message arg before the callback ?

Done.

https://codereview.chromium.org/13979003/diff/23003/chrome/browser/ui/sync/on...
File chrome/browser/ui/sync/one_click_signin_helper.cc (right):

https://codereview.chromium.org/13979003/diff/23003/chrome/browser/ui/sync/on...
chrome/browser/ui/sync/one_click_signin_helper.cc:982: Browser* browser =
chrome::FindBrowserWithWebContents(contents);
On 2013/04/17 17:53:25, Roger Tawa wrote:
> Move the var declaration closer to where its used (i.e. inside the if block).

Done.

https://codereview.chromium.org/13979003/diff/23003/chrome/browser/ui/sync/on...
chrome/browser/ui/sync/one_click_signin_helper.cc:1245: error_message_);
On 2013/04/17 17:53:25, Roger Tawa wrote:
> why not std::string() like line 1284 below?  Will there ever be an message in
> this case?

Done.

https://codereview.chromium.org/13979003/diff/23003/chrome/browser/ui/sync/on...
File chrome/browser/ui/sync/one_click_signin_helper_unittest.cc (left):

https://codereview.chromium.org/13979003/diff/23003/chrome/browser/ui/sync/on...
chrome/browser/ui/sync/one_click_signin_helper_unittest.cc:744: 
To make sure it didn't go untested, I've rewritten them as browser tests. All
these particular tests used to do was check if a "showPopup" preference was set
in the PrefsService, which isn't applicable anymore. I'm not sure there is
something equivalent to tell whether the native dialog is showing.

On 2013/04/17 17:53:25, Roger Tawa wrote:
> Instead of removing this tests, can they be rewritten to test that the new
> bubble is showing?  Is there a platform independent way to test for that?

https://codereview.chromium.org/13979003/diff/23003/chrome/browser/ui/views/s...
File chrome/browser/ui/views/sync/one_click_signin_bubble_view.cc (right):

https://codereview.chromium.org/13979003/diff/23003/chrome/browser/ui/views/s...
chrome/browser/ui/views/sync/one_click_signin_bubble_view.cc:224: bool
isAcceleratorPressed =
On 2013/04/17 17:53:25, Roger Tawa wrote:
> variable naming should be:
> 
> is_accelerator_pressed

Done.

https://codereview.chromium.org/13979003/diff/23003/chrome/browser/ui/views/s...
chrome/browser/ui/views/sync/one_click_signin_bubble_view.cc:265:
toolbar_view->GetWebContents(),
Added a comment that both the dialog and the bubble outlive the tab (the bubble
registers the click and closes, and the tab is modal)

On 2013/04/17 17:53:25, Roger Tawa wrote:
> I'm concerned about storing a contents pointer as a member of the bubble view
> class.  I suspect it could be possible for the bubble to outlive the contents.

> This does not concern me too much with the dialogs since they are modal, but
it
> *may* be a problem there too.
> 
> The contents is used to show the a tab when the user clicks on a link in the
> bubble or dialog.  I wonder if it would be better to save a pointer the
Browser
> object instead (which will certainly outlive the bubble or dialog) and then if
a
> link is clicked the code opens a new tab instead via Browser::OpenURL() ?

https://codereview.chromium.org/13979003/diff/23003/chrome/browser/ui/views/s...
chrome/browser/ui/views/sync/one_click_signin_bubble_view.cc:266:
toolbar_view->app_menu(), error_message);
On 2013/04/17 17:53:25, Roger Tawa wrote:
> ident lines 265 and 266 by 2 more.

Done.

https://codereview.chromium.org/13979003/diff/23003/chrome/browser/ui/views/s...
chrome/browser/ui/views/sync/one_click_signin_bubble_view.cc:295:
content::WebContents* web_content,
On 2013/04/17 17:53:25, Roger Tawa wrote:
> indent 2 more.

Done.

https://codereview.chromium.org/13979003/diff/23003/chrome/browser/ui/views/s...
chrome/browser/ui/views/sync/one_click_signin_bubble_view.cc:359:
l10n_util::GetStringUTF16(IDS_ONE_CLICK_SIGNIN_BUBBLE_MESSAGE));
On 2013/04/17 17:53:25, Roger Tawa wrote:
> indent 2 more.

Done.

https://codereview.chromium.org/13979003/diff/23003/chrome/browser/ui/views/s...
chrome/browser/ui/views/sync/one_click_signin_bubble_view.cc:376:
layout->AddView(ok_button_);
The two InitButtons functions got merged, so this isn't an issue anymre

On 2013/04/17 17:53:25, Roger Tawa wrote:
> I'm not sure InitButtons() needs to be virtual or reimplemented/overridden in
> the derived class.  If its implemented only in the base class, and it checks
the
> returned pointers for null before adding them to the layout, I think it can be
> made generic.
> 
> Also, since the bubble no longer needs an undo button (only the dialog does)
> then probably need to be creating it anyway.  Might be able to remove a lot of
> code in OneClickSigninBubbleView::GetButtons().

https://codereview.chromium.org/13979003/diff/23003/chrome/browser/ui/views/s...
chrome/browser/ui/views/sync/one_click_signin_bubble_view.cc:395:
l10n_util::GetStringUTF16(IDS_ONE_CLICK_SIGNIN_DIALOG_UNDO_BUTTON);
On 2013/04/17 17:53:25, Roger Tawa wrote:
> Just want to double check this is the correct string.  The IDS contains the
word
> "DIALOG", but this is not the dialog implementation.  Same for line 405 below.

Done.

https://codereview.chromium.org/13979003/diff/23003/chrome/browser/ui/views/s...
chrome/browser/ui/views/sync/one_click_signin_bubble_view.cc:413:
l10n_util::GetStringUTF16(IDS_LEARN_MORE));
On 2013/04/17 17:53:25, Roger Tawa wrote:
> indent 2 less.

Done.

https://codereview.chromium.org/13979003/diff/23003/chrome/browser/ui/views/s...
chrome/browser/ui/views/sync/one_click_signin_bubble_view.cc:417: return
learn_more_link_;
Made private, renamed it to InitXXXLink and removed the return type, to be
consistent with InitContent/InitButtons etc.

On 2013/04/17 17:53:25, Roger Tawa wrote:
> Local var should not have a trailing _ in name.
> 
> However, if this method were renamed to something like CreateLearnMoreLink(),
it
> could the member variable as well as return the pointer.  Then you can make
this
> member variable private.

https://codereview.chromium.org/13979003/diff/23003/chrome/browser/ui/views/s...
chrome/browser/ui/views/sync/one_click_signin_bubble_view.cc:424:
OneClickSigninBubbleView::Hide();
On 2013/04/17 17:53:25, Roger Tawa wrote:
> I don't think you need the class name here.  Same at line 448 below.

Done.

https://codereview.chromium.org/13979003/diff/23003/chrome/browser/ui/views/s...
File chrome/browser/ui/views/sync/one_click_signin_bubble_view.h (right):

https://codereview.chromium.org/13979003/diff/23003/chrome/browser/ui/views/s...
chrome/browser/ui/views/sync/one_click_signin_bubble_view.h:81: virtual
views::Link* GetAdvancedLink();
Moved all of them to private as I killed the silly inheritance structure.

On 2013/04/17 17:53:25, Roger Tawa wrote:
> The GetXXXLink() methods should probably moved below all the base class
> overrides.  Why are these methods protected?

https://codereview.chromium.org/13979003/diff/23003/chrome/browser/ui/views/s...
chrome/browser/ui/views/sync/one_click_signin_bubble_view.h:96:
content::WebContents* web_content_;
On 2013/04/17 17:53:25, Roger Tawa wrote:
> Member variables should always be private.  Might be better to name
> web_contents_ ?

Done.

[Roger Tawa OOO till Jul 10th, 2013-04-17 21:09:17]

lgtm

https://codereview.chromium.org/13979003/diff/34001/chrome/browser/ui/views/s...
File chrome/browser/ui/views/sync/one_click_signin_bubble_view.cc (right):

https://codereview.chromium.org/13979003/diff/34001/chrome/browser/ui/views/s...
chrome/browser/ui/views/sync/one_click_signin_bubble_view.cc:260:
(*undo_button)->SetStyle(views::Button::STYLE_NATIVE_TEXTBUTTON);
if !is_sync_dialog_, probably should not create the undo button.

[noms (inactive), 2013-04-18 17:25:52]

Hi sky/thakis/erg,

Could you please do an owners review? I've split this fix up in 3 different CLs,
and the GTK/Mac platform-specific implementations are coming in followup CLs
later on this week. I will only commit them all at the same time -- hope that
sounds ok (the alternative was a massive CL, but let me know if you'd prefer
that)

@thakis: 
chrome/browser/ui/cocoa/browser_window_cocoa.h	 
chrome/browser/ui/cocoa/browser_window_cocoa.mm

@erg
chrome/browser/ui/gtk/browser_window_gtk.h
chrome/browser/ui/gtk/browser_window_gtk.cc

@sky 
chrome/browser/ui/views/frame/browser_view.h
chrome/browser/ui/views/frame/browser_view.cc	
chrome/browser/ui/views/sync/one_click_signin_bubble_view.h
chrome/browser/ui/views/sync/one_click_signin_bubble_view.cc
chrome/browser/ui/views/sync/one_click_signin_bubble_view_browsertest.cc

Thanks!

[Nico, 2013-04-18 17:53:28]

cocoa/ lgtm

One inconsequential question below.

https://codereview.chromium.org/13979003/diff/54001/chrome/browser/ui/browser...
File chrome/browser/ui/browser_window.h (right):

https://codereview.chromium.org/13979003/diff/54001/chrome/browser/ui/browser...
chrome/browser/ui/browser_window.h:234: const std::string& error_message,
We used to use string16 for text that could possibly contain international text.
I think that's no longer true, but since the previous parameter is string16,
should error_message be string16 too?

[noms (inactive), 2013-04-18 18:08:28]

https://codereview.chromium.org/13979003/diff/54001/chrome/browser/ui/browser...
File chrome/browser/ui/browser_window.h (right):

https://codereview.chromium.org/13979003/diff/54001/chrome/browser/ui/browser...
chrome/browser/ui/browser_window.h:234: const std::string& error_message,
The error_message comes as a utf8 string from
OneClickSigninHelper::SigninFailed, where it seems to be
gotted/internationalized with l10n_util::GetStringUTF8.

I could convert it to string16() when ShowOneClickSigninBubble is called,
instead of when displaying it in the views label as I currently do -- would that
be better? I don't particularly know why it isn't got initially as a UTF16, but
one option I guess would also be changing that?


On 2013/04/18 17:53:28, Nico wrote:
> We used to use string16 for text that could possibly contain international
text.
> I think that's no longer true, but since the previous parameter is string16,
> should error_message be string16 too?

[Roger Tawa OOO till Jul 10th, 2013-04-18 19:04:11]

https://codereview.chromium.org/13979003/diff/54001/chrome/browser/ui/browser...
File chrome/browser/ui/browser_window.h (right):

https://codereview.chromium.org/13979003/diff/54001/chrome/browser/ui/browser...
chrome/browser/ui/browser_window.h:234: const std::string& error_message,
On 2013/04/18 18:08:28, Monica Dinculescu wrote:
> The error_message comes as a utf8 string from
> OneClickSigninHelper::SigninFailed, where it seems to be
> gotted/internationalized with l10n_util::GetStringUTF8.
> 
> I could convert it to string16() when ShowOneClickSigninBubble is called,
> instead of when displaying it in the views label as I currently do -- would
that
> be better? I don't particularly know why it isn't got initially as a UTF16,
but
> one option I guess would also be changing that?
> 
> 
> On 2013/04/18 17:53:28, Nico wrote:
> > We used to use string16 for text that could possibly contain international
> text.
> > I think that's no longer true, but since the previous parameter is string16,
> > should error_message be string16 too?
> 

It might have been utf8 previously because that is what the ntp bubble expected.
 But with the new bubble, changing to string16 in one-clicks-signin before
passing to ShowOneClickSigninBubble sounds good.

[noms (inactive), 2013-04-19 19:13:00]

Done.

I left the OneClickSigninHelper::SigninFailed to continue returning an
std::string, as that's what it gets from the GoogleServiceAuthError object, and
changing that is a pretty deep rabbit hole. :)

On 2013/04/18 19:04:11, Roger Tawa wrote:
>
https://codereview.chromium.org/13979003/diff/54001/chrome/browser/ui/browser...
> File chrome/browser/ui/browser_window.h (right):
> 
>
https://codereview.chromium.org/13979003/diff/54001/chrome/browser/ui/browser...
> chrome/browser/ui/browser_window.h:234: const std::string& error_message,
> On 2013/04/18 18:08:28, Monica Dinculescu wrote:
> > The error_message comes as a utf8 string from
> > OneClickSigninHelper::SigninFailed, where it seems to be
> > gotted/internationalized with l10n_util::GetStringUTF8.
> > 
> > I could convert it to string16() when ShowOneClickSigninBubble is called,
> > instead of when displaying it in the views label as I currently do -- would
> that
> > be better? I don't particularly know why it isn't got initially as a UTF16,
> but
> > one option I guess would also be changing that?
> > 
> > 
> > On 2013/04/18 17:53:28, Nico wrote:
> > > We used to use string16 for text that could possibly contain international
> > text.
> > > I think that's no longer true, but since the previous parameter is
string16,
> > > should error_message be string16 too?
> > 
> 
> It might have been utf8 previously because that is what the ntp bubble
expected.
>  But with the new bubble, changing to string16 in one-clicks-signin before
> passing to ShowOneClickSigninBubble sounds good.

[Roger Tawa OOO till Jul 10th, 2013-04-22 20:49:05]

https://codereview.chromium.org/13979003/diff/39003/chrome/browser/ui/sync/on...
File chrome/browser/ui/sync/one_click_signin_helper.cc (right):

https://codereview.chromium.org/13979003/diff/39003/chrome/browser/ui/sync/on...
chrome/browser/ui/sync/one_click_signin_helper.cc:1406:
RedirectToNtpOrAppsPage(show_bubble);
The sign in code does two redirects to the NTP: once at line 1326 and another
here.  The reason for this is to show the NTP as soon as possible so that the
user is not staring at a blank page (line 3126).  Then, once we know whether the
sign in was successful or not, we redirect again to display the bubble. 
Redirecting to the NTP is the only way to get that bubble shown (here).

Now that the bubble is not in the NTP, we probably don't need the extra redirect
here.  Brian is looking into a bug where the multiple redirect cause a race
condition.  Please go see him and see if removing the extra call would help him
out.  Even if it does not, seems like we should get rid the extra redirect
anyway.

[Elliot Glaysher, 2013-04-23 18:18:40]

gtk lgtm stamp.

[noms (inactive), 2013-04-23 19:21:22]

https://codereview.chromium.org/13979003/diff/39003/chrome/browser/ui/sync/on...
File chrome/browser/ui/sync/one_click_signin_helper.cc (right):

https://codereview.chromium.org/13979003/diff/39003/chrome/browser/ui/sync/on...
chrome/browser/ui/sync/one_click_signin_helper.cc:1406:
RedirectToNtpOrAppsPage(show_bubble);
Done. (I hope!) 

On 2013/04/22 20:49:05, Roger Tawa wrote:
> The sign in code does two redirects to the NTP: once at line 1326 and another
> here.  The reason for this is to show the NTP as soon as possible so that the
> user is not staring at a blank page (line 3126).  Then, once we know whether
the
> sign in was successful or not, we redirect again to display the bubble. 
> Redirecting to the NTP is the only way to get that bubble shown (here).
> 
> Now that the bubble is not in the NTP, we probably don't need the extra
redirect
> here.  Brian is looking into a bug where the multiple redirect cause a race
> condition.  Please go see him and see if removing the extra call would help
him
> out.  Even if it does not, seems like we should get rid the extra redirect
> anyway.

[Roger Tawa OOO till Jul 10th, 2013-04-23 19:49:12]

lgtm

https://codereview.chromium.org/13979003/diff/73001/chrome/browser/ui/sync/on...
File chrome/browser/ui/sync/one_click_signin_helper.cc (right):

https://codereview.chromium.org/13979003/diff/73001/chrome/browser/ui/sync/on...
chrome/browser/ui/sync/one_click_signin_helper.cc:979:
Profile::FromBrowserContext(contents->GetBrowserContext());
Can move these variables into the if block.

[noms (inactive), 2013-04-25 13:59:15]

+sky for views owners

[sky, 2013-04-25 16:04:04]

LGTM

https://codereview.chromium.org/13979003/diff/81001/chrome/browser/ui/views/s...
File chrome/browser/ui/views/sync/one_click_signin_bubble_view.h (right):

https://codereview.chromium.org/13979003/diff/81001/chrome/browser/ui/views/s...
chrome/browser/ui/views/sync/one_click_signin_bubble_view.h:123: string16
error_message_;
const

https://codereview.chromium.org/13979003/diff/81001/chrome/browser/ui/views/s...
chrome/browser/ui/views/sync/one_click_signin_bubble_view.h:132: bool
is_sync_dialog_;
const

[Roger Tawa OOO till Jul 10th, 2013-04-26 18:28:20]

Hi Monica,

A few smalls nits below, sorry :-)

https://codereview.chromium.org/13979003/diff/89001/chrome/browser/ui/browser...
File chrome/browser/ui/browser_window.h (right):

https://codereview.chromium.org/13979003/diff/89001/chrome/browser/ui/browser...
chrome/browser/ui/browser_window.h:8: #include <string>
You probably don't need this anymore since the new arg is a string16 and not an
std::string.

https://codereview.chromium.org/13979003/diff/89001/chrome/browser/ui/cocoa/b...
File chrome/browser/ui/cocoa/browser_window_cocoa.h (right):

https://codereview.chromium.org/13979003/diff/89001/chrome/browser/ui/cocoa/b...
chrome/browser/ui/cocoa/browser_window_cocoa.h:8: #include <string>
Probably not needed either.

https://codereview.chromium.org/13979003/diff/89001/chrome/browser/ui/sync/on...
File chrome/browser/ui/sync/one_click_signin_helper.cc (right):

https://codereview.chromium.org/13979003/diff/89001/chrome/browser/ui/sync/on...
chrome/browser/ui/sync/one_click_signin_helper.cc:841:
Profile::FromBrowserContext(contents->GetBrowserContext());
Indent two more spaces.

https://codereview.chromium.org/13979003/diff/89001/chrome/browser/ui/sync/on...
chrome/browser/ui/sync/one_click_signin_helper.cc:848: base::Bind(&StartSync,
Since the type is BUBBLE, the callback is never called.  Maybe better to pass an
empty callback than one that points to StartSync.  This goes with the DCHECK
suggested in one_click_signin_bubble_view.cc

https://codereview.chromium.org/13979003/diff/89001/chrome/browser/ui/sync/on...
chrome/browser/ui/sync/one_click_signin_helper.cc:851: false)));
Lines 845-851 should be indented two more spaces.

https://codereview.chromium.org/13979003/diff/89001/chrome/browser/ui/views/s...
File chrome/browser/ui/views/sync/one_click_signin_bubble_view.cc (right):

https://codereview.chromium.org/13979003/diff/89001/chrome/browser/ui/views/s...
chrome/browser/ui/views/sync/one_click_signin_bubble_view.cc:119: }
if |is_sync_dialog_| is false, might be a good idea to do:

   DCHECK(start_sync_callback_.is_null());

since its never used anyway.

[commit-bot: I haz the power, 2013-04-30 17:20:17]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/noms@chromium.org/13979003/116001

[commit-bot: I haz the power, 2013-05-01 14:04:18]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/noms@chromium.org/13979003/116001

[commit-bot: I haz the power, 2013-05-01 15:59:28]

Change committed as 197622

[M-A Ruel, 2013-05-21 17:48:25]

https://chromiumcodereview.appspot.com/13979003/diff/116001/chrome/browser/ui...
File chrome/browser/ui/views/sync/one_click_signin_bubble_view_browsertest.cc
(right):

https://chromiumcodereview.appspot.com/13979003/diff/116001/chrome/browser/ui...
chrome/browser/ui/views/sync/one_click_signin_bubble_view_browsertest.cc:51:
IN_PROC_BROWSER_TEST_F(OneClickSigninBubbleViewBrowserTest, ShowBubble) {
I saw this test fail on a try job for no obvious reason. To me, the name make it
sounds like it requires focus, which would mean it should be an
interactive_ui_test and not a browser_tests.