[Sync] Services can now always assume processor exists.

[Sync] Services can now always assume processor exists.

BUG=653219
Committed: https://crrev.com/345782f64469290397bb2a80087dcfe3cf09ab43
Cr-Commit-Position: refs/heads/master@{#425165}

[skym, 2016-10-12 19:14:01]

Patchset #1 (id:1) has been deleted

[skym, 2016-10-12 19:14:06]

Patchset #1 (id:20001) has been deleted

[skym, 2016-10-12 19:15:03]

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

[commit-bot: I haz the power, 2016-10-12 19:15:22]

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

[skym, 2016-10-12 19:16:19]

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

[skym, 2016-10-12 19:16:20]

PTAL

[commit-bot: I haz the power, 2016-10-12 19:30:11]

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

[commit-bot: I haz the power, 2016-10-12 19:30:12]

Dry run: Exceeded global retry quota

[maxbogue, 2016-10-12 23:40:35]

https://codereview.chromium.org/2406163006/diff/40001/components/sync/core/sh...
File components/sync/core/shared_model_type_processor.cc (right):

https://codereview.chromium.org/2406163006/diff/40001/components/sync/core/sh...
components/sync/core/shared_model_type_processor.cc:161:
DCHECK(CalledOnValidThread());
Should probably DCHECK(IsTrackingMetadata()); here, no?

https://codereview.chromium.org/2406163006/diff/40001/components/sync/core/sh...
File components/sync/core/shared_model_type_processor_unittest.cc (right):

https://codereview.chromium.org/2406163006/diff/40001/components/sync/core/sh...
components/sync/core/shared_model_type_processor_unittest.cc:765:
service()->GenerateSpecifics(kKey1, kValue1));
Huh. This one is not obviously related to the service; it's just a static
function on FakeModelTypeService... I'd make a helper function for it, or even
an anonymous one. Or can you just do "using
FakeModelTypeService::GenerateSpecifics;"? I have no idea if that works.

https://codereview.chromium.org/2406163006/diff/40001/components/sync/core/sh...
components/sync/core/shared_model_type_processor_unittest.cc:1033:
service()->GenerateEntityData(kKey1, kValue3)));
Same with GenerateEntityData.

https://codereview.chromium.org/2406163006/diff/40001/components/sync/device_...
File components/sync/device_info/device_info_service_unittest.cc (right):

https://codereview.chromium.org/2406163006/diff/40001/components/sync/device_...
components/sync/device_info/device_info_service_unittest.cc:182:
std::unique_ptr<RecordingModelTypeChangeProcessor> processor =
You can just do "auto processor". That's being encouraged across the codebase in
combination with MakeUnique, from what I've seen.

https://codereview.chromium.org/2406163006/diff/40001/components/sync/device_...
components/sync/device_info/device_info_service_unittest.cc:492: EXPECT_EQ(1,
change_count());
I'm confused... why did this change?

https://codereview.chromium.org/2406163006/diff/40001/components/sync/model/f...
File components/sync/model/fake_model_type_service.cc (left):

https://codereview.chromium.org/2406163006/diff/40001/components/sync/model/f...
components/sync/model/fake_model_type_service.cc:137: 
Why'd you remove this line? Put it back?

https://codereview.chromium.org/2406163006/diff/40001/components/sync/model/f...
File components/sync/model/fake_model_type_service.cc (right):

https://codereview.chromium.org/2406163006/diff/40001/components/sync/model/f...
components/sync/model/fake_model_type_service.cc:140:
db_(base::MakeUnique<Store>()) {}
It's probably better to just do new Store() in this case? I dunno, seems
wasteful to make a unique_ptr just to swap it into another one.

https://codereview.chromium.org/2406163006/diff/40001/components/sync/model/f...
File components/sync/model/fake_model_type_service.h (right):

https://codereview.chromium.org/2406163006/diff/40001/components/sync/model/f...
components/sync/model/fake_model_type_service.h:126: Store* db() { return
db_.get(); }
Out of curiosity, why change the signature instead of returning *db_?

https://codereview.chromium.org/2406163006/diff/40001/components/sync/model/m...
File components/sync/model/model_type_change_processor.h (right):

https://codereview.chromium.org/2406163006/diff/40001/components/sync/model/m...
components/sync/model/model_type_change_processor.h:66: // Returns a boolean
representing if the processor's metadata is currently up
In general, I find "if <foo>, or not." is better written as "whether <foo>."

https://codereview.chromium.org/2406163006/diff/40001/components/sync/model/m...
File components/sync/model/model_type_service_unittest.cc (right):

https://codereview.chromium.org/2406163006/diff/40001/components/sync/model/m...
components/sync/model/model_type_service_unittest.cc:121:
EXPECT_TRUE(service()->processor_disable_sync_called());
I'd move this line right under 117.

[skym, 2016-10-13 17:56:58]

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

[commit-bot: I haz the power, 2016-10-13 17:57:27]

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

[commit-bot: I haz the power, 2016-10-13 18:17:19]

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

[commit-bot: I haz the power, 2016-10-13 18:17:19]

Dry run: Try jobs failed on following builders:
  linux_chromium_clobber_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[skym, 2016-10-13 18:20:55]

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

[commit-bot: I haz the power, 2016-10-13 18:21:28]

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

[commit-bot: I haz the power, 2016-10-13 19:12:37]

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

[commit-bot: I haz the power, 2016-10-13 19:12:38]

Dry run: This issue passed the CQ dry run.

[skym, 2016-10-13 19:21:38]

Fixes for Max and trybots pass.

https://codereview.chromium.org/2406163006/diff/40001/components/sync/core/sh...
File components/sync/core/shared_model_type_processor.cc (right):

https://codereview.chromium.org/2406163006/diff/40001/components/sync/core/sh...
components/sync/core/shared_model_type_processor.cc:161:
DCHECK(CalledOnValidThread());
On 2016/10/12 23:40:35, maxbogue wrote:
> Should probably DCHECK(IsTrackingMetadata()); here, no?

Really? This doesn't seem like a safe assumption. Yes, metadata will have
loaded, but what if this is the initial sync and we error on connecting? Or
don't have an internet connection and GU never succeeds?

https://codereview.chromium.org/2406163006/diff/40001/components/sync/core/sh...
File components/sync/core/shared_model_type_processor_unittest.cc (right):

https://codereview.chromium.org/2406163006/diff/40001/components/sync/core/sh...
components/sync/core/shared_model_type_processor_unittest.cc:765:
service()->GenerateSpecifics(kKey1, kValue1));
On 2016/10/12 23:40:35, maxbogue wrote:
> Huh. This one is not obviously related to the service; it's just a static
> function on FakeModelTypeService... I'd make a helper function for it, or even
> an anonymous one. Or can you just do "using
> FakeModelTypeService::GenerateSpecifics;"? I have no idea if that works.

Oooh, you're right, just calling the static function is way better.
Unfortunately using doesn't work with static functions as far as I can tell.
Will create wrapping functions to keep these small.

https://codereview.chromium.org/2406163006/diff/40001/components/sync/core/sh...
components/sync/core/shared_model_type_processor_unittest.cc:1033:
service()->GenerateEntityData(kKey1, kValue3)));
On 2016/10/12 23:40:35, maxbogue wrote:
> Same with GenerateEntityData.

Done.

https://codereview.chromium.org/2406163006/diff/40001/components/sync/device_...
File components/sync/device_info/device_info_service_unittest.cc (right):

https://codereview.chromium.org/2406163006/diff/40001/components/sync/device_...
components/sync/device_info/device_info_service_unittest.cc:182:
std::unique_ptr<RecordingModelTypeChangeProcessor> processor =
On 2016/10/12 23:40:35, maxbogue wrote:
> You can just do "auto processor". That's being encouraged across the codebase
in
> combination with MakeUnique, from what I've seen.

Done.

https://codereview.chromium.org/2406163006/diff/40001/components/sync/device_...
components/sync/device_info/device_info_service_unittest.cc:492: EXPECT_EQ(1,
change_count());
On 2016/10/12 23:40:35, maxbogue wrote:
> I'm confused... why did this change?

So what was happening originally was that the service wouldn't reconcile until
it had a proc, and it refused to drive the processor of creating a proc.
However, now that we always have a proc, it always performs reconcile after
loading metadata. There's a big fat comment in device_info_service.cc explaining
that we don't make sure the processor is tracking our changes yet, and we don't
care. Although it'd probably be ideal to change that, I didn't want to do it in
this CL. See
https://cs.chromium.org/chromium/src/components/sync/device_info/device_info_...

https://codereview.chromium.org/2406163006/diff/40001/components/sync/model/f...
File components/sync/model/fake_model_type_service.cc (left):

https://codereview.chromium.org/2406163006/diff/40001/components/sync/model/f...
components/sync/model/fake_model_type_service.cc:137: 
On 2016/10/12 23:40:35, maxbogue wrote:
> Why'd you remove this line? Put it back?

Whoops. Done.

https://codereview.chromium.org/2406163006/diff/40001/components/sync/model/f...
File components/sync/model/fake_model_type_service.cc (right):

https://codereview.chromium.org/2406163006/diff/40001/components/sync/model/f...
components/sync/model/fake_model_type_service.cc:140:
db_(base::MakeUnique<Store>()) {}
On 2016/10/12 23:40:35, maxbogue wrote:
> It's probably better to just do new Store() in this case? I dunno, seems
> wasteful to make a unique_ptr just to swap it into another one.

What? The 'another one' is empty. I don't understand.

https://codereview.chromium.org/2406163006/diff/40001/components/sync/model/f...
File components/sync/model/fake_model_type_service.h (right):

https://codereview.chromium.org/2406163006/diff/40001/components/sync/model/f...
components/sync/model/fake_model_type_service.h:126: Store* db() { return
db_.get(); }
On 2016/10/12 23:40:35, maxbogue wrote:
> Out of curiosity, why change the signature instead of returning *db_?

Because I wanted it to be modifiable, and my understanding is that we don't like
non-const refs. But it looks like I actually don't need it to be modifiable
anymore, reverting.

https://codereview.chromium.org/2406163006/diff/40001/components/sync/model/m...
File components/sync/model/model_type_change_processor.h (right):

https://codereview.chromium.org/2406163006/diff/40001/components/sync/model/m...
components/sync/model/model_type_change_processor.h:66: // Returns a boolean
representing if the processor's metadata is currently up
On 2016/10/12 23:40:35, maxbogue wrote:
> In general, I find "if <foo>, or not." is better written as "whether <foo>."

Done.

https://codereview.chromium.org/2406163006/diff/40001/components/sync/model/m...
File components/sync/model/model_type_service_unittest.cc (right):

https://codereview.chromium.org/2406163006/diff/40001/components/sync/model/m...
components/sync/model/model_type_service_unittest.cc:121:
EXPECT_TRUE(service()->processor_disable_sync_called());
On 2016/10/12 23:40:35, maxbogue wrote:
> I'd move this line right under 117.

Why? I like this formatting because it's broken into precondition checks ->
actions -> post condition checks. Since precondition and actions are so small,
they're grouped. Post conditions are also grouped.

[maxbogue, 2016-10-13 20:11:05]

lgtm! Great to see things getting simpler for services :D

https://codereview.chromium.org/2406163006/diff/40001/components/sync/core/sh...
File components/sync/core/shared_model_type_processor.cc (right):

https://codereview.chromium.org/2406163006/diff/40001/components/sync/core/sh...
components/sync/core/shared_model_type_processor.cc:161:
DCHECK(CalledOnValidThread());
On 2016/10/13 19:21:38, skym wrote:
> On 2016/10/12 23:40:35, maxbogue wrote:
> > Should probably DCHECK(IsTrackingMetadata()); here, no?
> 
> Really? This doesn't seem like a safe assumption. Yes, metadata will have
> loaded, but what if this is the initial sync and we error on connecting? Or
> don't have an internet connection and GU never succeeds?

Err crap that's what I meant. You should DCHECK(is_metadata_loaded_) here.

https://codereview.chromium.org/2406163006/diff/40001/components/sync/model/m...
File components/sync/model/model_type_service_unittest.cc (right):

https://codereview.chromium.org/2406163006/diff/40001/components/sync/model/m...
components/sync/model/model_type_service_unittest.cc:121:
EXPECT_TRUE(service()->processor_disable_sync_called());
On 2016/10/13 19:21:38, skym wrote:
> On 2016/10/12 23:40:35, maxbogue wrote:
> > I'd move this line right under 117.
> 
> Why? I like this formatting because it's broken into precondition checks ->
> actions -> post condition checks. Since precondition and actions are so small,
> they're grouped. Post conditions are also grouped.

My suggestion was for symmetry and because the comment describes the whole block
except this line. Either way is fine.

https://codereview.chromium.org/2406163006/diff/100001/components/sync/model/...
File components/sync/model/fake_model_type_service.h (right):

https://codereview.chromium.org/2406163006/diff/100001/components/sync/model/...
components/sync/model/fake_model_type_service.h:126: const Store& db() { return
*db_.get(); }
super nit: Can't you just do *db_?

https://codereview.chromium.org/2406163006/diff/100001/components/sync/model/...
File components/sync/model/model_type_service_unittest.cc (right):

https://codereview.chromium.org/2406163006/diff/100001/components/sync/model/...
components/sync/model/model_type_service_unittest.cc:20: // A mock MTCP that
lets us know when DisableSync is called.
nit: Update the comment since it does more now?

https://codereview.chromium.org/2406163006/diff/100001/components/sync/model_...
File components/sync/model_impl/shared_model_type_processor_unittest.cc (right):

https://codereview.chromium.org/2406163006/diff/100001/components/sync/model_...
components/sync/model_impl/shared_model_type_processor_unittest.cc:97: //
FakeModelTypeService overrides.
nit: I'd do a line after this and between each function to make it more readable
since they're actual definitions.

[skym, 2016-10-13 20:36:28]

https://codereview.chromium.org/2406163006/diff/40001/components/sync/core/sh...
File components/sync/core/shared_model_type_processor.cc (right):

https://codereview.chromium.org/2406163006/diff/40001/components/sync/core/sh...
components/sync/core/shared_model_type_processor.cc:161:
DCHECK(CalledOnValidThread());
On 2016/10/13 20:11:04, maxbogue wrote:
> On 2016/10/13 19:21:38, skym wrote:
> > On 2016/10/12 23:40:35, maxbogue wrote:
> > > Should probably DCHECK(IsTrackingMetadata()); here, no?
> > 
> > Really? This doesn't seem like a safe assumption. Yes, metadata will have
> > loaded, but what if this is the initial sync and we error on connecting? Or
> > don't have an internet connection and GU never succeeds?
> 
> Err crap that's what I meant. You should DCHECK(is_metadata_loaded_) here.

Done.

https://codereview.chromium.org/2406163006/diff/40001/components/sync/model/m...
File components/sync/model/model_type_service_unittest.cc (right):

https://codereview.chromium.org/2406163006/diff/40001/components/sync/model/m...
components/sync/model/model_type_service_unittest.cc:121:
EXPECT_TRUE(service()->processor_disable_sync_called());
On 2016/10/13 20:11:04, maxbogue wrote:
> On 2016/10/13 19:21:38, skym wrote:
> > On 2016/10/12 23:40:35, maxbogue wrote:
> > > I'd move this line right under 117.
> > 
> > Why? I like this formatting because it's broken into precondition checks ->
> > actions -> post condition checks. Since precondition and actions are so
small,
> > they're grouped. Post conditions are also grouped.
> 
> My suggestion was for symmetry and because the comment describes the whole
block
> except this line. Either way is fine.

Good point, added a space after the EXPECT_TRUE so it's clear that the comment
isn't applying to the rest of the lines.

https://codereview.chromium.org/2406163006/diff/100001/components/sync/model/...
File components/sync/model/fake_model_type_service.h (right):

https://codereview.chromium.org/2406163006/diff/100001/components/sync/model/...
components/sync/model/fake_model_type_service.h:126: const Store& db() { return
*db_.get(); }
On 2016/10/13 20:11:05, maxbogue wrote:
> super nit: Can't you just do *db_?

You're right, that is much better. Done!

https://codereview.chromium.org/2406163006/diff/100001/components/sync/model/...
File components/sync/model/model_type_service_unittest.cc (right):

https://codereview.chromium.org/2406163006/diff/100001/components/sync/model/...
components/sync/model/model_type_service_unittest.cc:20: // A mock MTCP that
lets us know when DisableSync is called.
On 2016/10/13 20:11:05, maxbogue wrote:
> nit: Update the comment since it does more now?

Done.

https://codereview.chromium.org/2406163006/diff/100001/components/sync/model/...
components/sync/model/model_type_service_unittest.cc:43: base::Closure
disabled_callback_;
Whoops, I forgot to add a comment about why we use a callback here instead just
instance state. Done.

https://codereview.chromium.org/2406163006/diff/100001/components/sync/model_...
File components/sync/model_impl/shared_model_type_processor_unittest.cc (right):

https://codereview.chromium.org/2406163006/diff/100001/components/sync/model_...
components/sync/model_impl/shared_model_type_processor_unittest.cc:97: //
FakeModelTypeService overrides.
On 2016/10/13 20:11:05, maxbogue wrote:
> nit: I'd do a line after this and between each function to make it more
readable
> since they're actual definitions.

Done.

[skym, 2016-10-13 20:36:32]

The CQ bit was checked by skym@chromium.org

[skym, 2016-10-13 20:36:32]

The patchset sent to the CQ was uploaded after l-g-t-m from
maxbogue@chromium.org
Link to the patchset: https://codereview.chromium.org/2406163006/#ps120001
(title: "Updates for Max.")

[commit-bot: I haz the power, 2016-10-13 20:36:53]

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

[commit-bot: I haz the power, 2016-10-13 21:23:08]

Committed patchset #5 (id:120001)

[commit-bot: I haz the power, 2016-10-13 21:26:11]

Description was changed from

==========
[Sync] Services can now always assume processor exists.

BUG=653219
==========

to

==========
[Sync] Services can now always assume processor exists.

BUG=653219

Committed: https://crrev.com/345782f64469290397bb2a80087dcfe3cf09ab43
Cr-Commit-Position: refs/heads/master@{#425165}
==========

[commit-bot: I haz the power, 2016-10-13 21:26:12]

Patchset 5 (id:??) landed as
https://crrev.com/345782f64469290397bb2a80087dcfe3cf09ab43
Cr-Commit-Position: refs/heads/master@{#425165}