Show email confirmation for sign in from the hot-dog menu

Show email confirmation for sign in from the hot-dog menu

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

[fdoray, 2013-06-05 20:37:48]

Could you take a look at this CL?

[Roger Tawa OOO till Jul 10th, 2013-06-05 23:48:37]

Salut Francois,

You could make RedirectToNtpOrAppsPage() and
RedirectToNtpOrAppsPageIfNecessary() standalone functions inside the .cc file in
the anonymous namespace.  They would need to take two args: a contents pointer
and a source.  Then you could call RedirectToNtpOrAppsPageIfNecessary() from
inside StartExplicitSync(), and you would not need to add an extra closure arg. 
What do you think?

Note that you'll have a merge issue with Brian's CL
https://codereview.chromium.org/16337017/

https://codereview.chromium.org/16172007/diff/1/chrome/browser/ui/sync/one_cl...
File chrome/browser/ui/sync/one_click_signin_helper.cc (right):

https://codereview.chromium.org/16172007/diff/1/chrome/browser/ui/sync/one_cl...
chrome/browser/ui/sync/one_click_signin_helper.cc:924:
RedirectToNtpOrAppsPage();
Add { and } for if statement.

[fdoray, 2013-06-10 15:43:57]

On 2013/06/05 23:48:37, Roger Tawa wrote:
> Salut Francois,
> 
> You could make RedirectToNtpOrAppsPage() and
> RedirectToNtpOrAppsPageIfNecessary() standalone functions inside the .cc file
in
> the anonymous namespace.  They would need to take two args: a contents pointer
> and a source.  Then you could call RedirectToNtpOrAppsPageIfNecessary() from
> inside StartExplicitSync(), and you would not need to add an extra closure
arg. 
> What do you think?
> 
> Note that you'll have a merge issue with Brian's CL
> https://codereview.chromium.org/16337017/
> 
>
https://codereview.chromium.org/16172007/diff/1/chrome/browser/ui/sync/one_cl...
> File chrome/browser/ui/sync/one_click_signin_helper.cc (right):
> 
>
https://codereview.chromium.org/16172007/diff/1/chrome/browser/ui/sync/one_cl...
> chrome/browser/ui/sync/one_click_signin_helper.cc:924:
> RedirectToNtpOrAppsPage();
> Add { and } for if statement.

I've made the changes suggested by Roger. It's true that it's better to have
RedirectToNtpOrAppsPage() in the anonymous namespace.

[fdoray, 2013-06-10 15:44:13]

https://codereview.chromium.org/16172007/diff/1/chrome/browser/ui/sync/one_cl...
File chrome/browser/ui/sync/one_click_signin_helper.cc (right):

https://codereview.chromium.org/16172007/diff/1/chrome/browser/ui/sync/one_cl...
chrome/browser/ui/sync/one_click_signin_helper.cc:924:
RedirectToNtpOrAppsPage();
On 2013/06/05 23:48:37, Roger Tawa wrote:
> Add { and } for if statement.

Done.

[Roger Tawa OOO till Jul 10th, 2013-06-10 17:36:21]

lgtm

Thanks Francois.

[noms (inactive), 2013-06-10 17:41:29]

lgtm

[bcwhite, 2013-06-11 15:57:02]

https://codereview.chromium.org/16172007/diff/5001/chrome/browser/ui/sync/one...
File chrome/browser/ui/sync/one_click_signin_helper.cc (right):

https://codereview.chromium.org/16172007/diff/5001/chrome/browser/ui/sync/one...
chrome/browser/ui/sync/one_click_signin_helper.cc:1208:
RedirectToNtpOrAppsPageIfNecessary(web_contents(), source_);
This is now a conditional call whereas before it it was always done (below)
regardless of which block of the condition was executed.

[fdoray, 2013-06-11 16:03:34]

I answered Brian's question.

https://codereview.chromium.org/16172007/diff/5001/chrome/browser/ui/sync/one...
File chrome/browser/ui/sync/one_click_signin_helper.cc (right):

https://codereview.chromium.org/16172007/diff/5001/chrome/browser/ui/sync/one...
chrome/browser/ui/sync/one_click_signin_helper.cc:1208:
RedirectToNtpOrAppsPageIfNecessary(web_contents(), source_);
On 2013/06/11 15:57:02, bcwhite wrote:
> This is now a conditional call whereas before it it was always done (below)
> regardless of which block of the condition was executed.

If we go in the first code block, the StartExplicitSync callback will be
executed and RedirectToNtpOrAppsPageIfNecessary will be called in it (unless the
user clicks on the "Create a new user" button, but then we redirect to the
Settings page).

[bcwhite, 2013-06-11 17:45:51]

lgtm

[commit-bot: I haz the power, 2013-06-13 16:53:14]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/fdoray@chromium.org/16172007/18001

[commit-bot: I haz the power, 2013-06-13 21:46:50]

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

[commit-bot: I haz the power, 2013-06-13 22:47:28]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/fdoray@chromium.org/16172007/18001

[commit-bot: I haz the power, 2013-06-14 00:02:40]

Change committed as 206231