Move UI specific implementation from ProfileSyncService to SyncSetupFlowHandler.

Move UI specific implementation from ProfileSyncService to SyncSetupFlowHandler.

BUG=none
TEST=none

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

[Andrew T Wilson (Slow), 2011-07-07 17:44:23]

Overall looks good except for one change I'd suggest - what do you think?

http://codereview.chromium.org/7193031/diff/1/chrome/browser/sync/profile_syn...
File chrome/browser/sync/profile_sync_service.cc (right):

http://codereview.chromium.org/7193031/diff/1/chrome/browser/sync/profile_syn...
chrome/browser/sync/profile_sync_service.cc:858: void
ProfileSyncService::ShowSyncSetup(SyncSetupWizard::State state) {
So, I'm wondering if we should just completely get rid of this routine, and
instead add a ShowSyncSetup(SyncSetupWizard::State state) routine to
SyncSetupWizard that does exactly what this does (steps to that state, and
displays the setup appropriately). And all calls to this routine
(ShowSyncSetup(<state>)) would instead change to wizard_.ShowSyncSetup(<state>).

http://codereview.chromium.org/7193031/diff/1/chrome/browser/sync/sync_setup_...
File chrome/browser/sync/sync_setup_flow.h (right):

http://codereview.chromium.org/7193031/diff/1/chrome/browser/sync/sync_setup_...
chrome/browser/sync/sync_setup_flow.h:76: void ShowSyncSetup();
See previous comment about passing the new state in here and doing a step within
this routine.

[tim (not reviewing), 2011-07-07 17:49:47]

http://codereview.chromium.org/7193031/diff/1/chrome/browser/sync/profile_syn...
File chrome/browser/sync/profile_sync_service.cc (right):

http://codereview.chromium.org/7193031/diff/1/chrome/browser/sync/profile_syn...
chrome/browser/sync/profile_sync_service.cc:858: void
ProfileSyncService::ShowSyncSetup(SyncSetupWizard::State state) {
On 2011/07/07 17:44:23, Andrew T Wilson wrote:
> So, I'm wondering if we should just completely get rid of this routine, and
> instead add a ShowSyncSetup(SyncSetupWizard::State state) routine to
> SyncSetupWizard that does exactly what this does (steps to that state, and
> displays the setup appropriately). And all calls to this routine
> (ShowSyncSetup(<state>)) would instead change to
wizard_.ShowSyncSetup(<state>).

hm. wizard_.Step is supposed to do exactly that; advance, and show
appropriately... that's what the if (flow) and SyncSetupFlow::Run stuff is about
in there.

http://codereview.chromium.org/7193031/diff/1/chrome/browser/sync/sync_setup_...
File chrome/browser/sync/sync_setup_wizard.cc (right):

http://codereview.chromium.org/7193031/diff/1/chrome/browser/sync/sync_setup_...
chrome/browser/sync/sync_setup_wizard.cc:89: flow->ShowSyncSetup();
this just looks like a sub-case of Step as implemented already... what am I
missing?

[Andrew T Wilson (Slow), 2011-07-07 17:51:02]

So you think all the calls to ShowSyncSetup() should instead just be calling
wizard_.Step()?

[tim (not reviewing), 2011-07-07 17:54:56]

Right

On Thu, Jul 7, 2011 at 10:51 AM, <atwilson@chromium.org> wrote:

> So you think all the calls to ShowSyncSetup() should instead just be
> calling
> wizard_.Step()?
>
>
>
>
http://codereview.chromium.**org/7193031/<http://codereview.chromium.org/7193...
>

[qsr (NOT THE RIGHT qsr), 2011-07-08 08:51:42]

http://codereview.chromium.org/7193031/diff/1/chrome/browser/sync/sync_setup_...
File chrome/browser/sync/sync_setup_wizard.cc (right):

http://codereview.chromium.org/7193031/diff/1/chrome/browser/sync/sync_setup_...
chrome/browser/sync/sync_setup_wizard.cc:89: flow->ShowSyncSetup();
> this just looks like a sub-case of Step as implemented already... what am I
> missing?

 This is calling ShowSyncSetup method on flow, that will ultimately call:
 
BrowserList::GetLastActiveWithProfile(profile)->ShowOptionsTab(chrome::kSyncSetupSubPage);

 Step itself is never doing it, and the SyncService can call either of the 2
methods depending on the context.

 I'm not sure to understand what you propose. The aim of this patch is to move
all UI related code to the implementation of the flow handler.

[tim (not reviewing), 2011-07-08 16:23:15]

On 2011/07/08 08:51:42, qsr wrote:
>
http://codereview.chromium.org/7193031/diff/1/chrome/browser/sync/sync_setup_...
> File chrome/browser/sync/sync_setup_wizard.cc (right):
> 
>
http://codereview.chromium.org/7193031/diff/1/chrome/browser/sync/sync_setup_...
> chrome/browser/sync/sync_setup_wizard.cc:89: flow->ShowSyncSetup();
> > this just looks like a sub-case of Step as implemented already... what am I
> > missing?
> 
>  This is calling ShowSyncSetup method on flow, that will ultimately call:
>  
>
BrowserList::GetLastActiveWithProfile(profile)->ShowOptionsTab(chrome::kSyncSetupSubPage);
> 
>  Step itself is never doing it, and the SyncService can call either of the 2
> methods depending on the context.
> 
>  I'm not sure to understand what you propose. The aim of this patch is to move
> all UI related code to the implementation of the flow handler.

We discussed this offline and it LGTM now!

[tim (not reviewing), 2011-07-08 16:25:02]

On 2011/07/08 16:23:15, timsteele wrote:
> On 2011/07/08 08:51:42, qsr wrote:
> >
>
http://codereview.chromium.org/7193031/diff/1/chrome/browser/sync/sync_setup_...
> > File chrome/browser/sync/sync_setup_wizard.cc (right):
> > 
> >
>
http://codereview.chromium.org/7193031/diff/1/chrome/browser/sync/sync_setup_...
> > chrome/browser/sync/sync_setup_wizard.cc:89: flow->ShowSyncSetup();
> > > this just looks like a sub-case of Step as implemented already... what am
I
> > > missing?
> > 
> >  This is calling ShowSyncSetup method on flow, that will ultimately call:
> >  
> >
>
BrowserList::GetLastActiveWithProfile(profile)->ShowOptionsTab(chrome::kSyncSetupSubPage);
> > 
> >  Step itself is never doing it, and the SyncService can call either of the 2
> > methods depending on the context.
> > 
> >  I'm not sure to understand what you propose. The aim of this patch is to
move
> > all UI related code to the implementation of the flow handler.
> 
> We discussed this offline and it LGTM now!

-- sent too soon

Andrew's proposal could work as well.  Did you get a chance to address it before
I chimed in here?

[qsr (NOT THE RIGHT qsr), 2011-07-08 16:28:45]

>
>
> Andrew's proposal could work as well.  Did you get a chance to address it
> before
> I chimed in here?


 Nop. but I agree with the remark, and will do it. You'll get the CL monday.

-- 
        Benjamin

[qsr (NOT THE RIGHT qsr), 2011-07-11 06:34:35]

Can you take another look.

 I did as Drew suggested. I found out that if a method was to call
ShowSyncSetup, it first checked if the wizard is visible, and if so, will only
focus the wizard and not do anything else, so I refactor the method to add this
behavior in it, which simplifies a few methods in ProfileSyncService.

[tim (not reviewing), 2011-07-11 14:44:25]

Adding jhawkins + csilv to help address my question below.

http://codereview.chromium.org/7193031/diff/9002/chrome/browser/sync/sync_set...
File chrome/browser/sync/sync_setup_wizard.cc (right):

http://codereview.chromium.org/7193031/diff/9002/chrome/browser/sync/sync_set...
chrome/browser/sync/sync_setup_wizard.cc:95: flow->ShowSyncSetup();
I think I see what changed that caused my confusion last week... Flow::Run did
indeed used to "show" the UI, but it was removed ~6 weeks ago by
http://codereview.chromium.org/7057038.  But the method is still named Run,
which is really confusing to me.  

Could we not have Run(...) call flow_handler-> ShowSyncSetup, akin to how it
worked before that CR above, and then not need this new method?

[Andrew Wilson, 2011-07-11 16:58:41]

Yeah, I'm not sure that either Tim or I completely understand the motivation
behind that earlier CL, and it clearly continues to have ripple effects. I'd
say that having Run() call ShowSyncSetup() would be a reasonable change, and
the domUI folks can do whatever magic they need to down in their
SyncSetupFlowHandler to avoid jank in the case that the UI is already
visible. Any objections, jhawkins/csilv?


On Mon, Jul 11, 2011 at 7:44 AM, <tim@chromium.org> wrote:

> Adding jhawkins + csilv to help address my question below.
>
>
> http://codereview.chromium.**org/7193031/diff/9002/chrome/**
>
browser/sync/sync_setup_**wizard.cc<http://codereview.chromium.org/7193031/diff/9002/chrome/browser/sync/sync_setup_wizard.cc>
> File chrome/browser/sync/sync_**setup_wizard.cc (right):
>
> http://codereview.chromium.**org/7193031/diff/9002/chrome/**
>
browser/sync/sync_setup_**wizard.cc#newcode95<http://codereview.chromium.org/7193031/diff/9002/chrome/browser/sync/sync_setup_wizard.cc#newcode95>
> chrome/browser/sync/sync_**setup_wizard.cc:95: flow->ShowSyncSetup();
> I think I see what changed that caused my confusion last week...
> Flow::Run did indeed used to "show" the UI, but it was removed ~6 weeks
> ago by
http://codereview.chromium.**org/7057038<http://codereview.chromium.org/7057038>.
>  But the method is still
> named Run, which is really confusing to me.
>
> Could we not have Run(...) call flow_handler-> ShowSyncSetup, akin to
> how it worked before that CR above, and then not need this new method?
>
>
http://codereview.chromium.**org/7193031/<http://codereview.chromium.org/7193...
>

[James Hawkins, 2011-07-11 17:12:03]

On 2011/07/11 16:58:41, Andrew Wilson wrote:
> Yeah, I'm not sure that either Tim or I completely understand the motivation
> behind that earlier CL, and it clearly continues to have ripple effects. I'd
> say that having Run() call ShowSyncSetup() would be a reasonable change, and
> the domUI folks can do whatever magic they need to down in their
> SyncSetupFlowHandler to avoid jank in the case that the UI is already
> visible. Any objections, jhawkins/csilv?
> 
> 
> On Mon, Jul 11, 2011 at 7:44 AM, <mailto:tim@chromium.org> wrote:
> 
> > Adding jhawkins + csilv to help address my question below.
> >
> >
> > http://codereview.chromium.**org/7193031/diff/9002/chrome/**
> >
>
browser/sync/sync_setup_**wizard.cc<http://codereview.chromium.org/7193031/diff/9002/chrome/browser/sync/sync_setup_wizard.cc>
> > File chrome/browser/sync/sync_**setup_wizard.cc (right):
> >
> > http://codereview.chromium.**org/7193031/diff/9002/chrome/**
> >
>
browser/sync/sync_setup_**wizard.cc#newcode95<http://codereview.chromium.org/7193031/diff/9002/chrome/browser/sync/sync_setup_wizard.cc#newcode95>
> > chrome/browser/sync/sync_**setup_wizard.cc:95: flow->ShowSyncSetup();
> > I think I see what changed that caused my confusion last week...
> > Flow::Run did indeed used to "show" the UI, but it was removed ~6 weeks
> > ago by
>
http://codereview.chromium.**org/7057038%3Chttp://codereview.chromium.org/705...>.
> >  But the method is still
> > named Run, which is really confusing to me.
> >
> > Could we not have Run(...) call flow_handler-> ShowSyncSetup, akin to
> > how it worked before that CR above, and then not need this new method?
> >
> >
>
http://codereview.chromium.**org/7193031/%3Chttp://codereview.chromium.org/71...>
> >

If you have a solution for "whatever magic" we need, I'm all ears.

[Andrew T Wilson (Slow), 2011-07-11 17:17:51]

On 2011/07/11 17:12:03, James Hawkins wrote:
> If you have a solution for "whatever magic" we need, I'm all ears.

Again, I can't tell you, since I don't really understand the motivation behind
the previous CL. Can you clarify? Was the problem that calling ShowOptionsTab()
when the UI was already visible was slow/janky, or something else? Because it
seems to me that the domui SyncSetupFlowHandler knows whether its UI is visible
or not, and so would know whether to call ShowOptionsTab() in its
ShowSetupPage() handler.

[csilv, 2011-07-11 17:42:14]

On 2011/07/11 17:17:51, Andrew T Wilson wrote:
> On 2011/07/11 17:12:03, James Hawkins wrote:
> > If you have a solution for "whatever magic" we need, I'm all ears.
> 
> Again, I can't tell you, since I don't really understand the motivation behind
> the previous CL. Can you clarify? Was the problem that calling
ShowOptionsTab()
> when the UI was already visible was slow/janky, or something else? Because it
> seems to me that the domui SyncSetupFlowHandler knows whether its UI is
visible
> or not, and so would know whether to call ShowOptionsTab() in its
> ShowSetupPage() handler.

Hi Andrew & Tim,

The problem in the mentioned CL was that the old implemenation of Run was
calling Browser::ShowOptionsTab().  ShowOptionsTab() is pretty brute force.  It
may load a new settings tab, or cause an existing settings tab to do a page
reload.  For the case where a user is hitting the Customize button, having a
page reload is not what we want in terms of user experience.  My CL was flawed
in that I violated a code separation principle, and I appreciate James and Ben
later working to fix that problem.  The changes you are considering may be fine
as long as we don't re-introduce a situation where we're asking the browser to
do a reload.

[Andrew T Wilson (Slow), 2011-07-11 18:12:51]

Getting pretty close - thanks for hanging in there :)

http://codereview.chromium.org/7193031/diff/9002/chrome/browser/sync/profile_...
File chrome/browser/sync/profile_sync_service.cc (right):

http://codereview.chromium.org/7193031/diff/9002/chrome/browser/sync/profile_...
chrome/browser/sync/profile_sync_service.cc:829:
wizard_.ShowSyncSetup(SyncSetupWizard::NONFATAL_ERROR);
Now that these methods are basically just dumb wrappers for SyncSetupWizard
APIs, one thing I think we should do (in a separate CL, not this one :) is
remove these routines and have callers call directly into the wizard instead.

I'm not 100% sure, so this is fine for now.

http://codereview.chromium.org/7193031/diff/9002/chrome/browser/sync/sync_set...
File chrome/browser/sync/sync_setup_flow.h (right):

http://codereview.chromium.org/7193031/diff/9002/chrome/browser/sync/sync_set...
chrome/browser/sync/sync_setup_flow.h:75: // Show the sync setup tab.
Nit: "Show the sync setup ui." since it's not necessarily a tab on all
platforms.

http://codereview.chromium.org/7193031/diff/9002/chrome/browser/sync/sync_set...
File chrome/browser/sync/sync_setup_flow_handler.h (right):

http://codereview.chromium.org/7193031/diff/9002/chrome/browser/sync/sync_set...
chrome/browser/sync/sync_setup_flow_handler.h:27: virtual void
ShowSyncSetup(Profile* profile) = 0;
I don't think we should be passing the profile in here, especially since we
don't pass it in to any other API (typically it's up to the SyncSetupFlowHandler
to maintain its own association with the Profile, since it won't change over the
lifetime of the object).

http://codereview.chromium.org/7193031/diff/9002/chrome/browser/sync/sync_set...
File chrome/browser/sync/sync_setup_wizard.cc (right):

http://codereview.chromium.org/7193031/diff/9002/chrome/browser/sync/sync_set...
chrome/browser/sync/sync_setup_wizard.cc:90: return;
Don't we still need to Step in the case that there's already a flow object?

http://codereview.chromium.org/7193031/diff/9002/chrome/browser/ui/webui/opti...
File chrome/browser/ui/webui/options/sync_setup_handler.cc (right):

http://codereview.chromium.org/7193031/diff/9002/chrome/browser/ui/webui/opti...
chrome/browser/ui/webui/options/sync_setup_handler.cc:336: void
SyncSetupHandler::ShowSyncSetup(Profile* profile) {
When you get rid of the Profile* parameter, you can get the profile from
web_ui_->GetProfile().

http://codereview.chromium.org/7193031/diff/9002/chrome/browser/ui/webui/opti...
chrome/browser/ui/webui/options/sync_setup_handler.cc:338:
chrome::kSyncSetupSubPage);
csilv: Do we need to do anything else here to avoid the jank problem you ran
into, or is the code in SyncSetupWizard::ShowSyncSetup() that just calls Focus()
if the UI is already visible sufficient?

http://codereview.chromium.org/7193031/diff/9002/chrome/browser/ui/webui/opti...
File chrome/browser/ui/webui/options/sync_setup_handler.h (right):

http://codereview.chromium.org/7193031/diff/9002/chrome/browser/ui/webui/opti...
chrome/browser/ui/webui/options/sync_setup_handler.h:34: virtual void
ShowSyncSetup(Profile* profile);
Mark as OVERRIDE

[James Hawkins, 2011-07-11 18:15:38]

http://codereview.chromium.org/7193031/diff/9002/chrome/browser/sync/sync_set...
File chrome/browser/sync/sync_setup_flow.h (right):

http://codereview.chromium.org/7193031/diff/9002/chrome/browser/sync/sync_set...
chrome/browser/sync/sync_setup_flow.h:75: // Show the sync setup tab.
On 2011/07/11 18:12:51, Andrew T Wilson wrote:
> Nit: "Show the sync setup ui." since it's not necessarily a tab on all
> platforms.

I do prefer UI instead of tab, but which platform doesn't show Sync Setup in a
tab?

[Andrew T Wilson (Slow), 2011-07-11 18:46:26]

For various reasons, I'd like to take this discussion offline. Shoot me a chat
and I can answer your questions about non-tabbed UI.

[qsr (NOT THE RIGHT qsr), 2011-07-12 07:01:37]

http://codereview.chromium.org/7193031/diff/9002/chrome/browser/sync/sync_set...
File chrome/browser/sync/sync_setup_wizard.cc (right):

http://codereview.chromium.org/7193031/diff/9002/chrome/browser/sync/sync_set...
chrome/browser/sync/sync_setup_wizard.cc:90: return;
On 2011/07/11 18:12:51, Andrew T Wilson wrote:
> Don't we still need to Step in the case that there's already a flow object?

If you look at what was done in ProfileSyncService, there was not step done if
the wizard was visible. Mainly, the ShowSyncSetup must only be called to start a
new flow, otherwise it will just focus the UI.

http://codereview.chromium.org/7193031/diff/9002/chrome/browser/ui/webui/opti...
File chrome/browser/ui/webui/options/sync_setup_handler.cc (right):

http://codereview.chromium.org/7193031/diff/9002/chrome/browser/ui/webui/opti...
chrome/browser/ui/webui/options/sync_setup_handler.cc:338:
chrome::kSyncSetupSubPage);
On 2011/07/11 18:12:51, Andrew T Wilson wrote:
> csilv: Do we need to do anything else here to avoid the jank problem you ran
> into, or is the code in SyncSetupWizard::ShowSyncSetup() that just calls
Focus()
> if the UI is already visible sufficient?

 Right now, I'm pretty sure I didn't change the behavior of anything. So it
should do exactly the same as before. If/When we start refactoring the flow,
then it would be necessary to review this.

[Andrew T Wilson (Slow), 2011-07-13 16:51:08]

LGTM. Sorry for the delay - I didn't realize that you had made the other changes
yet (I thought you were just responding to some of my comments). I've started
replying with "Please take another look" when I am ready for review just to make
sure reviewers know I'm ready.

[tim (not reviewing), 2011-07-20 03:55:11]

On 2011/07/13 16:51:08, Andrew T Wilson wrote:
> LGTM. Sorry for the delay - I didn't realize that you had made the other
changes
> yet (I thought you were just responding to some of my comments). I've started
> replying with "Please take another look" when I am ready for review just to
make
> sure reviewers know I'm ready.

I just noticed that you left in all the calls to Step.  So now in some places we
call wizard_.Step, and in some cases we call wizard_.ShowSyncSetup.  I have
absolutely no idea which case is which. 
We should only have one of these, right?

[qsr (NOT THE RIGHT qsr), 2011-07-20 07:37:57]

>
> I just noticed that you left in all the calls to Step.  So now in some
> places we
> call wizard_.Step, and in some cases we call wizard_.ShowSyncSetup.  I have
> absolutely no idea which case is which.
> We should only have one of these, right?


 Not really, at least not without another round of refactoring, and this one
might not be trivial. ShowSyncSetup doesn't do anything if a flow is already
defined, while Step will always do something, creating the flow if needed.
The 2 use case are different. And I do not know an easy way to tell them
apart right now. I can add an option to Step instead of having
the ShowSyncSetup method if you want.

-- 
        Benjamin