[Sync] Enable adding notifier observers from ProfileSyncService

[Sync] Enable adding notifier observers from ProfileSyncService

Plumb the registrations from the UI thread to the sync thread, and plumb the invalidations back.

Add ObjectIdSet <-> ObjectIdPayloadMap conversions.

Rename syncapi_unittest.cc to sync_manager_impl_unittest.cc.

BUG=137087
TEST=


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

[akalin, 2012-07-18 01:19:29]

+tim, +nyquist, +dcheng for initial comments re. the interface. (This isn't
quite ready for a full review yet.)

What do you all think?

[akalin, 2012-07-18 18:48:02]

ping!

[tim (not reviewing), 2012-07-18 19:04:32]

On 2012/07/18 18:48:02, akalin wrote:
> ping!

This is for Chrome to Mobile, right? Or any use case that is fine only working
if sync is enabled?  Seems like a decent interface. Are we cool wiring these
observers up to all sources?

[akalin, 2012-07-18 19:18:10]

On 2012/07/18 19:04:32, timsteele wrote:
> On 2012/07/18 18:48:02, akalin wrote:
> > ping!
> 
> This is for Chrome to Mobile, right? Or any use case that is fine only working
> if sync is enabled?  Seems like a decent interface. Are we cool wiring these
> observers up to all sources?

Yes, this is for C2M or anything else that is okay with being tied to sync.  I
should mention that this will be refactored out once we pull the Invalidations
API out of sync (I will add comments to that effect).

Daniel's pending change will let observers pass in what they want to listen to. 
I'll probably wait for his change to land and then revise this CL.

[akalin, 2012-07-18 20:26:30]

On 2012/07/18 19:18:10, akalin wrote:
> On 2012/07/18 19:04:32, timsteele wrote:
> > On 2012/07/18 18:48:02, akalin wrote:
> > > ping!
> > 
> > This is for Chrome to Mobile, right? Or any use case that is fine only
working
> > if sync is enabled?  Seems like a decent interface. Are we cool wiring these
> > observers up to all sources?
> 
> Yes, this is for C2M or anything else that is okay with being tied to sync.  I
> should mention that this will be refactored out once we pull the Invalidations
> API out of sync (I will add comments to that effect).
> 
> Daniel's pending change will let observers pass in what they want to listen
to. 
> I'll probably wait for his change to land and then revise this CL.

Tommy, any comments?  I was hoping to catch you before you went out of town,
since this work is mainly for C2P. :)

[nyquist, 2012-07-19 00:12:52]

On 2012/07/18 20:26:30, akalin wrote:
> On 2012/07/18 19:18:10, akalin wrote:
> > On 2012/07/18 19:04:32, timsteele wrote:
> > > On 2012/07/18 18:48:02, akalin wrote:
> > > > ping!
> > > 
> > > This is for Chrome to Mobile, right? Or any use case that is fine only
> working
> > > if sync is enabled?  Seems like a decent interface. Are we cool wiring
these
> > > observers up to all sources?
> > 
> > Yes, this is for C2M or anything else that is okay with being tied to sync. 
I
> > should mention that this will be refactored out once we pull the
Invalidations
> > API out of sync (I will add comments to that effect).
> > 
> > Daniel's pending change will let observers pass in what they want to listen
> to. 
> > I'll probably wait for his change to land and then revise this CL.
> 
> Tommy, any comments?  I was hoping to catch you before you went out of town,
> since this work is mainly for C2P. :)

This API makes sense to me.

[akalin, 2012-07-21 01:11:13]

msw@, I updated this patch set on http://codereview.chromium.org/10702074/ so
you should be able to patch those two in (or just this one, when the other one
lands).

Also, feel free to review, although I still need to clean this up and add tests.

[akalin, 2012-07-21 01:23:45]

+tim for review also (still need to write tests)

[tim (not reviewing), 2012-07-26 22:53:53]

On 2012/07/21 01:23:45, akalin wrote:
> +tim for review also (still need to write tests)

Well, wait now I'm confused.  Patch set 1 made sense.  But if I want to register
for my new invalidation object, how do I do that?  I somehow need to know the
full set of ids already registered?

[akalin, 2012-07-26 23:18:19]

Yeah you need to keep track of those yourself. But you need to do so
anyway, to unregister on destruction.
On Jul 26, 2012 3:53 PM, <tim@chromium.org> wrote:

> On 2012/07/21 01:23:45, akalin wrote:
>
>> +tim for review also (still need to write tests)
>>
>
> Well, wait now I'm confused.  Patch set 1 made sense.  But if I want to
> register
> for my new invalidation object, how do I do that?  I somehow need to know
> the
> full set of ids already registered?
>
>
https://chromiumcodereview.**appspot.com/10805002/<https://chromiumcodereview...
>

[akalin, 2012-07-26 23:18:45]

Wait no you don't. Brain fart...
On Jul 26, 2012 4:17 PM, akalin@chromium.org wrote:

> Yeah you need to keep track of those yourself. But you need to do so
> anyway, to unregister on destruction.
> On Jul 26, 2012 3:53 PM, <tim@chromium.org> wrote:
>
>> On 2012/07/21 01:23:45, akalin wrote:
>>
>>> +tim for review also (still need to write tests)
>>>
>>
>> Well, wait now I'm confused.  Patch set 1 made sense.  But if I want to
>> register
>> for my new invalidation object, how do I do that?  I somehow need to know
>> the
>> full set of ids already registered?
>>
>>
https://chromiumcodereview.**appspot.com/10805002/<https://chromiumcodereview...
>>
>

[tim (not reviewing), 2012-07-26 23:41:08]

On 2012/07/26 23:18:45, akalin wrote:
> Wait no you don't. Brain fart...
> On Jul 26, 2012 4:17 PM, mailto:akalin@chromium.org wrote:
> 
> > Yeah you need to keep track of those yourself. But you need to do so
> > anyway, to unregister on destruction.
> > On Jul 26, 2012 3:53 PM, <mailto:tim@chromium.org> wrote:
> >
> >> On 2012/07/21 01:23:45, akalin wrote:
> >>
> >>> +tim for review also (still need to write tests)
> >>>
> >>
> >> Well, wait now I'm confused.  Patch set 1 made sense.  But if I want to
> >> register
> >> for my new invalidation object, how do I do that?  I somehow need to know
> >> the
> >> full set of ids already registered?
> >>
> >>
>
https://chromiumcodereview.**appspot.com/10805002/%3Chttps://chromiumcoderevi...>
> >>
> >

So, what?  If there are two separate clients (like say, apps and c2p), it would
be unfortunate if apps had to know the c2p ids and vice versa (etc at higher
dimensions).

[akalin, 2012-07-27 00:47:20]

On 2012/07/26 23:41:08, timsteele wrote:
> So, what?  If there are two separate clients (like say, apps and c2p), it
would
> be unfortunate if apps had to know the c2p ids and vice versa (etc at higher
> dimensions).

Hunh?  Maybe you're misunderstanding -- each client will need to know only its
own set of object ids to register for.

[akalin, 2012-07-31 21:23:53]

Okay, I added tests.  This is now ready for review.

PTAL.

[akalin, 2012-08-01 17:24:56]

On 2012/07/31 21:23:53, akalin wrote:
> Okay, I added tests.  This is now ready for review.
> 
> PTAL.

Ping!

[tim (not reviewing), 2012-08-01 21:52:52]

still looking

http://codereview.chromium.org/10805002/diff/14002/chrome/browser/sync/profil...
File chrome/browser/sync/profile_sync_service.cc (right):

http://codereview.chromium.org/10805002/diff/14002/chrome/browser/sync/profil...
chrome/browser/sync/profile_sync_service.cc:426: // |initialization mode).
What would it take to get rid of synchronous initialization mode, again?

http://codereview.chromium.org/10805002/diff/14002/sync/internal_api/sync_man...
File sync/internal_api/sync_manager_impl.cc (right):

http://codereview.chromium.org/10805002/diff/14002/sync/internal_api/sync_man...
sync/internal_api/sync_manager_impl.cc:742: void
SyncManagerImpl::UpdateRegisteredIds(
It'd be nice, maybe just while it's still explicitly plumbed through sync this
way, to call this RegisteredInvalidationIds or something, so it's clear what
"Ids" we're talking about.

[akalin, 2012-08-02 08:05:43]

PTAL

https://chromiumcodereview.appspot.com/10805002/diff/14002/chrome/browser/syn...
File chrome/browser/sync/profile_sync_service.cc (right):

https://chromiumcodereview.appspot.com/10805002/diff/14002/chrome/browser/syn...
chrome/browser/sync/profile_sync_service.cc:426: // |initialization mode).
On 2012/08/01 21:52:52, timsteele wrote:
> What would it take to get rid of synchronous initialization mode, again?

Actually, it might be easy now.  I think I put that in way back when when we had
the core thread and stuff.

https://chromiumcodereview.appspot.com/10805002/diff/14002/sync/internal_api/...
File sync/internal_api/sync_manager_impl.cc (right):

https://chromiumcodereview.appspot.com/10805002/diff/14002/sync/internal_api/...
sync/internal_api/sync_manager_impl.cc:742: void
SyncManagerImpl::UpdateRegisteredIds(
On 2012/08/01 21:52:52, timsteele wrote:
> It'd be nice, maybe just while it's still explicitly plumbed through sync this
> way, to call this RegisteredInvalidationIds or something, so it's clear what
> "Ids" we're talking about.

Done.  Renamed to UpdateRegisteredInvalidationIds.

[tim (not reviewing), 2012-08-02 17:43:23]

couple nits, LGTM once they're addressed

https://chromiumcodereview.appspot.com/10805002/diff/18025/chrome/browser/syn...
File chrome/browser/sync/profile_sync_service.cc (right):

https://chromiumcodereview.appspot.com/10805002/diff/18025/chrome/browser/syn...
chrome/browser/sync/profile_sync_service.cc:426: // initialization mode).
Well, if we can get rid of this, could you file a bug to go along with your
todo, w/ a brief explanation?  Might be a nice starter bug if you don't get to
it.

https://chromiumcodereview.appspot.com/10805002/diff/18025/chrome/browser/syn...
File chrome/browser/sync/profile_sync_service_unittest.cc (right):

https://chromiumcodereview.appspot.com/10805002/diff/18025/chrome/browser/syn...
chrome/browser/sync/profile_sync_service_unittest.cc:96: int
expected_create_dtm_calls,
Oh boy. 1 and 0 :| An enum CREATE_DTM, DONT_CREATE_DTM would be far more clear,
but frankly, we probably don't care much beyond the fact that the DTM is created
if needed (it's arguably brittle to expect more, imo?). 

Instead of the WillOnce / Times(0) call, I think we should mimic what a "fake"
would do, and say 

   ON_CALL(*factory, CreateDataTypeManager()).
        WillByDefault(ReturnNewDataTypeManager());

... and, wait for it, remove the param to Start...!!!

[akalin, 2012-08-02 22:06:00]

Addressed all comments.

Committing via commit-bot@.  msw@, if you have any concerns about the API, let
me know and I can address them in a follow-up.

https://chromiumcodereview.appspot.com/10805002/diff/18025/chrome/browser/syn...
File chrome/browser/sync/profile_sync_service.cc (right):

https://chromiumcodereview.appspot.com/10805002/diff/18025/chrome/browser/syn...
chrome/browser/sync/profile_sync_service.cc:426: // initialization mode).
On 2012/08/02 17:43:23, timsteele wrote:
> Well, if we can get rid of this, could you file a bug to go along with your
> todo, w/ a brief explanation?  Might be a nice starter bug if you don't get to
> it.

Done.

https://chromiumcodereview.appspot.com/10805002/diff/18025/chrome/browser/syn...
File chrome/browser/sync/profile_sync_service_unittest.cc (right):

https://chromiumcodereview.appspot.com/10805002/diff/18025/chrome/browser/syn...
chrome/browser/sync/profile_sync_service_unittest.cc:96: int
expected_create_dtm_calls,
On 2012/08/02 17:43:23, timsteele wrote:
> Oh boy. 1 and 0 :| An enum CREATE_DTM, DONT_CREATE_DTM would be far more
clear,
> but frankly, we probably don't care much beyond the fact that the DTM is
created
> if needed (it's arguably brittle to expect more, imo?). 
> 
> Instead of the WillOnce / Times(0) call, I think we should mimic what a "fake"
> would do, and say 
> 
>    ON_CALL(*factory, CreateDataTypeManager()).
>         WillByDefault(ReturnNewDataTypeManager());
> 
> ... and, wait for it, remove the param to Start...!!!

Done.

[commit-bot: I haz the power, 2012-08-02 22:06:51]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/akalin@chromium.org/10805002/27001

[commit-bot: I haz the power, 2012-08-02 22:07:16]

Presubmit check for 10805002-27001 failed and returned exit status 1.

Running presubmit commit checks ...

** Presubmit ERRORS **
Missing LGTM from an OWNER for files in these directories:
    chrome

Presubmit checks took 2.3s to calculate.

Was the presubmit check useful? Please send feedback & hate mail to
maruel@chromium.org!

[Lei Zhang, 2012-08-02 22:24:16]

chrome/*.gyp* changes LGTM, though I hate the idea of making unit_tests bigger.

[akalin, 2012-08-02 22:26:48]

On 2012/08/02 22:24:16, Lei Zhang wrote:
> chrome/*.gyp* changes LGTM, though I hate the idea of making unit_tests
bigger.

Thanks!

It actually already indirectly depends on it, so making it a direct dep wouldn't
make it bigger, right?

[commit-bot: I haz the power, 2012-08-02 22:27:06]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/akalin@chromium.org/10805002/27001

[commit-bot: I haz the power, 2012-08-02 23:49:57]

Change committed as 149747

[msw, 2012-08-03 05:14:21]

I'm sorry I didn't review in time, but I mostly just have nits / style comments;
as I'm not terribly familiar with this code. The tests are much appreciated.

If anything, I'd say that it's a little odd to group the notification ids
everywhere. If they're independently mapped to observers it seems a little
simpler to register / unregister them individually, etc. But that's a fairly
minor point, and you may have a good reason for your approach.

Thanks for this work, I'll bug you with any trouble integrating Chrome To Mobile
:)

https://chromiumcodereview.appspot.com/10805002/diff/27001/chrome/browser/syn...
File chrome/browser/sync/profile_sync_service.h (right):

https://chromiumcodereview.appspot.com/10805002/diff/27001/chrome/browser/syn...
chrome/browser/sync/profile_sync_service.h:554: // Updates the set of ObjectIds
associated with a given
nit: break lines at sentences or ~80 chars.

https://chromiumcodereview.appspot.com/10805002/diff/27001/chrome/browser/syn...
File chrome/browser/sync/profile_sync_service_unittest.cc (right):

https://chromiumcodereview.appspot.com/10805002/diff/27001/chrome/browser/syn...
chrome/browser/sync/profile_sync_service_unittest.cc:399:
backend->EmitOnIncomingNotification(payloads, syncer::REMOTE_NOTIFICATION);
nit: It seems odd to test emitting notifications after disabling... (same below)

https://chromiumcodereview.appspot.com/10805002/diff/27001/chrome/browser/syn...
chrome/browser/sync/profile_sync_service_unittest.cc:417: EXPECT_CALL(observer,
OnNotificationsEnabled());
Perhaps I'm confused, but this test doesn't appear to actually ensure that the
invalidation messages are received by the observer. Shouldn't there be an
"EXPECT_CALL(observer, OnIncomingNotification(...)"?

https://chromiumcodereview.appspot.com/10805002/diff/27001/sync/internal_api/...
File sync/internal_api/public/sync_manager.h (right):

https://chromiumcodereview.appspot.com/10805002/diff/27001/sync/internal_api/...
sync/internal_api/public/sync_manager.h:32: namespace syncer {
nit: if the syncer namespace contains more than just these forward decls, it
should probably have a blank line directly below it.

https://chromiumcodereview.appspot.com/10805002/diff/27001/sync/internal_api/...
sync/internal_api/public/sync_manager.h:412: SyncNotifierObserver* handler,
const ObjectIdSet& ids) = 0;
The style guide dictates that these parameters should be on their own lines.

https://chromiumcodereview.appspot.com/10805002/diff/27001/sync/internal_api/...
File sync/internal_api/public/test/fake_sync_manager.h (right):

https://chromiumcodereview.appspot.com/10805002/diff/27001/sync/internal_api/...
sync/internal_api/public/test/fake_sync_manager.h:58: const ObjectIdPayloadMap&
id_payloads,
nit: consider moving this to the line above, and indenting the next line to
match.

https://chromiumcodereview.appspot.com/10805002/diff/27001/sync/internal_api/...
sync/internal_api/public/test/fake_sync_manager.h:99: SyncNotifierObserver*
handler, const ObjectIdSet& ids) OVERRIDE;
One parameter per line.

https://chromiumcodereview.appspot.com/10805002/diff/27001/sync/internal_api/...
sync/internal_api/public/test/fake_sync_manager.h:128: const ObjectIdPayloadMap&
id_payloads,
nit: same suggestion to move this up a line and indent to match below.

https://chromiumcodereview.appspot.com/10805002/diff/27001/sync/internal_api/...
File sync/internal_api/sync_manager_impl.h (right):

https://chromiumcodereview.appspot.com/10805002/diff/27001/sync/internal_api/...
sync/internal_api/sync_manager_impl.h:90: SyncNotifierObserver* handler, const
ObjectIdSet& ids) OVERRIDE;
One parameter per line.

https://chromiumcodereview.appspot.com/10805002/diff/27001/sync/internal_api/...
File sync/internal_api/test/fake_sync_manager.cc (right):

https://chromiumcodereview.appspot.com/10805002/diff/27001/sync/internal_api/...
sync/internal_api/test/fake_sync_manager.cc:232: if
(!sync_task_runner_->PostTask(FROM_HERE, callback)) {
nit: remove unnecessary {}

https://chromiumcodereview.appspot.com/10805002/diff/27001/sync/notifier/obje...
File sync/notifier/object_id_payload_map.cc (right):

https://chromiumcodereview.appspot.com/10805002/diff/27001/sync/notifier/obje...
sync/notifier/object_id_payload_map.cc:20: ObjectIdSet ids, const std::string&
payload) {
One param per line

https://chromiumcodereview.appspot.com/10805002/diff/27001/sync/notifier/obje...
sync/notifier/object_id_payload_map.cc:22: for (ObjectIdSet::const_iterator it =
ids.begin(); it != ids.end(); ++it) {
nit: remove unnecessary {}

https://chromiumcodereview.appspot.com/10805002/diff/27001/sync/notifier/obje...
File sync/notifier/object_id_payload_map.h (right):

https://chromiumcodereview.appspot.com/10805002/diff/27001/sync/notifier/obje...
sync/notifier/object_id_payload_map.h:24: ObjectIdSet ids, const std::string&
payload);
One param per line.

https://chromiumcodereview.appspot.com/10805002/diff/27001/sync/notifier/sync...
File sync/notifier/sync_notifier.h (right):

https://chromiumcodereview.appspot.com/10805002/diff/27001/sync/notifier/sync...
sync/notifier/sync_notifier.h:25: // Updates the set of ObjectIds associated
with a given
nit: consider the same line breaking that I suggested earlier.

[akalin, 2012-08-04 00:01:25]

On 2012/08/03 05:14:21, msw wrote:
> I'm sorry I didn't review in time, but I mostly just have nits / style
comments;
> as I'm not terribly familiar with this code. The tests are much appreciated.
> 
> If anything, I'd say that it's a little odd to group the notification ids
> everywhere. If they're independently mapped to observers it seems a little
> simpler to register / unregister them individually, etc. But that's a fairly
> minor point, and you may have a good reason for your approach.

Forgot to reply to this.  Basically, passing around ObjectId sets is a little
more clunky, but allows for better batching.  Also, I want the clients to keep
track of their own ObjectId sets to avoid problems with 'dangling'
registrations, etc.

[msw, 2012-08-04 00:14:44]

On 2012/08/04 00:01:25, akalin wrote:
> On 2012/08/03 05:14:21, msw wrote:
> > I'm sorry I didn't review in time, but I mostly just have nits / style
> comments;
> > as I'm not terribly familiar with this code. The tests are much appreciated.
> > 
> > If anything, I'd say that it's a little odd to group the notification ids
> > everywhere. If they're independently mapped to observers it seems a little
> > simpler to register / unregister them individually, etc. But that's a fairly
> > minor point, and you may have a good reason for your approach.
> 
> Forgot to reply to this.  Basically, passing around ObjectId sets is a little
> more clunky, but allows for better batching.  Also, I want the clients to keep
> track of their own ObjectId sets to avoid problems with 'dangling'
> registrations, etc.

Okay, I can appreciate that; thanks for explaining.