Sync: Support multiple setup UIs.

Sync: Support multiple setup UIs.

Replaces the SetSetupInProgress mechanism with a scoped
GetSetupInProgressHandle mechanism. This has two advantages:

 1. Allows for multiple simultaneously open Setup UIs.
 2. Prevents a buggy UI from blocking Sync indefinitely by forgetting
    to SetSetupInProgress(false).

BUG=563721
Committed: https://crrev.com/12c4b7c0a33a0b4d4d2ea0746325e8a371272248
Cr-Commit-Position: refs/heads/master@{#399581}

[tommycli, 2016-06-09 17:01:26]

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

[commit-bot: I haz the power, 2016-06-09 17:02:53]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2044303004/1

[commit-bot: I haz the power, 2016-06-09 17:15:43]

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

[commit-bot: I haz the power, 2016-06-09 17:15:44]

Dry run: Try jobs failed on following builders:
  ios-device-gn on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-gn/bui...)
  ios-simulator-gn on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-gn/...)

[tommycli, 2016-06-09 17:30:03]

Patchset #3 (id:40001) has been deleted

[tommycli, 2016-06-09 17:30:14]

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

[commit-bot: I haz the power, 2016-06-09 17:30:42]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2044303004/60001

[tommycli, 2016-06-09 17:46:37]

tommycli@chromium.org changed reviewers:
+ zea%chromium.org@gtempaccount.com

[tommycli, 2016-06-09 17:46:38]

zea: PTAL, this is the GetSetupInProgressHandle() Sync service refactor we
discussed on email a few weeks ago.

Thanks,

Tommy

https://codereview.chromium.org/2044303004/diff/1/components/sync_driver/sync...
File components/sync_driver/sync_service.cc (right):

https://codereview.chromium.org/2044303004/diff/1/components/sync_driver/sync...
components/sync_driver/sync_service.cc:31: base::Unretained(this))));
Will the SyncService ever get destroyed before the UI holding these handles? If
that can happen, we need to use a weak pointer instead of the base::Unretained
raw ptr.

[tommycli, 2016-06-09 17:46:52]

Description was changed from

==========
Sync: Support multiple setup UIs.

Replaces the SetSetupInProgress mechanism with a scoped
GetSetupInProgressHandle mechanism. This has two advantages:

 1. Allows for multiple simultaneously open Setup UIs.
 2. Prevents a buggy UI from blocking Sync indefinitely by forgetting
    to SetSetupInProgress(false).

BUG=563721
==========

to

==========
Sync: Support multiple setup UIs.

Replaces the SetSetupInProgress mechanism with a scoped
GetSetupInProgressHandle mechanism. This has two advantages:

 1. Allows for multiple simultaneously open Setup UIs.
 2. Prevents a buggy UI from blocking Sync indefinitely by forgetting
    to SetSetupInProgress(false).

BUG=563721
==========

[tommycli, 2016-06-09 17:46:52]

tommycli@chromium.org changed reviewers:
+ zea@chromium.org
- zea%chromium.org@gtempaccount.com

[commit-bot: I haz the power, 2016-06-09 18:39:33]

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

[commit-bot: I haz the power, 2016-06-09 18:39:34]

Dry run: This issue passed the CQ dry run.

[maxbogue, 2016-06-09 20:04:11]

maxbogue@chromium.org changed reviewers:
+ maxbogue@chromium.org

[maxbogue, 2016-06-09 20:04:15]

Overall I like this change! A few comments on things, as well as a general
concern that the implementation isn't on PSS.

https://codereview.chromium.org/2044303004/diff/60001/chrome/browser/sync/pro...
File chrome/browser/sync/profile_sync_service_android.cc (right):

https://codereview.chromium.org/2044303004/diff/60001/chrome/browser/sync/pro...
chrome/browser/sync/profile_sync_service_android.cc:189: if (in_progress)
Curly braces please

https://codereview.chromium.org/2044303004/diff/60001/chrome/browser/sync/tes...
File chrome/browser/sync/test/integration/profile_sync_service_harness.cc
(right):

https://codereview.chromium.org/2044303004/diff/60001/chrome/browser/sync/tes...
chrome/browser/sync/test/integration/profile_sync_service_harness.cc:166:
setup_handle_.reset();
Why this extra reset, here and below?

https://codereview.chromium.org/2044303004/diff/60001/chrome/browser/sync/tes...
File chrome/browser/sync/test/integration/two_client_passwords_sync_test.cc
(right):

https://codereview.chromium.org/2044303004/diff/60001/chrome/browser/sync/tes...
chrome/browser/sync/test/integration/two_client_passwords_sync_test.cc:165:
GetSyncService(1)->SetFirstSetupComplete();
Shouldn't this just use ProfileSyncServiceHarness::FinishSyncSetup()?

https://codereview.chromium.org/2044303004/diff/60001/components/browser_sync...
File components/browser_sync/browser/profile_sync_service_startup_unittest.cc
(right):

https://codereview.chromium.org/2044303004/diff/60001/components/browser_sync...
components/browser_sync/browser/profile_sync_service_startup_unittest.cc:295:
sync_service_->SetFirstSetupComplete();
+1, good change.

https://codereview.chromium.org/2044303004/diff/60001/components/sync_driver/...
File components/sync_driver/sync_service.h (right):

https://codereview.chromium.org/2044303004/diff/60001/components/sync_driver/...
components/sync_driver/sync_service.h:49: class SyncSetupInProgressHandle {
Comment explaining the purpose of this class?

https://codereview.chromium.org/2044303004/diff/60001/components/sync_driver/...
components/sync_driver/sync_service.h:192:
std::unique_ptr<SyncSetupInProgressHandle> GetSetupInProgressHandle();
Why is the implementation of this on this class and not ProfileSyncService? That
would let you get rid of the SetSetupInProgress method entirely, and move the
counter to PSS as well.

The split between this class and PSS is pretty weird right now, but if we're
gonna move things here I'd rather it be a separate process.

https://codereview.chromium.org/2044303004/diff/60001/components/sync_driver/...
components/sync_driver/sync_service.h:336: friend class
SyncSetupInProgressHandle;
This shouldn't be necessary should it? That class doesn't actually require
direct access to this one.

https://codereview.chromium.org/2044303004/diff/60001/components/sync_driver/...
components/sync_driver/sync_service.h:338: SyncService() :
outstanding_setup_in_progress_handles_(0) {}
You can just use = 0 on the declaration below now.

https://codereview.chromium.org/2044303004/diff/60001/components/sync_driver/...
components/sync_driver/sync_service.h:341: // This should not be called directly
(except by the Android wrapper).
What Android wrapper?

https://codereview.chromium.org/2044303004/diff/60001/components/sync_driver/...
components/sync_driver/sync_service.h:344: int
outstanding_setup_in_progress_handles_;
Comment please.

[Nicolas Zea, 2016-06-09 23:02:01]

https://codereview.chromium.org/2044303004/diff/1/components/sync_driver/sync...
File components/sync_driver/sync_service.cc (right):

https://codereview.chromium.org/2044303004/diff/1/components/sync_driver/sync...
components/sync_driver/sync_service.cc:31: base::Unretained(this))));
On 2016/06/09 17:46:38, tommycli wrote:
> Will the SyncService ever get destroyed before the UI holding these handles?
If
> that can happen, we need to use a weak pointer instead of the base::Unretained
> raw ptr.

It shouldn't, but either way this seems brittle. I'd prefer using a weak ptr
here, which the profile sync service has access to (going along with Max's
comment about having this logic be in ProfileSyncService).

[tommycli, 2016-06-10 00:04:16]

maxbogue/zea: Thank you for the fast feedback.

I think I was able to implement all of your suggestions below.

Thanks,

Tommy

https://codereview.chromium.org/2044303004/diff/1/components/sync_driver/sync...
File components/sync_driver/sync_service.cc (right):

https://codereview.chromium.org/2044303004/diff/1/components/sync_driver/sync...
components/sync_driver/sync_service.cc:31: base::Unretained(this))));
On 2016/06/09 23:02:01, Nicolas Zea wrote:
> On 2016/06/09 17:46:38, tommycli wrote:
> > Will the SyncService ever get destroyed before the UI holding these handles?
> If
> > that can happen, we need to use a weak pointer instead of the
base::Unretained
> > raw ptr.
> 
> It shouldn't, but either way this seems brittle. I'd prefer using a weak ptr
> here, which the profile sync service has access to (going along with Max's
> comment about having this logic be in ProfileSyncService).

Done.

https://codereview.chromium.org/2044303004/diff/60001/chrome/browser/sync/pro...
File chrome/browser/sync/profile_sync_service_android.cc (right):

https://codereview.chromium.org/2044303004/diff/60001/chrome/browser/sync/pro...
chrome/browser/sync/profile_sync_service_android.cc:189: if (in_progress)
On 2016/06/09 20:04:11, maxbogue wrote:
> Curly braces please

Done.

https://codereview.chromium.org/2044303004/diff/60001/chrome/browser/sync/tes...
File chrome/browser/sync/test/integration/profile_sync_service_harness.cc
(right):

https://codereview.chromium.org/2044303004/diff/60001/chrome/browser/sync/tes...
chrome/browser/sync/test/integration/profile_sync_service_harness.cc:166:
setup_handle_.reset();
On 2016/06/09 20:04:11, maxbogue wrote:
> Why this extra reset, here and below?

Done. It was there to prevent the test to need to call FinishSyncSetup, but I
guess it's simpler to just require the test to call FinishSyncSetup.

https://codereview.chromium.org/2044303004/diff/60001/chrome/browser/sync/tes...
File chrome/browser/sync/test/integration/two_client_passwords_sync_test.cc
(right):

https://codereview.chromium.org/2044303004/diff/60001/chrome/browser/sync/tes...
chrome/browser/sync/test/integration/two_client_passwords_sync_test.cc:165:
GetSyncService(1)->SetFirstSetupComplete();
On 2016/06/09 20:04:13, maxbogue wrote:
> Shouldn't this just use ProfileSyncServiceHarness::FinishSyncSetup()?

Done.

https://codereview.chromium.org/2044303004/diff/60001/components/browser_sync...
File components/browser_sync/browser/profile_sync_service_startup_unittest.cc
(right):

https://codereview.chromium.org/2044303004/diff/60001/components/browser_sync...
components/browser_sync/browser/profile_sync_service_startup_unittest.cc:295:
sync_service_->SetFirstSetupComplete();
On 2016/06/09 20:04:13, maxbogue wrote:
> +1, good change.

Acknowledged.

https://codereview.chromium.org/2044303004/diff/60001/components/sync_driver/...
File components/sync_driver/sync_service.h (right):

https://codereview.chromium.org/2044303004/diff/60001/components/sync_driver/...
components/sync_driver/sync_service.h:49: class SyncSetupInProgressHandle {
On 2016/06/09 20:04:14, maxbogue wrote:
> Comment explaining the purpose of this class?

Done.

https://codereview.chromium.org/2044303004/diff/60001/components/sync_driver/...
components/sync_driver/sync_service.h:192:
std::unique_ptr<SyncSetupInProgressHandle> GetSetupInProgressHandle();
On 2016/06/09 20:04:14, maxbogue wrote:
> Why is the implementation of this on this class and not ProfileSyncService?
That
> would let you get rid of the SetSetupInProgress method entirely, and move the
> counter to PSS as well.
> 
> The split between this class and PSS is pretty weird right now, but if we're
> gonna move things here I'd rather it be a separate process.

Done.

https://codereview.chromium.org/2044303004/diff/60001/components/sync_driver/...
components/sync_driver/sync_service.h:336: friend class
SyncSetupInProgressHandle;
On 2016/06/09 20:04:13, maxbogue wrote:
> This shouldn't be necessary should it? That class doesn't actually require
> direct access to this one.

Done.

https://codereview.chromium.org/2044303004/diff/60001/components/sync_driver/...
components/sync_driver/sync_service.h:338: SyncService() :
outstanding_setup_in_progress_handles_(0) {}
On 2016/06/09 20:04:13, maxbogue wrote:
> You can just use = 0 on the declaration below now.

Done.

https://codereview.chromium.org/2044303004/diff/60001/components/sync_driver/...
components/sync_driver/sync_service.h:341: // This should not be called directly
(except by the Android wrapper).
On 2016/06/09 20:04:14, maxbogue wrote:
> What Android wrapper?

Done. Method removed.

https://codereview.chromium.org/2044303004/diff/60001/components/sync_driver/...
components/sync_driver/sync_service.h:344: int
outstanding_setup_in_progress_handles_;
On 2016/06/09 20:04:13, maxbogue wrote:
> Comment please.

Done.

https://codereview.chromium.org/2044303004/diff/80001/components/sync_driver/...
File components/sync_driver/sync_service.h (right):

https://codereview.chromium.org/2044303004/diff/80001/components/sync_driver/...
components/sync_driver/sync_service.h:57: friend class ::ProfileSyncService;
I added a friend here, since I wanted to ensure that readers knew that they
weren't supposed to construct this class directly.

I did not make this an inner class of SyncService because that makes forward
declarations impossible.

[maxbogue, 2016-06-10 17:28:47]

lgtm w/ a couple little comments; please wait for Nicolas to take a look as
well.

https://codereview.chromium.org/2044303004/diff/80001/components/sync_driver/...
File components/sync_driver/sync_service.h (right):

https://codereview.chromium.org/2044303004/diff/80001/components/sync_driver/...
components/sync_driver/sync_service.h:57: friend class ::ProfileSyncService;
On 2016/06/10 00:04:16, tommycli wrote:
> I added a friend here, since I wanted to ensure that readers knew that they
> weren't supposed to construct this class directly.
> 
> I did not make this an inner class of SyncService because that makes forward
> declarations impossible.

I think I would rather just have the constructor be public and have a comment
about not constructing it directly than have this weird reference to PSS in
sync_driver. Could you also mention the reasoning for not making it an inner
class in its class comment?

https://codereview.chromium.org/2044303004/diff/100001/components/browser_syn...
File components/browser_sync/browser/profile_sync_service.cc (right):

https://codereview.chromium.org/2044303004/diff/100001/components/browser_syn...
components/browser_sync/browser/profile_sync_service.cc:1536:
!startup_controller_->IsSetupInProgress()) {
You could probably DCHECK the !IsSetupInProgress part, no? Would be bad if the
count was 0 and that was true.

https://codereview.chromium.org/2044303004/diff/100001/components/browser_syn...
components/browser_sync/browser/profile_sync_service.cc:2556: if
(!startup_controller_->IsSetupInProgress())
Same as above, would it make sense to DCHECK instead?

[tommycli, 2016-06-10 18:58:21]

maxbogue: Thanks!

https://codereview.chromium.org/2044303004/diff/80001/components/sync_driver/...
File components/sync_driver/sync_service.h (right):

https://codereview.chromium.org/2044303004/diff/80001/components/sync_driver/...
components/sync_driver/sync_service.h:57: friend class ::ProfileSyncService;
On 2016/06/10 17:28:46, maxbogue wrote:
> On 2016/06/10 00:04:16, tommycli wrote:
> > I added a friend here, since I wanted to ensure that readers knew that they
> > weren't supposed to construct this class directly.
> > 
> > I did not make this an inner class of SyncService because that makes forward
> > declarations impossible.
> 
> I think I would rather just have the constructor be public and have a comment
> about not constructing it directly than have this weird reference to PSS in
> sync_driver. Could you also mention the reasoning for not making it an inner
> class in its class comment?

Done.

https://codereview.chromium.org/2044303004/diff/100001/components/browser_syn...
File components/browser_sync/browser/profile_sync_service.cc (right):

https://codereview.chromium.org/2044303004/diff/100001/components/browser_syn...
components/browser_sync/browser/profile_sync_service.cc:1536:
!startup_controller_->IsSetupInProgress()) {
On 2016/06/10 17:28:46, maxbogue wrote:
> You could probably DCHECK the !IsSetupInProgress part, no? Would be bad if the
> count was 0 and that was true.

Done. You're right that would really surprise me.

https://codereview.chromium.org/2044303004/diff/100001/components/browser_syn...
components/browser_sync/browser/profile_sync_service.cc:2556: if
(!startup_controller_->IsSetupInProgress())
On 2016/06/10 17:28:46, maxbogue wrote:
> Same as above, would it make sense to DCHECK instead?

Done.

[Nicolas Zea, 2016-06-13 20:01:00]

lgtm

https://codereview.chromium.org/2044303004/diff/140001/components/browser_syn...
File components/browser_sync/browser/profile_sync_service.cc (right):

https://codereview.chromium.org/2044303004/diff/140001/components/browser_syn...
components/browser_sync/browser/profile_sync_service.cc:2552: void
ProfileSyncService::OnSetupInProgressHandleDestroyed() {
nit: maybe DCHECK that the counter never goes negative?

https://codereview.chromium.org/2044303004/diff/140001/components/browser_syn...
File components/browser_sync/browser/profile_sync_service.h (right):

https://codereview.chromium.org/2044303004/diff/140001/components/browser_syn...
components/browser_sync/browser/profile_sync_service.h:854: // is decremented
back to zero, Sync setup in marked no longer in progress.
nit: "in marked" -> "is marked"

https://codereview.chromium.org/2044303004/diff/140001/components/sync_driver...
File components/sync_driver/sync_service.h (right):

https://codereview.chromium.org/2044303004/diff/140001/components/sync_driver...
components/sync_driver/sync_service.h:56: explicit
SyncSetupInProgressHandle(base::Closure on_destroy);
perhaps make this private, and friend SyncService?

[tommycli, 2016-06-13 20:27:32]

zea: thank you!

https://codereview.chromium.org/2044303004/diff/140001/components/browser_syn...
File components/browser_sync/browser/profile_sync_service.cc (right):

https://codereview.chromium.org/2044303004/diff/140001/components/browser_syn...
components/browser_sync/browser/profile_sync_service.cc:2552: void
ProfileSyncService::OnSetupInProgressHandleDestroyed() {
On 2016/06/13 20:01:00, Nicolas Zea wrote:
> nit: maybe DCHECK that the counter never goes negative?

Done.

https://codereview.chromium.org/2044303004/diff/140001/components/browser_syn...
File components/browser_sync/browser/profile_sync_service.h (right):

https://codereview.chromium.org/2044303004/diff/140001/components/browser_syn...
components/browser_sync/browser/profile_sync_service.h:854: // is decremented
back to zero, Sync setup in marked no longer in progress.
On 2016/06/13 20:01:00, Nicolas Zea wrote:
> nit: "in marked" -> "is marked"

Done.

https://codereview.chromium.org/2044303004/diff/140001/components/sync_driver...
File components/sync_driver/sync_service.h (right):

https://codereview.chromium.org/2044303004/diff/140001/components/sync_driver...
components/sync_driver/sync_service.h:56: explicit
SyncSetupInProgressHandle(base::Closure on_destroy);
On 2016/06/13 20:01:00, Nicolas Zea wrote:
> perhaps make this private, and friend SyncService?

It was like this in Patchset 5, but maxbogue thought that was more confusing. I
would have to friend ::ProfileSyncService, not SyncService, since it's created
in that class.

[tommycli, 2016-06-13 20:31:57]

tommycli@chromium.org changed reviewers:
+ thestig@chromium.org

[tommycli, 2016-06-13 20:31:58]

thestig: May I have an OWNERS stamp on the leftover files on chrome/?

Leftover files:
  chrome/browser/ui/webui/options/sync_setup_handler.cc [21]
  chrome/browser/ui/webui/options/sync_setup_handler.h [21]
  chrome/browser/ui/webui/options/sync_setup_handler_unittest.cc [21]
  chrome/browser/browsing_data/history_counter_browsertest.cc [14]
  chrome/browser/supervised_user/supervised_user_service.cc [12]
  chrome/browser/supervised_user/supervised_user_service.h [12]

[Nicolas Zea, 2016-06-13 20:36:30]

On 2016/06/13 20:27:32, tommycli wrote:
> zea: thank you!
> 
>
https://codereview.chromium.org/2044303004/diff/140001/components/browser_syn...
> File components/browser_sync/browser/profile_sync_service.cc (right):
> 
>
https://codereview.chromium.org/2044303004/diff/140001/components/browser_syn...
> components/browser_sync/browser/profile_sync_service.cc:2552: void
> ProfileSyncService::OnSetupInProgressHandleDestroyed() {
> On 2016/06/13 20:01:00, Nicolas Zea wrote:
> > nit: maybe DCHECK that the counter never goes negative?
> 
> Done.
> 
>
https://codereview.chromium.org/2044303004/diff/140001/components/browser_syn...
> File components/browser_sync/browser/profile_sync_service.h (right):
> 
>
https://codereview.chromium.org/2044303004/diff/140001/components/browser_syn...
> components/browser_sync/browser/profile_sync_service.h:854: // is decremented
> back to zero, Sync setup in marked no longer in progress.
> On 2016/06/13 20:01:00, Nicolas Zea wrote:
> > nit: "in marked" -> "is marked"
> 
> Done.
> 
>
https://codereview.chromium.org/2044303004/diff/140001/components/sync_driver...
> File components/sync_driver/sync_service.h (right):
> 
>
https://codereview.chromium.org/2044303004/diff/140001/components/sync_driver...
> components/sync_driver/sync_service.h:56: explicit
> SyncSetupInProgressHandle(base::Closure on_destroy);
> On 2016/06/13 20:01:00, Nicolas Zea wrote:
> > perhaps make this private, and friend SyncService?
> 
> It was like this in Patchset 5, but maxbogue thought that was more confusing.
I
> would have to friend ::ProfileSyncService, not SyncService, since it's created
> in that class.

Ah, yeah, that is a bit weird. SGTM

[tommycli, 2016-06-13 20:38:01]

zea / maxbogue: thanks for the reviews!

[Lei Zhang, 2016-06-13 20:38:51]

lgtm

https://codereview.chromium.org/2044303004/diff/180001/chrome/browser/supervi...
File chrome/browser/supervised_user/supervised_user_service.h (right):

https://codereview.chromium.org/2044303004/diff/180001/chrome/browser/supervi...
chrome/browser/supervised_user/supervised_user_service.h:416: // Handle
preventing Sync from running until configuration is complete.
Had a little trouble parsing this because Handle is a noun here and not a verb.
Maybe "A handle that prevents sync..." or just "Prevents sync..." ?

https://codereview.chromium.org/2044303004/diff/180001/chrome/browser/supervi...
chrome/browser/supervised_user/supervised_user_service.h:417:
std::unique_ptr<sync_driver::SyncSetupInProgressHandle> setup_handle_;
Maybe a name like |sync_blocker_| better describes what it does?

https://codereview.chromium.org/2044303004/diff/180001/chrome/browser/ui/webu...
File chrome/browser/ui/webui/options/sync_setup_handler.h (right):

https://codereview.chromium.org/2044303004/diff/180001/chrome/browser/ui/webu...
chrome/browser/ui/webui/options/sync_setup_handler.h:160: // Handle preventing
Sync from running until configuration is complete.
Ditto ditto

[tommycli, 2016-06-13 21:08:37]

https://codereview.chromium.org/2044303004/diff/180001/chrome/browser/supervi...
File chrome/browser/supervised_user/supervised_user_service.h (right):

https://codereview.chromium.org/2044303004/diff/180001/chrome/browser/supervi...
chrome/browser/supervised_user/supervised_user_service.h:416: // Handle
preventing Sync from running until configuration is complete.
On 2016/06/13 20:38:51, Lei Zhang wrote:
> Had a little trouble parsing this because Handle is a noun here and not a
verb.
> Maybe "A handle that prevents sync..." or just "Prevents sync..." ?

Done.

https://codereview.chromium.org/2044303004/diff/180001/chrome/browser/supervi...
chrome/browser/supervised_user/supervised_user_service.h:417:
std::unique_ptr<sync_driver::SyncSetupInProgressHandle> setup_handle_;
On 2016/06/13 20:38:51, Lei Zhang wrote:
> Maybe a name like |sync_blocker_| better describes what it does?

Done. I do like that variable name better.

Seems OK to me to leave the type name as it is, WDYT?

https://codereview.chromium.org/2044303004/diff/180001/chrome/browser/ui/webu...
File chrome/browser/ui/webui/options/sync_setup_handler.h (right):

https://codereview.chromium.org/2044303004/diff/180001/chrome/browser/ui/webu...
chrome/browser/ui/webui/options/sync_setup_handler.h:160: // Handle preventing
Sync from running until configuration is complete.
On 2016/06/13 20:38:51, Lei Zhang wrote:
> Ditto ditto

Done.

[Lei Zhang, 2016-06-13 21:19:31]

https://codereview.chromium.org/2044303004/diff/180001/chrome/browser/supervi...
File chrome/browser/supervised_user/supervised_user_service.h (right):

https://codereview.chromium.org/2044303004/diff/180001/chrome/browser/supervi...
chrome/browser/supervised_user/supervised_user_service.h:417:
std::unique_ptr<sync_driver::SyncSetupInProgressHandle> setup_handle_;
On 2016/06/13 21:08:34, tommycli wrote:
> Seems OK to me to leave the type name as it is, WDYT?

I wasn't suggesting changing SyncSetupInProgressHandle. I'm fine with the
renames in patch set 10.

[tommycli, 2016-06-13 21:21:02]

thestig: thanks!

[tommycli, 2016-06-13 21:21:20]

The CQ bit was checked by tommycli@chromium.org

[tommycli, 2016-06-13 21:21:21]

The patchset sent to the CQ was uploaded after l-g-t-m from
maxbogue@chromium.org, zea@chromium.org, thestig@chromium.org
Link to the patchset: https://codereview.chromium.org/2044303004/#ps200001
(title: "rename variable")

[commit-bot: I haz the power, 2016-06-13 21:22:43]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2044303004/200001

[commit-bot: I haz the power, 2016-06-13 22:38:22]

Description was changed from

==========
Sync: Support multiple setup UIs.

Replaces the SetSetupInProgress mechanism with a scoped
GetSetupInProgressHandle mechanism. This has two advantages:

 1. Allows for multiple simultaneously open Setup UIs.
 2. Prevents a buggy UI from blocking Sync indefinitely by forgetting
    to SetSetupInProgress(false).

BUG=563721
==========

to

==========
Sync: Support multiple setup UIs.

Replaces the SetSetupInProgress mechanism with a scoped
GetSetupInProgressHandle mechanism. This has two advantages:

 1. Allows for multiple simultaneously open Setup UIs.
 2. Prevents a buggy UI from blocking Sync indefinitely by forgetting
    to SetSetupInProgress(false).

BUG=563721
==========

[commit-bot: I haz the power, 2016-06-13 22:38:24]

Committed patchset #10 (id:200001)

[commit-bot: I haz the power, 2016-06-13 22:38:50]

CQ bit was unchecked

[commit-bot: I haz the power, 2016-06-13 22:40:10]

Description was changed from

==========
Sync: Support multiple setup UIs.

Replaces the SetSetupInProgress mechanism with a scoped
GetSetupInProgressHandle mechanism. This has two advantages:

 1. Allows for multiple simultaneously open Setup UIs.
 2. Prevents a buggy UI from blocking Sync indefinitely by forgetting
    to SetSetupInProgress(false).

BUG=563721
==========

to

==========
Sync: Support multiple setup UIs.

Replaces the SetSetupInProgress mechanism with a scoped
GetSetupInProgressHandle mechanism. This has two advantages:

 1. Allows for multiple simultaneously open Setup UIs.
 2. Prevents a buggy UI from blocking Sync indefinitely by forgetting
    to SetSetupInProgress(false).

BUG=563721

Committed: https://crrev.com/12c4b7c0a33a0b4d4d2ea0746325e8a371272248
Cr-Commit-Position: refs/heads/master@{#399581}
==========

[commit-bot: I haz the power, 2016-06-13 22:40:11]

Patchset 10 (id:??) landed as
https://crrev.com/12c4b7c0a33a0b4d4d2ea0746325e8a371272248
Cr-Commit-Position: refs/heads/master@{#399581}

[kjellander_chromium, 2016-06-14 08:32:24]

A revert of this CL (patchset #10 id:200001) has been created in
https://codereview.chromium.org/2066493003/ by kjellander@chromium.org.

The reason for reverting is: Breaks browser_tests at Win7 Tests (dbg)(1): 
https://build.chromium.org/p/chromium.win/builders/Win7%20Tests%20%28dbg%29%2...

SupervisedUserServiceTest.ClearOmitOnRegistration
[2608:2460:0613/165536:ERROR:singleton_hwnd.cc(34)] Cannot create windows on
non-UI thread!
[4836:2060:0613/165537:FATAL:dependency_manager.cc(111)] Check failed: false.
Attempted to access a context that was ShutDown(). This is most likely a heap
smasher in progress. After KeyedService::Shutdown() completes, your service MUST
NOT refer to depended services again.
.

[bzanotti, 2016-06-14 09:47:18]

bzanotti@chromium.org changed reviewers:
+ bzanotti@chromium.org

[bzanotti, 2016-06-14 09:47:20]

https://codereview.chromium.org/2044303004/diff/200001/components/browser_syn...
File components/browser_sync/browser/profile_sync_service_mock.h (right):

https://codereview.chromium.org/2044303004/diff/200001/components/browser_syn...
components/browser_sync/browser/profile_sync_service_mock.h:99:
MOCK_METHOD0(OnSetupInProgressHandleDestroyed, void());
This is only a replacement for SetSetupInProgress(false), not
SetSetupInProgress(true). When relanding this change, could you add the
GetSetupInProgressHandle as a mock method?

[tommycli, 2016-06-14 17:11:35]

On 2016/06/14 09:47:20, bzanotti wrote:
>
https://codereview.chromium.org/2044303004/diff/200001/components/browser_syn...
> File components/browser_sync/browser/profile_sync_service_mock.h (right):
> 
>
https://codereview.chromium.org/2044303004/diff/200001/components/browser_syn...
> components/browser_sync/browser/profile_sync_service_mock.h:99:
> MOCK_METHOD0(OnSetupInProgressHandleDestroyed, void());
> This is only a replacement for SetSetupInProgress(false), not
> SetSetupInProgress(true). When relanding this change, could you add the
> GetSetupInProgressHandle as a mock method?

bzanotti: Hi, it's not possible to add a MOCK for GetSetupInProgressHandle, as
that returns a noncopyable smart pointer. Gmock can only mock methods that
return copyable types.

That being said - The error isn't caused by the missing mock, but rather
SupervisedUserService not releasing sync_blocker_ within its Shutdown method.
That was fixed in Patchset 2 here: https://codereview.chromium.org/2064183002/#

Thanks!

[bzanotti, 2016-06-15 08:45:51]

On 2016/06/14 17:11:35, tommycli wrote:
> bzanotti: Hi, it's not possible to add a MOCK for GetSetupInProgressHandle, as
> that returns a noncopyable smart pointer. Gmock can only mock methods that
> return copyable types.
> 
> That being said - The error isn't caused by the missing mock, but rather
> SupervisedUserService not releasing sync_blocker_ within its Shutdown method.
> That was fixed in Patchset 2 here: https://codereview.chromium.org/2064183002/

Sorry, I didn't add the context, but I was not referring to this failure. This
was actually also breaking some unittests in Chrome iOS (downstream, so
invisible from Chromium) where we mock SetSetupInProgress(true).

I now understand why this can't be done, and we'll fix our issue another way.
Thanks!