Setup from settings should allow configuration first

Setup from settings should allow configuration first.

BUG=81265
TEST=When setting up sync using the gaia-based web flow, the user should have
the option to configure sync before it starts.  The UX should be same as the
native ui flow.


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

[Roger Tawa OOO till Jul 10th, 2012-11-28 01:49:53]

Hi Tim,

This CL makes is that signing in from the settings page using the gaia web-flow
behave like it does today with the native flow.  That is, after sign in, you get
a small dialog asking if you want to do advanced config or not before actual
sign in.

The code in sync_setup_handler is pretty complex though.  I think I got all the
cases, but I'm still not sure of all the subtleties in there.

Thanks.

[tim (not reviewing), 2012-11-29 06:39:07]

Hey Roger, I'm not sure I'm the best person to review this code given the time
constraints, complexity, and my partial OOO-ness...  I'm not sure if Drew might
be able to pick this up?  I'll do my best otherwise!

https://chromiumcodereview.appspot.com/11418200/diff/10001/chrome/browser/ui/...
File chrome/browser/ui/webui/sync_setup_handler.cc (right):

https://chromiumcodereview.appspot.com/11418200/diff/10001/chrome/browser/ui/...
chrome/browser/ui/webui/sync_setup_handler.cc:505: // Make sure the tab used for
the Gaia sign in does not cover this tab.
They're separate tabs?

https://chromiumcodereview.appspot.com/11418200/diff/10001/chrome/browser/ui/...
chrome/browser/ui/webui/sync_setup_handler.cc:695:
DCHECK(active_sync_setup_handler_ == NULL);
DCHECK_EQ, here and elsewhere

https://chromiumcodereview.appspot.com/11418200/diff/10001/chrome/browser/ui/...
chrome/browser/ui/webui/sync_setup_handler.cc:1159:
BringTabToFront(active_sync_setup_handler_->active_gaia_signin_tab_);
There can be multiple active_sync_setup_handler_'s?  Why wouldn't this be
this->active_gaia_signin_tab_?

[Roger Tawa OOO till Jul 10th, 2012-11-29 20:20:56]

Thanks Tim.  Comments addressed, changes uploaded.  I'll make sure Drew takes a
look.

https://codereview.chromium.org/11418200/diff/10001/chrome/browser/ui/webui/s...
File chrome/browser/ui/webui/sync_setup_handler.cc (right):

https://codereview.chromium.org/11418200/diff/10001/chrome/browser/ui/webui/s...
chrome/browser/ui/webui/sync_setup_handler.cc:505: // Make sure the tab used for
the Gaia sign in does not cover this tab.
On 2012/11/29 06:39:07, timsteele wrote:
> They're separate tabs? 

yes

https://codereview.chromium.org/11418200/diff/10001/chrome/browser/ui/webui/s...
chrome/browser/ui/webui/sync_setup_handler.cc:695:
DCHECK(active_sync_setup_handler_ == NULL);
On 2012/11/29 06:39:07, timsteele wrote:
> DCHECK_EQ, here and elsewhere

Done.

https://codereview.chromium.org/11418200/diff/10001/chrome/browser/ui/webui/s...
chrome/browser/ui/webui/sync_setup_handler.cc:1159:
BringTabToFront(active_sync_setup_handler_->active_gaia_signin_tab_);
On 2012/11/29 06:39:07, timsteele wrote:
> There can be multiple active_sync_setup_handler_'s?  Why wouldn't this be
> this->active_gaia_signin_tab_?

No, there can be only one.  However, if you open a second settings page, you can
still click the "sign in to chrome" button on that one.  So
this->active_gaia_signin_tab_ may be null even though there is an active sign in
ui.

This is actually exactly the same as before.  You could have one settings page
with a popup sign in window, and press sign in on another setting pages.  It
brings focus to the first.  See IsActiveLogin() for more details.

[Roger Tawa OOO till Jul 10th, 2012-11-29 20:21:32]

Hi Drew,

Can you take a look at this CL please?  Thanks.

[tim (not reviewing), 2012-11-30 01:14:44]

Couple more questions to help me understand, but so far I think this look
right...

https://chromiumcodereview.appspot.com/11418200/diff/1013/chrome/browser/ui/w...
File chrome/browser/ui/webui/sync_setup_handler.cc (right):

https://chromiumcodereview.appspot.com/11418200/diff/1013/chrome/browser/ui/w...
chrome/browser/ui/webui/sync_setup_handler.cc:196: Browser* browser =
browser::FindBrowserWithWebContents(web_contents);
Are all of the calls in here guaranteed not to fail / return null / invalid
indicies?

https://chromiumcodereview.appspot.com/11418200/diff/1013/chrome/browser/ui/w...
chrome/browser/ui/webui/sync_setup_handler.cc:591:
DCHECK(!active_gaia_signin_tab_);
So there's no way for the user to say, open a new settings tab and try this
again (while another existing gaia sign tab is open)?

[Roger Tawa OOO till Jul 10th, 2012-11-30 01:37:14]

Thanks Tim.  Comments addressed, changes uploaded.

https://codereview.chromium.org/11418200/diff/1013/chrome/browser/ui/webui/sy...
File chrome/browser/ui/webui/sync_setup_handler.cc (right):

https://codereview.chromium.org/11418200/diff/1013/chrome/browser/ui/webui/sy...
chrome/browser/ui/webui/sync_setup_handler.cc:196: Browser* browser =
browser::FindBrowserWithWebContents(web_contents);
On 2012/11/30 01:14:44, timsteele wrote:
> Are all of the calls in here guaranteed not to fail / return null / invalid
> indicies?

Good point.  Made the code more robust.  Not sure what to do in case of failurer
though.  What do you think?

https://codereview.chromium.org/11418200/diff/1013/chrome/browser/ui/webui/sy...
chrome/browser/ui/webui/sync_setup_handler.cc:591:
DCHECK(!active_gaia_signin_tab_);
On 2012/11/30 01:14:44, timsteele wrote:
> So there's no way for the user to say, open a new settings tab and try this
> again (while another existing gaia sign tab is open)?

Yes, a user can do that.  For example, they can open a new tab or a new window
and  type "chrome://settings".  If they then click on "Sign into chrome" in this
new tab, the code makes sure to focus the existing sign in page, and not create
a new one.

This is done a level above this function though, in OpenSyncSetup().  You'll
notice that in this scenario, PrepareSyncSetup() will return false, so
DisplayGaiaLogin() will not be called.

[Andrew T Wilson (Slow), 2012-11-30 16:40:11]

https://codereview.chromium.org/11418200/diff/3011/chrome/browser/ui/sync/one...
File chrome/browser/ui/sync/one_click_signin_sync_starter.h (right):

https://codereview.chromium.org/11418200/diff/3011/chrome/browser/ui/sync/one...
chrome/browser/ui/sync/one_click_signin_sync_starter.h:19: enum StartSyncMode
{SYNC_WITH_DEFAULT_SETTINGS, CONFIGURE_SYNC_FIRST,
I think we ought to put this enum with one value per line and some documentation
of the settings, now that there are 3 items.

https://codereview.chromium.org/11418200/diff/3011/chrome/browser/ui/webui/sy...
File chrome/browser/ui/webui/sync_setup_handler.cc (right):

https://codereview.chromium.org/11418200/diff/3011/chrome/browser/ui/webui/sy...
chrome/browser/ui/webui/sync_setup_handler.cc:195: void
BringTabToFront(WebContents* web_contents) {
We already have FocusUI() - does that not do what we need here?

https://codereview.chromium.org/11418200/diff/3011/chrome/browser/ui/webui/sy...
File chrome/browser/ui/webui/sync_setup_handler.h (right):

https://codereview.chromium.org/11418200/diff/3011/chrome/browser/ui/webui/sy...
chrome/browser/ui/webui/sync_setup_handler.h:177: // When using web-flow, closes
the Gaia used to collection user credentials.
nit: "closes the Gaia"...page?

https://codereview.chromium.org/11418200/diff/3011/chrome/browser/ui/webui/sy...
chrome/browser/ui/webui/sync_setup_handler.h:222: static SyncSetupHandler*
active_sync_setup_handler_;
Hmmm. This makes me nervous. Should we be using LoginUIService here? It seems
odd to have LoginUIService track this, and also track it ourselves.

https://codereview.chromium.org/11418200/diff/10028/chrome/browser/ui/webui/s...
File chrome/browser/ui/webui/sync_setup_handler.cc (right):

https://codereview.chromium.org/11418200/diff/10028/chrome/browser/ui/webui/s...
chrome/browser/ui/webui/sync_setup_handler.cc:196: Browser* browser =
browser::FindBrowserWithWebContents(web_contents);
We have FocusUI() to do the same thing - make this the visible tab. Do we need
this routine? Maybe we should combine the two routines if this routine does
something more?

https://codereview.chromium.org/11418200/diff/10028/chrome/browser/ui/webui/s...
chrome/browser/ui/webui/sync_setup_handler.cc:538: return
active_sync_setup_handler_ == this;
I'm not sure I understand why we need different logic in the WebBasedSigninFlow
case. Shouldn't we still be using LoginUIService() and ensuring that there's
only one SyncSetupHandler active in the system?

https://codereview.chromium.org/11418200/diff/10028/chrome/browser/ui/webui/s...
chrome/browser/ui/webui/sync_setup_handler.cc:1056: CloseGaiaSigninPage();
Should we not also call LoginUIClosed() here?

https://codereview.chromium.org/11418200/diff/10028/chrome/browser/ui/webui/s...
chrome/browser/ui/webui/sync_setup_handler.cc:1150: active_gaia_signin_tab_ =
NULL;
Should we call CloseSyncSetup here? Seems like we want to track this as if it
were CANCEL_DURING_SIGNON.

[Roger Tawa OOO till Jul 10th, 2012-12-01 16:09:54]

Thanks Drew.  All comments addressed, changes uploaded.  I had misunderstood the
LoginUI interface, but now that I get it, the code is much cleaner as you
suggested.

https://codereview.chromium.org/11418200/diff/3011/chrome/browser/ui/sync/one...
File chrome/browser/ui/sync/one_click_signin_sync_starter.h (right):

https://codereview.chromium.org/11418200/diff/3011/chrome/browser/ui/sync/one...
chrome/browser/ui/sync/one_click_signin_sync_starter.h:19: enum StartSyncMode
{SYNC_WITH_DEFAULT_SETTINGS, CONFIGURE_SYNC_FIRST,
On 2012/11/30 16:40:12, Andrew T Wilson wrote:
> I think we ought to put this enum with one value per line and some
documentation
> of the settings, now that there are 3 items.

Done.

https://codereview.chromium.org/11418200/diff/3011/chrome/browser/ui/webui/sy...
File chrome/browser/ui/webui/sync_setup_handler.cc (right):

https://codereview.chromium.org/11418200/diff/3011/chrome/browser/ui/webui/sy...
chrome/browser/ui/webui/sync_setup_handler.cc:195: void
BringTabToFront(WebContents* web_contents) {
On 2012/11/30 16:40:12, Andrew T Wilson wrote:
> We already have FocusUI() - does that not do what we need here?

This is now called from FocusUI.

https://codereview.chromium.org/11418200/diff/3011/chrome/browser/ui/webui/sy...
File chrome/browser/ui/webui/sync_setup_handler.h (right):

https://codereview.chromium.org/11418200/diff/3011/chrome/browser/ui/webui/sy...
chrome/browser/ui/webui/sync_setup_handler.h:177: // When using web-flow, closes
the Gaia used to collection user credentials.
On 2012/11/30 16:40:12, Andrew T Wilson wrote:
> nit: "closes the Gaia"...page?

Done.

https://codereview.chromium.org/11418200/diff/3011/chrome/browser/ui/webui/sy...
chrome/browser/ui/webui/sync_setup_handler.h:222: static SyncSetupHandler*
active_sync_setup_handler_;
On 2012/11/30 16:40:12, Andrew T Wilson wrote:
> Hmmm. This makes me nervous. Should we be using LoginUIService here? It seems
> odd to have LoginUIService track this, and also track it ourselves.

Removed.

https://codereview.chromium.org/11418200/diff/10028/chrome/browser/ui/webui/s...
File chrome/browser/ui/webui/sync_setup_handler.cc (right):

https://codereview.chromium.org/11418200/diff/10028/chrome/browser/ui/webui/s...
chrome/browser/ui/webui/sync_setup_handler.cc:196: Browser* browser =
browser::FindBrowserWithWebContents(web_contents);
On 2012/11/30 16:40:12, Andrew T Wilson wrote:
> We have FocusUI() to do the same thing - make this the visible tab. Do we need
> this routine? Maybe we should combine the two routines if this routine does
> something more?

Done.

https://codereview.chromium.org/11418200/diff/10028/chrome/browser/ui/webui/s...
chrome/browser/ui/webui/sync_setup_handler.cc:538: return
active_sync_setup_handler_ == this;
On 2012/11/30 16:40:12, Andrew T Wilson wrote:
> I'm not sure I understand why we need different logic in the
WebBasedSigninFlow
> case. Shouldn't we still be using LoginUIService() and ensuring that there's
> only one SyncSetupHandler active in the system?

Done.

https://codereview.chromium.org/11418200/diff/10028/chrome/browser/ui/webui/s...
chrome/browser/ui/webui/sync_setup_handler.cc:1056: CloseGaiaSigninPage();
On 2012/11/30 16:40:12, Andrew T Wilson wrote:
> Should we not also call LoginUIClosed() here?

Done.

https://codereview.chromium.org/11418200/diff/10028/chrome/browser/ui/webui/s...
chrome/browser/ui/webui/sync_setup_handler.cc:1150: active_gaia_signin_tab_ =
NULL;
On 2012/11/30 16:40:12, Andrew T Wilson wrote:
> Should we call CloseSyncSetup here? Seems like we want to track this as if it
> were CANCEL_DURING_SIGNON.

Done.

[Andrew T Wilson (Slow), 2012-12-03 13:19:02]

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

https://codereview.chromium.org/11418200/diff/9030/chrome/browser/ui/sync/one...
chrome/browser/ui/sync/one_click_signin_helper.cc:114:
switches::kUseWebBasedSigninFlow);
Why do we have a separate variable here?

https://codereview.chromium.org/11418200/diff/9030/chrome/browser/ui/sync/one...
File chrome/browser/ui/sync/one_click_signin_helper_unittest.cc (right):

https://codereview.chromium.org/11418200/diff/9030/chrome/browser/ui/sync/one...
chrome/browser/ui/sync/one_click_signin_helper_unittest.cc:54:
switches::kUseWebBasedSigninFlow);
Why do we have a separate variable? Also, should be "signin" not "singin"
although forcing the user to sing to log in would be amusing :)

https://codereview.chromium.org/11418200/diff/9030/chrome/browser/ui/sync/one...
File chrome/browser/ui/sync/one_click_signin_sync_starter.h (right):

https://codereview.chromium.org/11418200/diff/9030/chrome/browser/ui/sync/one...
chrome/browser/ui/sync/one_click_signin_sync_starter.h:31: // the web-based sign
in flow directly from the settings page.  This mode
I guess SyncSetupHandler uses SigninTracker to determine when signin is done and
continues the login by itself?

What internal state gets messed up? It would obviously be nice to combine this
with CONFIGURE_SYNC_FIRST.

https://codereview.chromium.org/11418200/diff/9030/chrome/browser/ui/sync/one...
chrome/browser/ui/sync/one_click_signin_sync_starter.h:34: SIGNIN_ONLY};
nit: close paren on its own line.

https://codereview.chromium.org/11418200/diff/9030/chrome/browser/ui/webui/sy...
File chrome/browser/ui/webui/sync_setup_handler.cc (right):

https://codereview.chromium.org/11418200/diff/9030/chrome/browser/ui/webui/sy...
chrome/browser/ui/webui/sync_setup_handler.cc:600: active_gaia_signin_tab_ =
browser->OpenURL(
So, who closes this GAIA tab when signin is complete? It looks like we keep it
around until sync config is finished (at which point CloseSyncSetup() is called?
Shouldn't we close it once we get SigninSuccess?

https://codereview.chromium.org/11418200/diff/9030/chrome/browser/ui/webui/sy...
chrome/browser/ui/webui/sync_setup_handler.cc:1130: if (UseWebBasedSigninFlow()
&& !configuring_sync_) {
So previously, configuring_sync_ was just used to log a UMA event (i.e. if the
user cancels out of signin, it lets us log CANCEL_DURING_CONFIGURE instead of
CANCEL_FROM_SIGNON_WITHOUT_AUTH).

I've been reticent to start using it for something else. Perhaps you could gate
this on signin_tracker_ != NULL instead?

[Roger Tawa OOO till Jul 10th, 2012-12-03 19:08:28]

Thank Drew.  Comments addressed, changes uploaded.  PTAL.

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

https://codereview.chromium.org/11418200/diff/9030/chrome/browser/ui/sync/one...
chrome/browser/ui/sync/one_click_signin_helper.cc:114:
switches::kUseWebBasedSigninFlow);
On 2012/12/03 13:19:02, Andrew T Wilson wrote:
> Why do we have a separate variable here?

No need.  Removed.

https://codereview.chromium.org/11418200/diff/9030/chrome/browser/ui/sync/one...
File chrome/browser/ui/sync/one_click_signin_helper_unittest.cc (right):

https://codereview.chromium.org/11418200/diff/9030/chrome/browser/ui/sync/one...
chrome/browser/ui/sync/one_click_signin_helper_unittest.cc:54:
switches::kUseWebBasedSigninFlow);
On 2012/12/03 13:19:02, Andrew T Wilson wrote:
> Why do we have a separate variable? Also, should be "signin" not "singin"
> although forcing the user to sing to log in would be amusing :)

No need.  Too bad we're now loosing the singin'

https://codereview.chromium.org/11418200/diff/9030/chrome/browser/ui/sync/one...
File chrome/browser/ui/sync/one_click_signin_sync_starter.h (right):

https://codereview.chromium.org/11418200/diff/9030/chrome/browser/ui/sync/one...
chrome/browser/ui/sync/one_click_signin_sync_starter.h:31: // the web-based sign
in flow directly from the settings page.  This mode
On 2012/12/03 13:19:02, Andrew T Wilson wrote:
> I guess SyncSetupHandler uses SigninTracker to determine when signin is done
and
> continues the login by itself?
> 
> What internal state gets messed up? It would obviously be nice to combine this
> with CONFIGURE_SYNC_FIRST.

I tried it again and the state is OK. I suspect when I cleaned the handling of
the LoginUI calls, I also fixed this up.  Thanks for persisting on this :-)

https://codereview.chromium.org/11418200/diff/9030/chrome/browser/ui/sync/one...
chrome/browser/ui/sync/one_click_signin_sync_starter.h:34: SIGNIN_ONLY};
On 2012/12/03 13:19:02, Andrew T Wilson wrote:
> nit: close paren on its own line.

Done.

https://codereview.chromium.org/11418200/diff/9030/chrome/browser/ui/webui/sy...
File chrome/browser/ui/webui/sync_setup_handler.cc (right):

https://codereview.chromium.org/11418200/diff/9030/chrome/browser/ui/webui/sy...
chrome/browser/ui/webui/sync_setup_handler.cc:600: active_gaia_signin_tab_ =
browser->OpenURL(
On 2012/12/03 13:19:02, Andrew T Wilson wrote:
> So, who closes this GAIA tab when signin is complete? It looks like we keep it
> around until sync config is finished (at which point CloseSyncSetup() is
called?
> Shouldn't we close it once we get SigninSuccess?

Right its closed by CloseSyncSetup().  It always gets closed on all cases,
success or failure.  No problems as is.

https://codereview.chromium.org/11418200/diff/9030/chrome/browser/ui/webui/sy...
chrome/browser/ui/webui/sync_setup_handler.cc:1130: if (UseWebBasedSigninFlow()
&& !configuring_sync_) {
On 2012/12/03 13:19:02, Andrew T Wilson wrote:
> So previously, configuring_sync_ was just used to log a UMA event (i.e. if the
> user cancels out of signin, it lets us log CANCEL_DURING_CONFIGURE instead of
> CANCEL_FROM_SIGNON_WITHOUT_AUTH).
> 
> I've been reticent to start using it for something else. Perhaps you could
gate
> this on signin_tracker_ != NULL instead?

Done.

[tim (not reviewing), 2012-12-03 20:53:52]

Having looked this over with atwilson's comments / guidance, I'm comfortable
enough playing Drew-regent for a day and give a sync LGTM (since he's likely
asleep now), so you can land https://chromiumcodereview.appspot.com/11316299/
which he already gave a green light.  He can of course follow up tomorrow!

[Roger Tawa OOO till Jul 10th, 2012-12-03 21:24:00]

Hi James,

Can you do an owner review for the webui changes?  Thanks.

[James Hawkins, 2012-12-04 00:14:48]

https://codereview.chromium.org/11418200/diff/11017/chrome/browser/ui/webui/s...
File chrome/browser/ui/webui/sync_setup_handler.cc (left):

https://codereview.chromium.org/11418200/diff/11017/chrome/browser/ui/webui/s...
chrome/browser/ui/webui/sync_setup_handler.cc:517: // (i.e. if the user is
running in guest mode in cros and brings up settings).
Did this not fine on one line?

https://codereview.chromium.org/11418200/diff/11017/chrome/browser/ui/webui/s...
File chrome/browser/ui/webui/sync_setup_handler.cc (right):

https://codereview.chromium.org/11418200/diff/11017/chrome/browser/ui/webui/s...
chrome/browser/ui/webui/sync_setup_handler.cc:198: TabStripModel*
tab_strip_model = browser->tab_strip_model();
Optional nit: Save indentation by reversing the logic and returning early.

https://codereview.chromium.org/11418200/diff/11017/chrome/browser/ui/webui/s...
chrome/browser/ui/webui/sync_setup_handler.cc:508: // Make sure the tab used for
the Gaia sign in does not cover this tab.
nit: What is 'this' tab?

https://codereview.chromium.org/11418200/diff/11017/chrome/browser/ui/webui/s...
chrome/browser/ui/webui/sync_setup_handler.cc:591: // We are no longer
configuring sync if the login screen is visible.
nit: Who is 'we' here?

https://codereview.chromium.org/11418200/diff/11017/chrome/browser/ui/webui/s...
chrome/browser/ui/webui/sync_setup_handler.cc:1128: // we are focusing to bring
the configure popup window to the top, not the
nit: This sentence can use some clarification:

if |configuring_sync_| is true, bring the tab containing the active gaia signing
to the top.

Note that it appears the comment does not match the code.  The comment talks
about |!sign in page| and the code references the |active_gaia_signin_tab_|. 
The comment also talks about |configure_sync_| which I don't see in the code.

https://codereview.chromium.org/11418200/diff/11017/chrome/browser/ui/webui/s...
File chrome/browser/ui/webui/sync_setup_handler.h (right):

https://codereview.chromium.org/11418200/diff/11017/chrome/browser/ui/webui/s...
chrome/browser/ui/webui/sync_setup_handler.h:218: // When using web-flow,
pointer to the tab that holds the Gaia sign in page.
nit: 'weak pointer' I imagine.

[Roger Tawa OOO till Jul 10th, 2012-12-04 00:36:36]

Thanks James.  Comments addressed, changes uploaded.  PTAL.

https://codereview.chromium.org/11418200/diff/11017/chrome/browser/ui/webui/s...
File chrome/browser/ui/webui/sync_setup_handler.cc (left):

https://codereview.chromium.org/11418200/diff/11017/chrome/browser/ui/webui/s...
chrome/browser/ui/webui/sync_setup_handler.cc:517: // (i.e. if the user is
running in guest mode in cros and brings up settings).
On 2012/12/04 00:14:48, James Hawkins wrote:
> Did this not fine on one line?

Done.

https://codereview.chromium.org/11418200/diff/11017/chrome/browser/ui/webui/s...
File chrome/browser/ui/webui/sync_setup_handler.cc (right):

https://codereview.chromium.org/11418200/diff/11017/chrome/browser/ui/webui/s...
chrome/browser/ui/webui/sync_setup_handler.cc:198: TabStripModel*
tab_strip_model = browser->tab_strip_model();
On 2012/12/04 00:14:48, James Hawkins wrote:
> Optional nit: Save indentation by reversing the logic and returning early.

I think I'll keep it as is.

https://codereview.chromium.org/11418200/diff/11017/chrome/browser/ui/webui/s...
chrome/browser/ui/webui/sync_setup_handler.cc:508: // Make sure the tab used for
the Gaia sign in does not cover this tab.
On 2012/12/04 00:14:48, James Hawkins wrote:
> nit: What is 'this' tab?

Done.

https://codereview.chromium.org/11418200/diff/11017/chrome/browser/ui/webui/s...
chrome/browser/ui/webui/sync_setup_handler.cc:591: // We are no longer
configuring sync if the login screen is visible.
On 2012/12/04 00:14:48, James Hawkins wrote:
> nit: Who is 'we' here?

Done.

https://codereview.chromium.org/11418200/diff/11017/chrome/browser/ui/webui/s...
chrome/browser/ui/webui/sync_setup_handler.cc:1128: // we are focusing to bring
the configure popup window to the top, not the
On 2012/12/04 00:14:48, James Hawkins wrote:
> nit: This sentence can use some clarification:
> 
> if |configuring_sync_| is true, bring the tab containing the active gaia
signing
> to the top.
> 
> Note that it appears the comment does not match the code.  The comment talks
> about |!sign in page| and the code references the |active_gaia_signin_tab_|. 
> The comment also talks about |configure_sync_| which I don't see in the code.

Good catch.  Done.

https://codereview.chromium.org/11418200/diff/11017/chrome/browser/ui/webui/s...
File chrome/browser/ui/webui/sync_setup_handler.h (right):

https://codereview.chromium.org/11418200/diff/11017/chrome/browser/ui/webui/s...
chrome/browser/ui/webui/sync_setup_handler.h:218: // When using web-flow,
pointer to the tab that holds the Gaia sign in page.
On 2012/12/04 00:14:48, James Hawkins wrote:
> nit: 'weak pointer' I imagine.

Done.

[James Hawkins, 2012-12-04 01:02:24]

LGTM w/ optional nit.

https://codereview.chromium.org/11418200/diff/14041/chrome/browser/ui/webui/s...
File chrome/browser/ui/webui/sync_setup_handler.cc (right):

https://codereview.chromium.org/11418200/diff/14041/chrome/browser/ui/webui/s...
chrome/browser/ui/webui/sync_setup_handler.cc:1127: // if |signin_tracker_| is
valid and web-based sign flow is enabled, bring
Optional nit: I don't think this comment adds much value since it's almost
verbatim what the code says.

[Roger Tawa OOO till Jul 10th, 2012-12-04 01:10:14]

Thanks James.

https://codereview.chromium.org/11418200/diff/14041/chrome/browser/ui/webui/s...
File chrome/browser/ui/webui/sync_setup_handler.cc (right):

https://codereview.chromium.org/11418200/diff/14041/chrome/browser/ui/webui/s...
chrome/browser/ui/webui/sync_setup_handler.cc:1127: // if |signin_tracker_| is
valid and web-based sign flow is enabled, bring
On 2012/12/04 01:02:24, James Hawkins wrote:
> Optional nit: I don't think this comment adds much value since it's almost
> verbatim what the code says.

True, done.

[commit-bot: I haz the power, 2012-12-04 01:12:06]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rogerta@chromium.org/11418200/1035

[commit-bot: I haz the power, 2012-12-04 05:07:52]

Retried try job too often on mac_rel for step(s) browser_tests

[Andrew T Wilson (Slow), 2012-12-04 11:08:24]

LGTM, with one comment - I suspect CloseSyncSetup() may be a little late to
close the GAIA tab. Wouldn't we want to close it as soon as we've completed
credential entry (i.e. in DisplayGaiaSuccessAndSettingUp)?

[Roger Tawa OOO till Jul 10th, 2012-12-04 15:11:06]

Hi Drew,

Moved closing of gaia tab earlier as you suggested.  Thanks.

[commit-bot: I haz the power, 2012-12-04 15:11:23]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rogerta@chromium.org/11418200/1052

[commit-bot: I haz the power, 2012-12-04 17:59:53]

Change committed as 170999