Handle refresh on sync setup page.

Handle refresh on sync setup page.

BUG=138338, 141682
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=151405

[Evan Stade, 2012-08-11 00:36:12]

http://codereview.chromium.org/10831273/diff/2001/chrome/browser/resources/sy...
File chrome/browser/resources/sync_promo/sync_promo.js (left):

http://codereview.chromium.org/10831273/diff/2001/chrome/browser/resources/sy...
chrome/browser/resources/sync_promo/sync_promo.js:65: this.showSetupUI_();
this is redundant with the Initialize right below.

http://codereview.chromium.org/10831273/diff/2001/chrome/browser/ui/webui/syn...
File chrome/browser/ui/webui/sync_promo/sync_promo_handler.cc (left):

http://codereview.chromium.org/10831273/diff/2001/chrome/browser/ui/webui/syn...
chrome/browser/ui/webui/sync_promo/sync_promo_handler.cc:214:
GetLoginUIService()->LoginUIClosed(this);
the code was calling ShowSetupUI (which called OpenSyncSetup), then Initialize
(this function). Therefore we called: OpenSyncSetup, LoginUIClosed,
OpenSyncSetup. This broke the identity api (the only subscriber to
OnLoginUIClosed).

[Andrew T Wilson (Slow), 2012-08-11 01:10:33]

http://codereview.chromium.org/10831273/diff/2001/chrome/browser/ui/webui/syn...
File chrome/browser/ui/webui/sync_promo/sync_promo_handler.cc (right):

http://codereview.chromium.org/10831273/diff/2001/chrome/browser/ui/webui/syn...
chrome/browser/ui/webui/sync_promo/sync_promo_handler.cc:215:
DisplayGaiaLogin(false);
Hmmm. Is this the correct thing to do here? I'm concerned about the case where:

1) The user has a custom passphrase set for his acct.
2) He logs in to the promo
3) He is presented with the "advanced" sync setup page to enter his custom
passphrase
4) He hits reload.

Not sure what happens in this case if you call DisplayGaiaLogin(false). The user
is in a signed-in state so displaying a login seems like the wrong thing (note
the code in SyncSetupHandler::OpenSyncSetup() that displays different UI
depending on the current state of sync - you are bypassing this code by calling
DisplayGaiaLogin() directly).

It feels like the right thing to do is always call OpenSyncSetup(true) as the
code was doing before - I don't quite understand what the fail case is.

[Andrew T Wilson (Slow), 2012-08-11 01:27:09]

Hmmm, interesting. I see now that OpenSyncSetup(true) always just calls
DisplayGaiaLogin(false) (since force_login == true). I still think this is the
wrong thing to do if we're displaying sync configure UI, but that's an existing
issue, not new from this patch.

So, when we hit reload, I am expecting that SyncSetupHandler::OnDidClosePage()
would be called, which would then call LoginUIClosed(), long before
SyncPromoHandler::HandleInitializeSyncPromo() is called. I'm guessing that on a
reload, we don't get this call? That seems like the root cause of all of these
issues, and is officially a Bad Thing because it means we are in the middle of
some signin activity and aren't properly cleaning up.

I would say that the right fix to 138338 is to make sure that OnDidClosePage()
gets called when the user hits reload in the sync settings/sync promo page.

[Evan Stade, 2012-08-11 01:28:55]

The failure is that the page is blank on reload. I tried to fix this with
r150244 and it seemed to work, but it broke the identity api because of
extraneous loginuiclosed calls.

http://codereview.chromium.org/10831273/diff/2001/chrome/browser/ui/webui/syn...
File chrome/browser/ui/webui/sync_promo/sync_promo_handler.cc (right):

http://codereview.chromium.org/10831273/diff/2001/chrome/browser/ui/webui/syn...
chrome/browser/ui/webui/sync_promo/sync_promo_handler.cc:215:
DisplayGaiaLogin(false);
On 2012/08/11 01:10:33, Andrew T Wilson wrote:
> Hmmm. Is this the correct thing to do here? I'm concerned about the case
where:
> 
> 1) The user has a custom passphrase set for his acct.
> 2) He logs in to the promo
> 3) He is presented with the "advanced" sync setup page to enter his custom
> passphrase
> 4) He hits reload.
> 
> Not sure what happens in this case if you call DisplayGaiaLogin(false). The
user
> is in a signed-in state so displaying a login seems like the wrong thing (note
> the code in SyncSetupHandler::OpenSyncSetup() that displays different UI
> depending on the current state of sync - you are bypassing this code by
calling
> DisplayGaiaLogin() directly).

that code always displays the gaia login when force_login is true, as it is
below. The difference is that OpenSyncSetup frequently does nothing (it only
works the first time you call it).

I just tested the scenario you suggested by adding a custom passphrase to my
test account, signing out and trying to sign back in with chrome://signin. When
I reloaded on the advanced page, it dumped me back to the login page with my
username filled in but the password field blank. This seems reasonable and it is
the same thing that happens when I sign in via chrome://settings, but reload
before configuring.

> 
> It feels like the right thing to do is always call OpenSyncSetup(true) as the
> code was doing before - I don't quite understand what the fail case is.

When you reload, OpenSyncSetup(true) does nothing because PrepareSyncSetup
returns false.

[Evan Stade, 2012-08-11 01:37:45]

On 2012/08/11 01:27:09, Andrew T Wilson wrote:
> Hmmm, interesting. I see now that OpenSyncSetup(true) always just calls
> DisplayGaiaLogin(false) (since force_login == true). I still think this is the
> wrong thing to do if we're displaying sync configure UI, but that's an
existing
> issue, not new from this patch.
> 
> So, when we hit reload, I am expecting that SyncSetupHandler::OnDidClosePage()
> would be called, which would then call LoginUIClosed(), long before
> SyncPromoHandler::HandleInitializeSyncPromo() is called. I'm guessing that on
a
> reload, we don't get this call? That seems like the root cause of all of these
> issues, and is officially a Bad Thing because it means we are in the middle of
> some signin activity and aren't properly cleaning up.

the reason I didn't want to do that is because the page hasn't really been
closed, it's just sort of started over. Imagine when an app is prompting a user
to identify --- if the user reloads the signin page then the app will be
notified that signin failed, even though the user may go on to successfully sign
in.

The destructor also calls the clean up function.

> 
> I would say that the right fix to 138338 is to make sure that OnDidClosePage()
> gets called when the user hits reload in the sync settings/sync promo page.

[Andrew T Wilson (Slow), 2012-08-11 01:56:42]

It just seems like you're jumping through hoops to avoid the call to
PrepareSyncSetup() in OpenSyncSetup() to get around this case, which is going to
make this (already hard to maintain) code even harder to maintain - any new
logic people add to OpenSyncSetup() will have to be duplicated in
SyncPromoHandler::HandleInitializeSyncPromo().

I think it's OK to treat a reload like a "close and reopen the sync UI" and have
it act like a cancellation from the point of view of the identity API. It's kind
of a weird case and if it results in a hiccup in the identity flow, so be it
(it's basically no different than if the user accidentally cancels the signin,
then pulls down the wrench menu and clicks "Sign in to Chrome" again).

IMO, if we're doing something that's blowing away the sync UI overlay (like
reloading the page), then the SyncSetupHandler should be notified via the
DidClosePage() handler. If this has undesirable effects downstream in the
identity API, then we should address those in other ways (maybe via some kind of
debouncing in the identity API's LoginUIService observer) rather than changing
the invariants in SyncSetupHandler.

[Evan Stade, 2012-08-11 02:32:11]

ok, done.

[Andrew T Wilson (Slow), 2012-08-13 17:00:26]

On 2012/08/11 02:32:11, Evan Stade wrote:
> ok, done.

LGTM. Thanks.

[Dan Beam, 2012-08-13 19:16:29]

lgtm w/talk about event handlers