Implement Invalidator::Acknowledge

Implement Invalidator::Acknowledge.

We implement this by creating a local queue of entries we've received invalidations for and then immediately acknowledging to Tango. When InvalidationHandlers acknowledge that they've finished processing for an id, we erase their entry from the queue; otherwise, we send reminder invalidations on an exponentially increasing delay.

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

[dcheng, 2012-09-12 20:39:08]

Please take a first pass at this CL.

Several known issues that you may or may not want to comment on:
1) I haven't updated any of the existing tests to compile yet, to minimize the
amount of refactoring I need to do. As this patch becomes more mature, I will
fix up the tests.
2) Testing in general. I think I need to implement it so the delay can be
calculated through a helper that can be injected in tests, or there needs to be
some other artificial way to speed up time. I haven't yet determined the best
strategy.
3) InvalidationHandler implementations haven't been updated. In the initial
implementation, I'm planning on just making them all Ack immediately in their
OnIncomingInvalidation handlers to preserve the current behavior and then
gradually updating the implementations to be more intelligent. Any objections?

http://codereview.chromium.org/10911084/diff/7001/sync/notifier/invalidation_...
File sync/notifier/invalidation_state_tracker.cc (right):

http://codereview.chromium.org/10911084/diff/7001/sync/notifier/invalidation_...
sync/notifier/invalidation_state_tracker.cc:1: // Copyright (c) 2012 The
Chromium Authors. All rights reserved.
I need to revert my local hack to make git cl extra sensitive to move detection.
This is just an add.

[dcheng, 2012-09-12 21:19:00]

FWIW, I've gone ahead and implemented the strategy described in (3) and just
acknowledge on receipt in all current
InvalidationHandler::OnIncomingInvalidation methods. I'm going to try running
the sync integration tests to make sure nothing new is going to explode.

[Pete Williamson, 2012-09-14 18:28:22]

http://codereview.chromium.org/10911084/diff/8024/chrome/browser/sync/invalid...
File chrome/browser/sync/invalidations/invalidator_storage.cc (right):

http://codereview.chromium.org/10911084/diff/8024/chrome/browser/sync/invalid...
chrome/browser/sync/invalidations/invalidator_storage.cc:285:
pref_service_->Set(prefs::kInvalidatorMaxInvalidationVersions,
Is the preference service the right place for this?  I would expect it to be
used for preferences, this seems more like session state data to me.

Also, kInvalidatorMaxInvalidationVersions seems like an outdated name.  Maybe
kInvalidatorLatestVersionsSeen?

http://codereview.chromium.org/10911084/diff/8024/sync/internal_api/public/ba...
File sync/internal_api/public/base/invalidation_state.cc (right):

http://codereview.chromium.org/10911084/diff/8024/sync/internal_api/public/ba...
sync/internal_api/public/base/invalidation_state.cc:24: return
AckHandle(base::RandBytesAsString(kBytesInHandle));
Why not just something that increments by one?  Does this have to be thread
safe?
Also, I found when debugging that strings with non-typeable characters are a
royal pain.  So, while we don't necessarily need Base64, some kind of typeable
encoding might be a good idea.

http://codereview.chromium.org/10911084/diff/8024/sync/internal_api/sync_mana...
File sync/internal_api/sync_manager_impl.cc (right):

http://codereview.chromium.org/10911084/diff/8024/sync/internal_api/sync_mana...
sync/internal_api/sync_manager_impl.cc:1288: // TODO(dcheng): Acknowledge
immediately for now. Fix this logic eventually.
What fix is needed here?  Ack later instead of immediately or something else?

http://codereview.chromium.org/10911084/diff/8024/sync/notifier/invalidation_...
File sync/notifier/invalidation_state_tracker.h (right):

http://codereview.chromium.org/10911084/diff/8024/sync/notifier/invalidation_...
sync/notifier/invalidation_state_tracker.h:23: struct LocalState {
ObjectVersionReportingState?
InvalidationVersionState?
MessageForReplay?

http://codereview.chromium.org/10911084/diff/8024/sync/notifier/invalidation_...
sync/notifier/invalidation_state_tracker.h:36: AckHandleMap;
Why this second map?  The first map already has the AckHandle.

http://codereview.chromium.org/10911084/diff/8024/sync/notifier/non_blocking_...
File sync/notifier/non_blocking_invalidator.cc (right):

http://codereview.chromium.org/10911084/diff/8024/sync/notifier/non_blocking_...
sync/notifier/non_blocking_invalidator.cc:209: 
This travels through a lot of levels.  What does a normal call sequence look
like?  I would imagine you start on one thread with a call to acknowledge on one
class that calls a contained object which passes it to another thread where
another contained object gets it, but it's getting confusing from just looking
at the diffs instead of looking at it in the debugger.  It would be nice to know
the actual names of the function in the order that they get executed during a
call.

http://codereview.chromium.org/10911084/diff/8024/sync/notifier/sync_invalida...
File sync/notifier/sync_invalidation_listener.cc (right):

http://codereview.chromium.org/10911084/diff/8024/sync/notifier/sync_invalida...
sync/notifier/sync_invalidation_listener.cc:27: // TODO(dcheng): This is not
testable yet.
How about a friend method that lets a test class change the default timeout, and
set it to something small, like 60ms?  You'd have to change the units on the
delay to be in microseconds or milliseconds instead of seconds.

http://codereview.chromium.org/10911084/diff/8024/sync/notifier/sync_invalida...
sync/notifier/sync_invalidation_listener.cc:285: DCHECK(!timer_queue_.empty());
Should we start a timer every time, or just start one, and start another for the
shortest time outstanding whenever a timer runs out?

http://codereview.chromium.org/10911084/diff/8024/sync/notifier/sync_invalida...
sync/notifier/sync_invalidation_listener.cc:291: 
A comment here would be helpful - does remit mean "cancel", "fulfill",
"re-emit", or something else in this context?  From the code, it looks like this
is the routine that gets fired when a timer times out.

http://codereview.chromium.org/10911084/diff/8024/sync/notifier/sync_invalida...
sync/notifier/sync_invalidation_listener.cc:296: it != timer_queue_.end() &&
it->first <= now; ) {
Is the queue sorted?  If not, we might end up with some timers we should be
revisiting here that we don't revisit.

http://codereview.chromium.org/10911084/diff/8024/sync/notifier/sync_invalida...
sync/notifier/sync_invalidation_listener.cc:299:
insert_it->second.retry_count++;
It looks like the first two calls will have a retry count of 0, was that
intended?  (the call from Emit has a retry count of 0, and when we call Remit
the first time it seems to still be 0)

http://codereview.chromium.org/10911084/diff/8024/sync/notifier/sync_invalida...
File sync/notifier/sync_invalidation_listener.h (right):

http://codereview.chromium.org/10911084/diff/8024/sync/notifier/sync_invalida...
sync/notifier/sync_invalidation_listener.h:150: typedef
std::multimap<base::TimeTicks, QueueEntry> TimerQueue;
Is a multimap the right data structure here?  I would have expected a priority
queue or a heap with the next timeout to be reached on top.

[dcheng, 2012-09-14 19:08:49]

http://codereview.chromium.org/10911084/diff/8024/chrome/browser/sync/invalid...
File chrome/browser/sync/invalidations/invalidator_storage.cc (right):

http://codereview.chromium.org/10911084/diff/8024/chrome/browser/sync/invalid...
chrome/browser/sync/invalidations/invalidator_storage.cc:285:
pref_service_->Set(prefs::kInvalidatorMaxInvalidationVersions,
On 2012/09/14 18:28:22, Pete Williamson wrote:
> Is the preference service the right place for this?  I would expect it to be
> used for preferences, this seems more like session state data to me.
> 

It seemed logical to group it here, since it's where we've (always?) stored what
versions we've seen.

> Also, kInvalidatorMaxInvalidationVersions seems like an outdated name.  Maybe
> kInvalidatorLatestVersionsSeen?

I'm still trying to think of a better name, since I think we already have an
invalidator state pref, which doesn't mean quite the same thing either (!!!).

http://codereview.chromium.org/10911084/diff/8024/sync/internal_api/public/ba...
File sync/internal_api/public/base/invalidation_state.cc (right):

http://codereview.chromium.org/10911084/diff/8024/sync/internal_api/public/ba...
sync/internal_api/public/base/invalidation_state.cc:24: return
AckHandle(base::RandBytesAsString(kBytesInHandle));
On 2012/09/14 18:28:22, Pete Williamson wrote:
> Why not just something that increments by one?  Does this have to be thread
> safe?
> Also, I found when debugging that strings with non-typeable characters are a
> royal pain.  So, while we don't necessarily need Base64, some kind of typeable
> encoding might be a good idea.

I thought about doing that, but I think it's easier to reason this way, since
there's no dependency between ack handles.
Debugging could be an issue, but the main issue I was concerned about is that
PrefService would sanely handle binary strings (I think it does, since we
actually already store some other binary data in there).

http://codereview.chromium.org/10911084/diff/8024/sync/internal_api/sync_mana...
File sync/internal_api/sync_manager_impl.cc (right):

http://codereview.chromium.org/10911084/diff/8024/sync/internal_api/sync_mana...
sync/internal_api/sync_manager_impl.cc:1288: // TODO(dcheng): Acknowledge
immediately for now. Fix this logic eventually.
On 2012/09/14 18:28:22, Pete Williamson wrote:
> What fix is needed here?  Ack later instead of immediately or something else?

Eventually, the final patches for crbug.com/124149 (probably not this patch)
will want to adjust this logic to only acknowledge once the sync is finished for
that sync type.

http://codereview.chromium.org/10911084/diff/8024/sync/notifier/invalidation_...
File sync/notifier/invalidation_state_tracker.h (right):

http://codereview.chromium.org/10911084/diff/8024/sync/notifier/invalidation_...
sync/notifier/invalidation_state_tracker.h:23: struct LocalState {
On 2012/09/14 18:28:22, Pete Williamson wrote:
> ObjectVersionReportingState?
> InvalidationVersionState?
> MessageForReplay?

What about "VersionData"?

http://codereview.chromium.org/10911084/diff/8024/sync/notifier/invalidation_...
sync/notifier/invalidation_state_tracker.h:36: AckHandleMap;
On 2012/09/14 18:28:22, Pete Williamson wrote:
> Why this second map?  The first map already has the AckHandle.

For GenerateAckHandles. The first one is currently only used for persisting the
data.

I think passing a pointer to the in-memory InvalidationStateMap* in
SyncInvalidationListener would probably not work as well, since it's possible
there are multiple requests in flight so we'd have to make a copy each time.

http://codereview.chromium.org/10911084/diff/8024/sync/notifier/non_blocking_...
File sync/notifier/non_blocking_invalidator.cc (right):

http://codereview.chromium.org/10911084/diff/8024/sync/notifier/non_blocking_...
sync/notifier/non_blocking_invalidator.cc:209: 
On 2012/09/14 18:28:22, Pete Williamson wrote:
> This travels through a lot of levels.  What does a normal call sequence look
> like?  I would imagine you start on one thread with a call to acknowledge on
one
> class that calls a contained object which passes it to another thread where
> another contained object gets it, but it's getting confusing from just looking
> at the diffs instead of looking at it in the debugger.  It would be nice to
know
> the actual names of the function in the order that they get executed during a
> call.

It would be easier to explain this in person rather than type it out. I'll try
to see if I can come by the office.

http://codereview.chromium.org/10911084/diff/8024/sync/notifier/sync_invalida...
File sync/notifier/sync_invalidation_listener.cc (right):

http://codereview.chromium.org/10911084/diff/8024/sync/notifier/sync_invalida...
sync/notifier/sync_invalidation_listener.cc:27: // TODO(dcheng): This is not
testable yet.
On 2012/09/14 18:28:22, Pete Williamson wrote:
> How about a friend method that lets a test class change the default timeout,
and
> set it to something small, like 60ms?  You'd have to change the units on the
> delay to be in microseconds or milliseconds instead of seconds.

I was actually thinking that we'd allow a TimeTicks value or a testing timer
implementation to be injected the test. I'd prefer not to have tests block for
any amount of time; perhaps something like
"TriggerTimerForTestAt(base::TimeTicks) would work.

http://codereview.chromium.org/10911084/diff/8024/sync/notifier/sync_invalida...
sync/notifier/sync_invalidation_listener.cc:285: DCHECK(!timer_queue_.empty());
On 2012/09/14 18:28:22, Pete Williamson wrote:
> Should we start a timer every time, or just start one, and start another for
the
> shortest time outstanding whenever a timer runs out?

I forgot to update the Start() call before uploading it. However, either way, we
want the timer to fire when the deadline for the first item in the queue has
expired no matter what.

http://codereview.chromium.org/10911084/diff/8024/sync/notifier/sync_invalida...
sync/notifier/sync_invalidation_listener.cc:291: 
On 2012/09/14 18:28:22, Pete Williamson wrote:
> A comment here would be helpful - does remit mean "cancel", "fulfill",
> "re-emit", or something else in this context?  From the code, it looks like
this
> is the routine that gets fired when a timer times out.

I need to think of a better name for this function. It was shortened for
re-emit, but it's not very clear.

http://codereview.chromium.org/10911084/diff/8024/sync/notifier/sync_invalida...
sync/notifier/sync_invalidation_listener.cc:296: it != timer_queue_.end() &&
it->first <= now; ) {
On 2012/09/14 18:28:22, Pete Williamson wrote:
> Is the queue sorted?  If not, we might end up with some timers we should be
> revisiting here that we don't revisit.

multimaps are sorted.

http://codereview.chromium.org/10911084/diff/8024/sync/notifier/sync_invalida...
sync/notifier/sync_invalidation_listener.cc:299:
insert_it->second.retry_count++;
On 2012/09/14 18:28:22, Pete Williamson wrote:
> It looks like the first two calls will have a retry count of 0, was that
> intended?  (the call from Emit has a retry count of 0, and when we call Remit
> the first time it seems to still be 0)

Good catch. Fixed =)

http://codereview.chromium.org/10911084/diff/8024/sync/notifier/sync_invalida...
File sync/notifier/sync_invalidation_listener.h (right):

http://codereview.chromium.org/10911084/diff/8024/sync/notifier/sync_invalida...
sync/notifier/sync_invalidation_listener.h:150: typedef
std::multimap<base::TimeTicks, QueueEntry> TimerQueue;
On 2012/09/14 18:28:22, Pete Williamson wrote:
> Is a multimap the right data structure here?  I would have expected a priority
> queue or a heap with the next timeout to be reached on top.

std::priority_queue doesn't allow erasing arbitrary elements. We may be able to
eventually share the PQ implementation in net/base; however, that's probably
outside the scope of this CL.

[akalin, 2012-09-17 23:59:54]

overall looks pretty good.  initial comments.

http://codereview.chromium.org/10911084/diff/12001/sync/internal_api/public/b...
File sync/internal_api/public/base/invalidation_state.cc (right):

http://codereview.chromium.org/10911084/diff/12001/sync/internal_api/public/b...
sync/internal_api/public/base/invalidation_state.cc:14: const size_t
kBytesInHandle = 16;
include cstddef for size_t

http://codereview.chromium.org/10911084/diff/12001/sync/internal_api/public/b...
sync/internal_api/public/base/invalidation_state.cc:23: // TODO(dcheng): Do we
want to base64 encode this?
i think it would be nice to have a readable ackhandle string.  But that does
increase the probability of collision, although I think it's still negligible.

http://codereview.chromium.org/10911084/diff/12001/sync/internal_api/public/b...
sync/internal_api/public/base/invalidation_state.cc:35: scoped_ptr<base::Value>
AckHandle::ToValue() const {
return a StringValue?

http://codereview.chromium.org/10911084/diff/12001/sync/internal_api/public/b...
sync/internal_api/public/base/invalidation_state.cc:39: bool
AckHandle::ResetFromValue(const base::Value& value) {
take a StringValue?

http://codereview.chromium.org/10911084/diff/12001/sync/internal_api/public/b...
File sync/internal_api/public/base/invalidation_state.h (right):

http://codereview.chromium.org/10911084/diff/12001/sync/internal_api/public/b...
sync/internal_api/public/base/invalidation_state.h:35: std::string state_;
since we have complete freedom in what the AckHandle contains, I think it would
be useful to stuff some useful data in there.  I'm thinking some or all of the
following:

ObjectId id_;
base::Time timestamp_;
std::string payload_;
std::string tango_ack_handle_str_; // need better name, may also not be present
(if we synthesize an ackhandle ourselves)
std::string random_bits_;

I feel like this would help us quite a bit in tracking down problems.

http://codereview.chromium.org/10911084/diff/12001/sync/notifier/invalidation...
File sync/notifier/invalidation_state_tracker.h (left):

http://codereview.chromium.org/10911084/diff/12001/sync/notifier/invalidation...
sync/notifier/invalidation_state_tracker.h:19: typedef
std::map<invalidation::ObjectId, int64, ObjectIdLessThan>
since this CL is pretty big and will take a while, may be worth prepping a
separate CL to rename InvalidationVersionMap to InvalidationStateMap, add
LocalState, etc.

http://codereview.chromium.org/10911084/diff/12001/sync/notifier/sync_invalid...
File sync/notifier/sync_invalidation_listener.cc (right):

http://codereview.chromium.org/10911084/diff/12001/sync/notifier/sync_invalid...
sync/notifier/sync_invalidation_listener.cc:236:
invalidation_state_tracker_.CallWithReply(
Hmm I don't think we need to add new CallWithReply methods.  You can make
GenerateAckHandles not take an AckHandleMap, but take the ids and a
Callback<void(const AckHandleMap&)> which it will call when done.  Then you can
bind that to EmitInvalidation (although I guess that does force AckHandleMap to
go last).

[akalin, 2012-09-18 00:00:18]

On 2012/09/12 21:19:00, dcheng wrote:
> FWIW, I've gone ahead and implemented the strategy described in (3) and just
> acknowledge on receipt in all current
> InvalidationHandler::OnIncomingInvalidation methods. I'm going to try running
> the sync integration tests to make sure nothing new is going to explode.

Yeah, this sounds good to me.

[dcheng, 2012-09-25 22:35:06]

I'm still in the process of updating the tests.

http://codereview.chromium.org/10911084/diff/12001/sync/internal_api/public/b...
File sync/internal_api/public/base/invalidation_state.cc (right):

http://codereview.chromium.org/10911084/diff/12001/sync/internal_api/public/b...
sync/internal_api/public/base/invalidation_state.cc:14: const size_t
kBytesInHandle = 16;
On 2012/09/17 23:59:54, akalin wrote:
> include cstddef for size_t

Done.

http://codereview.chromium.org/10911084/diff/12001/sync/internal_api/public/b...
sync/internal_api/public/base/invalidation_state.cc:23: // TODO(dcheng): Do we
want to base64 encode this?
On 2012/09/17 23:59:54, akalin wrote:
> i think it would be nice to have a readable ackhandle string.  But that does
> increase the probability of collision, although I think it's still negligible.

How about formatting it as an UUID? My original rationale for 16 bytes was the
following:
- 16 bytes is the typical size of the internal buffer of a std::string, so it
avoids an extra dynamic allocation.
- 16 bytes happens to be how long UUIDs are.

However, according to 21.3.4 (disclaimer: didn't actually read, read this in a
comment in gcc's basic string implementation), the buffer always needs to be
length+1 bytes to keep a terminating NULL around, so I'm not getting the
behavior I desire already.

http://codereview.chromium.org/10911084/diff/12001/sync/internal_api/public/b...
sync/internal_api/public/base/invalidation_state.cc:35: scoped_ptr<base::Value>
AckHandle::ToValue() const {
On 2012/09/17 23:59:54, akalin wrote:
> return a StringValue?

It's actually more convenient to keep using base::Value here. Unless I'm missing
something, there's no good way to convert from base::Value to base::StringValue
in InvalidationState::ResetFromValue (I could use GetAsString, but at that
point, I may as well use std::string here).

http://codereview.chromium.org/10911084/diff/12001/sync/internal_api/public/b...
sync/internal_api/public/base/invalidation_state.cc:39: bool
AckHandle::ResetFromValue(const base::Value& value) {
On 2012/09/17 23:59:54, akalin wrote:
> take a StringValue?

See above.

http://codereview.chromium.org/10911084/diff/12001/sync/internal_api/public/b...
File sync/internal_api/public/base/invalidation_state.h (right):

http://codereview.chromium.org/10911084/diff/12001/sync/internal_api/public/b...
sync/internal_api/public/base/invalidation_state.h:35: std::string state_;
On 2012/09/17 23:59:54, akalin wrote:
> since we have complete freedom in what the AckHandle contains, I think it
would
> be useful to stuff some useful data in there.  I'm thinking some or all of the
> following:
> 
> ObjectId id_;
> base::Time timestamp_;
> std::string payload_;
> std::string tango_ack_handle_str_; // need better name, may also not be
present
> (if we synthesize an ackhandle ourselves)
> std::string random_bits_;
> 
> I feel like this would help us quite a bit in tracking down problems.

I'd prefer to leave this sort of thing out of release builds by default so that
people don't try to start exposing/depending on it, especially since these
fields won't be relevant outside of debugging scenarios. What do you think?

http://codereview.chromium.org/10911084/diff/12001/sync/notifier/invalidation...
File sync/notifier/invalidation_state_tracker.h (left):

http://codereview.chromium.org/10911084/diff/12001/sync/notifier/invalidation...
sync/notifier/invalidation_state_tracker.h:19: typedef
std::map<invalidation::ObjectId, int64, ObjectIdLessThan>
On 2012/09/17 23:59:54, akalin wrote:
> since this CL is pretty big and will take a while, may be worth prepping a
> separate CL to rename InvalidationVersionMap to InvalidationStateMap, add
> LocalState, etc.

If you're relatively comfortable with the naming, then I can proceed with
pulling this out into a separate CL (after some thought, I still think
LocalState is the most concise name).

That change would probably just be a straight rename; the struct would only have
a version field. Does that sound about right to you?

http://codereview.chromium.org/10911084/diff/12001/sync/notifier/sync_invalid...
File sync/notifier/sync_invalidation_listener.cc (right):

http://codereview.chromium.org/10911084/diff/12001/sync/notifier/sync_invalid...
sync/notifier/sync_invalidation_listener.cc:236:
invalidation_state_tracker_.CallWithReply(
On 2012/09/17 23:59:54, akalin wrote:
> Hmm I don't think we need to add new CallWithReply methods.  You can make
> GenerateAckHandles not take an AckHandleMap, but take the ids and a
> Callback<void(const AckHandleMap&)> which it will call when done.  Then you
can
> bind that to EmitInvalidation (although I guess that does force AckHandleMap
to
> go last).

Per our discussion, we pass TaskRunner/Callback now. I personally favor the
original approach still, since it avoids exposing the fact that IST may execute
on a different thread to people who have to implement IST--that was more or less
encapsulated inside WeakHandle. Now it's a bit leaky. =/

[akalin, 2012-10-01 23:23:08]

few more

http://codereview.chromium.org/10911084/diff/12001/sync/internal_api/public/b...
File sync/internal_api/public/base/invalidation_state.cc (right):

http://codereview.chromium.org/10911084/diff/12001/sync/internal_api/public/b...
sync/internal_api/public/base/invalidation_state.cc:23: // TODO(dcheng): Do we
want to base64 encode this?
On 2012/09/25 22:35:06, dcheng wrote:
> On 2012/09/17 23:59:54, akalin wrote:
> > i think it would be nice to have a readable ackhandle string.  But that does
> > increase the probability of collision, although I think it's still
negligible.
> 
> How about formatting it as an UUID? My original rationale for 16 bytes was the
> following:
> - 16 bytes is the typical size of the internal buffer of a std::string, so it
> avoids an extra dynamic allocation.
> - 16 bytes happens to be how long UUIDs are.
> 
> However, according to 21.3.4 (disclaimer: didn't actually read, read this in a
> comment in gcc's basic string implementation), the buffer always needs to be
> length+1 bytes to keep a terminating NULL around, so I'm not getting the
> behavior I desire already.

Yeah, the standard doesn't require keeping space for the terminating null
(data() and c_str() are allowed to differ, i.e., c_str() points to a
lazy-initialized separate buffer) but most implementations just do it anyway.

If you're concerned about dynamic alloc, why not just store a char[16]?

http://codereview.chromium.org/10911084/diff/12001/sync/internal_api/public/b...
sync/internal_api/public/base/invalidation_state.cc:35: scoped_ptr<base::Value>
AckHandle::ToValue() const {
On 2012/09/25 22:35:06, dcheng wrote:
> On 2012/09/17 23:59:54, akalin wrote:
> > return a StringValue?
> 
> It's actually more convenient to keep using base::Value here. Unless I'm
missing
> something, there's no good way to convert from base::Value to
base::StringValue
> in InvalidationState::ResetFromValue (I could use GetAsString, but at that
> point, I may as well use std::string here).

True.  Fair enough.

http://codereview.chromium.org/10911084/diff/12001/sync/internal_api/public/b...
File sync/internal_api/public/base/invalidation_state.h (right):

http://codereview.chromium.org/10911084/diff/12001/sync/internal_api/public/b...
sync/internal_api/public/base/invalidation_state.h:35: std::string state_;
On 2012/09/25 22:35:06, dcheng wrote:
> On 2012/09/17 23:59:54, akalin wrote:
> > since we have complete freedom in what the AckHandle contains, I think it
> would
> > be useful to stuff some useful data in there.  I'm thinking some or all of
the
> > following:
> > 
> > ObjectId id_;
> > base::Time timestamp_;
> > std::string payload_;
> > std::string tango_ack_handle_str_; // need better name, may also not be
> present
> > (if we synthesize an ackhandle ourselves)
> > std::string random_bits_;
> > 
> > I feel like this would help us quite a bit in tracking down problems.
> 
> I'd prefer to leave this sort of thing out of release builds by default so
that
> people don't try to start exposing/depending on it, especially since these
> fields won't be relevant outside of debugging scenarios. What do you think?

We can keep a lid on people trying to expose it, right?  I hope no one will
actually try to parse the string representation.

Unfortunately, I think we'll be spending a significant amount of time debugging
invalidations that the debug info would be useful. :/

http://codereview.chromium.org/10911084/diff/12001/sync/notifier/invalidation...
File sync/notifier/invalidation_state_tracker.h (left):

http://codereview.chromium.org/10911084/diff/12001/sync/notifier/invalidation...
sync/notifier/invalidation_state_tracker.h:19: typedef
std::map<invalidation::ObjectId, int64, ObjectIdLessThan>
On 2012/09/25 22:35:06, dcheng wrote:
> On 2012/09/17 23:59:54, akalin wrote:
> > since this CL is pretty big and will take a while, may be worth prepping a
> > separate CL to rename InvalidationVersionMap to InvalidationStateMap, add
> > LocalState, etc.
> 
> If you're relatively comfortable with the naming, then I can proceed with
> pulling this out into a separate CL (after some thought, I still think
> LocalState is the most concise name).
> 
> That change would probably just be a straight rename; the struct would only
have
> a version field. Does that sound about right to you?

We were talking about better names than 'state', right?  We should figure that
out, and then do a rename CL.

We have a lot of 'State's. :(

http://codereview.chromium.org/10911084/diff/12001/sync/notifier/sync_invalid...
File sync/notifier/sync_invalidation_listener.cc (right):

http://codereview.chromium.org/10911084/diff/12001/sync/notifier/sync_invalid...
sync/notifier/sync_invalidation_listener.cc:236:
invalidation_state_tracker_.CallWithReply(
On 2012/09/25 22:35:06, dcheng wrote:
> On 2012/09/17 23:59:54, akalin wrote:
> > Hmm I don't think we need to add new CallWithReply methods.  You can make
> > GenerateAckHandles not take an AckHandleMap, but take the ids and a
> > Callback<void(const AckHandleMap&)> which it will call when done.  Then you
> can
> > bind that to EmitInvalidation (although I guess that does force AckHandleMap
> to
> > go last).
> 
> Per our discussion, we pass TaskRunner/Callback now. I personally favor the
> original approach still, since it avoids exposing the fact that IST may
execute
> on a different thread to people who have to implement IST--that was more or
less
> encapsulated inside WeakHandle. Now it's a bit leaky. =/

Yeah. :/ WeakHandle only really works well in one-way scenarios.  It's possible
to have the IST have a WeakHandle back, but that means defining another
interface.  May not be worth it.

[dcheng, 2012-10-18 23:42:14]

The CL has been updated. The reminder feature is now (mostly) tested, test
non-determinism has been stamped out (as much as reasonably possible), and
things more or less work.

There's two more things I plan on adding that I didn't include in this CL, since
I wanted to get it out for review again.
1. The extra fields you've requested that we store in AckHandle. I'm planning on
including the original tango object ID (is this always a valid UTF-8 string?)
and the timestamp, which I think will be enough for debugging purposes.
2. A unit test that unacknowledged invalidations are handled properly when we
restart SyncInvalidationListener.

[akalin, 2012-10-19 13:27:16]

Moar comments.  Suggested a couple places to pull out in separate CLs.  I think
that may be worth doing to reduce the size of this one and also to iterate
faster on those smaller pieces.

http://codereview.chromium.org/10911084/diff/34001/chrome/browser/chrome_to_m...
File chrome/browser/chrome_to_mobile_service.cc (right):

http://codereview.chromium.org/10911084/diff/34001/chrome/browser/chrome_to_m...
chrome/browser/chrome_to_mobile_service.cc:434: // TODO(msw): Unit tests do not
provide profiles; see http://crbug.com/122183
I think it should be acked only when the device search request finishes, no? 
Maybe add a TODO / file a bug with C2M folks.

http://codereview.chromium.org/10911084/diff/34001/chrome/browser/extensions/...
File
chrome/browser/extensions/api/push_messaging/push_messaging_invalidation_handler.cc
(right):

http://codereview.chromium.org/10911084/diff/34001/chrome/browser/extensions/...
chrome/browser/extensions/api/push_messaging/push_messaging_invalidation_handler.cc:132:
delegate_->OnMessage(extension_id, subchannel, it->second.payload);
Question: Is this synchronous?  i.e., can we ack immediately after this?

http://codereview.chromium.org/10911084/diff/34001/chrome/browser/sync/glue/s...
File chrome/browser/sync/glue/sync_backend_host.h (right):

http://codereview.chromium.org/10911084/diff/34001/chrome/browser/sync/glue/s...
chrome/browser/sync/glue/sync_backend_host.h:187: // TODO(dcheng): Add a
comment.
!

http://codereview.chromium.org/10911084/diff/34001/chrome/browser/sync/invali...
File chrome/browser/sync/invalidation_frontend.h (right):

http://codereview.chromium.org/10911084/diff/34001/chrome/browser/sync/invali...
chrome/browser/sync/invalidation_frontend.h:85: // TODO(dcheng): Add a comment.
!

http://codereview.chromium.org/10911084/diff/34001/chrome/browser/sync/invali...
File chrome/browser/sync/invalidations/invalidator_storage.cc (right):

http://codereview.chromium.org/10911084/diff/34001/chrome/browser/sync/invali...
chrome/browser/sync/invalidations/invalidator_storage.cc:33: // TODO(dcheng):
Rename.
!

http://codereview.chromium.org/10911084/diff/34001/chrome/browser/sync/invali...
chrome/browser/sync/invalidations/invalidator_storage.cc:73: }
does it make sense to have only one ack handle field and not the other?  perhaps
null out unless both are present?

http://codereview.chromium.org/10911084/diff/34001/chrome/browser/sync/invali...
chrome/browser/sync/invalidations/invalidator_storage.cc:91: void
GenerateAckHandleReplyHelper(
almost certain you can just rebind callback with ack_handles directly

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

http://codereview.chromium.org/10911084/diff/34001/chrome/browser/sync/profil...
chrome/browser/sync/profile_sync_service.cc:483: DCHECK(backend_.get());
you might have to check for backend_ and stash away the acks until it gets
initialized

http://codereview.chromium.org/10911084/diff/34001/sync/internal_api/public/b...
File sync/internal_api/public/base/invalidation.cc (right):

http://codereview.chromium.org/10911084/diff/34001/sync/internal_api/public/b...
sync/internal_api/public/base/invalidation.cc:39: return
scoped_ptr<base::Value>(new StringValue(state_));
more future-proof to have a dictionary with a single string field?

http://codereview.chromium.org/10911084/diff/34001/sync/internal_api/public/b...
File sync/internal_api/public/base/invalidation.h (right):

http://codereview.chromium.org/10911084/diff/34001/sync/internal_api/public/b...
sync/internal_api/public/base/invalidation.h:20: // Opaque class that represents
an ack handle. We don't reuse the Tango ack
Tango -> invalidation::

http://codereview.chromium.org/10911084/diff/34001/sync/internal_api/public/b...
sync/internal_api/public/base/invalidation.h:30: bool ResetFromValue(const
base::Value& value);
comment that copy constructor and assignment operator welcome

(make default constructor private also?)

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

http://codereview.chromium.org/10911084/diff/34001/sync/internal_api/sync_man...
sync/internal_api/sync_manager_impl.cc:1255: for
(ObjectIdInvalidationMap::const_iterator it = invalidation_map.begin();
move this outside if block and after?

http://codereview.chromium.org/10911084/diff/34001/sync/internal_api/test/fak...
File sync/internal_api/test/fake_sync_manager.cc (right):

http://codereview.chromium.org/10911084/diff/34001/sync/internal_api/test/fak...
sync/internal_api/test/fake_sync_manager.cc:174: }
add "// Do nothing." comment

http://codereview.chromium.org/10911084/diff/34001/sync/notifier/invalidation...
File sync/notifier/invalidation_state_tracker.h (right):

http://codereview.chromium.org/10911084/diff/34001/sync/notifier/invalidation...
sync/notifier/invalidation_state_tracker.h:28: // TODO(dcheng): Need a better
name.
may want to have a separate smaller CL to add the struct (containing only the
version) and rename

http://codereview.chromium.org/10911084/diff/34001/sync/notifier/invalidation...
sync/notifier/invalidation_state_tracker.h:29: struct LocalState {
i think since all the other "state" vars have been renamed, this can be
InvalidationState now, right?

http://codereview.chromium.org/10911084/diff/34001/sync/notifier/invalidation...
sync/notifier/invalidation_state_tracker.h:47: virtual InvalidationStateMap
GetStateMap() const = 0;
perhaps GetAllInvalidationStates()?

http://codereview.chromium.org/10911084/diff/34001/sync/notifier/invalidator.h
File sync/notifier/invalidator.h (right):

http://codereview.chromium.org/10911084/diff/34001/sync/notifier/invalidator....
sync/notifier/invalidator.h:71: // TODO(dcheng): Add comment.
!

http://codereview.chromium.org/10911084/diff/34001/sync/notifier/sync_invalid...
File sync/notifier/sync_invalidation_listener.cc (right):

http://codereview.chromium.org/10911084/diff/34001/sync/notifier/sync_invalid...
sync/notifier/sync_invalidation_listener.cc:27: base::TimeDelta
CalculateBackoffDelay(int retry_count) {
almost certain there's already an exp. backoff class somewhere in net/ that has
the right logic.  joi@ would know.

http://codereview.chromium.org/10911084/diff/34001/sync/notifier/sync_invalid...
sync/notifier/sync_invalidation_listener.cc:217:
&InvalidationStateTracker::SetMaxVersion,
can we combine the SetMaxVersion and GenerateAckHandles calls?  We'd have to
relax the check that the new version > old version to new version >= old version
or old version doesn't exist and new version is 0.  WDYT?

http://codereview.chromium.org/10911084/diff/34001/sync/notifier/sync_invalid...
File sync/notifier/sync_invalidation_listener.h (right):

http://codereview.chromium.org/10911084/diff/34001/sync/notifier/sync_invalid...
sync/notifier/sync_invalidation_listener.h:90: // TODO(dcheng): Add a comment.
!

http://codereview.chromium.org/10911084/diff/34001/sync/notifier/sync_invalid...
sync/notifier/sync_invalidation_listener.h:173: void PrepareInvalidation(const
ObjectIdSet& ids,
I feel we want to decomp out the queue-managing logic to a separate class.  It
would make it easier to test, and also easier to use the existing exp. backoff
code.

Also, you can then pull that out into its own CL that we can iterate faster
over.  What do you think?

http://codereview.chromium.org/10911084/diff/34001/sync/tools/sync_listen_not...
File sync/tools/sync_listen_notifications.cc (right):

http://codereview.chromium.org/10911084/diff/34001/sync/tools/sync_listen_not...
sync/tools/sync_listen_notifications.cc:54: void GenerateAckHandlesReplyHelper(
bind callback / ackhandles directly

[Jói, 2012-10-19 13:29:49]

http://codereview.chromium.org/10911084/diff/34001/sync/notifier/sync_invalid...
File sync/notifier/sync_invalidation_listener.cc (right):

http://codereview.chromium.org/10911084/diff/34001/sync/notifier/sync_invalid...
sync/notifier/sync_invalidation_listener.cc:27: base::TimeDelta
CalculateBackoffDelay(int retry_count) {
On 2012/10/19 13:27:16, akalin wrote:
> almost certain there's already an exp. backoff class somewhere in net/ that
has
> the right logic.  joi@ would know.

net/base/backoff_entry.h

[dcheng, 2012-10-19 19:38:11]

http://codereview.chromium.org/10911084/diff/34001/chrome/browser/chrome_to_m...
File chrome/browser/chrome_to_mobile_service.cc (right):

http://codereview.chromium.org/10911084/diff/34001/chrome/browser/chrome_to_m...
chrome/browser/chrome_to_mobile_service.cc:434: // TODO(msw): Unit tests do not
provide profiles; see http://crbug.com/122183
On 2012/10/19 13:27:16, akalin wrote:
> I think it should be acked only when the device search request finishes, no? 
> Maybe add a TODO / file a bug with C2M folks.

Done. Added a TODO and filed a crbug.

http://codereview.chromium.org/10911084/diff/34001/chrome/browser/extensions/...
File
chrome/browser/extensions/api/push_messaging/push_messaging_invalidation_handler.cc
(right):

http://codereview.chromium.org/10911084/diff/34001/chrome/browser/extensions/...
chrome/browser/extensions/api/push_messaging/push_messaging_invalidation_handler.cc:132:
delegate_->OnMessage(extension_id, subchannel, it->second.payload);
On 2012/10/19 13:27:16, akalin wrote:
> Question: Is this synchronous?  i.e., can we ack immediately after this?

Doing this properly is somewhat harder, as this involves a message to the
renderer and then being notified when the renderer is finished--the glue for
this does not currently exist. I do plan on improving this outside of this CL;
the current implementation is just to preserve legacy behavior.

http://codereview.chromium.org/10911084/diff/34001/chrome/browser/sync/glue/s...
File chrome/browser/sync/glue/sync_backend_host.h (right):

http://codereview.chromium.org/10911084/diff/34001/chrome/browser/sync/glue/s...
chrome/browser/sync/glue/sync_backend_host.h:187: // TODO(dcheng): Add a
comment.
On 2012/10/19 13:27:16, akalin wrote:
> !

Done.

http://codereview.chromium.org/10911084/diff/34001/chrome/browser/sync/invali...
File chrome/browser/sync/invalidation_frontend.h (right):

http://codereview.chromium.org/10911084/diff/34001/chrome/browser/sync/invali...
chrome/browser/sync/invalidation_frontend.h:85: // TODO(dcheng): Add a comment.
On 2012/10/19 13:27:16, akalin wrote:
> !

Done.

http://codereview.chromium.org/10911084/diff/34001/chrome/browser/sync/invali...
File chrome/browser/sync/invalidations/invalidator_storage.cc (right):

http://codereview.chromium.org/10911084/diff/34001/chrome/browser/sync/invali...
chrome/browser/sync/invalidations/invalidator_storage.cc:33: // TODO(dcheng):
Rename.
On 2012/10/19 13:27:16, akalin wrote:
> !

Done. I thought I updated this when I updated the corresponding function at line
78...  but obviously not.

http://codereview.chromium.org/10911084/diff/34001/chrome/browser/sync/invali...
chrome/browser/sync/invalidations/invalidator_storage.cc:73: }
On 2012/10/19 13:27:16, akalin wrote:
> does it make sense to have only one ack handle field and not the other? 
perhaps
> null out unless both are present?

It's possible to have an expected value but no current value (e.g. the handler
in question has never acked). However, I've updated the code to handle a
situation that *should* be impossible: if there's no expected value, make sure
we clear the current value.
I've also updated the serialization code to not dump out a value if it's an
invalid handle.

http://codereview.chromium.org/10911084/diff/34001/chrome/browser/sync/invali...
chrome/browser/sync/invalidations/invalidator_storage.cc:91: void
GenerateAckHandleReplyHelper(
On 2012/10/19 13:27:16, akalin wrote:
> almost certain you can just rebind callback with ack_handles directly

Apparently base::Bind(callback, ack_handles) works!

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

http://codereview.chromium.org/10911084/diff/34001/chrome/browser/sync/profil...
chrome/browser/sync/profile_sync_service.cc:483: DCHECK(backend_.get());
On 2012/10/19 13:27:16, akalin wrote:
> you might have to check for backend_ and stash away the acks until it gets
> initialized

Done.

http://codereview.chromium.org/10911084/diff/34001/sync/internal_api/public/b...
File sync/internal_api/public/base/invalidation.cc (right):

http://codereview.chromium.org/10911084/diff/34001/sync/internal_api/public/b...
sync/internal_api/public/base/invalidation.cc:39: return
scoped_ptr<base::Value>(new StringValue(state_));
On 2012/10/19 13:27:16, akalin wrote:
> more future-proof to have a dictionary with a single string field?

Done.

http://codereview.chromium.org/10911084/diff/34001/sync/internal_api/public/b...
File sync/internal_api/public/base/invalidation.h (right):

http://codereview.chromium.org/10911084/diff/34001/sync/internal_api/public/b...
sync/internal_api/public/base/invalidation.h:20: // Opaque class that represents
an ack handle. We don't reuse the Tango ack
On 2012/10/19 13:27:16, akalin wrote:
> Tango -> invalidation::

Done.

http://codereview.chromium.org/10911084/diff/34001/sync/internal_api/public/b...
sync/internal_api/public/base/invalidation.h:30: bool ResetFromValue(const
base::Value& value);
On 2012/10/19 13:27:16, akalin wrote:
> comment that copy constructor and assignment operator welcome
> 
> (make default constructor private also?)

Done. Unless I'm missing something, no default ctor will be emitted (and we
don't define one--that's why I had to add a default ctor for
syncer::Invalidation).

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

http://codereview.chromium.org/10911084/diff/34001/sync/internal_api/sync_man...
sync/internal_api/sync_manager_impl.cc:1255: for
(ObjectIdInvalidationMap::const_iterator it = invalidation_map.begin();
On 2012/10/19 13:27:16, akalin wrote:
> move this outside if block and after?

Done.

http://codereview.chromium.org/10911084/diff/34001/sync/internal_api/test/fak...
File sync/internal_api/test/fake_sync_manager.cc (right):

http://codereview.chromium.org/10911084/diff/34001/sync/internal_api/test/fak...
sync/internal_api/test/fake_sync_manager.cc:174: }
On 2012/10/19 13:27:16, akalin wrote:
> add "// Do nothing." comment

Done.

http://codereview.chromium.org/10911084/diff/34001/sync/notifier/invalidation...
File sync/notifier/invalidation_state_tracker.h (right):

http://codereview.chromium.org/10911084/diff/34001/sync/notifier/invalidation...
sync/notifier/invalidation_state_tracker.h:28: // TODO(dcheng): Need a better
name.
On 2012/10/19 13:27:16, akalin wrote:
> may want to have a separate smaller CL to add the struct (containing only the
> version) and rename

Sure, I'll pull this out into a separate CL.

http://codereview.chromium.org/10911084/diff/34001/sync/notifier/invalidation...
sync/notifier/invalidation_state_tracker.h:29: struct LocalState {
On 2012/10/19 13:27:16, akalin wrote:
> i think since all the other "state" vars have been renamed, this can be
> InvalidationState now, right?

Done.

http://codereview.chromium.org/10911084/diff/34001/sync/notifier/invalidation...
sync/notifier/invalidation_state_tracker.h:47: virtual InvalidationStateMap
GetStateMap() const = 0;
On 2012/10/19 13:27:16, akalin wrote:
> perhaps GetAllInvalidationStates()?

Done.

http://codereview.chromium.org/10911084/diff/34001/sync/notifier/invalidator.h
File sync/notifier/invalidator.h (right):

http://codereview.chromium.org/10911084/diff/34001/sync/notifier/invalidator....
sync/notifier/invalidator.h:71: // TODO(dcheng): Add comment.
On 2012/10/19 13:27:16, akalin wrote:
> !

Done.

http://codereview.chromium.org/10911084/diff/34001/sync/notifier/sync_invalid...
File sync/notifier/sync_invalidation_listener.cc (right):

http://codereview.chromium.org/10911084/diff/34001/sync/notifier/sync_invalid...
sync/notifier/sync_invalidation_listener.cc:27: base::TimeDelta
CalculateBackoffDelay(int retry_count) {
On 2012/10/19 13:29:49, Jói wrote:
> On 2012/10/19 13:27:16, akalin wrote:
> > almost certain there's already an exp. backoff class somewhere in net/ that
> has
> > the right logic.  joi@ would know.
> 
> net/base/backoff_entry.h

I like this much better and will update the code appropriately. Thanks for the
pointer.

http://codereview.chromium.org/10911084/diff/34001/sync/notifier/sync_invalid...
sync/notifier/sync_invalidation_listener.cc:217:
&InvalidationStateTracker::SetMaxVersion,
On 2012/10/19 13:27:16, akalin wrote:
> can we combine the SetMaxVersion and GenerateAckHandles calls?  We'd have to
> relax the check that the new version > old version to new version >= old
version
> or old version doesn't exist and new version is 0.  WDYT?

The reason they are split up is because  InvalidateUnknownVersion and
InvalidateAll don't pass in a version number. Or are you suggesting we treat
InvalidateUnknownVersion/InvalidateAll as version == 0?

http://codereview.chromium.org/10911084/diff/34001/sync/notifier/sync_invalid...
File sync/notifier/sync_invalidation_listener.h (right):

http://codereview.chromium.org/10911084/diff/34001/sync/notifier/sync_invalid...
sync/notifier/sync_invalidation_listener.h:90: // TODO(dcheng): Add a comment.
On 2012/10/19 13:27:16, akalin wrote:
> !

Done.

http://codereview.chromium.org/10911084/diff/34001/sync/notifier/sync_invalid...
sync/notifier/sync_invalidation_listener.h:173: void PrepareInvalidation(const
ObjectIdSet& ids,
On 2012/10/19 13:27:16, akalin wrote:
> I feel we want to decomp out the queue-managing logic to a separate class.  It
> would make it easier to test, and also easier to use the existing exp. backoff
> code.
> 
> Also, you can then pull that out into its own CL that we can iterate faster
> over.  What do you think?

I didn't do it originally because there were already a lot of moving pieces and
it seemed to me at the time that it'd result in more complexity. However, I
think this class is now overly complicated and that would be a good idea (I feel
a bit uncertain about pulling it out into a separate CL though).

http://codereview.chromium.org/10911084/diff/34001/sync/tools/sync_listen_not...
File sync/tools/sync_listen_notifications.cc (right):

http://codereview.chromium.org/10911084/diff/34001/sync/tools/sync_listen_not...
sync/tools/sync_listen_notifications.cc:54: void GenerateAckHandlesReplyHelper(
On 2012/10/19 13:27:16, akalin wrote:
> bind callback / ackhandles directly

Done.

[akalin, 2012-11-19 19:37:12]

Is this ready for review?  (If not, let me know when it is!)

[dcheng, 2012-11-20 00:07:09]

On 2012/11/19 19:37:12, akalin wrote:
> Is this ready for review?  (If not, let me know when it is!)

Not quite yet. I've split out another portion of this patch for a review (see
https://codereview.chromium.org/11415049/, though I haven't gone over it myself
yet to make sure it's in reviewable state); what do you think of the proposed
split?

[akalin, 2012-11-20 18:41:38]

On 2012/11/20 00:07:09, dcheng wrote:
what do you think of the proposed split?

looks reasonable!

[akalin, 2012-12-29 09:41:18]

On 2012/11/20 18:41:38, akalin wrote:
> On 2012/11/20 00:07:09, dcheng wrote:
> what do you think of the proposed split?
> 
> looks reasonable!

Ready for another look?

[dcheng, 2013-02-08 20:58:45]

Mind taking another look now? This is the last part of the change. rlarocque@
has mentioned it might be worth it to move the timeout logic elsewhere, since it
may only be used by push messaging but that would probably be a separate patch.

I've updated the tests for SyncInvalidationListener as well. There aren't too
many since most of the interesting edge cases are already covered by the
AckTracker unit test.

[dcheng, 2013-02-12 20:18:37]

On 2013/02/08 20:58:45, dcheng wrote:
> Mind taking another look now? This is the last part of the change. rlarocque@
> has mentioned it might be worth it to move the timeout logic elsewhere, since
it
> may only be used by push messaging but that would probably be a separate
patch.
> 
> I've updated the tests for SyncInvalidationListener as well. There aren't too
> many since most of the interesting edge cases are already covered by the
> AckTracker unit test.

Ping!

[rlarocque, 2013-02-12 21:48:04]

LGTM, but I'm not sure I have enough context to properly review this patch.  You
should probably wait for other reviews before submitting.

https://codereview.chromium.org/10911084/diff/84034/chrome/browser/sync/profi...
File chrome/browser/sync/profile_sync_service.cc (right):

https://codereview.chromium.org/10911084/diff/84034/chrome/browser/sync/profi...
chrome/browser/sync/profile_sync_service.cc:501: for
(AckHandleReplayQueue::const_iterator it = ack_replay_queue_.begin();
I don't like the way this code (or the line above it, for that matter) are
communicating with the sync thread.  

InitializeBackend() will start the sync thread and post a task to it to
initialize the sync manager.  Eventually that task will end up in
SyncBackendHost::Core::OnInitializationComplete(), which may decide to destroy
the SyncManager if it detects that initialization failed.  

The tasks posted by UpdateRegisteredInvalidationIds and AcknowledgeInvalidation
will be queued up on the sync thread behind the initialization task.  They must
(and do) support the sync_manager_ being NULL, but not for the right reasons. 
The PSS shouldn't be trying to communicate with the backend until we receive the
OnBackendInitialized() callback.  

Would it be possible to do this from OnBackendInitialized() instead?

=====

On the other hand, I intend to move this code out of here as we transition
towards an independent InvalidationService.  So it's probably OK to leave it as
is.

https://codereview.chromium.org/10911084/diff/84034/sync/internal_api/sync_ma...
File sync/internal_api/sync_manager_impl.cc (right):

https://codereview.chromium.org/10911084/diff/84034/sync/internal_api/sync_ma...
sync/internal_api/sync_manager_impl.cc:1248: // TODO(dcheng): Acknowledge
immediately for now. Fix this logic eventually.
Could you add a comment to remind us not to remove this until the invalidator
doesn't repeatedly ping us when we don't immediately acknowledge?  Otherwise,
this would interfere with our scheduler's backoff algorithms.  

(I blame this on some unnecessary complexity in the scheduler more than the
invalidator code, but it's a problem nonetheless.)

[Jói, 2013-02-12 22:10:23]

Removing myself as a reviewer as I think I only got added due to a drive-by
comment.  Let me know if this is a misunderstanding.

Cheers,
Jói

[dcheng, 2013-02-12 22:39:28]

https://codereview.chromium.org/10911084/diff/84034/chrome/browser/sync/profi...
File chrome/browser/sync/profile_sync_service.cc (right):

https://codereview.chromium.org/10911084/diff/84034/chrome/browser/sync/profi...
chrome/browser/sync/profile_sync_service.cc:501: for
(AckHandleReplayQueue::const_iterator it = ack_replay_queue_.begin();
On 2013/02/12 21:48:04, rlarocque wrote:
> I don't like the way this code (or the line above it, for that matter) are
> communicating with the sync thread.  
> 
> InitializeBackend() will start the sync thread and post a task to it to
> initialize the sync manager.  Eventually that task will end up in
> SyncBackendHost::Core::OnInitializationComplete(), which may decide to destroy
> the SyncManager if it detects that initialization failed.  
> 
> The tasks posted by UpdateRegisteredInvalidationIds and
AcknowledgeInvalidation
> will be queued up on the sync thread behind the initialization task.  They
must
> (and do) support the sync_manager_ being NULL, but not for the right reasons. 
> The PSS shouldn't be trying to communicate with the backend until we receive
the
> OnBackendInitialized() callback.  
> 
> Would it be possible to do this from OnBackendInitialized() instead?
> 
> =====
> 
> On the other hand, I intend to move this code out of here as we transition
> towards an independent InvalidationService.  So it's probably OK to leave it
as
> is.  

I don't mind fixing this, but perhaps it would be better done in a separate CL
(since the existing code is busted already in this respect).

https://codereview.chromium.org/10911084/diff/84034/sync/internal_api/sync_ma...
File sync/internal_api/sync_manager_impl.cc (right):

https://codereview.chromium.org/10911084/diff/84034/sync/internal_api/sync_ma...
sync/internal_api/sync_manager_impl.cc:1248: // TODO(dcheng): Acknowledge
immediately for now. Fix this logic eventually.
On 2013/02/12 21:48:04, rlarocque wrote:
> Could you add a comment to remind us not to remove this until the invalidator
> doesn't repeatedly ping us when we don't immediately acknowledge?  Otherwise,
> this would interfere with our scheduler's backoff algorithms.  
> 
> (I blame this on some unnecessary complexity in the scheduler more than the
> invalidator code, but it's a problem nonetheless.)

Done.

[akalin, 2013-02-15 22:32:12]

LGTM, modulo rclaroque's comments

https://codereview.chromium.org/10911084/diff/86006/chrome/browser/extensions...
File
chrome/browser/extensions/api/push_messaging/push_messaging_invalidation_handler_unittest.cc
(right):

https://codereview.chromium.org/10911084/diff/86006/chrome/browser/extensions...
chrome/browser/extensions/api/push_messaging/push_messaging_invalidation_handler_unittest.cc:153:
// Invalid object IDs should still be acknowledged though.
i think you can leave off 'though'

https://codereview.chromium.org/10911084/diff/86006/chrome/browser/sync/glue/...
File chrome/browser/sync/glue/android_invalidator_bridge.cc (right):

https://codereview.chromium.org/10911084/diff/86006/chrome/browser/sync/glue/...
chrome/browser/sync/glue/android_invalidator_bridge.cc:170: }
this is on purpose, right? I suggest having a '// Do nothing.' comment, perhaps
with details.

https://codereview.chromium.org/10911084/diff/86006/sync/notifier/fake_invali...
File sync/notifier/fake_invalidator.cc (right):

https://codereview.chromium.org/10911084/diff/86006/sync/notifier/fake_invali...
sync/notifier/fake_invalidator.cc:62: const AckHandle& ack_handle) {
// Do nothing.

https://codereview.chromium.org/10911084/diff/86006/sync/notifier/sync_invali...
File sync/notifier/sync_invalidation_listener.h (right):

https://codereview.chromium.org/10911084/diff/86006/sync/notifier/sync_invali...
sync/notifier/sync_invalidation_listener.h:157: // AckTracker::Delegate
implementation.
newline before

[akalin, 2013-02-15 22:32:36]

probably can update the CL description (since AckTracker was in a previous patch
already)

[Pete Williamson, 2013-02-16 01:41:06]

ACK - The changes look good to me.  I have a few suggestions

https://codereview.chromium.org/10911084/diff/86006/chrome/browser/sync/glue/...
File chrome/browser/sync/glue/android_invalidator_bridge_proxy.cc (right):

https://codereview.chromium.org/10911084/diff/86006/chrome/browser/sync/glue/...
chrome/browser/sync/glue/android_invalidator_bridge_proxy.cc:47: }
Does it make sense to add push messaging to android?  They already have GCM. 
(or maybe this is not needed to add push messaging, but just needed because we
added push messaging to the rest of the system.)

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

https://codereview.chromium.org/10911084/diff/86006/sync/notifier/sync_invali...
sync/notifier/sync_invalidation_listener.cc:156: // keep track of |id| since it
is up-to-date.
This comment confuses me a bit, we say that we don't need to keep track of it,
but then we add it to the ids.

https://codereview.chromium.org/10911084/diff/86006/sync/notifier/sync_invali...
sync/notifier/sync_invalidation_listener.cc:284: void
SyncInvalidationListener::OnTimeout(const ObjectIdSet& ids) {
A comment on the "OnTimeout" function might be nice.  I'm not sure what is
timing out or why or what needs to be done when a timeout happens from just
reading the code.

[dcheng, 2013-02-22 02:51:30]

https://codereview.chromium.org/10911084/diff/86006/chrome/browser/extensions...
File
chrome/browser/extensions/api/push_messaging/push_messaging_invalidation_handler_unittest.cc
(right):

https://codereview.chromium.org/10911084/diff/86006/chrome/browser/extensions...
chrome/browser/extensions/api/push_messaging/push_messaging_invalidation_handler_unittest.cc:153:
// Invalid object IDs should still be acknowledged though.
On 2013/02/15 22:32:12, akalin wrote:
> i think you can leave off 'though'

Done.

https://codereview.chromium.org/10911084/diff/86006/chrome/browser/sync/glue/...
File chrome/browser/sync/glue/android_invalidator_bridge.cc (right):

https://codereview.chromium.org/10911084/diff/86006/chrome/browser/sync/glue/...
chrome/browser/sync/glue/android_invalidator_bridge.cc:170: }
On 2013/02/15 22:32:12, akalin wrote:
> this is on purpose, right? I suggest having a '// Do nothing.' comment,
perhaps
> with details.

Done.

https://codereview.chromium.org/10911084/diff/86006/chrome/browser/sync/glue/...
File chrome/browser/sync/glue/android_invalidator_bridge_proxy.cc (right):

https://codereview.chromium.org/10911084/diff/86006/chrome/browser/sync/glue/...
chrome/browser/sync/glue/android_invalidator_bridge_proxy.cc:47: }
On 2013/02/16 01:41:06, Pete Williamson wrote:
> Does it make sense to add push messaging to android?  They already have GCM. 
> (or maybe this is not needed to add push messaging, but just needed because we
> added push messaging to the rest of the system.)

Android doesn't support extensions. If this changes, we may have to revisit
this.

https://codereview.chromium.org/10911084/diff/86006/sync/notifier/fake_invali...
File sync/notifier/fake_invalidator.cc (right):

https://codereview.chromium.org/10911084/diff/86006/sync/notifier/fake_invali...
sync/notifier/fake_invalidator.cc:62: const AckHandle& ack_handle) {
On 2013/02/15 22:32:12, akalin wrote:
> // Do nothing.

Done.

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

https://codereview.chromium.org/10911084/diff/86006/sync/notifier/sync_invali...
sync/notifier/sync_invalidation_listener.cc:156: // keep track of |id| since it
is up-to-date.
On 2013/02/16 01:41:06, Pete Williamson wrote:
> This comment confuses me a bit, we say that we don't need to keep track of it,
> but then we add it to the ids.

Right, AckTracker has a Track() and an Ack() function. Ack() informs it that it
no longer needs to track the IDs in question.

I guess we could rename it to Untrack()... but that should probably go in a
separate CL (and I'm not sure if that would be any less confusing).

https://codereview.chromium.org/10911084/diff/86006/sync/notifier/sync_invali...
sync/notifier/sync_invalidation_listener.cc:284: void
SyncInvalidationListener::OnTimeout(const ObjectIdSet& ids) {
On 2013/02/16 01:41:06, Pete Williamson wrote:
> A comment on the "OnTimeout" function might be nice.  I'm not sure what is
> timing out or why or what needs to be done when a timeout happens from just
> reading the code.

Hmm. The delegate interface describes what this callback is for, I'm not sure
duplicating the comment here would do too much, as then I'd have to describe
what AckTracker, etc all do.

https://codereview.chromium.org/10911084/diff/86006/sync/notifier/sync_invali...
File sync/notifier/sync_invalidation_listener.h (right):

https://codereview.chromium.org/10911084/diff/86006/sync/notifier/sync_invali...
sync/notifier/sync_invalidation_listener.h:157: // AckTracker::Delegate
implementation.
On 2013/02/15 22:32:12, akalin wrote:
> newline before

Done.

[commit-bot: I haz the power, 2013-02-22 02:51:59]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dcheng@chromium.org/10911084/109001

[commit-bot: I haz the power, 2013-02-22 02:52:14]

Presubmit check for 10911084-109001 failed and returned exit status 1.

INFO:root:Found 33 file(s).

Running presubmit commit checks ...
Running /b/commit-queue/workdir/chromium/PRESUBMIT.py
Running /b/commit-queue/workdir/chromium/chrome/PRESUBMIT.py
Running /b/commit-queue/workdir/chromium/chrome/browser/extensions/PRESUBMIT.py
Running /b/commit-queue/workdir/chromium/chrome/browser/sync/PRESUBMIT.py
Running /b/commit-queue/workdir/chromium/sync/PRESUBMIT.py

** Presubmit Messages **
You might be calling functions intended only for testing from
production code.  It is OK to ignore this warning if you know what
you are doing, as the heuristics used to detect the situation are
not perfect.  The commit queue will not block on this warning.
Email joi@chromium.org if you have questions.
  chrome/browser/sync/glue/android_invalidator_bridge.cc:41
    bool IsHandlerRegisteredForTest(syncer::InvalidationHandler* handler) const;
\
  chrome/browser/sync/glue/android_invalidator_bridge.cc:42
    syncer::ObjectIdSet GetRegisteredIdsForTest( \
  chrome/browser/sync/glue/android_invalidator_bridge.cc:113
    return invalidator_registrar_->IsHandlerRegisteredForTest(handler); \
  chrome/browser/sync/glue/android_invalidator_bridge.cc:187
    return core_->IsHandlerRegisteredForTest(handler); \
  chrome/browser/sync/glue/android_invalidator_bridge.cc:193
    return core_->GetRegisteredIdsForTest(handler);

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

Presubmit checks took 3.4s to calculate.

[dcheng, 2013-02-22 20:57:22]

+sky for chrome OWNERS rubberstamp. I believe the only file not covered by
another OWNERS file is chrome_to_mobile_service.cc.

[sky, 2013-02-22 21:21:00]

sky->msw

[dcheng, 2013-02-26 18:24:31]

On 2013/02/22 21:21:00, sky wrote:
> sky->msw

Ping!

[msw, 2013-02-26 23:17:25]

chrome_to_mobile_service.cc LGTM!
Sorry for the delayed response; I was on vacation.

[dcheng, 2013-02-28 19:22:08]

On 2013/02/26 23:17:25, msw wrote:
> chrome_to_mobile_service.cc LGTM!
> Sorry for the delayed response; I was on vacation.

sky, do you mind rubberstamping now that msw has signed off on the
chrome_to_mobile* changes? Thanks!

[sky, 2013-02-28 21:53:21]

LGTM

[commit-bot: I haz the power, 2013-02-28 22:00:17]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dcheng@chromium.org/10911084/109001

[commit-bot: I haz the power, 2013-02-28 22:40:52]

Sorry for I got bad news for ya.
Compile failed with a clobber build on ios_rel_device.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=ios_rel_de...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[dcheng, 2013-02-28 23:34:41]

Fred, mind taking one more look? I've updated the patch to use the new TickClock
interface.

[dcheng, 2013-03-04 22:12:20]

On 2013/02/28 23:34:41, dcheng wrote:
> Fred, mind taking one more look? I've updated the patch to use the new
TickClock
> interface.

Ping!

[akalin, 2013-03-04 22:39:22]

still lgtm

[commit-bot: I haz the power, 2013-03-04 22:39:50]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dcheng@chromium.org/10911084/129003

[commit-bot: I haz the power, 2013-03-05 02:30:28]

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

[commit-bot: I haz the power, 2013-03-05 02:32:33]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dcheng@chromium.org/10911084/129003

[commit-bot: I haz the power, 2013-03-05 03:51:46]

Change committed as 186079