Show sync setup in same tab as sign in instead of active tab.

Show sync setup in same tab as sign in instead of active tab.

When the user wanted to "Choose what to sync" in an explicit sign in,
the sync setup page was displayed in the active tab at the end of the
sign in process (except for "Sign in" initiated from the Settings page).
However, the user can change the active tab before that happens.

With this fix, the sync setup page is loaded in the same tab as the
sign in flow. If the sign in tab has been closed, a new tab is created.
The sync setup tab is also activated if it's not already.

TEST=
1. Open Chrome with a clean profile.
2. Sign in from the NTP, hot-dog menu or startup promo. Check "Choose what to sync".
3. After clicking on 'Submit', immediately navigate to some other tab.
Expected result: The sync setup page is loaded in the sign in tab and this tab is activated.
BUG=242428
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=216859

[fdoray, 2013-08-08 19:47:02]

Could you review this CL?
Thanks.

[fdoray, 2013-08-08 19:48:46]

+estade@
Owner for chrome/browser/ui/webui/*

+guohui@
Can you review chrome/browser/ui/webui/inline_login_ui.cc?

[noms, 2013-08-08 20:19:24]

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

https://codereview.chromium.org/22510004/diff/5001/chrome/browser/ui/sync/one...
chrome/browser/ui/sync/one_click_signin_helper.cc:1191: sync_setup_contents =
web_contents();
What's the difference between web_contents() here and contents on line 1197
below?

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

https://codereview.chromium.org/22510004/diff/5001/chrome/browser/ui/sync/one...
chrome/browser/ui/sync/one_click_signin_sync_starter.cc:462: void
OneClickSigninSyncStarter::ShowSyncSettingsPageOnSameTab() {
Can you maybe pass the web_contents here as a parameter so that you don't both
check for null and DCHECK? You already check that it's not null on line 427, how
could the web contents have become invalidated here?

[guohui, 2013-08-08 20:19:27]

On 2013/08/08 19:48:46, fdoray wrote:
> +estade@
> Owner for chrome/browser/ui/webui/*
> 
> +guohui@
> Can you review chrome/browser/ui/webui/inline_login_ui.cc?

lgtm for inline_login_ui.cc

[Roger Tawa OOO till Jul 10th, 2013-08-08 20:49:54]

lgtm

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

https://codereview.chromium.org/22510004/diff/5001/chrome/browser/ui/sync/one...
chrome/browser/ui/sync/one_click_signin_helper.cc:1191: sync_setup_contents =
web_contents();
On 2013/08/08 20:19:24, noms wrote:
> What's the difference between web_contents() here and contents on line 1197
> below?

They are the same.  See line 1076 above.

[fdoray, 2013-08-09 15:31:13]

Monica: I addressed your comments.
Evan: Could you review this CL?

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

https://codereview.chromium.org/22510004/diff/5001/chrome/browser/ui/sync/one...
chrome/browser/ui/sync/one_click_signin_helper.cc:1191: sync_setup_contents =
web_contents();
Should I use |contents|?

On 2013/08/08 20:49:54, Roger Tawa wrote:
> On 2013/08/08 20:19:24, noms wrote:
> > What's the difference between web_contents() here and contents on line 1197
> > below?
> 
> They are the same.  See line 1076 above.

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

https://codereview.chromium.org/22510004/diff/5001/chrome/browser/ui/sync/one...
chrome/browser/ui/sync/one_click_signin_sync_starter.cc:462: void
OneClickSigninSyncStarter::ShowSyncSettingsPageOnSameTab() {
On 2013/08/08 20:19:24, noms wrote:
> Can you maybe pass the web_contents here as a parameter so that you don't both
> check for null and DCHECK? You already check that it's not null on line 427,
how
> could the web contents have become invalidated here?

Done.

[noms, 2013-08-09 15:32:36]

lgtm

[Evan Stade, 2013-08-09 20:51:12]

lgtm

[commit-bot: I haz the power, 2013-08-10 01:10:22]

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

[commit-bot: I haz the power, 2013-08-10 14:59:56]

Change committed as 216859