Refactor common invalidation framework types

Refactor common invalidation framework types

Convert the Invalidation struct into a class.  This allows us to hide
its members, so we can do things like add a DCHECK to make sure we never
access the payload field of an unknown version invalidation.  It also
lets us use factory methods and constructors to initialize it rather
than having to initialize its fields one by one.

Convert the ObjectIdInvalidationMap from a typedef into a class.  Unlike
the typedef, the class supports multiple invalidations for the same
object ID.  It uses the newly introduced SingleObjectInvalidationSet to
manage the set of invalidations belonging to a particular ID.  Note that
the current code still sends only one invalidation per type; that will
be changed in a future commit.

The end goal of this refactoring is to make these classes smarter so
they can help manage the complexity that will be introduced when we
implement invalidation 'trickles' support.

Note that this commit changes the on-disk format for invalidations, so
any invalidations currently stored in the profile may be lost on
upgrade.  There should be no other notable changes to invalidations
behavior in this CL.

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

[rlarocque, 2013-09-09 18:41:24]

This is a big change, and, unfortunately, not all of it is trivial.  This was
split off from the CL that will eventually implement trickles.

I'll need to pull in the OWNERS for a bunch of different components eventually,
but let's start with the sync review.

Tim: Please review the sync parts.  Or, if you'd prefer, I can delegate the
review to someone else.

[rlarocque, 2013-09-18 22:32:19]

On 2013/09/09 18:41:24, rlarocque wrote:
> This is a big change, and, unfortunately, not all of it is trivial.  This was
> split off from the CL that will eventually implement trickles.
> 
> I'll need to pull in the OWNERS for a bunch of different components
eventually,
> but let's start with the sync review.
> 
> Tim: Please review the sync parts.  Or, if you'd prefer, I can delegate the
> review to someone else.

I haven't been aggressively pinging this because I considered
https://codereview.chromium.org/23717047/ to be more urgent.

Now that that's done, PTAL at this patch.

[tim (not reviewing), 2013-09-20 21:53:45]

https://codereview.chromium.org/23441042/diff/36001/sync/internal_api/public/...
File sync/internal_api/public/base/invalidation.cc (right):

https://codereview.chromium.org/23441042/diff/36001/sync/internal_api/public/...
sync/internal_api/public/base/invalidation.cc:33: return Invalidation(id, true,
-1, "", AckHandle::CreateUnique());
std::string vs ""

https://codereview.chromium.org/23441042/diff/36001/sync/internal_api/public/...
sync/internal_api/public/base/invalidation.cc:41: invalidation::ObjectId
Invalidation::GetObjectId() const {
style guide would have this called id().  Or object_id() if you change the
variable name.

Same for is_unknown_version_, payload, ack_handle...

https://codereview.chromium.org/23441042/diff/36001/sync/internal_api/public/...
sync/internal_api/public/base/invalidation.cc:50: DCHECK(!is_unknown_version_);
should be named version()... this is logically just an accessor, and the DCHECK
doesn't impact performance (which would be the only real reason to deviate and
use mixed-case function naming).

https://codereview.chromium.org/23441042/diff/36001/sync/internal_api/public/...
sync/internal_api/public/base/invalidation.cc:71: && payload_ == other.payload_;
You're not including ack_handle_, was that intentional?  Also, tests with
appropriate use of a factory function could help protect a method like this from
breaking down in the future.

https://codereview.chromium.org/23441042/diff/36001/sync/internal_api/public/...
sync/internal_api/public/base/invalidation.cc:100: payload_ = "";
std::string

https://codereview.chromium.org/23441042/diff/36001/sync/internal_api/public/...
File sync/internal_api/public/base/invalidation.h (right):

https://codereview.chromium.org/23441042/diff/36001/sync/internal_api/public/...
sync/internal_api/public/base/invalidation.h:23: // invalidation::Invalidation.
This comment raises the question of why we need it... can you distinguish it
from invalidation::Invalidation in some way here?

https://codereview.chromium.org/23441042/diff/36001/sync/internal_api/public/...
sync/internal_api/public/base/invalidation.h:39: Invalidation();
Remove this?

https://codereview.chromium.org/23441042/diff/36001/sync/internal_api/public/...
sync/internal_api/public/base/invalidation.h:73: AckHandle ack_handle_;
It's worth commenting these fields (some, at least).  e.g payload may be empty,
what is ack_handle for, ...

https://codereview.chromium.org/23441042/diff/36001/sync/notifier/invalidator...
File sync/notifier/invalidator_registrar.cc (left):

https://codereview.chromium.org/23441042/diff/36001/sync/notifier/invalidator...
sync/notifier/invalidator_registrar.cc:20: // |id_to_handler_map_| may be
non-empty but that's okay.
Hmm.. remind me what was happening here?  Why was this OK?

https://codereview.chromium.org/23441042/diff/36001/sync/notifier/invalidator...
File sync/notifier/invalidator_registrar.cc (right):

https://codereview.chromium.org/23441042/diff/36001/sync/notifier/invalidator...
sync/notifier/invalidator_registrar.cc:93: const ObjectIdInvalidationMap&
invalidation_map) {
So, |invalidation_map| contains all the invalidations... and the task of the
filtering below is to collect the ones that pertain to each handler in our
handler map and then tell that handler about them?

https://codereview.chromium.org/23441042/diff/36001/sync/notifier/invalidator...
File sync/notifier/invalidator_registrar.h (right):

https://codereview.chromium.org/23441042/diff/36001/sync/notifier/invalidator...
sync/notifier/invalidator_registrar.h:84: HandlerIdMap handler_to_id_map_;
it's a map to a set, right?  handler_to_ids_map?

https://codereview.chromium.org/23441042/diff/36001/sync/notifier/object_id_i...
File sync/notifier/object_id_invalidation_map.h (right):

https://codereview.chromium.org/23441042/diff/36001/sync/notifier/object_id_i...
sync/notifier/object_id_invalidation_map.h:18: class SYNC_EXPORT
ObjectIdInvalidationMap {
What is this class supposed to do?  Looking at it I have a hard time figuring
out what 'WithObjects' is doing... at first I thought it was a factory function.

https://codereview.chromium.org/23441042/diff/36001/sync/notifier/object_id_i...
sync/notifier/object_id_invalidation_map.h:31: ObjectIdInvalidationMap
WithObjects(const ObjectIdSet& ids) const;
Are there any basic tests for this filtering?

https://codereview.chromium.org/23441042/diff/36001/sync/notifier/ordered_inv...
File sync/notifier/ordered_invalidation_list.cc (right):

https://codereview.chromium.org/23441042/diff/36001/sync/notifier/ordered_inv...
sync/notifier/ordered_invalidation_list.cc:36:
invalidations_.insert(invalidation);
It's a bit weird that the same-ID constraint is lazily initialized by an Insert
operation, and nothing in this class aside from the comment makes it seem like
the list pertains to a specific object. I'd expect to pass it in on construction
or something.   

OrderedInvalidationListForObjectId ?
OrderedObjectIdInvalidationList

https://codereview.chromium.org/23441042/diff/36001/sync/notifier/sync_invali...
File sync/notifier/sync_invalidation_listener.cc (right):

https://codereview.chromium.org/23441042/diff/36001/sync/notifier/sync_invali...
sync/notifier/sync_invalidation_listener.cc:287: if (version == kUnknownVersion)
{
So is the contract with clients such that they decide whether they want to use X
as an "unknown version", but the underlying Invalidation object supports
whatever? Add a comment to this effect where you define kUnknownVersion.

[rlarocque, 2013-09-23 18:38:19]

Patch updated.  PTAL.

https://codereview.chromium.org/23441042/diff/36001/sync/internal_api/public/...
File sync/internal_api/public/base/invalidation.cc (right):

https://codereview.chromium.org/23441042/diff/36001/sync/internal_api/public/...
sync/internal_api/public/base/invalidation.cc:33: return Invalidation(id, true,
-1, "", AckHandle::CreateUnique());
On 2013/09/20 21:53:46, timsteele wrote:
> std::string vs ""

Done.

https://codereview.chromium.org/23441042/diff/36001/sync/internal_api/public/...
sync/internal_api/public/base/invalidation.cc:41: invalidation::ObjectId
Invalidation::GetObjectId() const {
On 2013/09/20 21:53:46, timsteele wrote:
> style guide would have this called id().  Or object_id() if you change the
> variable name.
> 
> Same for is_unknown_version_, payload, ack_handle...

True.  Lately, I've been reluctant to follow that guideline, because it means
that I'll have to change a lot of code if I ever decide to change the
implementation of a trivial member function into something less trivial.  

But that's a discussion for chromium-dev, not this CL.  I'll bring this code
into conformance with the guidelines.

https://codereview.chromium.org/23441042/diff/36001/sync/internal_api/public/...
sync/internal_api/public/base/invalidation.cc:50: DCHECK(!is_unknown_version_);
On 2013/09/20 21:53:46, timsteele wrote:
> should be named version()... this is logically just an accessor, and the
DCHECK
> doesn't impact performance (which would be the only real reason to deviate and
> use mixed-case function naming).

Done.

https://codereview.chromium.org/23441042/diff/36001/sync/internal_api/public/...
sync/internal_api/public/base/invalidation.cc:71: && payload_ == other.payload_;
On 2013/09/20 21:53:46, timsteele wrote:
> You're not including ack_handle_, was that intentional?  Also, tests with
> appropriate use of a factory function could help protect a method like this
from
> breaking down in the future.

It was more or less intentional.  The decision will make more sense in the next
patch.

My plan is to replace these IDs with WeakHandles that point back at the
SyncInvalidationListener, so clients can call invalidation.Acknowledge() from
any thread.  It's also a lot safer this way.  We won't have to worry about
sending acknowledgements that were generated by one SyncInvalidationListener to
a newer one, which can happen today if the invalidator is torn down and
restarted.

Once that change is made, it won't make sense to include the ack_handle_ in the
comparison, because there won't be any ack_handle_.

https://codereview.chromium.org/23441042/diff/36001/sync/internal_api/public/...
sync/internal_api/public/base/invalidation.cc:100: payload_ = "";
On 2013/09/20 21:53:46, timsteele wrote:
> std::string

Done.

https://codereview.chromium.org/23441042/diff/36001/sync/internal_api/public/...
File sync/internal_api/public/base/invalidation.h (right):

https://codereview.chromium.org/23441042/diff/36001/sync/internal_api/public/...
sync/internal_api/public/base/invalidation.h:23: // invalidation::Invalidation.
On 2013/09/20 21:53:46, timsteele wrote:
> This comment raises the question of why we need it... can you distinguish it
> from invalidation::Invalidation in some way here?

Added some comments.

The main difference is support for local ack-tracking.  This requires it to
include ack handles and serialization functions.  There are also some difference
in the API, but I intend to minimize those in the next patch.

https://codereview.chromium.org/23441042/diff/36001/sync/internal_api/public/...
sync/internal_api/public/base/invalidation.h:39: Invalidation();
On 2013/09/20 21:53:46, timsteele wrote:
> Remove this?

I can't remove support for the zero-argument constructor, because we need it for
statements of the form

Invalidation i = Invalidation::Init(...);

I think it's also useful to explicitly define this constructor, since I'd like
to provide some reasonable values for its members in this case.

https://codereview.chromium.org/23441042/diff/36001/sync/internal_api/public/...
sync/internal_api/public/base/invalidation.h:73: AckHandle ack_handle_;
On 2013/09/20 21:53:46, timsteele wrote:
> It's worth commenting these fields (some, at least).  e.g payload may be
empty,
> what is ack_handle for, ...

Done.

https://codereview.chromium.org/23441042/diff/36001/sync/notifier/invalidator...
File sync/notifier/invalidator_registrar.cc (left):

https://codereview.chromium.org/23441042/diff/36001/sync/notifier/invalidator...
sync/notifier/invalidator_registrar.cc:20: // |id_to_handler_map_| may be
non-empty but that's okay.
On 2013/09/20 21:53:46, timsteele wrote:
> Hmm.. remind me what was happening here?  Why was this OK?

In this part of the code, "registration" is an overloaded term.

The invalidations client registers a set of object IDs with the server.  Those
registrations persist across restarts.

There are also handlers of invalidations that register with the invalidation
service because they want to be called back when certain invalidations are
received.  These registrations must be unregistered during shutdown; otherwise
we would risk calling into deleted handlers.

We expose this two our clients by exposing a two-step
registration/unregistration process.  Registration for callbacks is done with
RegisterHandler and UnregisterHandler.  Setting up to listen for specific
ObjectIDs (and requesting that the client register with the server as a listener
of these IDs) is done with UpdateRegisteredIds.  

During normal shutdown, we expect that handlers will call UnregisterHandler(),
but not call UpdateReisteredIds().  The id_to_handler_map_ is managed mainly by
UpdateRegisteredIds(), so it won't be empty at shutdown.

https://codereview.chromium.org/23441042/diff/36001/sync/notifier/invalidator...
File sync/notifier/invalidator_registrar.cc (right):

https://codereview.chromium.org/23441042/diff/36001/sync/notifier/invalidator...
sync/notifier/invalidator_registrar.cc:93: const ObjectIdInvalidationMap&
invalidation_map) {
On 2013/09/20 21:53:46, timsteele wrote:
> So, |invalidation_map| contains all the invalidations... and the task of the
> filtering below is to collect the ones that pertain to each handler in our
> handler map and then tell that handler about them?  

Correct.

https://codereview.chromium.org/23441042/diff/36001/sync/notifier/invalidator...
File sync/notifier/invalidator_registrar.h (right):

https://codereview.chromium.org/23441042/diff/36001/sync/notifier/invalidator...
sync/notifier/invalidator_registrar.h:84: HandlerIdMap handler_to_id_map_;
On 2013/09/20 21:53:46, timsteele wrote:
> it's a map to a set, right?  handler_to_ids_map?

Correct.  Renamed.

https://codereview.chromium.org/23441042/diff/36001/sync/notifier/object_id_i...
File sync/notifier/object_id_invalidation_map.h (right):

https://codereview.chromium.org/23441042/diff/36001/sync/notifier/object_id_i...
sync/notifier/object_id_invalidation_map.h:18: class SYNC_EXPORT
ObjectIdInvalidationMap {
On 2013/09/20 21:53:46, timsteele wrote:
> What is this class supposed to do?  Looking at it I have a hard time figuring
> out what 'WithObjects' is doing... at first I thought it was a factory
function.

This class is supposed to be a container for many invalidations which may or may
not have different ObjectIds.

I made it a map because there aren't many interesting things one can do with two
invalidations that have different object IDs.  It's likely that the first thing
most clients will want to do with this object is iterate over the sets of
invalidations that share object IDs.

WithObjects is a helper to select the subset of contents of this map that have
the same ObjectId.  That's often a useful operation, but I guess I didn't do a
very good job of naming it.

Maybe GetInvalidationsWithIdsMatching(...) would be a better name?

https://codereview.chromium.org/23441042/diff/36001/sync/notifier/object_id_i...
sync/notifier/object_id_invalidation_map.h:31: ObjectIdInvalidationMap
WithObjects(const ObjectIdSet& ids) const;
On 2013/09/20 21:53:46, timsteele wrote:
> Are there any basic tests for this filtering?

No, there are no tests for any part of this class.  I'll fix that.

https://codereview.chromium.org/23441042/diff/36001/sync/notifier/ordered_inv...
File sync/notifier/ordered_invalidation_list.cc (right):

https://codereview.chromium.org/23441042/diff/36001/sync/notifier/ordered_inv...
sync/notifier/ordered_invalidation_list.cc:36:
invalidations_.insert(invalidation);
On 2013/09/20 21:53:46, timsteele wrote:
> It's a bit weird that the same-ID constraint is lazily initialized by an
Insert
> operation, and nothing in this class aside from the comment makes it seem like
> the list pertains to a specific object. I'd expect to pass it in on
construction
> or something.   
> 
> OrderedInvalidationListForObjectId ?
> OrderedObjectIdInvalidationList

I was kind of surprised that this didn't need the ID to be explicitly defined,
too.  This does have its uses.  It's pretty convenient to be able to initialize
one of these things without having to provide any constructor parameters.

Also, this is not as dangerous as it looks.  This class is read-only for most of
its users.  Any use of dangerous functionality, like Insert(), tends to be
wrapped by the ObjectIdInvalidationMap.

https://codereview.chromium.org/23441042/diff/36001/sync/notifier/sync_invali...
File sync/notifier/sync_invalidation_listener.cc (right):

https://codereview.chromium.org/23441042/diff/36001/sync/notifier/sync_invali...
sync/notifier/sync_invalidation_listener.cc:287: if (version == kUnknownVersion)
{
On 2013/09/20 21:53:46, timsteele wrote:
> So is the contract with clients such that they decide whether they want to use
X
> as an "unknown version", but the underlying Invalidation object supports
> whatever? Add a comment to this effect where you define kUnknownVersion.

No, I've rewritten the interface to the Invalidation class to force clients to
explicitly check the IsUnknownVersion() flag.  The TICL API does not specify a
version or payload on unknown version invalidations, so there is no valid value
for those fields on an unknown version invalidation.

The only reason kUnknownVersion still exists is that I haven't refactored the
"PrepareInvalidation" function yet.  That will be part of the next patch.

[tim (not reviewing), 2013-09-24 21:16:53]

https://codereview.chromium.org/23441042/diff/36001/sync/internal_api/public/...
File sync/internal_api/public/base/invalidation.cc (right):

https://codereview.chromium.org/23441042/diff/36001/sync/internal_api/public/...
sync/internal_api/public/base/invalidation.cc:71: && payload_ == other.payload_;
On 2013/09/23 18:38:19, rlarocque wrote:
> On 2013/09/20 21:53:46, timsteele wrote:
> > You're not including ack_handle_, was that intentional?  Also, tests with
> > appropriate use of a factory function could help protect a method like this
> from
> > breaking down in the future.
> 
> It was more or less intentional.  The decision will make more sense in the
next
> patch.
> 
> My plan is to replace these IDs with WeakHandles that point back at the
> SyncInvalidationListener, so clients can call invalidation.Acknowledge() from
> any thread.  It's also a lot safer this way.  We won't have to worry about
> sending acknowledgements that were generated by one SyncInvalidationListener
to
> a newer one, which can happen today if the invalidator is torn down and
> restarted.
> 
> Once that change is made, it won't make sense to include the ack_handle_ in
the
> comparison, because there won't be any ack_handle_.

That makes sense.  However in general, and especially in cases where it's not a
straight POD object and == works in the most trivial of ways, you should provide
your own Equals method (as the previous code had) instead of overloading
operators so that you need to be explicitly aware of what's happening.  See also
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Operat...

https://codereview.chromium.org/23441042/diff/36001/sync/internal_api/public/...
File sync/internal_api/public/base/invalidation.h (right):

https://codereview.chromium.org/23441042/diff/36001/sync/internal_api/public/...
sync/internal_api/public/base/invalidation.h:39: Invalidation();
On 2013/09/23 18:38:19, rlarocque wrote:
> On 2013/09/20 21:53:46, timsteele wrote:
> > Remove this?
> 
> I can't remove support for the zero-argument constructor, because we need it
for
> statements of the form
> 
> Invalidation i = Invalidation::Init(...);
> 
> I think it's also useful to explicitly define this constructor, since I'd like
> to provide some reasonable values for its members in this case.

You don't need a default constructor for statements like that, you need a copy
constructor. If you omit it altogether you don't have to worry about providing
reasonable values, because no one could ever invoke it (because you have a
private: constructor below).

https://codereview.chromium.org/23441042/diff/36001/sync/notifier/invalidator...
File sync/notifier/invalidator_registrar.cc (left):

https://codereview.chromium.org/23441042/diff/36001/sync/notifier/invalidator...
sync/notifier/invalidator_registrar.cc:20: // |id_to_handler_map_| may be
non-empty but that's okay.
On 2013/09/23 18:38:19, rlarocque wrote:
> On 2013/09/20 21:53:46, timsteele wrote:
> > Hmm.. remind me what was happening here?  Why was this OK?
> 
> In this part of the code, "registration" is an overloaded term.
> 
> The invalidations client registers a set of object IDs with the server.  Those
> registrations persist across restarts.
> 
> There are also handlers of invalidations that register with the invalidation
> service because they want to be called back when certain invalidations are
> received.  These registrations must be unregistered during shutdown; otherwise
> we would risk calling into deleted handlers.
> 
> We expose this two our clients by exposing a two-step
> registration/unregistration process.  Registration for callbacks is done with
> RegisterHandler and UnregisterHandler.  Setting up to listen for specific
> ObjectIDs (and requesting that the client register with the server as a
listener
> of these IDs) is done with UpdateRegisteredIds.  
> 
> During normal shutdown, we expect that handlers will call UnregisterHandler(),
> but not call UpdateReisteredIds().  The id_to_handler_map_ is managed mainly
by
> UpdateRegisteredIds(), so it won't be empty at shutdown.

I see.  So the two parts are separate in your new implementation and hence you
can assert one of them is empty.  Thanks.

https://codereview.chromium.org/23441042/diff/36001/sync/notifier/object_id_i...
File sync/notifier/object_id_invalidation_map.h (right):

https://codereview.chromium.org/23441042/diff/36001/sync/notifier/object_id_i...
sync/notifier/object_id_invalidation_map.h:18: class SYNC_EXPORT
ObjectIdInvalidationMap {
On 2013/09/23 18:38:19, rlarocque wrote:
> On 2013/09/20 21:53:46, timsteele wrote:
> > What is this class supposed to do?  Looking at it I have a hard time
figuring
> > out what 'WithObjects' is doing... at first I thought it was a factory
> function.
> 
> This class is supposed to be a container for many invalidations which may or
may
> not have different ObjectIds.
> 
> I made it a map because there aren't many interesting things one can do with
two
> invalidations that have different object IDs.  It's likely that the first
thing
> most clients will want to do with this object is iterate over the sets of
> invalidations that share object IDs.
> 
> WithObjects is a helper to select the subset of contents of this map that have
> the same ObjectId.  That's often a useful operation, but I guess I didn't do a
> very good job of naming it.
> 
> Maybe GetInvalidationsWithIdsMatching(...) would be a better name?

I see.  So it's returning a subset / projection of the tuples in the full
mapping. I find it pretty informative to include the subset aspect of it in the
name (as opposed to builder/factory functions where you can create an object
from scratch WithObjects(...))

[Get]SubsetWith[Objects|Ids](...)?

I was also hunting for a class comment here :)

https://codereview.chromium.org/23441042/diff/36001/sync/notifier/object_id_i...
sync/notifier/object_id_invalidation_map.h:31: ObjectIdInvalidationMap
WithObjects(const ObjectIdSet& ids) const;
On 2013/09/23 18:38:19, rlarocque wrote:
> On 2013/09/20 21:53:46, timsteele wrote:
> > Are there any basic tests for this filtering?
> 
> No, there are no tests for any part of this class.  I'll fix that.

Great!

https://codereview.chromium.org/23441042/diff/36001/sync/notifier/ordered_inv...
File sync/notifier/ordered_invalidation_list.cc (right):

https://codereview.chromium.org/23441042/diff/36001/sync/notifier/ordered_inv...
sync/notifier/ordered_invalidation_list.cc:36:
invalidations_.insert(invalidation);
On 2013/09/23 18:38:19, rlarocque wrote:
> On 2013/09/20 21:53:46, timsteele wrote:
> > It's a bit weird that the same-ID constraint is lazily initialized by an
> Insert
> > operation, and nothing in this class aside from the comment makes it seem
like
> > the list pertains to a specific object. I'd expect to pass it in on
> construction
> > or something.   
> > 
> > OrderedInvalidationListForObjectId ?
> > OrderedObjectIdInvalidationList
> 
> I was kind of surprised that this didn't need the ID to be explicitly defined,
> too.  This does have its uses.  It's pretty convenient to be able to
initialize
> one of these things without having to provide any constructor parameters.
> 
> Also, this is not as dangerous as it looks.  This class is read-only for most
of
> its users.  Any use of dangerous functionality, like Insert(), tends to be
> wrapped by the ObjectIdInvalidationMap.

It's just not obvious why you wouldn't use this if you wanted an ordered
invalidation list with whatever custom ordering.
[Single|Per]ObjectOrderedInvalidationList seems better to me, or s/List/Group,
but I can't really think of anything great :( I guess it's ok either way..

[rlarocque, 2013-09-25 00:40:00]

Patch updated.  PTAL.

https://codereview.chromium.org/23441042/diff/36001/sync/internal_api/public/...
File sync/internal_api/public/base/invalidation.cc (right):

https://codereview.chromium.org/23441042/diff/36001/sync/internal_api/public/...
sync/internal_api/public/base/invalidation.cc:71: && payload_ == other.payload_;
On 2013/09/24 21:16:54, timsteele wrote:
> On 2013/09/23 18:38:19, rlarocque wrote:
> > On 2013/09/20 21:53:46, timsteele wrote:
> > > You're not including ack_handle_, was that intentional?  Also, tests with
> > > appropriate use of a factory function could help protect a method like
this
> > from
> > > breaking down in the future.
> > 
> > It was more or less intentional.  The decision will make more sense in the
> next
> > patch.
> > 
> > My plan is to replace these IDs with WeakHandles that point back at the
> > SyncInvalidationListener, so clients can call invalidation.Acknowledge()
from
> > any thread.  It's also a lot safer this way.  We won't have to worry about
> > sending acknowledgements that were generated by one SyncInvalidationListener
> to
> > a newer one, which can happen today if the invalidator is torn down and
> > restarted.
> > 
> > Once that change is made, it won't make sense to include the ack_handle_ in
> the
> > comparison, because there won't be any ack_handle_.
> 
> That makes sense.  However in general, and especially in cases where it's not
a
> straight POD object and == works in the most trivial of ways, you should
provide
> your own Equals method (as the previous code had) instead of overloading
> operators so that you need to be explicitly aware of what's happening.  See
also
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Operat...

Fixed.  I had to add some boilerplate to get some STL comparisons to work, but
it wasn't as hard as I thought it would be.

https://codereview.chromium.org/23441042/diff/36001/sync/internal_api/public/...
File sync/internal_api/public/base/invalidation.h (right):

https://codereview.chromium.org/23441042/diff/36001/sync/internal_api/public/...
sync/internal_api/public/base/invalidation.h:39: Invalidation();
On 2013/09/24 21:16:54, timsteele wrote:
> On 2013/09/23 18:38:19, rlarocque wrote:
> > On 2013/09/20 21:53:46, timsteele wrote:
> > > Remove this?
> > 
> > I can't remove support for the zero-argument constructor, because we need it
> for
> > statements of the form
> > 
> > Invalidation i = Invalidation::Init(...);
> > 
> > I think it's also useful to explicitly define this constructor, since I'd
like
> > to provide some reasonable values for its members in this case.
> 
> You don't need a default constructor for statements like that, you need a copy
> constructor. If you omit it altogether you don't have to worry about providing
> reasonable values, because no one could ever invoke it (because you have a
> private: constructor below).

Right.  That was a bad example.

I might have remembered a real reason for it, though.  The function
Invalidation::ResetFromValue() must be able to fail.  I can see a few ways to
implement this:

- Have it return an Invalidation*, with NULL representing failure.  That gets a
bit weird, since it brings up a bunch of memory ownership issues that I'd rather
avoid.

- Make it a factory method that returns an Invalidation.  Then we would have to
maintain and check some flag to see if the invalidation it returned was actually
valid.

- The current solution, which is to have it operate on an existing Invalidation
and return a bool.  We can check the return value to see whether or not it
failed.

I don't think there's any solution that doesn't require either the use of an
Invalidation* or the concept of an "invalid" Invalidation.  This constructor
allows us to create an uninitialized Invalidation that can be initialized later
by ResetFromValue().

https://codereview.chromium.org/23441042/diff/36001/sync/notifier/object_id_i...
File sync/notifier/object_id_invalidation_map.h (right):

https://codereview.chromium.org/23441042/diff/36001/sync/notifier/object_id_i...
sync/notifier/object_id_invalidation_map.h:18: class SYNC_EXPORT
ObjectIdInvalidationMap {
On 2013/09/24 21:16:54, timsteele wrote:
> On 2013/09/23 18:38:19, rlarocque wrote:
> > On 2013/09/20 21:53:46, timsteele wrote:
> > > What is this class supposed to do?  Looking at it I have a hard time
> figuring
> > > out what 'WithObjects' is doing... at first I thought it was a factory
> > function.
> > 
> > This class is supposed to be a container for many invalidations which may or
> may
> > not have different ObjectIds.
> > 
> > I made it a map because there aren't many interesting things one can do with
> two
> > invalidations that have different object IDs.  It's likely that the first
> thing
> > most clients will want to do with this object is iterate over the sets of
> > invalidations that share object IDs.
> > 
> > WithObjects is a helper to select the subset of contents of this map that
have
> > the same ObjectId.  That's often a useful operation, but I guess I didn't do
a
> > very good job of naming it.
> > 
> > Maybe GetInvalidationsWithIdsMatching(...) would be a better name?
> 
> I see.  So it's returning a subset / projection of the tuples in the full
> mapping. I find it pretty informative to include the subset aspect of it in
the
> name (as opposed to builder/factory functions where you can create an object
> from scratch WithObjects(...))
> 
> [Get]SubsetWith[Objects|Ids](...)?

GetSubsetWithObjectIds() makes sense to me.

> 
> I was also hunting for a class comment here :)

I've added one.  Check patch set 12 or later.

https://codereview.chromium.org/23441042/diff/36001/sync/notifier/ordered_inv...
File sync/notifier/ordered_invalidation_list.cc (right):

https://codereview.chromium.org/23441042/diff/36001/sync/notifier/ordered_inv...
sync/notifier/ordered_invalidation_list.cc:36:
invalidations_.insert(invalidation);
On 2013/09/24 21:16:54, timsteele wrote:
> On 2013/09/23 18:38:19, rlarocque wrote:
> > On 2013/09/20 21:53:46, timsteele wrote:
> > > It's a bit weird that the same-ID constraint is lazily initialized by an
> > Insert
> > > operation, and nothing in this class aside from the comment makes it seem
> like
> > > the list pertains to a specific object. I'd expect to pass it in on
> > construction
> > > or something.   
> > > 
> > > OrderedInvalidationListForObjectId ?
> > > OrderedObjectIdInvalidationList
> > 
> > I was kind of surprised that this didn't need the ID to be explicitly
defined,
> > too.  This does have its uses.  It's pretty convenient to be able to
> initialize
> > one of these things without having to provide any constructor parameters.
> > 
> > Also, this is not as dangerous as it looks.  This class is read-only for
most
> of
> > its users.  Any use of dangerous functionality, like Insert(), tends to be
> > wrapped by the ObjectIdInvalidationMap.
> 
> It's just not obvious why you wouldn't use this if you wanted an ordered
> invalidation list with whatever custom ordering.
> [Single|Per]ObjectOrderedInvalidationList seems better to me, or s/List/Group,
> but I can't really think of anything great :( I guess it's ok either way..
> 

Renamed to SingleObjectInvalidationSet.

[rlarocque, 2013-09-27 20:21:14]

ping.

[rlarocque, 2013-09-30 18:14:47]

On 2013/09/27 20:21:14, rlarocque wrote:
> ping.

Patch set 14 refactors Invalidation::RestFromValue as we discussed on Friday. 
PTAL.

[tim (not reviewing), 2013-09-30 18:23:01]

On 2013/09/30 18:14:47, rlarocque wrote:
> On 2013/09/27 20:21:14, rlarocque wrote:
> > ping.
> 
> Patch set 14 refactors Invalidation::RestFromValue as we discussed on Friday. 
> PTAL.

Just took a look, looks good, except I don't see why InitFromValue wouldn't
return a scoped_ptr<Invalidation> to clearly signify ownership transfer from a
factory function; it looks suspicious otherwise. (There's some discussion at the
top of scoped_ptr.h that's relevant).

[rlarocque, 2013-09-30 22:17:12]

On 2013/09/30 18:23:01, timsteele wrote:
> On 2013/09/30 18:14:47, rlarocque wrote:
> > On 2013/09/27 20:21:14, rlarocque wrote:
> > > ping.
> > 
> > Patch set 14 refactors Invalidation::RestFromValue as we discussed on
Friday. 
> > PTAL.
> 
> Just took a look, looks good, except I don't see why InitFromValue wouldn't
> return a scoped_ptr<Invalidation> to clearly signify ownership transfer from a
> factory function; it looks suspicious otherwise. (There's some discussion at
the
> top of scoped_ptr.h that's relevant).

Good point.  Fixed in patch set #17.

[rlarocque, 2013-10-01 18:19:44]

On 2013/09/30 22:17:12, rlarocque wrote:
> On 2013/09/30 18:23:01, timsteele wrote:
> > On 2013/09/30 18:14:47, rlarocque wrote:
> > > On 2013/09/27 20:21:14, rlarocque wrote:
> > > > ping.
> > > 
> > > Patch set 14 refactors Invalidation::RestFromValue as we discussed on
> Friday. 
> > > PTAL.
> > 
> > Just took a look, looks good, except I don't see why InitFromValue wouldn't
> > return a scoped_ptr<Invalidation> to clearly signify ownership transfer from
a
> > factory function; it looks suspicious otherwise. (There's some discussion at
> the
> > top of scoped_ptr.h that's relevant).
> 
> Good point.  Fixed in patch set #17.

Now that Tim's review is wrapping up, I think it's time to pull in the OWNERs of
other files.

satorux: Please review chrome/browser/drive/*
dcheng: Please review chrome/browser/extensions/api/push_messaging/*
stepco: Please review chrome/browser/cloud/*

Although this CL changes the types used to call back InvalidationService's
listeners, the InvalidationService's behavior should remain unchanged.  For now,
it will send back at most one Invalidation per ObjectId, just as it always has.

Perhaps a more significant change is the conversion of Invalidation from a
struct to a class.  This is intended to make handling of "unknown versions" more
explicit.  Listeners are now expected to check the IsUnknownVersion() flag
before accessing the payload or version fields of an Invalidation.  Trying to
access these fields on an invalidation that has IsUnknownVersion() == true will
result in a DCHECK.

[Steve Condie, 2013-10-01 18:53:10]

lgtm

https://codereview.chromium.org/23441042/diff/110001/chrome/browser/policy/cl...
File chrome/browser/policy/cloud/cloud_policy_invalidator.cc (right):

https://codereview.chromium.org/23441042/diff/110001/chrome/browser/policy/cl...
chrome/browser/policy/cloud/cloud_policy_invalidator.cc:191: payload =
invalidation.payload();
Optional. To avoid making an unnecessary copy of the payload here the payload
variable could still be a const ref with a conditional operator in the
initializer to make it refer to either invalidation.payload() or a new local
variable set to empty string.

https://codereview.chromium.org/23441042/diff/110001/chrome/browser/policy/cl...
chrome/browser/policy/cloud/cloud_policy_invalidator.cc:193: // If there is a
payload, the invalidate callback can run at any time, so set
It looks like you accidentally modified this comment when merging one of my
changes. Please revert the comment change.

https://codereview.chromium.org/23441042/diff/110001/chrome/browser/sync/prof...
File chrome/browser/sync/profile_sync_service_android.cc (right):

https://codereview.chromium.org/23441042/diff/110001/chrome/browser/sync/prof...
chrome/browser/sync/profile_sync_service_android.cc:123:
syncer::Invalidation::Init(object_id, version, state));
It is possible for the version coming from the Java code to be
ipc::invalidation::Constants::UNKNOWN, in which case this invalidation should be
initialized with InitUnknownVersion.

[Steve Condie, 2013-10-01 18:55:25]

On 2013/10/01 18:53:10, stepco wrote:
> lgtm
> 
>
https://codereview.chromium.org/23441042/diff/110001/chrome/browser/policy/cl...
> File chrome/browser/policy/cloud/cloud_policy_invalidator.cc (right):
> 
>
https://codereview.chromium.org/23441042/diff/110001/chrome/browser/policy/cl...
> chrome/browser/policy/cloud/cloud_policy_invalidator.cc:191: payload =
> invalidation.payload();
> Optional. To avoid making an unnecessary copy of the payload here the payload
> variable could still be a const ref with a conditional operator in the
> initializer to make it refer to either invalidation.payload() or a new local
> variable set to empty string.
> 
>
https://codereview.chromium.org/23441042/diff/110001/chrome/browser/policy/cl...
> chrome/browser/policy/cloud/cloud_policy_invalidator.cc:193: // If there is a
> payload, the invalidate callback can run at any time, so set
> It looks like you accidentally modified this comment when merging one of my
> changes. Please revert the comment change.
> 
>
https://codereview.chromium.org/23441042/diff/110001/chrome/browser/sync/prof...
> File chrome/browser/sync/profile_sync_service_android.cc (right):
> 
>
https://codereview.chromium.org/23441042/diff/110001/chrome/browser/sync/prof...
> chrome/browser/sync/profile_sync_service_android.cc:123:
> syncer::Invalidation::Init(object_id, version, state));
> It is possible for the version coming from the Java code to be
> ipc::invalidation::Constants::UNKNOWN, in which case this invalidation should
be
> initialized with InitUnknownVersion.

Added Joao to review since I'm not an owner of the policy files.

[rlarocque, 2013-10-01 19:29:56]

https://codereview.chromium.org/23441042/diff/110001/chrome/browser/policy/cl...
File chrome/browser/policy/cloud/cloud_policy_invalidator.cc (right):

https://codereview.chromium.org/23441042/diff/110001/chrome/browser/policy/cl...
chrome/browser/policy/cloud/cloud_policy_invalidator.cc:191: payload =
invalidation.payload();
On 2013/10/01 18:53:10, stepco wrote:
> Optional. To avoid making an unnecessary copy of the payload here the payload
> variable could still be a const ref with a conditional operator in the
> initializer to make it refer to either invalidation.payload() or a new local
> variable set to empty string.

That's a good suggestion, but I'd prefer to leave the code as is.  I'm working
under the assumption that none of this invalidations code is performance
critical, so I'm trying to optimize for clarity.  I'm also biased against the
ternary operator, and try to avoid it if possible.

https://codereview.chromium.org/23441042/diff/110001/chrome/browser/policy/cl...
chrome/browser/policy/cloud/cloud_policy_invalidator.cc:193: // If there is a
payload, the invalidate callback can run at any time, so set
On 2013/10/01 18:53:10, stepco wrote:
> It looks like you accidentally modified this comment when merging one of my
> changes. Please revert the comment change.

Sorry about that.  Reverted.

https://codereview.chromium.org/23441042/diff/110001/chrome/browser/sync/prof...
File chrome/browser/sync/profile_sync_service_android.cc (right):

https://codereview.chromium.org/23441042/diff/110001/chrome/browser/sync/prof...
chrome/browser/sync/profile_sync_service_android.cc:123:
syncer::Invalidation::Init(object_id, version, state));
On 2013/10/01 18:53:10, stepco wrote:
> It is possible for the version coming from the Java code to be
> ipc::invalidation::Constants::UNKNOWN, in which case this invalidation should
be
> initialized with InitUnknownVersion.

Thanks for double-checking this.

The compiler also caught this because kUnknownVersion is no longer defined, so
it choked on line 109.  This was another bad rebase on my part.  It should be
fixed in the latest patch.

[Joao da Silva, 2013-10-02 08:36:32]

policy/ lgtm

https://codereview.chromium.org/23441042/diff/125001/chrome/browser/policy/cl...
File chrome/browser/policy/cloud/DEPS (right):

https://codereview.chromium.org/23441042/diff/125001/chrome/browser/policy/cl...
chrome/browser/policy/cloud/DEPS:149:
"+sync/internal_api/public/base/invalidation.h",
Move this above line 35, before the TODO; this rule is not meant to be
refactored.

[tim (not reviewing), 2013-10-02 22:48:45]

LGTM for:

sync/*
chrome/browser/sync/*
chrome/browser/invalidations/*

[dcheng, 2013-10-02 22:57:38]

chrome/browser/extensions/api/push_messaging/* LGTM

[rlarocque, 2013-10-02 23:25:06]

Thanks.  The latest upload addresses Joao's comments.

+TBR=brettw for adding google/cacheinvalidation to the
sync/internal_api/public/base/DEPS.

Just one more approval to go.
satorux: ping.

[satorux1, 2013-10-03 00:50:39]

sorry for the belated response.
chrome/browser/drive/drive_notification_manager.cc LGTM

[commit-bot: I haz the power, 2013-10-03 00:52:57]

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

[commit-bot: I haz the power, 2013-10-03 03:01:30]

Retried try job too often on win_rel for step(s) browser_tests,
chrome_frame_net_tests, chrome_frame_tests, chrome_frame_unittests,
content_browsertests, content_unittests, interactive_ui_tests,
mini_installer_test, nacl_integration, sync_integration_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[commit-bot: I haz the power, 2013-10-03 16:41:25]

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

[commit-bot: I haz the power, 2013-10-03 23:05:04]

Retried try job too often on win_rel for step(s) browser_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[commit-bot: I haz the power, 2013-10-03 23:11:58]

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

[commit-bot: I haz the power, 2013-10-04 03:51:13]

Change committed as 226949