[Signin] Factor out a helper class to load a continue URL once Sync is configured.

[Signin] Factor out a helper class to load a continue URL once Sync is configured.

BUG=347247
TEST=unit_tests
R=guohui@chromium.org, rogerta@chromium.org

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

[Roger Tawa OOO till Jul 10th, 2014-03-31 18:24:11]

Looking good Ilya.  A few comments below.

https://codereview.chromium.org/217883004/diff/80001/chrome/browser/ui/sync/o...
File chrome/browser/ui/sync/one_click_signin_helper.cc (left):

https://codereview.chromium.org/217883004/diff/80001/chrome/browser/ui/sync/o...
chrome/browser/ui/sync/one_click_signin_helper.cc:706: DCHECK(!web_contents());
Why is this DCHECK no longer valid?

https://codereview.chromium.org/217883004/diff/80001/chrome/browser/ui/sync/o...
File chrome/browser/ui/sync/one_click_signin_helper.cc (right):

https://codereview.chromium.org/217883004/diff/80001/chrome/browser/ui/sync/o...
chrome/browser/ui/sync/one_click_signin_helper.cc:1242: original_continue_url_ =
GURL();
Because this member is used in
OneClickSigninHelper::SyncSetupCompletedCallback(), it should not be cleared
here.

https://codereview.chromium.org/217883004/diff/80001/chrome/browser/ui/sync/o...
File chrome/browser/ui/sync/one_click_signin_sync_observer.cc (right):

https://codereview.chromium.org/217883004/diff/80001/chrome/browser/ui/sync/o...
chrome/browser/ui/sync/one_click_signin_sync_observer.cc:37: delete this;
From my reading of the c++ standard, I believe calling delete in the ctor is
legal in this particular case because the class has a trivial dtor.  However,
this seem quite brittle since the dtor could become non-trivial later.

I'd rather see a public Initialize() method that contains lines 32-38, and the
owner of OneClickSigninSyncObserver should call it after constructing the
object.

[Ilya Sherman, 2014-03-31 19:34:06]

https://chromiumcodereview.appspot.com/217883004/diff/80001/chrome/browser/ui...
File chrome/browser/ui/sync/one_click_signin_helper.cc (left):

https://chromiumcodereview.appspot.com/217883004/diff/80001/chrome/browser/ui...
chrome/browser/ui/sync/one_click_signin_helper.cc:706: DCHECK(!web_contents());
On 2014/03/31 18:24:11, Roger Tawa wrote:
> Why is this DCHECK no longer valid?

It's valid, but no longer relevant, since this class no longer listens for
WebContentsDestroyed().  I can't move it over into the new class because there,
the destructor can be called *from* WebContentsDestroyed().

https://chromiumcodereview.appspot.com/217883004/diff/80001/chrome/browser/ui...
File chrome/browser/ui/sync/one_click_signin_sync_observer.cc (right):

https://chromiumcodereview.appspot.com/217883004/diff/80001/chrome/browser/ui...
chrome/browser/ui/sync/one_click_signin_sync_observer.cc:37: delete this;
On 2014/03/31 18:24:11, Roger Tawa wrote:
> From my reading of the c++ standard, I believe calling delete in the ctor is
> legal in this particular case because the class has a trivial dtor.  However,
> this seem quite brittle since the dtor could become non-trivial later.
> 
> I'd rather see a public Initialize() method that contains lines 32-38, and the
> owner of OneClickSigninSyncObserver should call it after constructing the
> object.

I agree, this seemed a little iffy to me.  I'd prefer to instead post a task to
delete the object, though, so that clients of the class don't need to be
affected by this implementation detail.  WDYT?

[Roger Tawa OOO till Jul 10th, 2014-03-31 20:10:37]

https://chromiumcodereview.appspot.com/217883004/diff/80001/chrome/browser/ui...
File chrome/browser/ui/sync/one_click_signin_sync_observer.cc (right):

https://chromiumcodereview.appspot.com/217883004/diff/80001/chrome/browser/ui...
chrome/browser/ui/sync/one_click_signin_sync_observer.cc:37: delete this;
On 2014/03/31 19:34:06, Ilya Sherman wrote:
> On 2014/03/31 18:24:11, Roger Tawa wrote:
> > From my reading of the c++ standard, I believe calling delete in the ctor is
> > legal in this particular case because the class has a trivial dtor. 
However,
> > this seem quite brittle since the dtor could become non-trivial later.
> > 
> > I'd rather see a public Initialize() method that contains lines 32-38, and
the
> > owner of OneClickSigninSyncObserver should call it after constructing the
> > object.
> 
> I agree, this seemed a little iffy to me.  I'd prefer to instead post a task
to
> delete the object, though, so that clients of the class don't need to be
> affected by this implementation detail.  WDYT?

sgtm

https://chromiumcodereview.appspot.com/217883004/diff/80001/chrome/browser/ui...
File chrome/browser/ui/sync/one_click_signin_sync_observer.h (right):

https://chromiumcodereview.appspot.com/217883004/diff/80001/chrome/browser/ui...
chrome/browser/ui/sync/one_click_signin_sync_observer.h:19: public
ProfileSyncServiceObserver {
Oops, hit send too quickly in previous comment.  Why do we need this class
instead of adding this functionality to OneClickSigninSyncStarter?

[guohui, 2014-03-31 20:12:27]

LGTM with one comment

https://codereview.chromium.org/217883004/diff/80001/chrome/browser/ui/sync/o...
File chrome/browser/ui/sync/one_click_signin_sync_observer.cc (right):

https://codereview.chromium.org/217883004/diff/80001/chrome/browser/ui/sync/o...
chrome/browser/ui/sync/one_click_signin_sync_observer.cc:37: delete this;
On 2014/03/31 19:34:06, Ilya Sherman wrote:
> On 2014/03/31 18:24:11, Roger Tawa wrote:
> > From my reading of the c++ standard, I believe calling delete in the ctor is
> > legal in this particular case because the class has a trivial dtor. 
However,
> > this seem quite brittle since the dtor could become non-trivial later.
> > 
> > I'd rather see a public Initialize() method that contains lines 32-38, and
the
> > owner of OneClickSigninSyncObserver should call it after constructing the
> > object.
> 
> I agree, this seemed a little iffy to me.  I'd prefer to instead post a task
to
> delete the object, though, so that clients of the class don't need to be
> affected by this implementation detail.  WDYT?

delayed task +1

[Ilya Sherman, 2014-04-01 02:00:35]

https://codereview.chromium.org/217883004/diff/80001/chrome/browser/ui/sync/o...
File chrome/browser/ui/sync/one_click_signin_helper.cc (right):

https://codereview.chromium.org/217883004/diff/80001/chrome/browser/ui/sync/o...
chrome/browser/ui/sync/one_click_signin_helper.cc:1242: original_continue_url_ =
GURL();
On 2014/03/31 18:24:11, Roger Tawa wrote:
> Because this member is used in
> OneClickSigninHelper::SyncSetupCompletedCallback(), it should not be cleared
> here.

Good catch.  Is it ok to never reset the continue URL, or will something break
if I do that?

https://codereview.chromium.org/217883004/diff/80001/chrome/browser/ui/sync/o...
File chrome/browser/ui/sync/one_click_signin_sync_observer.cc (right):

https://codereview.chromium.org/217883004/diff/80001/chrome/browser/ui/sync/o...
chrome/browser/ui/sync/one_click_signin_sync_observer.cc:37: delete this;
On 2014/03/31 20:12:27, guohui wrote:
> On 2014/03/31 19:34:06, Ilya Sherman wrote:
> > On 2014/03/31 18:24:11, Roger Tawa wrote:
> > > From my reading of the c++ standard, I believe calling delete in the ctor
is
> > > legal in this particular case because the class has a trivial dtor. 
> However,
> > > this seem quite brittle since the dtor could become non-trivial later.
> > > 
> > > I'd rather see a public Initialize() method that contains lines 32-38, and
> the
> > > owner of OneClickSigninSyncObserver should call it after constructing the
> > > object.
> > 
> > I agree, this seemed a little iffy to me.  I'd prefer to instead post a task
> to
> > delete the object, though, so that clients of the class don't need to be
> > affected by this implementation detail.  WDYT?
> 
> delayed task +1

Done.

https://codereview.chromium.org/217883004/diff/80001/chrome/browser/ui/sync/o...
File chrome/browser/ui/sync/one_click_signin_sync_observer.h (right):

https://codereview.chromium.org/217883004/diff/80001/chrome/browser/ui/sync/o...
chrome/browser/ui/sync/one_click_signin_sync_observer.h:19: public
ProfileSyncServiceObserver {
On 2014/03/31 20:10:38, Roger Tawa wrote:
> Oops, hit send too quickly in previous comment.  Why do we need this class
> instead of adding this functionality to OneClickSigninSyncStarter?

So, there are two ways to interpret this question:
(1) Why factor out a separate class for this functionality, rather than keeping
all the code in an existing class?
(2) Why does the OneClickSigninHelper class need to know about this factored-out
class, rather than having OneClickSigninSyncStarter handle this detail?

With respect to (1), I'm pretty firmly of the opinion that having many small
classes is an improvement over having a few large classes in terms of
testability, and hence in terms of maintainability.  The OneClickSigninHelper
class is a good example: It does so much, that it's hard for a new developer
like myself to fully grok it.  It also had much less test coverage for this
functionality, even though this CL introduces no changes in functionality.

With respect to (2), that might be possible; I'll need to do more research.  If
you're ok with this, I'd prefer to investigate that in a follow-up CL, since (a)
this one is already relatively large, and (b) my next CL was going to add a call
from the OneClickSigninSyncStarter to launch a OneClickSigninSyncObserver.  It
would definitely be nice if we could remove this logic from the OCSHelper class
entirely, since it would better unify the code paths between the web-based
sign-in flow and the inline one.

[Roger Tawa OOO till Jul 10th, 2014-04-01 15:20:27]

On 2014/04/01 02:00:35, Ilya Sherman wrote:
>
https://codereview.chromium.org/217883004/diff/80001/chrome/browser/ui/sync/o...
> File chrome/browser/ui/sync/one_click_signin_helper.cc (right):
> 
>
https://codereview.chromium.org/217883004/diff/80001/chrome/browser/ui/sync/o...
> chrome/browser/ui/sync/one_click_signin_helper.cc:1242: original_continue_url_
=
> GURL();
> On 2014/03/31 18:24:11, Roger Tawa wrote:
> > Because this member is used in
> > OneClickSigninHelper::SyncSetupCompletedCallback(), it should not be cleared
> > here.
> 
> Good catch.  Is it ok to never reset the continue URL, or will something break
> if I do that?
> 
>
https://codereview.chromium.org/217883004/diff/80001/chrome/browser/ui/sync/o...
> File chrome/browser/ui/sync/one_click_signin_sync_observer.cc (right):
> 
>
https://codereview.chromium.org/217883004/diff/80001/chrome/browser/ui/sync/o...
> chrome/browser/ui/sync/one_click_signin_sync_observer.cc:37: delete this;
> On 2014/03/31 20:12:27, guohui wrote:
> > On 2014/03/31 19:34:06, Ilya Sherman wrote:
> > > On 2014/03/31 18:24:11, Roger Tawa wrote:
> > > > From my reading of the c++ standard, I believe calling delete in the
ctor
> is
> > > > legal in this particular case because the class has a trivial dtor. 
> > However,
> > > > this seem quite brittle since the dtor could become non-trivial later.
> > > > 
> > > > I'd rather see a public Initialize() method that contains lines 32-38,
and
> > the
> > > > owner of OneClickSigninSyncObserver should call it after constructing
the
> > > > object.
> > > 
> > > I agree, this seemed a little iffy to me.  I'd prefer to instead post a
task
> > to
> > > delete the object, though, so that clients of the class don't need to be
> > > affected by this implementation detail.  WDYT?
> > 
> > delayed task +1
> 
> Done.
> 
>
https://codereview.chromium.org/217883004/diff/80001/chrome/browser/ui/sync/o...
> File chrome/browser/ui/sync/one_click_signin_sync_observer.h (right):
> 
>
https://codereview.chromium.org/217883004/diff/80001/chrome/browser/ui/sync/o...
> chrome/browser/ui/sync/one_click_signin_sync_observer.h:19: public
> ProfileSyncServiceObserver {
> On 2014/03/31 20:10:38, Roger Tawa wrote:
> > Oops, hit send too quickly in previous comment.  Why do we need this class
> > instead of adding this functionality to OneClickSigninSyncStarter?
> 
> So, there are two ways to interpret this question:
> (1) Why factor out a separate class for this functionality, rather than
keeping
> all the code in an existing class?
> (2) Why does the OneClickSigninHelper class need to know about this
factored-out
> class, rather than having OneClickSigninSyncStarter handle this detail?
> 
> With respect to (1), I'm pretty firmly of the opinion that having many small
> classes is an improvement over having a few large classes in terms of
> testability, and hence in terms of maintainability.  The OneClickSigninHelper
> class is a good example: It does so much, that it's hard for a new developer
> like myself to fully grok it.  It also had much less test coverage for this
> functionality, even though this CL introduces no changes in functionality.
> 
> With respect to (2), that might be possible; I'll need to do more research. 
If
> you're ok with this, I'd prefer to investigate that in a follow-up CL, since
(a)
> this one is already relatively large, and (b) my next CL was going to add a
call
> from the OneClickSigninSyncStarter to launch a OneClickSigninSyncObserver.  It
> would definitely be nice if we could remove this logic from the OCSHelper
class
> entirely, since it would better unify the code paths between the web-based
> sign-in flow and the inline one.

Agree completely with point 1.

One thing I don't like is classes that delete themselves.  This can lead to code
that runs way beyond when it's safe to run or desired to run.  It makes it
impossible to cancel the operation if you want to.  It might lead to falsely
detecting memory leaks at exit.  It can make the code harder to test.

This new class is always used after OneClickSigninSyncStarter, in both the
web-based sign-in code path and the inline sign-in code path.  Both
OneClickSigninSyncObserver and OneClickSigninSyncStarter need to outlive the UI
that initially created them.  So OneClickSigninSyncObserver could be an instance
data member of OneClickSigninSyncStarter, as you were suggesting.  This is what
I had in mind too.

I'm OK with looking into this a follow up CL.

lgtm

[Roger Tawa OOO till Jul 10th, 2014-04-01 15:21:08]

https://codereview.chromium.org/217883004/diff/80001/chrome/browser/ui/sync/o...
File chrome/browser/ui/sync/one_click_signin_helper.cc (right):

https://codereview.chromium.org/217883004/diff/80001/chrome/browser/ui/sync/o...
chrome/browser/ui/sync/one_click_signin_helper.cc:1242: original_continue_url_ =
GURL();
On 2014/04/01 02:00:35, Ilya Sherman wrote:
> On 2014/03/31 18:24:11, Roger Tawa wrote:
> > Because this member is used in
> > OneClickSigninHelper::SyncSetupCompletedCallback(), it should not be cleared
> > here.
> 
> Good catch.  Is it ok to never reset the continue URL, or will something break
> if I do that?

Its OK to not reset here, it would be reinitialized should the user reattempt a
sign-in.  This is the current behaviour in any case.

[Ilya Sherman, 2014-04-02 03:30:07]

The CQ bit was checked by isherman@chromium.org

[commit-bot: I haz the power, 2014-04-02 03:30:20]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/isherman@chromium.org/217883004/120001

[commit-bot: I haz the power, 2014-04-02 05:58:08]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-04-02 05:58:08]

Retried try job too often on chromium_presubmit for step(s) presubmit
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...

[Ilya Sherman, 2014-04-02 06:42:25]

Tim, please take a look at the changes to the TestProfileSyncService.  Let me
know if there's a better way to do what I'm trying to do there.

[Ilya Sherman, 2014-04-07 21:52:42]

+zea for sync OWNERship

[Nicolas Zea, 2014-04-07 22:24:05]

sync lgtm

[Ilya Sherman, 2014-04-07 22:28:01]

The CQ bit was checked by isherman@chromium.org

[commit-bot: I haz the power, 2014-04-07 22:29:14]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/isherman@chromium.org/217883004/120001

[commit-bot: I haz the power, 2014-04-07 23:49:43]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-04-07 23:49:44]

Retried try job too often on android_dbg for step(s) slave_steps
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...

[Ilya Sherman, 2014-04-10 00:37:25]

The CQ bit was checked by isherman@chromium.org

[commit-bot: I haz the power, 2014-04-10 00:38:09]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/isherman@chromium.org/217883004/160001

[commit-bot: I haz the power, 2014-04-10 04:41:45]

Change committed as 262909