Implement new invalidations ack tracking system

Implement new invalidations ack tracking system

Enable the new invalidation ack tracking API.  This system allows the
invalidations system to remember invalidations that have not been acted upon
yet across browser restarts.

New features include:
- Supports saving multiple invalidation payloads per ObjectID.
- Supports tracking lost or dropped invalidations.
- Remembers "unknown version" invalidations across restarts.
- Can send multiple payloads in a single callback.
- Does not periodically ping unacked invalidations.

A series of commits prior to this one have put much of the framework in place
already.  This CL does not change the types and signatures of client-facing
APIs very much, but it does enable perviously disbaled behavior.  This CL
includes updates to the client code to allow them to support the new semantics
with (hopefully) few changes to externally visible behavior.

Most of the big changes are in the classes and tests associated with the
internals of Invalidations.  Large parts of the SyncInvalidationListener and
InvalidatorStorage have been rewritten.

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

[rlarocque, 2013-11-11 21:10:11]

Here's the final patch in this series.

Although it touches a lot of files, most of the changes are fairly small.  The
big changes are in SyncInvalidationListener, InvalidatorStorage.  The rest was
reviewed and committed as dead code in earlier reviews.

Tim: Please review the changes in sync/, chrome/browser/sync/ and
chrome/browser/invalidation/.

[rlarocque, 2013-11-14 20:20:13]

+Pavel.

Pavel might have to maintain some of this code in the future.  He should be
given a chance to question and criticize the code before it's committed.

[tim (not reviewing), 2013-11-18 21:59:05]

Still reviewing SyncInvalidationListener.  From what I can tell and your
description, the rest of the patch mostly consists of nice, small changes.

https://codereview.chromium.org/56113003/diff/130001/chrome/browser/drive/dri...
File chrome/browser/drive/drive_notification_manager.cc (right):

https://codereview.chromium.org/56113003/diff/130001/chrome/browser/drive/dri...
chrome/browser/drive/drive_notification_manager.cc:79: // TODO: Acknowledge only
after fetch completes.
File a new bug for this since (old bug is closed).

https://codereview.chromium.org/56113003/diff/130001/chrome/browser/invalidat...
File chrome/browser/invalidation/invalidator_storage.cc (right):

https://codereview.chromium.org/56113003/diff/130001/chrome/browser/invalidat...
chrome/browser/invalidation/invalidator_storage.cc:43: const
syncer::UnackedInvalidationsMap& map) {
nit - indent.

https://codereview.chromium.org/56113003/diff/130001/chrome/browser/invalidat...
chrome/browser/invalidation/invalidator_storage.cc:70: // This prefs is
obsolete.  We register it so we can clear it.
nit - 'this pref'.

https://codereview.chromium.org/56113003/diff/130001/chrome/browser/invalidat...
File chrome/browser/invalidation/invalidator_storage_unittest.cc (right):

https://codereview.chromium.org/56113003/diff/130001/chrome/browser/invalidat...
chrome/browser/invalidation/invalidator_storage_unittest.cc:82:
syncer::Invalidation non_invalid_inv =
Wow. Non invalid invalidation :)

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

https://codereview.chromium.org/56113003/diff/130001/sync/internal_api/public...
sync/internal_api/public/base/invalidation.h:57: // of the Invalidations API
should not need to call this.
This comment makes it sound like this could go in the constructor / factory
function and be fully hidden from clients of this class?

https://codereview.chromium.org/56113003/diff/130001/sync/notifier/invalidati...
File sync/notifier/invalidation_state_tracker.h (right):

https://codereview.chromium.org/56113003/diff/130001/sync/notifier/invalidati...
sync/notifier/invalidation_state_tracker.h:51: // handled by our clients.  We
stores these invalidations on disk so we won't
nit - 'We store'

https://codereview.chromium.org/56113003/diff/130001/sync/notifier/sync_inval...
File sync/notifier/sync_invalidation_listener.cc (right):

https://codereview.chromium.org/56113003/diff/130001/sync/notifier/sync_inval...
sync/notifier/sync_invalidation_listener.cc:200: for (ObjectIdSet::iterator it =
objects_to_save.begin();
const_iterator?

https://codereview.chromium.org/56113003/diff/130001/sync/notifier/unacked_in...
File sync/notifier/unacked_invalidation_set_test_util.cc (right):

https://codereview.chromium.org/56113003/diff/130001/sync/notifier/unacked_in...
sync/notifier/unacked_invalidation_set_test_util.cc:179: }
// namespace test_util

[rlarocque, 2013-11-19 00:35:48]

https://codereview.chromium.org/56113003/diff/130001/chrome/browser/drive/dri...
File chrome/browser/drive/drive_notification_manager.cc (right):

https://codereview.chromium.org/56113003/diff/130001/chrome/browser/drive/dri...
chrome/browser/drive/drive_notification_manager.cc:79: // TODO: Acknowledge only
after fetch completes.
On 2013/11/18 21:59:05, timsteele wrote:
> File a new bug for this since (old bug is closed).

Done.

https://codereview.chromium.org/56113003/diff/130001/chrome/browser/invalidat...
File chrome/browser/invalidation/invalidator_storage.cc (right):

https://codereview.chromium.org/56113003/diff/130001/chrome/browser/invalidat...
chrome/browser/invalidation/invalidator_storage.cc:43: const
syncer::UnackedInvalidationsMap& map) {
On 2013/11/18 21:59:05, timsteele wrote:
> nit - indent.

Done.

https://codereview.chromium.org/56113003/diff/130001/chrome/browser/invalidat...
File chrome/browser/invalidation/invalidator_storage_unittest.cc (right):

https://codereview.chromium.org/56113003/diff/130001/chrome/browser/invalidat...
chrome/browser/invalidation/invalidator_storage_unittest.cc:82:
syncer::Invalidation non_invalid_inv =
On 2013/11/18 21:59:05, timsteele wrote:
> Wow. Non invalid invalidation :)
> 
> 

Renamed.

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

https://codereview.chromium.org/56113003/diff/130001/sync/internal_api/public...
sync/internal_api/public/base/invalidation.h:57: // of the Invalidations API
should not need to call this.
On 2013/11/18 21:59:05, timsteele wrote:
> This comment makes it sound like this could go in the constructor / factory
> function and be fully hidden from clients of this class?

Almost, but not quite.  The UnackedInvalidationSet::ExportInvalidations function
uses it for mostly good reasons.  The Invalidations we load from the preferences
file during startup can't be attached to a SyncInvalidationListener right away,
since it might not exist yet.  We end up storing a set of invalidations without
an attached ack_handler in the UnackedInvalidationSet for that reason.

We could do more to avoid calling this function as often as we do (there are
five different invocations of it), but we would need to keep at least one
invocation for the case described above, and another for tests.

https://codereview.chromium.org/56113003/diff/130001/sync/notifier/invalidati...
File sync/notifier/invalidation_state_tracker.h (right):

https://codereview.chromium.org/56113003/diff/130001/sync/notifier/invalidati...
sync/notifier/invalidation_state_tracker.h:51: // handled by our clients.  We
stores these invalidations on disk so we won't
On 2013/11/18 21:59:05, timsteele wrote:
> nit - 'We store'

Done.

https://codereview.chromium.org/56113003/diff/130001/sync/notifier/sync_inval...
File sync/notifier/sync_invalidation_listener.cc (right):

https://codereview.chromium.org/56113003/diff/130001/sync/notifier/sync_inval...
sync/notifier/sync_invalidation_listener.cc:200: for (ObjectIdSet::iterator it =
objects_to_save.begin();
On 2013/11/18 21:59:05, timsteele wrote:
> const_iterator?

Done.

https://codereview.chromium.org/56113003/diff/130001/sync/notifier/unacked_in...
File sync/notifier/unacked_invalidation_set_test_util.cc (right):

https://codereview.chromium.org/56113003/diff/130001/sync/notifier/unacked_in...
sync/notifier/unacked_invalidation_set_test_util.cc:179: }
On 2013/11/18 21:59:05, timsteele wrote:
> // namespace test_util

Done.

[tim (not reviewing), 2013-11-20 18:27:04]

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

https://codereview.chromium.org/56113003/diff/130001/sync/internal_api/public...
sync/internal_api/public/base/invalidation.cc:115: ack_handler_ = handler;
At the very least, this shouldn't ever change from a non-null value to a
different non-null value, correct?  A dcheck would be a good idea.  Might
consider renaming to InitAckHandler(...), but you don't have to.. up to you.

https://codereview.chromium.org/56113003/diff/130001/sync/notifier/sync_inval...
File sync/notifier/sync_invalidation_listener.cc (right):

https://codereview.chromium.org/56113003/diff/130001/sync/notifier/sync_inval...
sync/notifier/sync_invalidation_listener.cc:218: const ObjectIdInvalidationMap&
to_emit) {
|to_emit| is a subset of unacked_invalidations_map_, right?

https://codereview.chromium.org/56113003/diff/130001/sync/notifier/sync_inval...
sync/notifier/sync_invalidation_listener.cc:319:
invalidation_state_tracker_.Call(
What happens if this doesn't make it through before Chrome shuts down?

https://codereview.chromium.org/56113003/diff/130001/sync/notifier/sync_inval...
sync/notifier/sync_invalidation_listener.cc:356:
EmitInvalidations(object_id_invalidation_map);
Add a comment before this line that we need only Emit, not Dispatch, because the
invalidations are already saved (we just read from them).

In general it seems like you never want to call Emit directly, right? This makes
me wonder if we couldn't name Emit/Dispatch more clearly.  Is it a prerequisite
for Emit to operate on saved invalidations?  Does it need to take a parameter? 
Could we distinguish it that way by calling it EmitSavedInvalidations()?

https://codereview.chromium.org/56113003/diff/130001/sync/notifier/sync_inval...
File sync/notifier/sync_invalidation_listener.h (right):

https://codereview.chromium.org/56113003/diff/130001/sync/notifier/sync_inval...
sync/notifier/sync_invalidation_listener.h:151: void DispatchInvalidations(const
ObjectIdInvalidationMap& invalidations);
Comment the difference between these functions, especially Dispatch vs Emit.

[tim (not reviewing), 2013-11-20 18:27:43]

https://codereview.chromium.org/56113003/diff/130001/sync/notifier/sync_inval...
File sync/notifier/sync_invalidation_listener.cc (right):

https://codereview.chromium.org/56113003/diff/130001/sync/notifier/sync_inval...
sync/notifier/sync_invalidation_listener.cc:356:
EmitInvalidations(object_id_invalidation_map);
On 2013/11/20 18:27:04, timsteele wrote:
> Add a comment before this line that we need only Emit, not Dispatch, because
the
> invalidations are already saved (we just read from them).
> 
> In general it seems like you never want to call Emit directly, right? This
makes
> me wonder if we couldn't name Emit/Dispatch more clearly.  Is it a
prerequisite
> for Emit to operate on saved invalidations?  Does it need to take a parameter?

> Could we distinguish it that way by calling it EmitSavedInvalidations()?

s/never/rarely

[akalin, 2013-11-21 04:31:35]

took a look. couple of comments, but other than that I think I'll leave the rest
of the review to tim/pavel.

https://codereview.chromium.org/56113003/diff/490001/sync/notifier/invalidati...
File sync/notifier/invalidation_util.h (right):

https://codereview.chromium.org/56113003/diff/490001/sync/notifier/invalidati...
sync/notifier/invalidation_util.h:15: #include
"google/cacheinvalidation/include/types.h"
is this necessary? don't we want to keep the invalidation protobuf headers from
being pulled in by clients?

https://codereview.chromium.org/56113003/diff/490001/sync/notifier/sync_inval...
File sync/notifier/sync_invalidation_listener.cc (right):

https://codereview.chromium.org/56113003/diff/490001/sync/notifier/sync_inval...
sync/notifier/sync_invalidation_listener.cc:34: :
push_client_(push_client.get()),
Should not have .get(). Otherwise, you'd have two scoped_ptrs to the same thing
O_O

[rlarocque, 2013-11-21 20:09:26]

I think the latest upload addresses all of your comments.  I'm a bit unsure
since they're scattered all over the place by now and there's no good way to see
which ones haven't been resolved yet.  Please let me know if I've missed any.

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

https://codereview.chromium.org/56113003/diff/130001/sync/internal_api/public...
sync/internal_api/public/base/invalidation.cc:115: ack_handler_ = handler;
On 2013/11/20 18:27:04, timsteele wrote:
> At the very least, this shouldn't ever change from a non-null value to a
> different non-null value, correct?  A dcheck would be a good idea.  Might
> consider renaming to InitAckHandler(...), but you don't have to.. up to you.

Done.

https://codereview.chromium.org/56113003/diff/130001/sync/notifier/sync_inval...
File sync/notifier/sync_invalidation_listener.cc (right):

https://codereview.chromium.org/56113003/diff/130001/sync/notifier/sync_inval...
sync/notifier/sync_invalidation_listener.cc:211:
invalidation_state_tracker_.Call(
What happens if this call doesn't make it through before shutdown?

If that happens, we could very well drop an invalidation.  This is not a
regression, though.  The old code had more complicated thread logic, but it
still acknowledged the the invalidation to the server before it wrote the
invalidation to disk.  

What the old system did accomplish but this one does not is ensure that  it
waited until the invalidation has been written to the prefs system (in memory
only; it's guaranteed to be written during a graceful shutdown but not during a
crash) before the invalidation is communicated to the observers.  I'm not sure
why it did this.  Maybe it had something to do with the periodic retries.

Could we actually close this loophole and prevent invalidations from being
dropped here?  Probably.  We have a reasonable amount of time (tens of seconds)
to acknowledge the invalidation to the server before anything bad happens.  We
can probably delay that acknowledgement at least until the invalidation has been
written to the prefs system, if not to disk.  But the server periodically pings
the client with retries if we don't acknowledge right away, so that could
complicate matters.  I'd prefer to deal with those issues in a separate CL.
Because this is not a regression, I feel justified in delaying this work until
later.

https://codereview.chromium.org/56113003/diff/130001/sync/notifier/sync_inval...
sync/notifier/sync_invalidation_listener.cc:218: const ObjectIdInvalidationMap&
to_emit) {
On 2013/11/20 18:27:04, timsteele wrote:
> |to_emit| is a subset of unacked_invalidations_map_, right?

Yep, they should be.  For every invalidation we emit, we should have a copy of
it sitting in the unacked_invalidations_map_, periodically saved the the storage
(the preferences store), and waiting for acknowledgement to tell us we no longer
need to keep saving them.

https://codereview.chromium.org/56113003/diff/130001/sync/notifier/sync_inval...
sync/notifier/sync_invalidation_listener.cc:319:
invalidation_state_tracker_.Call(
On 2013/11/20 18:27:04, timsteele wrote:
> What happens if this doesn't make it through before Chrome shuts down?

In this case it's no big deal.  

We'll fail to permanently drop the invalidation.  We'll fail to record the
"missing invalidations" flag-equivalent (though it may be still set from
earlier) and fail to remove this invalidation from the list of saved
invalidations.

On the next restart, the client will likely receive this invalidation and act on
it.  So we actually get a better result than we would have gotten if this
invalidation hadn't gotten through.

You may have meant to ask this question about a different call to
SetSavedInvalidations.  I'll add a comment there.

https://codereview.chromium.org/56113003/diff/130001/sync/notifier/sync_inval...
sync/notifier/sync_invalidation_listener.cc:356:
EmitInvalidations(object_id_invalidation_map);
On 2013/11/20 18:27:44, timsteele wrote:
> On 2013/11/20 18:27:04, timsteele wrote:
> > Add a comment before this line that we need only Emit, not Dispatch, because
> the
> > invalidations are already saved (we just read from them).
> > 
> > In general it seems like you never want to call Emit directly, right? This
> makes
> > me wonder if we couldn't name Emit/Dispatch more clearly.  Is it a
> prerequisite
> > for Emit to operate on saved invalidations?  Does it need to take a
parameter?
> 
> > Could we distinguish it that way by calling it EmitSavedInvalidations()?
> 
> s/never/rarely

You're right.  The rename makes sense.

I've added a comment here.

I've also added some comments to the Dispatch, Emit, and Save functions in the
header file.

https://codereview.chromium.org/56113003/diff/130001/sync/notifier/sync_inval...
File sync/notifier/sync_invalidation_listener.h (right):

https://codereview.chromium.org/56113003/diff/130001/sync/notifier/sync_inval...
sync/notifier/sync_invalidation_listener.h:151: void DispatchInvalidations(const
ObjectIdInvalidationMap& invalidations);
On 2013/11/20 18:27:04, timsteele wrote:
> Comment the difference between these functions, especially Dispatch vs Emit.  

Done.

https://codereview.chromium.org/56113003/diff/490001/sync/notifier/invalidati...
File sync/notifier/invalidation_util.h (right):

https://codereview.chromium.org/56113003/diff/490001/sync/notifier/invalidati...
sync/notifier/invalidation_util.h:15: #include
"google/cacheinvalidation/include/types.h"
On 2013/11/21 04:31:36, akalin wrote:
> is this necessary? don't we want to keep the invalidation protobuf headers
from
> being pulled in by clients?

This one can be removed.  It's fixed in the latest upload.

There are other places where it must be included, though.  Most often it's
included so we can get the definition of invalidation::ObjectId.

Maybe some day we can land a change to the upstream cacheinvalidation code that
pulls that definition into a separate header file.  That would let us eliminate
most of these includes.

https://codereview.chromium.org/56113003/diff/490001/sync/notifier/sync_inval...
File sync/notifier/sync_invalidation_listener.cc (right):

https://codereview.chromium.org/56113003/diff/490001/sync/notifier/sync_inval...
sync/notifier/sync_invalidation_listener.cc:34: :
push_client_(push_client.get()),
On 2013/11/21 04:31:36, akalin wrote:
> Should not have .get(). Otherwise, you'd have two scoped_ptrs to the same
thing
> O_O

This hasn't changed in a long time.  I think this is confusing, but safe.

The SyncSystemResources takes ownership of the PushClient on line 35.  The
push_client_ member initialized here is a PushClient*, not a scoped_ptr.

[tim (not reviewing), 2013-11-22 01:24:53]

Changes I reviewed & commented on LGTM.

https://codereview.chromium.org/56113003/diff/130001/sync/notifier/sync_inval...
File sync/notifier/sync_invalidation_listener.cc (right):

https://codereview.chromium.org/56113003/diff/130001/sync/notifier/sync_inval...
sync/notifier/sync_invalidation_listener.cc:211:
invalidation_state_tracker_.Call(
On 2013/11/21 20:09:27, rlarocque wrote:
> What happens if this call doesn't make it through before shutdown?
> 
> If that happens, we could very well drop an invalidation.  This is not a
> regression, though.  The old code had more complicated thread logic, but it
> still acknowledged the the invalidation to the server before it wrote the
> invalidation to disk.  
> 
> What the old system did accomplish but this one does not is ensure that  it
> waited until the invalidation has been written to the prefs system (in memory
> only; it's guaranteed to be written during a graceful shutdown but not during
a
> crash) before the invalidation is communicated to the observers.  I'm not sure
> why it did this.  Maybe it had something to do with the periodic retries.
> 
> Could we actually close this loophole and prevent invalidations from being
> dropped here?  Probably.  We have a reasonable amount of time (tens of
seconds)
> to acknowledge the invalidation to the server before anything bad happens.  We
> can probably delay that acknowledgement at least until the invalidation has
been
> written to the prefs system, if not to disk.  But the server periodically
pings
> the client with retries if we don't acknowledge right away, so that could
> complicate matters.  I'd prefer to deal with those issues in a separate CL.
> Because this is not a regression, I feel justified in delaying this work until
> later.

The server won't ping periodicially within the 10s interval, will it? 

OK, as long as we don't forget / track this somewhere. Is there a bug?

https://codereview.chromium.org/56113003/diff/600001/sync/notifier/sync_inval...
File sync/notifier/sync_invalidation_listener.h (right):

https://codereview.chromium.org/56113003/diff/600001/sync/notifier/sync_inval...
sync/notifier/sync_invalidation_listener.h:169: void
EmitSavedInvalidations(const ObjectIdInvalidationMap& to_emit);
+1

[rlarocque, 2013-11-22 21:03:31]

Thanks for the review.

https://codereview.chromium.org/56113003/diff/130001/sync/notifier/sync_inval...
File sync/notifier/sync_invalidation_listener.cc (right):

https://codereview.chromium.org/56113003/diff/130001/sync/notifier/sync_inval...
sync/notifier/sync_invalidation_listener.cc:211:
invalidation_state_tracker_.Call(
On 2013/11/22 01:24:54, timsteele wrote:
> On 2013/11/21 20:09:27, rlarocque wrote:
> > What happens if this call doesn't make it through before shutdown?
> > 
> > If that happens, we could very well drop an invalidation.  This is not a
> > regression, though.  The old code had more complicated thread logic, but it
> > still acknowledged the the invalidation to the server before it wrote the
> > invalidation to disk.  
> > 
> > What the old system did accomplish but this one does not is ensure that  it
> > waited until the invalidation has been written to the prefs system (in
memory
> > only; it's guaranteed to be written during a graceful shutdown but not
during
> a
> > crash) before the invalidation is communicated to the observers.  I'm not
sure
> > why it did this.  Maybe it had something to do with the periodic retries.
> > 
> > Could we actually close this loophole and prevent invalidations from being
> > dropped here?  Probably.  We have a reasonable amount of time (tens of
> seconds)
> > to acknowledge the invalidation to the server before anything bad happens. 
We
> > can probably delay that acknowledgement at least until the invalidation has
> been
> > written to the prefs system, if not to disk.  But the server periodically
> pings
> > the client with retries if we don't acknowledge right away, so that could
> > complicate matters.  I'd prefer to deal with those issues in a separate CL.
> > Because this is not a regression, I feel justified in delaying this work
until
> > later.
> 
> The server won't ping periodicially within the 10s interval, will it? 
> 
> OK, as long as we don't forget / track this somewhere. Is there a bug?

Yes, it will send the same invalidation repeatedly for a while.  I'm counting
the time of the periodic retry period in the "tens of seconds" estimate.  The
first retry might arrive after a much shorter period of time.  (My memory is
foggy on this point.  I'd have to look it up to be sure.)

I filed http://crbug.com/322668.

[rlarocque, 2013-11-22 21:20:36]

Hi Everyone,

Remember https://codereview.chromium.org/23441042, the CL that refactored the
invalidations types?  It's time for another CL along those lines.

This CL takes advantage of those new types.  I've changed the internals of the
invalidations system so now it actually does send multiple invalidations at once
and does support local ack tracking (through a newly introduced API).

As a result of those changes, the clients needed to be updated, too.  They need
to acknowledge invalidations as they receive them; otherwise they would leak
memory.  They need to handle multiple incoming invalidations since the
invalidations service no longer filters incoming invalidations to try to deliver
only the most recent one.

The changes are different for each client.  I've tried to respect the original
authors' intents and minimize changes as much as possible.  When reviewing,
please be on the lookout for changes to client behavior.  Any such changes are
probably unintentional, and may indicate a bug.

Reviewers, here are your assignments:
- stepco and joaodasilva: Please review chrome/browser/cloud/*
- dcheng: Please review chrome/browser/extensions/api/push_messaging/*
- satorux: Please review chrome/browser/drive/*

Thanks,
Richard.

[satorux1, 2013-11-25 01:57:45]

chrome/browser/drive/drive_notification_manager.cc lgtm

https://codereview.chromium.org/56113003/diff/600001/chrome/browser/drive/dri...
File chrome/browser/drive/drive_notification_manager.cc (right):

https://codereview.chromium.org/56113003/diff/600001/chrome/browser/drive/dri...
chrome/browser/drive/drive_notification_manager.cc:79: // TODO: Acknowledge only
after fetch completes.  See crbug.com/320878.
nit: TODO should have a name.

[Joao da Silva, 2013-11-25 10:11:18]

policy lgtm. Steve should have a look too though

https://codereview.chromium.org/56113003/diff/600001/chrome/browser/policy/cl...
File chrome/browser/policy/cloud/cloud_policy_invalidator.h (right):

https://codereview.chromium.org/56113003/diff/600001/chrome/browser/policy/cl...
chrome/browser/policy/cloud/cloud_policy_invalidator.h:171:
scoped_ptr<syncer::Invalidation> invalidation_;
#include "base/memory/scoped_ptr.h"

[rlarocque, 2013-11-25 19:34:23]

https://codereview.chromium.org/56113003/diff/600001/chrome/browser/drive/dri...
File chrome/browser/drive/drive_notification_manager.cc (right):

https://codereview.chromium.org/56113003/diff/600001/chrome/browser/drive/dri...
chrome/browser/drive/drive_notification_manager.cc:79: // TODO: Acknowledge only
after fetch completes.  See crbug.com/320878.
On 2013/11/25 01:57:46, satorux1 wrote:
> nit: TODO should have a name.

Can you suggest an owner?

Implementing this would require intimate knowledge of the
DriveNotificationManager.  I don't think I'm qualified for the task.

I was just carrying over the old TODO without much thought.  Now that I think
about it, maybe this should be rephrased as "investigate if supporting
invalidations local acknowledgements is worthwhile", or removed entirely.  If
the current invalidations behavior suits your needs, then I guess there's no
need to spend much effort on improving its reliability.

[Steve Condie, 2013-11-25 19:57:28]

policy code LGTM after minor comments

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

https://codereview.chromium.org/56113003/diff/600001/chrome/browser/policy/cl...
chrome/browser/policy/cloud/cloud_policy_invalidator.cc:338: // Cancel any
scheduled invalidate callbacks.
Change back to current comment. I think this was the comment in an old version
of this code.

https://codereview.chromium.org/56113003/diff/600001/chrome/browser/policy/cl...
File chrome/browser/policy/cloud/cloud_policy_invalidator_unittest.cc (right):

https://codereview.chromium.org/56113003/diff/600001/chrome/browser/policy/cl...
chrome/browser/policy/cloud/cloud_policy_invalidator_unittest.cc:136: bool
CheckStateChangedNotCalled();
CheckStateChangedNotCalled not used.

[rlarocque, 2013-11-25 21:51:24]

Uploaded some fixes from client-specific reviews.

Also reverted a change I made in response to Tim's review.  I believe I was
correct in thinking that the ack handler was never reset in test code, but
forgot that invalidations saved to the UnackedInvalidationsSet might have their
ack_handles reset when they're eventually sent out.  I've reverted the
s/set_ack_handler/InitAckHandler/ change and removed the DCHECK for now.  Other
solutions are possible, but I'd like to start with this one just to get the
trybots green again.

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

https://codereview.chromium.org/56113003/diff/600001/chrome/browser/policy/cl...
chrome/browser/policy/cloud/cloud_policy_invalidator.cc:338: // Cancel any
scheduled invalidate callbacks.
On 2013/11/25 19:57:29, stepco wrote:
> Change back to current comment. I think this was the comment in an old version
> of this code.

Thanks for noticing.  This patch has been in progress for months.  I'm not
surprised some rebase issues made it in here.

https://codereview.chromium.org/56113003/diff/600001/chrome/browser/policy/cl...
File chrome/browser/policy/cloud/cloud_policy_invalidator.h (right):

https://codereview.chromium.org/56113003/diff/600001/chrome/browser/policy/cl...
chrome/browser/policy/cloud/cloud_policy_invalidator.h:171:
scoped_ptr<syncer::Invalidation> invalidation_;
On 2013/11/25 10:11:19, Joao da Silva wrote:
> #include "base/memory/scoped_ptr.h"

Done.

https://codereview.chromium.org/56113003/diff/600001/chrome/browser/policy/cl...
File chrome/browser/policy/cloud/cloud_policy_invalidator_unittest.cc (right):

https://codereview.chromium.org/56113003/diff/600001/chrome/browser/policy/cl...
chrome/browser/policy/cloud/cloud_policy_invalidator_unittest.cc:136: bool
CheckStateChangedNotCalled();
On 2013/11/25 19:57:29, stepco wrote:
> CheckStateChangedNotCalled not used.

Done.

[dcheng, 2013-11-26 03:19:05]

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

[rlarocque, 2013-11-26 17:59:17]

Thanks for the reviews, everyone!

I've made a few changes following the reviews:
- I've rewritten the TODO comment satorux mentioned in his review.
- I've reverted some changes to the ack handler initialization that Tim had
suggested and I had originally agreed with.
- Made a tiny change to logging in sync_manager_impl.cc to fix a log spam
presubmit check.
- Rebased the patch.

Let me know if you have any objections or further comments.

I'm going to start some trybots in preparation for sending this to the CQ.

[commit-bot: I haz the power, 2013-11-26 20:59:13]

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

[commit-bot: I haz the power, 2013-11-26 21:45:34]

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

[commit-bot: I haz the power, 2013-11-26 21:47:50]

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

[commit-bot: I haz the power, 2013-11-26 22:46:37]

Change committed as 237421