sync: Remove ModelTypeInvalidationMap

sync: Remove ModelTypeInvalidationMap

Removes the definition and all uses of ModelTypeInvalidationMap.

The ModelTypeInvalidationMap was useful only for sync-related
invalidations.  Its existence made sense when sync was the only client
for invalidations.  Now that we have many invalidations clients, it
makes sense to replace it with the more generic ObjectIdInvalidationMap.

The reason for doing this now is that the ObjectIdInvalidationMap will
soon be modified to be incompatible with the current definition of
ModelTypeInvalidationMap.  In order to support trickles it will be
modified to allow it to contain several invalidations per ObjectId.
Although it would have been possible to maintain compatibility by making
a corresponding modification to ModelTypeInvalidationMap, there's really
no point in having two invalidation map types.  In the long run, it
makes more sense to deprecate ModelTypeInvalidationMap.

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

[rlarocque, 2013-08-23 20:03:48]

Here's part two of the trickles patches.  There should be no significant
behavioral changes in this one.

Please review.

[tim (not reviewing), 2013-08-27 17:49:03]

On 2013/08/23 20:03:48, rlarocque wrote:
> Here's part two of the trickles patches.  There should be no significant
> behavioral changes in this one.
> 
> Please review.

(ACK: I'm reviewing this today, sorry for the delay.)

[tim (not reviewing), 2013-08-28 00:12:59]

https://codereview.chromium.org/23238005/diff/1/sync/internal_api/debug_info_...
File sync/internal_api/debug_info_event_listener.cc (right):

https://codereview.chromium.org/23238005/diff/1/sync/internal_api/debug_info_...
sync/internal_api/debug_info_event_listener.cc:146: ModelType type;
nit - init to UNSPECIFIED.

https://codereview.chromium.org/23238005/diff/1/sync/internal_api/debug_info_...
sync/internal_api/debug_info_event_listener.cc:147: if
(ObjectIdToRealModelType(it->first, &type)) {
LOG(ERROR) if this fails (or some LOG variant)?

https://codereview.chromium.org/23238005/diff/1/sync/internal_api/public/base...
File sync/internal_api/public/base/model_type_test_util.cc (right):

https://codereview.chromium.org/23238005/diff/1/sync/internal_api/public/base...
sync/internal_api/public/base/model_type_test_util.cc:17:
DCHECK(RealModelTypeToObjectId(type, &id));
You shouldn't put "real code" inside DCHECKs like this (assuming you want to
init |id| here). This won't be invoked in release mode.

https://codereview.chromium.org/23238005/diff/1/sync/internal_api/public/base...
File sync/internal_api/public/base/model_type_test_util.h (right):

https://codereview.chromium.org/23238005/diff/1/sync/internal_api/public/base...
sync/internal_api/public/base/model_type_test_util.h:16: ObjectIdInvalidationMap
BuildInvalidationMap(
Comment.

https://codereview.chromium.org/23238005/diff/1/sync/internal_api/sync_manage...
File sync/internal_api/sync_manager_impl.cc (right):

https://codereview.chromium.org/23238005/diff/1/sync/internal_api/sync_manage...
sync/internal_api/sync_manager_impl.cc:1157: ModelType type;
Initialize to UNSPECIFIED.

https://codereview.chromium.org/23238005/diff/1/sync/internal_api/sync_manage...
sync/internal_api/sync_manager_impl.cc:1192: LOG(WARNING) << "Sync received
invalidation without any type information.";
Do we expect to be invoked here with non-sync Object IDs?

https://codereview.chromium.org/23238005/diff/1/sync/internal_api/sync_manage...
sync/internal_api/sync_manager_impl.cc:1211: const std::string& model_type_str =
ModelTypeToString(it.Get());
nit - I tend to prefer explicitly forcing the copy (i.e const std::string vs
const std::string&) in cases like this vs relying on the
assign-result-to-const-ref-surprisingly-works-and-might-not-require-copy,
because it looks less weird.  But not a big deal.

https://codereview.chromium.org/23238005/diff/1/sync/internal_api/sync_manage...
File sync/internal_api/sync_manager_impl_unittest.cc (right):

https://codereview.chromium.org/23238005/diff/1/sync/internal_api/sync_manage...
sync/internal_api/sync_manager_impl_unittest.cc:987:
ObjectIdSetToInvalidationMap(id_set, Invalidation::kUnknownVersion, "");
Prefer std::string() to "". It's more explicit, the prevailing / preferred
convention in Chrome code (according to our very own readability reviewer
brettw) and technically speaking, cheaper.

[rlarocque, 2013-08-28 01:00:35]

Patch updated.  PTAL.

https://codereview.chromium.org/23238005/diff/1/sync/internal_api/debug_info_...
File sync/internal_api/debug_info_event_listener.cc (right):

https://codereview.chromium.org/23238005/diff/1/sync/internal_api/debug_info_...
sync/internal_api/debug_info_event_listener.cc:146: ModelType type;
On 2013/08/28 00:13:00, timsteele wrote:
> nit - init to UNSPECIFIED.

Done.

https://codereview.chromium.org/23238005/diff/1/sync/internal_api/debug_info_...
sync/internal_api/debug_info_event_listener.cc:147: if
(ObjectIdToRealModelType(it->first, &type)) {
On 2013/08/28 00:13:00, timsteele wrote:
> LOG(ERROR) if this fails (or some LOG variant)?

Done.  I used DLOG(ERROR).

https://codereview.chromium.org/23238005/diff/1/sync/internal_api/public/base...
File sync/internal_api/public/base/model_type_test_util.cc (right):

https://codereview.chromium.org/23238005/diff/1/sync/internal_api/public/base...
sync/internal_api/public/base/model_type_test_util.cc:17:
DCHECK(RealModelTypeToObjectId(type, &id));
On 2013/08/28 00:13:00, timsteele wrote:
> You shouldn't put "real code" inside DCHECKs like this (assuming you want to
> init |id| here). This won't be invoked in release mode.

Thanks for catching this.  It would have taken me a while to diagnose the trybot
failures that would have resulted had this gone through.

https://codereview.chromium.org/23238005/diff/1/sync/internal_api/public/base...
File sync/internal_api/public/base/model_type_test_util.h (right):

https://codereview.chromium.org/23238005/diff/1/sync/internal_api/public/base...
sync/internal_api/public/base/model_type_test_util.h:16: ObjectIdInvalidationMap
BuildInvalidationMap(
On 2013/08/28 00:13:00, timsteele wrote:
> Comment.

Done.

https://codereview.chromium.org/23238005/diff/1/sync/internal_api/sync_manage...
File sync/internal_api/sync_manager_impl.cc (right):

https://codereview.chromium.org/23238005/diff/1/sync/internal_api/sync_manage...
sync/internal_api/sync_manager_impl.cc:1157: ModelType type;
On 2013/08/28 00:13:00, timsteele wrote:
> Initialize to UNSPECIFIED.

Done.

https://codereview.chromium.org/23238005/diff/1/sync/internal_api/sync_manage...
sync/internal_api/sync_manager_impl.cc:1192: LOG(WARNING) << "Sync received
invalidation without any type information.";
On 2013/08/28 00:13:00, timsteele wrote:
> Do we expect to be invoked here with non-sync Object IDs?

No, that should never happen.

https://codereview.chromium.org/23238005/diff/1/sync/internal_api/sync_manage...
sync/internal_api/sync_manager_impl.cc:1211: const std::string& model_type_str =
ModelTypeToString(it.Get());
On 2013/08/28 00:13:00, timsteele wrote:
> nit - I tend to prefer explicitly forcing the copy (i.e const std::string vs
> const std::string&) in cases like this vs relying on the
> assign-result-to-const-ref-surprisingly-works-and-might-not-require-copy,
> because it looks less weird.  But not a big deal.

Fixed.  Either way is OK with me.

https://codereview.chromium.org/23238005/diff/1/sync/internal_api/sync_manage...
File sync/internal_api/sync_manager_impl_unittest.cc (right):

https://codereview.chromium.org/23238005/diff/1/sync/internal_api/sync_manage...
sync/internal_api/sync_manager_impl_unittest.cc:987:
ObjectIdSetToInvalidationMap(id_set, Invalidation::kUnknownVersion, "");
On 2013/08/28 00:13:00, timsteele wrote:
> Prefer std::string() to "". It's more explicit, the prevailing / preferred
> convention in Chrome code (according to our very own readability reviewer
> brettw) and technically speaking, cheaper.

Done.

[tim (not reviewing), 2013-08-28 01:11:07]

LGTM with two comments below considered / addressed.

https://codereview.chromium.org/23238005/diff/1/sync/internal_api/debug_info_...
File sync/internal_api/debug_info_event_listener.cc (right):

https://codereview.chromium.org/23238005/diff/1/sync/internal_api/debug_info_...
sync/internal_api/debug_info_event_listener.cc:147: if
(ObjectIdToRealModelType(it->first, &type)) {
On 2013/08/28 01:00:35, rlarocque wrote:
> On 2013/08/28 00:13:00, timsteele wrote:
> > LOG(ERROR) if this fails (or some LOG variant)?
> 
> Done.  I used DLOG(ERROR).

There were a few other places I noticed where we call ObjectIdToRealModelType
and don't handle the error case; was that intended? e.g
sync_manager_impl.cc:1158

https://codereview.chromium.org/23238005/diff/1/sync/internal_api/sync_manage...
File sync/internal_api/sync_manager_impl.cc (right):

https://codereview.chromium.org/23238005/diff/1/sync/internal_api/sync_manage...
sync/internal_api/sync_manager_impl.cc:1192: LOG(WARNING) << "Sync received
invalidation without any type information.";
On 2013/08/28 01:00:35, rlarocque wrote:
> On 2013/08/28 00:13:00, timsteele wrote:
> > Do we expect to be invoked here with non-sync Object IDs?
> 
> No, that should never happen.

I ask just because before we were checking for that condition, whereas now it's
"did we get anything at all".  Maybe add a DCHECK(!nudged_types.empty()) below?

[rlarocque, 2013-08-28 17:50:22]

https://codereview.chromium.org/23238005/diff/1/sync/internal_api/debug_info_...
File sync/internal_api/debug_info_event_listener.cc (right):

https://codereview.chromium.org/23238005/diff/1/sync/internal_api/debug_info_...
sync/internal_api/debug_info_event_listener.cc:147: if
(ObjectIdToRealModelType(it->first, &type)) {
On 2013/08/28 01:11:07, timsteele wrote:
> On 2013/08/28 01:00:35, rlarocque wrote:
> > On 2013/08/28 00:13:00, timsteele wrote:
> > > LOG(ERROR) if this fails (or some LOG variant)?
> > 
> > Done.  I used DLOG(ERROR).
> 
> There were a few other places I noticed where we call ObjectIdToRealModelType
> and don't handle the error case; was that intended? e.g
> sync_manager_impl.cc:1158

I think I was consistently ignoring the error, which is what I had thought the
ObjectIdInvalidationMap to ModelTypeInvalidationMap conversion did.  Turns out
it printed logs if anything went wrong.

I think I can do better than that, though.  I'll remove the logging here, and
add a DLOG(WARNING) at the beginning of OnNotificationReceived.  This should
make it similar to the old check, but it will only notify once per invalidation
sent.

https://codereview.chromium.org/23238005/diff/1/sync/internal_api/sync_manage...
File sync/internal_api/sync_manager_impl.cc (right):

https://codereview.chromium.org/23238005/diff/1/sync/internal_api/sync_manage...
sync/internal_api/sync_manager_impl.cc:1192: LOG(WARNING) << "Sync received
invalidation without any type information.";
On 2013/08/28 01:11:07, timsteele wrote:
> On 2013/08/28 01:00:35, rlarocque wrote:
> > On 2013/08/28 00:13:00, timsteele wrote:
> > > Do we expect to be invoked here with non-sync Object IDs?
> > 
> > No, that should never happen.
> 
> I ask just because before we were checking for that condition, whereas now
it's
> "did we get anything at all".  Maybe add a DCHECK(!nudged_types.empty())
below?

Done.

While we're on the subject, I also can't think of any scenario where
OnInvalidationReceived would be sent an empty invalidation map.  I'll try to
convert more of these conditions to DCHECKs in a future CL.  For now, though,
I'd prefer to be conservative.  This code needs to be compatible with the
Android version of the invalidator, which I don't understand very well.

(In case you're wondering, I think adding the DCHECK(!nudged_types.empty()) is
safe because I'm pretty sure the Android invalidator is incapable of sending
invalidations with object IDs that are not convertible to model types.)

[rlarocque, 2013-08-30 20:47:43]

+rsimha

The trybots seem to like the latest gyp changes.  Raghu, do you see any red
flags among the .gyp changes?

[Raghu Simha, 2013-08-30 22:12:42]

LGTM after the comments below are addressed one way or another.

https://codereview.chromium.org/23238005/diff/28001/sync/engine/sync_scheduler.h
File sync/engine/sync_scheduler.h (right):

https://codereview.chromium.org/23238005/diff/28001/sync/engine/sync_schedule...
sync/engine/sync_scheduler.h:16: #include
"sync/notifier/object_id_invalidation_map.h"
Perhaps you can avoid this include by forward declaring ObjectIdInvalidationMap?
See my comment in sync.gyp.

https://codereview.chromium.org/23238005/diff/28001/sync/sync.gyp
File sync/sync.gyp (right):

https://codereview.chromium.org/23238005/diff/28001/sync/sync.gyp#newcode71
sync/sync.gyp:71: 'sync_proto',
You will need to add sync_notifier to the export_dependent_settings section too,
because sync/engine/sync_scheduler.h includes
sync/notifier/object_id_invalidation_map.h, and we  want all targets dependent
on sync_core to inherit its dependency on sync_notifier.

OTOH, this won't be necessary if you can get away with forward declaring
ObjectIdInvalidationMap in sync_scheduler.h. See my comment in sync_scheduler.h.

[rlarocque, 2013-08-30 23:28:08]

https://codereview.chromium.org/23238005/diff/28001/sync/engine/sync_scheduler.h
File sync/engine/sync_scheduler.h (right):

https://codereview.chromium.org/23238005/diff/28001/sync/engine/sync_schedule...
sync/engine/sync_scheduler.h:16: #include
"sync/notifier/object_id_invalidation_map.h"
On 2013/08/30 22:12:43, Raghu Simha wrote:
> Perhaps you can avoid this include by forward declaring
ObjectIdInvalidationMap?
> See my comment in sync.gyp.

Nope.  It's a typedef.

I could re-declare the typedef here (and elsewhere), and that might help a bit,
but, eventually, I think someting in sync's core is going to have to know what
an ObjectId is.

https://codereview.chromium.org/23238005/diff/28001/sync/sync.gyp
File sync/sync.gyp (right):

https://codereview.chromium.org/23238005/diff/28001/sync/sync.gyp#newcode71
sync/sync.gyp:71: 'sync_proto',
On 2013/08/30 22:12:43, Raghu Simha wrote:
> You will need to add sync_notifier to the export_dependent_settings section
too,
> because sync/engine/sync_scheduler.h includes
> sync/notifier/object_id_invalidation_map.h, and we  want all targets dependent
> on sync_core to inherit its dependency on sync_notifier.
> 
> OTOH, this won't be necessary if you can get away with forward declaring
> ObjectIdInvalidationMap in sync_scheduler.h. See my comment in
sync_scheduler.h.

Done.

[Raghu Simha, 2013-08-30 23:35:49]

https://codereview.chromium.org/23238005/diff/28001/sync/sync.gyp
File sync/sync.gyp (right):

https://codereview.chromium.org/23238005/diff/28001/sync/sync.gyp#newcode71
sync/sync.gyp:71: 'sync_proto',
nit: In case you haven't already landed this, sync_proto and sync_notifier
aren't alphabetized (here and in the section above).

[commit-bot: I haz the power, 2013-09-03 17:37:04]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rlarocque@chromium.org/23238005/24002

[commit-bot: I haz the power, 2013-09-03 21:20:58]

Change committed as 221025