Start moving signin code out of browser/sync.

Start moving signin code out of browser/sync.

This starts to break the dependency between sync and the signin UI.

BUG=111859
TEST=run ALL the login test cases!


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=122030

[Andrew T Wilson (Slow), 2012-02-10 01:53:26]

OK, this is the biggest part of the refactoring - hopefully moving the rest of
the sync config code out of browser/sync will be much simpler. Sorry about the
size of this patch, but there really wasn't any good way to only pull *part* of
the login code out of SyncSetupFlow.

James, can you take a look at the browser/ui stuff, and Tim, the browser/sync
stuff?

I'm working on some test coverage in a separate CL now, but I wanted to get this
out for review ASAP.

[Andrew T Wilson (Slow), 2012-02-10 20:52:31]

FYI, Yaron/Nyquist - I'm happy to help merge this to Chrome Mobile.

[James Hawkins, 2012-02-10 23:08:01]

https://chromiumcodereview.appspot.com/9295044/diff/20004/chrome/browser/ui/w...
File chrome/browser/ui/webui/signin/login_ui_service.cc (right):

https://chromiumcodereview.appspot.com/9295044/diff/20004/chrome/browser/ui/w...
chrome/browser/ui/webui/signin/login_ui_service.cc:30: if (current_login_ui() ==
ui)
What are the use cases for allowing |ui| to not match |current_login_ui()|?

https://chromiumcodereview.appspot.com/9295044/diff/20004/chrome/browser/ui/w...
File chrome/browser/ui/webui/signin/login_ui_service.h (right):

https://chromiumcodereview.appspot.com/9295044/diff/20004/chrome/browser/ui/w...
chrome/browser/ui/webui/signin/login_ui_service.h:21: explicit
LoginUIService(Profile* profile);
Document |profile|.

https://chromiumcodereview.appspot.com/9295044/diff/20004/chrome/browser/ui/w...
chrome/browser/ui/webui/signin/login_ui_service.h:35: void
ClearLoginUI(content::WebUI* ui);
s/Clear/Reset/ may be clearer given that we're referring to UI, and clear could
be misconstrued as a visual command.

https://chromiumcodereview.appspot.com/9295044/diff/20004/chrome/browser/ui/w...
chrome/browser/ui/webui/signin/login_ui_service.h:46: content::WebUI* ui_;
Document ownership (likely weak ptr).

https://chromiumcodereview.appspot.com/9295044/diff/20004/chrome/browser/ui/w...
File chrome/browser/ui/webui/signin/signin_tracker.cc (right):

https://chromiumcodereview.appspot.com/9295044/diff/20004/chrome/browser/ui/w...
chrome/browser/ui/webui/signin/signin_tracker.cc:46: return;
nit: Don't handle DCHECKs (NOTREACHED in this case) by returning.

https://chromiumcodereview.appspot.com/9295044/diff/20004/chrome/browser/ui/w...
File chrome/browser/ui/webui/signin/signin_tracker.h (right):

https://chromiumcodereview.appspot.com/9295044/diff/20004/chrome/browser/ui/w...
chrome/browser/ui/webui/signin/signin_tracker.h:35: // |profile|, and notifies
the |observer| on status changes.
|observer| must not be NULL.

https://chromiumcodereview.appspot.com/9295044/diff/20004/chrome/browser/ui/w...
chrome/browser/ui/webui/signin/signin_tracker.h:52: 
nit: Remove extra blank line.

https://chromiumcodereview.appspot.com/9295044/diff/20004/chrome/browser/ui/w...
chrome/browser/ui/webui/signin/signin_tracker.h:73:
content::NotificationRegistrar registrar_;
DISALLOW_COPY_AND_ASSIGN?

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

https://chromiumcodereview.appspot.com/9295044/diff/20004/chrome/browser/ui/w...
chrome/browser/ui/webui/sync_setup_handler.cc:359:
OptionsPageUIHandler::Initialize();
What is this part of the change about?

https://chromiumcodereview.appspot.com/9295044/diff/20004/chrome/browser/ui/w...
File chrome/browser/ui/webui/sync_setup_handler.h (right):

https://chromiumcodereview.appspot.com/9295044/diff/20004/chrome/browser/ui/w...
chrome/browser/ui/webui/sync_setup_handler.h:152: scoped_ptr<SigninTracker>
signin_tracker_;
Document var.

[Andrew T Wilson (Slow), 2012-02-11 01:50:45]

Updated per James' review feedback.

https://chromiumcodereview.appspot.com/9295044/diff/20004/chrome/browser/ui/w...
File chrome/browser/ui/webui/signin/login_ui_service.cc (right):

https://chromiumcodereview.appspot.com/9295044/diff/20004/chrome/browser/ui/w...
chrome/browser/ui/webui/signin/login_ui_service.cc:30: if (current_login_ui() ==
ui)
On 2012/02/10 23:08:01, James Hawkins wrote:
> What are the use cases for allowing |ui| to not match |current_login_ui()|?

The use cases are that you might have multiple sync setup handlers around (say,
a promo *and* a settings page). Or you might open settings in multiple windows,
etc. ClearLoginUI() is called defensively in some places as a catch-all (i.e. in
the destructor for SyncSetupHandler()) to make sure we don't accidentally leave
ourselves set as the current login UI (say, if the renderer crashes, or you hit
the back button, or close the tab, or whatever).

So the use case might be:

1) Bring up promo
2) Bring up settings but don't open the overlay.
3) Close settings. We call ClearLoginUI(), but we still want the promo to be
left as the current_login_ui.

https://chromiumcodereview.appspot.com/9295044/diff/20004/chrome/browser/ui/w...
File chrome/browser/ui/webui/signin/login_ui_service.h (right):

https://chromiumcodereview.appspot.com/9295044/diff/20004/chrome/browser/ui/w...
chrome/browser/ui/webui/signin/login_ui_service.h:21: explicit
LoginUIService(Profile* profile);
On 2012/02/10 23:08:01, James Hawkins wrote:
> Document |profile|.

Done.

https://chromiumcodereview.appspot.com/9295044/diff/20004/chrome/browser/ui/w...
chrome/browser/ui/webui/signin/login_ui_service.h:35: void
ClearLoginUI(content::WebUI* ui);
On 2012/02/10 23:08:01, James Hawkins wrote:
> s/Clear/Reset/ may be clearer given that we're referring to UI, and clear
could
> be misconstrued as a visual command.

Done.

https://chromiumcodereview.appspot.com/9295044/diff/20004/chrome/browser/ui/w...
chrome/browser/ui/webui/signin/login_ui_service.h:46: content::WebUI* ui_;
On 2012/02/10 23:08:01, James Hawkins wrote:
> Document ownership (likely weak ptr).

Done.

https://chromiumcodereview.appspot.com/9295044/diff/20004/chrome/browser/ui/w...
File chrome/browser/ui/webui/signin/signin_tracker.cc (right):

https://chromiumcodereview.appspot.com/9295044/diff/20004/chrome/browser/ui/w...
chrome/browser/ui/webui/signin/signin_tracker.cc:46: return;
On 2012/02/10 23:08:01, James Hawkins wrote:
> nit: Don't handle DCHECKs (NOTREACHED in this case) by returning.

Done.

https://chromiumcodereview.appspot.com/9295044/diff/20004/chrome/browser/ui/w...
File chrome/browser/ui/webui/signin/signin_tracker.h (right):

https://chromiumcodereview.appspot.com/9295044/diff/20004/chrome/browser/ui/w...
chrome/browser/ui/webui/signin/signin_tracker.h:35: // |profile|, and notifies
the |observer| on status changes.
On 2012/02/10 23:08:01, James Hawkins wrote:
> |observer| must not be NULL.

Done.

https://chromiumcodereview.appspot.com/9295044/diff/20004/chrome/browser/ui/w...
chrome/browser/ui/webui/signin/signin_tracker.h:52: 
On 2012/02/10 23:08:01, James Hawkins wrote:
> nit: Remove extra blank line.

Done.

https://chromiumcodereview.appspot.com/9295044/diff/20004/chrome/browser/ui/w...
chrome/browser/ui/webui/signin/signin_tracker.h:73:
content::NotificationRegistrar registrar_;
On 2012/02/10 23:08:01, James Hawkins wrote:
> DISALLOW_COPY_AND_ASSIGN?

Good catch. Added to LoginUIService[Factory] as well.

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

https://chromiumcodereview.appspot.com/9295044/diff/20004/chrome/browser/ui/w...
chrome/browser/ui/webui/sync_setup_handler.cc:359:
OptionsPageUIHandler::Initialize();
On 2012/02/10 23:08:01, James Hawkins wrote:
> What is this part of the change about?

Ah, I was originally registering for some notifications here after calling our
superclass, but I refactored that logic out into signin_tracker.cc. Thanks for
catching this.

https://chromiumcodereview.appspot.com/9295044/diff/20004/chrome/browser/ui/w...
File chrome/browser/ui/webui/sync_setup_handler.h (right):

https://chromiumcodereview.appspot.com/9295044/diff/20004/chrome/browser/ui/w...
chrome/browser/ui/webui/sync_setup_handler.h:152: scoped_ptr<SigninTracker>
signin_tracker_;
On 2012/02/10 23:08:01, James Hawkins wrote:
> Document var.

Done.

[Andrew T Wilson (Slow), 2012-02-13 17:54:45]

FYI Roger re: new signin refactoring.

James, any further comments?

Tim: Qui tacet consentire...

[tim (not reviewing), 2012-02-14 02:11:20]

still reviewing most of the bulk of this change, sorry for slowness.. taking me
a while to wrap my head around things!

http://codereview.chromium.org/9295044/diff/28003/chrome/browser/sync/profile...
File chrome/browser/sync/profile_sync_service.cc (right):

http://codereview.chromium.org/9295044/diff/28003/chrome/browser/sync/profile...
chrome/browser/sync/profile_sync_service.cc:734: // Got some kind of auth error
while the wizard is visible. Exit out of
err... why is this fatal?  what happens to any common GSAE like invalid gaia?

http://codereview.chromium.org/9295044/diff/28003/chrome/browser/sync/profile...
chrome/browser/sync/profile_sync_service.cc:969: // itself and should not result
in a call to ShowErrorUI.
hm. so what about a failure to initialize / load the database in sync_manager?

http://codereview.chromium.org/9295044/diff/28003/chrome/browser/sync/profile...
chrome/browser/sync/profile_sync_service.cc:1441: // TODO(sync): Remove this
when sync is no longer tied to browser signin.
should tie this to a bug (and mention in the bug to grep for the bug # before
closing)

http://codereview.chromium.org/9295044/diff/28003/chrome/browser/sync/profile...
chrome/browser/sync/profile_sync_service.cc:1451: if (!sync_initialized() ||
Hm, so there's no way we can call UpdateAuthErrorState here instead, is there?

http://codereview.chromium.org/9295044/diff/28003/chrome/browser/sync/profile...
File chrome/browser/sync/profile_sync_service.h (right):

http://codereview.chromium.org/9295044/diff/28003/chrome/browser/sync/profile...
chrome/browser/sync/profile_sync_service.h:328: virtual bool
UIShouldDepictAuthInProgress() const;
do we still need this if we now have AuthInProgress?  Are you leaving it here
out of convenience for now (totally fine, just update todo)?

http://codereview.chromium.org/9295044/diff/28003/chrome/browser/sync/profile...
chrome/browser/sync/profile_sync_service.h:517: bool waiting_for_auth() const {
return is_auth_in_progress_; }
why do we need this?

http://codereview.chromium.org/9295044/diff/28003/chrome/browser/ui/webui/sig...
File chrome/browser/ui/webui/signin/signin_tracker.h (right):

http://codereview.chromium.org/9295044/diff/28003/chrome/browser/ui/webui/sig...
chrome/browser/ui/webui/signin/signin_tracker.h:18: class SigninTracker : public
ProfileSyncServiceObserver,
you don't think this one belongs in browser/signin?

[Andrew T Wilson (Slow), 2012-02-14 05:23:51]

Please take another look.

http://codereview.chromium.org/9295044/diff/28003/chrome/browser/sync/profile...
File chrome/browser/sync/profile_sync_service.cc (right):

http://codereview.chromium.org/9295044/diff/28003/chrome/browser/sync/profile...
chrome/browser/sync/profile_sync_service.cc:734: // Got some kind of auth error
while the wizard is visible. Exit out of
On 2012/02/14 02:11:21, timsteele wrote:
> err... why is this fatal?  what happens to any common GSAE like invalid gaia?

Remember that this is dealing with a GAIA error *after* login, while the config
screen is visible (not during signin). But, yeah, I guess nothing bad happens if
we leave the config screen up, so I'll remove this (the case of
SERVICE_UNAVAILABLE due to StopSyncingPermanently is already handled below).

http://codereview.chromium.org/9295044/diff/28003/chrome/browser/sync/profile...
chrome/browser/sync/profile_sync_service.cc:752: if (SetupInProgress()) {
BTW, I'm changing this to abort whenever the wizard is visible. If we've stopped
syncing, it doesn't matter if this is initial setup or not, if the wizard is up
we need to close it.

http://codereview.chromium.org/9295044/diff/28003/chrome/browser/sync/profile...
chrome/browser/sync/profile_sync_service.cc:969: // itself and should not result
in a call to ShowErrorUI.
On 2012/02/14 02:11:21, timsteele wrote:
> hm. so what about a failure to initialize / load the database in sync_manager?
This change just makes explicit what was previously hidden away in
SyncSetupWizard::GetStepForNonFatalError(). That code assumed that either you
have passphrase required, or else an auth error, anything else hit a NOTREACHED
- so this shouldn't be a change in functionality, it's just clearer now.

The code in sync_ui_util.cc is dense and i'm still wrapping my head around it,
but I believe the intent is that unrecoverable errors like the type you
described don't result in calls to ShowErrorUI().

http://codereview.chromium.org/9295044/diff/28003/chrome/browser/sync/profile...
chrome/browser/sync/profile_sync_service.cc:1441: // TODO(sync): Remove this
when sync is no longer tied to browser signin.
On 2012/02/14 02:11:21, timsteele wrote:
> should tie this to a bug (and mention in the bug to grep for the bug # before
> closing)

Done.

http://codereview.chromium.org/9295044/diff/28003/chrome/browser/sync/profile...
chrome/browser/sync/profile_sync_service.cc:1451: if (!sync_initialized() ||
On 2012/02/14 02:11:21, timsteele wrote:
> Hm, so there's no way we can call UpdateAuthErrorState here instead, is there?

We aren't actually changing the state - all we're doing is setting
is_auth_in_progress if we decide we need to wait for auth to complete.

http://codereview.chromium.org/9295044/diff/28003/chrome/browser/sync/profile...
File chrome/browser/sync/profile_sync_service.h (right):

http://codereview.chromium.org/9295044/diff/28003/chrome/browser/sync/profile...
chrome/browser/sync/profile_sync_service.h:328: virtual bool
UIShouldDepictAuthInProgress() const;
On 2012/02/14 02:11:21, timsteele wrote:
> do we still need this if we now have AuthInProgress?  Are you leaving it here
> out of convenience for now (totally fine, just update todo)?
Done, updated todo.

http://codereview.chromium.org/9295044/diff/28003/chrome/browser/sync/profile...
chrome/browser/sync/profile_sync_service.h:517: bool waiting_for_auth() const {
return is_auth_in_progress_; }
On 2012/02/14 02:11:21, timsteele wrote:
> why do we need this?
Now that ProfileSyncService isn't explicitly driving the signin flow state
(instead the UI is just a dumb ProfileSyncServiceObserver) the UI needs to be
able to tell the difference between "ProfileSyncService is reporting an error
via GetAuthError() because it hasn't finished contacting the server yet" and
"ProfileSyncService has already tried and failed to contact the server". I'll
add a comment.

http://codereview.chromium.org/9295044/diff/28003/chrome/browser/ui/webui/sig...
File chrome/browser/ui/webui/signin/signin_tracker.h (right):

http://codereview.chromium.org/9295044/diff/28003/chrome/browser/ui/webui/sig...
chrome/browser/ui/webui/signin/signin_tracker.h:18: class SigninTracker : public
ProfileSyncServiceObserver,
On 2012/02/14 02:11:21, timsteele wrote:
> you don't think this one belongs in browser/signin?

I don't think so - it's really a UI helper class and it's listening for things
like service initialization that's technically post-signin. I haven't decided
yet whether in the future this class will add more state to include things like
service configuration (ala the current SyncSetupWizard) or if I'll move that
state back into SyncSetupHandler in which case this class will probably just go
away in favor of having SyncSetupHandler listen to notifications.

[tim (not reviewing), 2012-02-14 17:22:28]

http://codereview.chromium.org/9295044/diff/10004/chrome/browser/ui/webui/sig...
File chrome/browser/ui/webui/signin/login_ui_service.cc (right):

http://codereview.chromium.org/9295044/diff/10004/chrome/browser/ui/webui/sig...
chrome/browser/ui/webui/signin/login_ui_service.cc:31: ui_ = NULL;
Oh, I see.  Wait, when is this called?

Could we instead name this function OnWebUIGoingAway or something to that effect
(BeingClosed, etc)... that makes it more clear that we're just telling |this|
about some event that it may want to take action on, based on the parameter.

http://codereview.chromium.org/9295044/diff/10004/chrome/browser/ui/webui/sig...
File chrome/browser/ui/webui/signin/login_ui_service.h (right):

http://codereview.chromium.org/9295044/diff/10004/chrome/browser/ui/webui/sig...
chrome/browser/ui/webui/signin/login_ui_service.h:20: class LoginUIService :
public ProfileKeyedService {
So this class isn't meant to be used on all platforms/embeddings, correct?  Can
you add a comment explaining where the bypasses might happen?

http://codereview.chromium.org/9295044/diff/10004/chrome/browser/ui/webui/sig...
chrome/browser/ui/webui/signin/login_ui_service.h:39: void
ResetLoginUI(content::WebUI* ui);
I'm substantially confused about the difference between Set and Reset here.  I
think in your comment 'resets it' implies 'sets it to NULL', but in the
function, 'Reset' can imply 'Change to other ui*'?

http://codereview.chromium.org/9295044/diff/10004/chrome/browser/ui/webui/sig...
File chrome/browser/ui/webui/signin/signin_tracker.cc (right):

http://codereview.chromium.org/9295044/diff/10004/chrome/browser/ui/webui/sig...
chrome/browser/ui/webui/signin/signin_tracker.cc:70: // of ProfileSyncService
and over to here.
Bug#

http://codereview.chromium.org/9295044/diff/10004/chrome/browser/ui/webui/sig...
File chrome/browser/ui/webui/signin/signin_tracker.h (right):

http://codereview.chromium.org/9295044/diff/10004/chrome/browser/ui/webui/sig...
chrome/browser/ui/webui/signin/signin_tracker.h:67: Observer* observer_;
Brief comment that we require this to live beyond |this| (actually maybe add
that comment at the ctor).

http://codereview.chromium.org/9295044/diff/10004/chrome/browser/ui/webui/syn...
File chrome/browser/ui/webui/sync_setup_handler.cc (right):

http://codereview.chromium.org/9295044/diff/10004/chrome/browser/ui/webui/syn...
chrome/browser/ui/webui/sync_setup_handler.cc:476: // By default, do nothing -
subclasses can override.
Wait, what is the subclass again? I'm tempted to ask for a paragraph-or-two
comment / ascii art at the top of one of the new files, perhaps LoginUIService,
explaining how the Signin Process works at a high level and the relationships
between the different classes.  It might rot, but I think it'll be useful long
enough to make it worthwhile.

http://codereview.chromium.org/9295044/diff/10004/chrome/browser/ui/webui/syn...
chrome/browser/ui/webui/sync_setup_handler.cc:736: // User is not logged in -
need to display login UI.
My initial reaction to the "User is not logged in" comment made me wonder why
you're not calling IsUserLoggedIn instead.  What's the difference? Where is that
explained?

I think here you mean User is not logged in _to at least one service_, and
presumably we can't mint tokens for that service from whatever we have in the
TokenService, even though the user may have logged in in the past.  Or something
like that?

http://codereview.chromium.org/9295044/diff/10004/chrome/browser/ui/webui/syn...
chrome/browser/ui/webui/sync_setup_handler.cc:739: StartConfigureSync();
So this case is reached if the user clicks 'Advanced' for example, on the
settings page? (May be nice to have a comment).

http://codereview.chromium.org/9295044/diff/10004/chrome/browser/ui/webui/syn...
chrome/browser/ui/webui/sync_setup_handler.cc:767: bool
SyncSetupHandler::FocusExistingWizard() {
Maybe 'FocusExistingWizardIfPresent'? Something to convey the fact that this
function doesn't require an existing wizard as a precondition.

[Andrew T Wilson (Slow), 2012-02-14 20:52:36]

OK, addressed your feedback (I think) - PTAL.

http://codereview.chromium.org/9295044/diff/10004/chrome/browser/ui/webui/sig...
File chrome/browser/ui/webui/signin/login_ui_service.cc (right):

http://codereview.chromium.org/9295044/diff/10004/chrome/browser/ui/webui/sig...
chrome/browser/ui/webui/signin/login_ui_service.cc:31: ui_ = NULL;
I think the renaming addressed this. Let me know if you want further renaming.

http://codereview.chromium.org/9295044/diff/10004/chrome/browser/ui/webui/sig...
File chrome/browser/ui/webui/signin/login_ui_service.h (right):

http://codereview.chromium.org/9295044/diff/10004/chrome/browser/ui/webui/sig...
chrome/browser/ui/webui/signin/login_ui_service.h:20: class LoginUIService :
public ProfileKeyedService {
On 2012/02/14 17:22:29, timsteele wrote:
> So this class isn't meant to be used on all platforms/embeddings, correct? 
Can
> you add a comment explaining where the bypasses might happen.

I'm not sure what you mean. Can you clarify? This class basically just keeps
track of the current login screen, because our current login UI flow enforces
having only a single login screen visible, so trying to bring up login UI should
pop up the existing UI if one is visible.

http://codereview.chromium.org/9295044/diff/10004/chrome/browser/ui/webui/sig...
chrome/browser/ui/webui/signin/login_ui_service.h:39: void
ResetLoginUI(content::WebUI* ui);
On 2012/02/14 17:22:29, timsteele wrote:
> I'm substantially confused about the difference between Set and Reset here.  I
> think in your comment 'resets it' implies 'sets it to NULL', but in the
> function, 'Reset' can imply 'Change to other ui*'?  

Sigh, this used to be ClearLoginUI(), but jhawkins asked me to change it (and I
agree with his reasoning). I'm going to change this to LoginUIClosed() instead
which I hope addresses everyone's concerns.

http://codereview.chromium.org/9295044/diff/10004/chrome/browser/ui/webui/sig...
File chrome/browser/ui/webui/signin/signin_tracker.cc (right):

http://codereview.chromium.org/9295044/diff/10004/chrome/browser/ui/webui/sig...
chrome/browser/ui/webui/signin/signin_tracker.cc:70: // of ProfileSyncService
and over to here.
On 2012/02/14 17:22:29, timsteele wrote:
> Bug#

Done.

http://codereview.chromium.org/9295044/diff/10004/chrome/browser/ui/webui/sig...
File chrome/browser/ui/webui/signin/signin_tracker.h (right):

http://codereview.chromium.org/9295044/diff/10004/chrome/browser/ui/webui/sig...
chrome/browser/ui/webui/signin/signin_tracker.h:67: Observer* observer_;
On 2012/02/14 17:22:29, timsteele wrote:
> Brief comment that we require this to live beyond |this| (actually maybe add
> that comment at the ctor).

Done.

http://codereview.chromium.org/9295044/diff/10004/chrome/browser/ui/webui/syn...
File chrome/browser/ui/webui/sync_setup_handler.cc (right):

http://codereview.chromium.org/9295044/diff/10004/chrome/browser/ui/webui/syn...
chrome/browser/ui/webui/sync_setup_handler.cc:476: // By default, do nothing -
subclasses can override.
The subclasses are SyncPromoHandler and OptionsSyncSetupHandler. The promo
handler wants to do special logging of signins which is why this API is here.

I've added documentation to SigninTracker as that's more of a core class than
LoginUIService, I think. Let me know if you disagree.

http://codereview.chromium.org/9295044/diff/10004/chrome/browser/ui/webui/syn...
chrome/browser/ui/webui/sync_setup_handler.cc:736: // User is not logged in -
need to display login UI.
IsUserLoggedIn() was obsolete/never actually called. I decided to move that
logic into SigninTracker, but forgot to remove the obsolete routine. Thanks for
catching that!

http://codereview.chromium.org/9295044/diff/10004/chrome/browser/ui/webui/syn...
chrome/browser/ui/webui/sync_setup_handler.cc:739: StartConfigureSync();
On 2012/02/14 17:22:29, timsteele wrote:
> So this case is reached if the user clicks 'Advanced' for example, on the
> settings page? (May be nice to have a comment).

Done. BTW, there are possibly lots of ways for this to get called (through
wrench menus, etc) and I'm trying not to embed comments here that will easily
get obsolete as the specifics of the UI change. In theory this class shouldn't
care about exactly what UI elements lead to OpenSyncSetup() being called.

http://codereview.chromium.org/9295044/diff/10004/chrome/browser/ui/webui/syn...
chrome/browser/ui/webui/sync_setup_handler.cc:767: bool
SyncSetupHandler::FocusExistingWizard() {
On 2012/02/14 17:22:29, timsteele wrote:
> Maybe 'FocusExistingWizardIfPresent'? Something to convey the fact that this
> function doesn't require an existing wizard as a precondition.

Sure. It's a pre-existing function (I just updated the implementation to work in
the new refactored world), but I'll rename it.

[tim (not reviewing), 2012-02-15 01:10:16]

Thanks for the changes. LGTM!

[Andrew T Wilson (Slow), 2012-02-15 01:17:33]

On 2012/02/15 01:10:16, timsteele wrote:
> Thanks for the changes. LGTM!

Thanks, Tim. James, you're the last man standing. Any further feedback?

[James Hawkins, 2012-02-15 01:50:50]

LGTM with two nits.

http://codereview.chromium.org/9295044/diff/12009/chrome/browser/sync/profile...
File chrome/browser/sync/profile_sync_service_mock.h (right):

http://codereview.chromium.org/9295044/diff/12009/chrome/browser/sync/profile...
chrome/browser/sync/profile_sync_service_mock.h:21: class SigninManager;
nit: Appears unused.

http://codereview.chromium.org/9295044/diff/12009/chrome/browser/sync/sync_ui...
File chrome/browser/sync/sync_ui_util.cc (right):

http://codereview.chromium.org/9295044/diff/12009/chrome/browser/sync/sync_ui...
chrome/browser/sync/sync_ui_util.cc:14: #include
"chrome/browser/signin/signin_manager.h"
nit: Is this used?

[Andrew T Wilson (Slow), 2012-02-15 02:08:50]

http://codereview.chromium.org/9295044/diff/12009/chrome/browser/sync/profile...
File chrome/browser/sync/profile_sync_service_mock.h (right):

http://codereview.chromium.org/9295044/diff/12009/chrome/browser/sync/profile...
chrome/browser/sync/profile_sync_service_mock.h:21: class SigninManager;
On 2012/02/15 01:50:50, James Hawkins wrote:
> nit: Appears unused.

Done.

http://codereview.chromium.org/9295044/diff/12009/chrome/browser/sync/sync_ui...
File chrome/browser/sync/sync_ui_util.cc (right):

http://codereview.chromium.org/9295044/diff/12009/chrome/browser/sync/sync_ui...
chrome/browser/sync/sync_ui_util.cc:14: #include
"chrome/browser/signin/signin_manager.h"
On 2012/02/15 01:50:50, James Hawkins wrote:
> nit: Is this used?

Done.

[commit-bot: I haz the power, 2012-02-15 02:12:33]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/atwilson@chromium.org/9295044/38001

[commit-bot: I haz the power, 2012-02-15 04:08:43]

Change committed as 122030