Remove useless blocking handle from sync integ tests.

Remove useless blocking handle from sync integ tests.

ProfileSyncService already requires SetFirstSetupComplete() to be
called, signifying that the user has finished setting any sync related
settings, and sync should begin.

It seems GetSetupInProgressHandle() should only be needed if there are
two of more logical pieces trying to block sync from initializing. That
is not the case here, and as such it is not needed.

BUG=

[skym, 2017-03-07 20:37:44]

The CQ bit was checked by skym@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-07 20:39:17]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[skym, 2017-03-07 21:42:51]

Description was changed from

==========
Remove useless blocking handle from sync integ tests.

BUG=
==========

to

==========
Remove useless blocking handle from sync integ tests.

ProfileSyncService already requires SetFirstSetupComplete() to be
called, signifying that the user has finished setting any sync related
settings, and sync should begin.

It seems GetSetupInProgressHandle() should only be needed if there are
two of more logical pieces trying to block sync from initializing. That
is not the case here, and as such it is not needed.

BUG=
==========

[skym, 2017-03-07 21:42:51]

skym@chromium.org changed reviewers:
+ pavely@chromium.org, wylieb@chromium.org

[skym, 2017-03-07 21:43:00]

PTAL

[commit-bot: I haz the power, 2017-03-07 21:43:44]

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

[commit-bot: I haz the power, 2017-03-07 21:43:44]

Dry run: This issue passed the CQ dry run.

[pavely, 2017-03-07 22:57:53]

lgtm. Consider my comment.

https://codereview.chromium.org/2735253002/diff/1/chrome/browser/sync/test/in...
File chrome/browser/sync/test/integration/profile_sync_service_harness.cc
(left):

https://codereview.chromium.org/2735253002/diff/1/chrome/browser/sync/test/in...
chrome/browser/sync/test/integration/profile_sync_service_harness.cc:157:
sync_blocker_ = service()->GetSetupInProgressHandle();
Sync blocker guarantees that configuration will not start until after the call
to OnUserChoseDatatypes() while SetFirstSetupComplete() ensures that if
configuration didn't start before, it will at least start now. Configuration
could start earlier if in the future we change code to emulate clicking on sync
confirmation dialog.

I believe your change is correct (you tested that configuration only starts
after SetFirstSyncComplete, right?), but keeping sync_blocker_ is safer.

[wylieb, 2017-03-07 23:02:31]

I think we should wait for:
https://codereview.chromium.org/2716413003/

To be approved & committed. It'll be easier to uncover any errors in the E2E
testing when we have the interface with external servers up and running.

[skym, 2017-03-07 23:31:11]

https://codereview.chromium.org/2735253002/diff/1/chrome/browser/sync/test/in...
File chrome/browser/sync/test/integration/profile_sync_service_harness.cc
(left):

https://codereview.chromium.org/2735253002/diff/1/chrome/browser/sync/test/in...
chrome/browser/sync/test/integration/profile_sync_service_harness.cc:157:
sync_blocker_ = service()->GetSetupInProgressHandle();
On 2017/03/07 22:57:53, pavely wrote:
> Sync blocker guarantees that configuration will not start until after the call
> to OnUserChoseDatatypes() while SetFirstSetupComplete() ensures that if
> configuration didn't start before, it will at least start now. Configuration
> could start earlier if in the future we change code to emulate clicking on
sync
> confirmation dialog.
> 
> I believe your change is correct (you tested that configuration only starts
> after SetFirstSyncComplete, right?), but keeping sync_blocker_ is safer.

My understanding is that Sync will not start until SetFirstSetupComplete() has
been called at least once. The potential threat is that someone else may call it
before you, which is why you'd use the blocker.

You're right, it makes us more fragile to sign in methods, but I'm not convinced
that's a bad thing. If the goal here is "end to end", it makes sense to avoid
cheating as much as possible, right? I think removing the blocker is only good.
Maybe we should even add a check that verifies SetFirstSetupComplete() hasn't
been called yet, before we call it ourselves.

Digging into what SigninType actually does, it looks like UI_SIGNIN is actually
what we use for E2E tests,
https://cs.chromium.org/chromium/src/chrome/browser/sync/test/integration/syn...
. While FAKE_SIGNIN is used for the "normal" integ test runs.

It seems to be like it'd be ideal if on FAKE_SIGNIN we only called
SetFirstSyncComplete() and on UI_SIGNIN we did nothing. Instead we'd let the
dismissal of the dialog call SetFirstSyncComplete() for us, right?

Now, I cannot test this, since E2E tests don't work right now. But I think in
the future we could wrap this as such: 

// Comments.
if (signin_type == SigninType::FAKE_SIGNIN) {
  service()->SetFirstSetupComplete();
}

WDYT?

[skym, 2017-03-07 23:32:24]

On 2017/03/07 23:02:31, wylieb wrote:
> I think we should wait for:
> https://codereview.chromium.org/2716413003/
> 
> To be approved & committed. It'll be easier to uncover any errors in the E2E
> testing when we have the interface with external servers up and running.

I was trying to simplify things for your CL by removing the member blocker
field. I can certainly wait though!

[pavely, 2017-03-08 01:13:52]

still lgtm

https://codereview.chromium.org/2735253002/diff/1/chrome/browser/sync/test/in...
File chrome/browser/sync/test/integration/profile_sync_service_harness.cc
(left):

https://codereview.chromium.org/2735253002/diff/1/chrome/browser/sync/test/in...
chrome/browser/sync/test/integration/profile_sync_service_harness.cc:157:
sync_blocker_ = service()->GetSetupInProgressHandle();
On 2017/03/07 23:31:10, skym wrote:
> On 2017/03/07 22:57:53, pavely wrote:
> > Sync blocker guarantees that configuration will not start until after the
call
> > to OnUserChoseDatatypes() while SetFirstSetupComplete() ensures that if
> > configuration didn't start before, it will at least start now. Configuration
> > could start earlier if in the future we change code to emulate clicking on
> sync
> > confirmation dialog.
> > 
> > I believe your change is correct (you tested that configuration only starts
> > after SetFirstSyncComplete, right?), but keeping sync_blocker_ is safer.
> 
> My understanding is that Sync will not start until SetFirstSetupComplete() has
> been called at least once. The potential threat is that someone else may call
it
> before you, which is why you'd use the blocker.
> 
> You're right, it makes us more fragile to sign in methods, but I'm not
convinced
> that's a bad thing. If the goal here is "end to end", it makes sense to avoid
> cheating as much as possible, right? I think removing the blocker is only
good.
> Maybe we should even add a check that verifies SetFirstSetupComplete() hasn't
> been called yet, before we call it ourselves.
> 
> Digging into what SigninType actually does, it looks like UI_SIGNIN is
actually
> what we use for E2E tests,
>
https://cs.chromium.org/chromium/src/chrome/browser/sync/test/integration/syn...
> . While FAKE_SIGNIN is used for the "normal" integ test runs.
> 
> It seems to be like it'd be ideal if on FAKE_SIGNIN we only called
> SetFirstSyncComplete() and on UI_SIGNIN we did nothing. Instead we'd let the
> dismissal of the dialog call SetFirstSyncComplete() for us, right?
> 
> Now, I cannot test this, since E2E tests don't work right now. But I think in
> the future we could wrap this as such: 
> 
> // Comments.
> if (signin_type == SigninType::FAKE_SIGNIN) {
>   service()->SetFirstSetupComplete();
> }
> 
> WDYT?

Actually login_ui_test_utils::SignInWithUI only signs user in, it doesn't
dismiss sync confirmation. Instead, call to
login_ui_test_utils::DismissSyncConfirmationDialog does that.

I think your change is safe.

I agree about calling SetFirstSetupComplete() only for FAKE_SIGNIN.