Implement features needed for local ack handling in InvalidationStateTracker.

Implement features needed for local ack handling in InvalidationStateTracker.

Adds the ability to save payloads in InvalidationStateTracker, and also adds
the functionality to generate, track, and acknowledge local ack handles.

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

[dcheng, 2012-11-22 01:14:53]

Note that I thought more about the debug info you asked about last time. Here
are my thoughts (which is part of the reason it's not part of the currently
uploaded CL):
- Timestamp: this is the only debugging field I think could be valuable to
capture. However, it's not clear whether it's better to embed TimeTicks (pros:
Good for reference times. cons: Not actually correlated to real time, dumping
the raw TimeTicks value isn't likely to be useful, though you could convert it
to a TimeDelta...) or Time (pros: easily convertible to text. cons: can go
backwards).

- Object ID: I'm not convinced this will be particularly helpful since the
generated ack handles are always returned in a mapping of object IDs -> ack
handles. Reversing the mapping for debugging should be pretty trivial.

- Other fields (like the original ack handle that the server provided): Requires
passing in debug information to GenerateAckHandles. This seems... meh. It's
certainly possible to create a mapping of object IDs to any associated debug
info, but I think that object ID (from InvalidatorStorage) + timestamp + the
actual ack handle string should be plenty for debugging.

https://codereview.chromium.org/11415049/diff/2001/chrome/browser/sync/invali...
File chrome/browser/sync/invalidations/invalidator_storage.cc (right):

https://codereview.chromium.org/11415049/diff/2001/chrome/browser/sync/invali...
chrome/browser/sync/invalidations/invalidator_storage.cc:78: }
This code kind of makes me wonder if we shouldn't be using protobuf here... =/

[akalin, 2012-11-28 00:00:07]

On 2012/11/22 01:14:53, dcheng wrote:
> Note that I thought more about the debug info you asked about last time. Here
> are my thoughts (which is part of the reason it's not part of the currently
> uploaded CL):
> - Timestamp: this is the only debugging field I think could be valuable to
> capture. However, it's not clear whether it's better to embed TimeTicks (pros:
> Good for reference times. cons: Not actually correlated to real time, dumping
> the raw TimeTicks value isn't likely to be useful, though you could convert it
> to a TimeDelta...) or Time (pros: easily convertible to text. cons: can go
> backwards).

I like using Time.  Is the going backwards part a real problem?

> - Object ID: I'm not convinced this will be particularly helpful since the
> generated ack handles are always returned in a mapping of object IDs -> ack
> handles. Reversing the mapping for debugging should be pretty trivial.
> 
> - Other fields (like the original ack handle that the server provided):
Requires
> passing in debug information to GenerateAckHandles. This seems... meh. It's
> certainly possible to create a mapping of object IDs to any associated debug
> info, but I think that object ID (from InvalidatorStorage) + timestamp + the
> actual ack handle string should be plenty for debugging.
> 
>
https://codereview.chromium.org/11415049/diff/2001/chrome/browser/sync/invali...
> File chrome/browser/sync/invalidations/invalidator_storage.cc (right):

Fair enough.  We can always add it later if needed.

> 
>
https://codereview.chromium.org/11415049/diff/2001/chrome/browser/sync/invali...
> chrome/browser/sync/invalidations/invalidator_storage.cc:78: }
> This code kind of makes me wonder if we shouldn't be using protobuf here... =/

Yeah. :/

[akalin, 2012-11-28 00:11:42]

some nits

https://codereview.chromium.org/11415049/diff/18/chrome/browser/sync/invalida...
File chrome/browser/sync/invalidations/invalidator_storage.cc (right):

https://codereview.chromium.org/11415049/diff/18/chrome/browser/sync/invalida...
chrome/browser/sync/invalidations/invalidator_storage.cc:55: if
(value.GetString(kMaxVersionKey, &max_version_str)) {
isn't kMaxVersionkey always present?  looks like it's always set in
ObjectIdAndStateToValue

https://codereview.chromium.org/11415049/diff/18/chrome/browser/sync/invalida...
chrome/browser/sync/invalidations/invalidator_storage.cc:62: *id =
invalidation::ObjectId(source, name);
move this closer to where source and name are set?  newline afterwards perhaps?

https://codereview.chromium.org/11415049/diff/18/chrome/browser/sync/invalida...
chrome/browser/sync/invalidations/invalidator_storage.cc:63: // The ack handle
fields won't be set if upgrading from previous versions of
clear state first?

https://codereview.chromium.org/11415049/diff/18/chrome/browser/sync/invalida...
chrome/browser/sync/invalidations/invalidator_storage.cc:69: // TODO(dcheng):
Consider regenerating the latest ack handle value on Chrome
hmm i don't understand this comment.  what do you mean?

https://codereview.chromium.org/11415049/diff/18/chrome/browser/sync/invalida...
chrome/browser/sync/invalidations/invalidator_storage.cc:304:
task_runner->PostTask(FROM_HERE, base::Bind(callback, ack_handles));
do something with return value?  maybe just ignore_result?

https://codereview.chromium.org/11415049/diff/18/chrome/browser/sync/invalida...
File chrome/browser/sync/invalidations/invalidator_storage_unittest.cc (right):

https://codereview.chromium.org/11415049/diff/18/chrome/browser/sync/invalida...
chrome/browser/sync/invalidations/invalidator_storage_unittest.cc:60: // Set max
versions for various keys and verify that they are written and read
update comment and test name (version -> state)

https://codereview.chromium.org/11415049/diff/18/chrome/browser/sync/invalida...
chrome/browser/sync/invalidations/invalidator_storage_unittest.cc:65:
InvalidationStateMap expected_max_versions;
upate name

https://codereview.chromium.org/11415049/diff/18/chrome/browser/sync/invalida...
chrome/browser/sync/invalidations/invalidator_storage_unittest.cc:93:
InvalidationStateMap expected_max_versions;
names

https://codereview.chromium.org/11415049/diff/18/chrome/browser/sync/invalida...
chrome/browser/sync/invalidations/invalidator_storage_unittest.cc:242:
TEST_F(InvalidatorStorageTest, DeserializeFromListBasic) {
perhaps split this test up into smaller ones?

https://codereview.chromium.org/11415049/diff/18/sync/internal_api/public/bas...
File sync/internal_api/public/base/invalidation.h (right):

https://codereview.chromium.org/11415049/diff/18/sync/internal_api/public/bas...
sync/internal_api/public/base/invalidation.h:25: 
comment somewhere that default constructor creates an invalid ack handle

https://codereview.chromium.org/11415049/diff/18/sync/internal_api/public/bas...
File sync/internal_api/public/base/invalidation_test_util.h (right):

https://codereview.chromium.org/11415049/diff/18/sync/internal_api/public/bas...
sync/internal_api/public/base/invalidation_test_util.h:19: const AckHandle&
expected);
fit on prev line?

https://codereview.chromium.org/11415049/diff/18/sync/notifier/fake_invalidat...
File sync/notifier/fake_invalidation_state_tracker.cc (right):

https://codereview.chromium.org/11415049/diff/18/sync/notifier/fake_invalidat...
sync/notifier/fake_invalidation_state_tracker.cc:68:
task_runner->PostTask(FROM_HERE, base::Bind(callback, ack_handles));
do something with return value

(ADD_FAILURE?)

https://codereview.chromium.org/11415049/diff/18/sync/notifier/invalidation_s...
File sync/notifier/invalidation_state_tracker.h (right):

https://codereview.chromium.org/11415049/diff/18/sync/notifier/invalidation_s...
sync/notifier/invalidation_state_tracker.h:77: virtual void Acknowledge(const
invalidation::ObjectId& id,
newline before, add comment?

https://codereview.chromium.org/11415049/diff/18/sync/tools/sync_client.cc
File sync/tools/sync_client.cc (right):

https://codereview.chromium.org/11415049/diff/18/sync/tools/sync_client.cc#ne...
sync/tools/sync_client.cc:112: task_runner->PostTask(FROM_HERE,
base::Bind(callback, ack_handles));
process return value (CHECK?)

https://codereview.chromium.org/11415049/diff/18/sync/tools/sync_listen_notif...
File sync/tools/sync_listen_notifications.cc (right):

https://codereview.chromium.org/11415049/diff/18/sync/tools/sync_listen_notif...
sync/tools/sync_listen_notifications.cc:117: LOG(INFO) << "Setting bootstrap
data to: " << base64_data;
boo, duplicated code.  can you pull this out into its own file while you're in
here?

https://codereview.chromium.org/11415049/diff/18/sync/tools/sync_listen_notif...
sync/tools/sync_listen_notifications.cc:128: task_runner->PostTask(FROM_HERE,
base::Bind(callback, ack_handles));
process return values (CHECK?)

[dcheng, 2012-11-30 01:42:54]

I've also added base::Time as a member of AckHandle.

https://codereview.chromium.org/11415049/diff/18/chrome/browser/sync/invalida...
File chrome/browser/sync/invalidations/invalidator_storage.cc (right):

https://codereview.chromium.org/11415049/diff/18/chrome/browser/sync/invalida...
chrome/browser/sync/invalidations/invalidator_storage.cc:55: if
(value.GetString(kMaxVersionKey, &max_version_str)) {
On 2012/11/28 00:11:42, akalin wrote:
> isn't kMaxVersionkey always present?  looks like it's always set in
> ObjectIdAndStateToValue

Hm. Yeah, you're right. I was thinking from the wrong perspective; clearly, by
the time it's persisted to disk, a version number will be present. Fixed.

https://codereview.chromium.org/11415049/diff/18/chrome/browser/sync/invalida...
chrome/browser/sync/invalidations/invalidator_storage.cc:62: *id =
invalidation::ObjectId(source, name);
On 2012/11/28 00:11:42, akalin wrote:
> move this closer to where source and name are set?  newline afterwards
perhaps?

I did this intentionally to avoid a state where the output arguments are "half"
initialized. But it doesn't matter.

https://codereview.chromium.org/11415049/diff/18/chrome/browser/sync/invalida...
chrome/browser/sync/invalidations/invalidator_storage.cc:63: // The ack handle
fields won't be set if upgrading from previous versions of
On 2012/11/28 00:11:42, akalin wrote:
> clear state first?

While this could be less error prone, it's also redundant (we pass in a default
constructed state which does all the initialization currently). I'm not sure of
a good way to defer that initialization here =/

https://codereview.chromium.org/11415049/diff/18/chrome/browser/sync/invalida...
chrome/browser/sync/invalidations/invalidator_storage.cc:69: // TODO(dcheng):
Consider regenerating the latest ack handle value on Chrome
On 2012/11/28 00:11:42, akalin wrote:
> hmm i don't understand this comment.  what do you mean?

One of my original ideas was to ignore the persist current/expected ack handle
values (other than comparing for equality), and then regenerate new ones. That
way, we can easily update the AckHandle serialization format if needed.

However, I'm not convinced this is needed so I've removed the comment.

https://codereview.chromium.org/11415049/diff/18/chrome/browser/sync/invalida...
chrome/browser/sync/invalidations/invalidator_storage.cc:304:
task_runner->PostTask(FROM_HERE, base::Bind(callback, ack_handles));
On 2012/11/28 00:11:42, akalin wrote:
> do something with return value?  maybe just ignore_result?

Done.

https://codereview.chromium.org/11415049/diff/18/chrome/browser/sync/invalida...
File chrome/browser/sync/invalidations/invalidator_storage_unittest.cc (right):

https://codereview.chromium.org/11415049/diff/18/chrome/browser/sync/invalida...
chrome/browser/sync/invalidations/invalidator_storage_unittest.cc:60: // Set max
versions for various keys and verify that they are written and read
On 2012/11/28 00:11:42, akalin wrote:
> update comment and test name (version -> state)

Done.

https://codereview.chromium.org/11415049/diff/18/chrome/browser/sync/invalida...
chrome/browser/sync/invalidations/invalidator_storage_unittest.cc:65:
InvalidationStateMap expected_max_versions;
On 2012/11/28 00:11:42, akalin wrote:
> upate name

Done.

https://codereview.chromium.org/11415049/diff/18/chrome/browser/sync/invalida...
chrome/browser/sync/invalidations/invalidator_storage_unittest.cc:93:
InvalidationStateMap expected_max_versions;
On 2012/11/28 00:11:42, akalin wrote:
> names

Done.

https://codereview.chromium.org/11415049/diff/18/chrome/browser/sync/invalida...
chrome/browser/sync/invalidations/invalidator_storage_unittest.cc:242:
TEST_F(InvalidatorStorageTest, DeserializeFromListBasic) {
On 2012/11/28 00:11:42, akalin wrote:
> perhaps split this test up into smaller ones?

Done.

https://codereview.chromium.org/11415049/diff/18/sync/internal_api/public/bas...
File sync/internal_api/public/base/invalidation.h (right):

https://codereview.chromium.org/11415049/diff/18/sync/internal_api/public/bas...
sync/internal_api/public/base/invalidation.h:25: 
On 2012/11/28 00:11:42, akalin wrote:
> comment somewhere that default constructor creates an invalid ack handle

There is no default constructor =)

https://codereview.chromium.org/11415049/diff/18/sync/internal_api/public/bas...
File sync/internal_api/public/base/invalidation_test_util.h (right):

https://codereview.chromium.org/11415049/diff/18/sync/internal_api/public/bas...
sync/internal_api/public/base/invalidation_test_util.h:19: const AckHandle&
expected);
On 2012/11/28 00:11:42, akalin wrote:
> fit on prev line?

Done.

https://codereview.chromium.org/11415049/diff/18/sync/notifier/fake_invalidat...
File sync/notifier/fake_invalidation_state_tracker.cc (right):

https://codereview.chromium.org/11415049/diff/18/sync/notifier/fake_invalidat...
sync/notifier/fake_invalidation_state_tracker.cc:68:
task_runner->PostTask(FROM_HERE, base::Bind(callback, ack_handles));
On 2012/11/28 00:11:42, akalin wrote:
> do something with return value
> 
> (ADD_FAILURE?)

Done.

https://codereview.chromium.org/11415049/diff/18/sync/notifier/invalidation_s...
File sync/notifier/invalidation_state_tracker.h (right):

https://codereview.chromium.org/11415049/diff/18/sync/notifier/invalidation_s...
sync/notifier/invalidation_state_tracker.h:77: virtual void Acknowledge(const
invalidation::ObjectId& id,
On 2012/11/28 00:11:42, akalin wrote:
> newline before, add comment?

Done.

https://codereview.chromium.org/11415049/diff/18/sync/tools/sync_client.cc
File sync/tools/sync_client.cc (right):

https://codereview.chromium.org/11415049/diff/18/sync/tools/sync_client.cc#ne...
sync/tools/sync_client.cc:112: task_runner->PostTask(FROM_HERE,
base::Bind(callback, ack_handles));
On 2012/11/28 00:11:42, akalin wrote:
> process return value (CHECK?)

Done.

https://codereview.chromium.org/11415049/diff/18/sync/tools/sync_listen_notif...
File sync/tools/sync_listen_notifications.cc (right):

https://codereview.chromium.org/11415049/diff/18/sync/tools/sync_listen_notif...
sync/tools/sync_listen_notifications.cc:117: LOG(INFO) << "Setting bootstrap
data to: " << base64_data;
On 2012/11/28 00:11:42, akalin wrote:
> boo, duplicated code.  can you pull this out into its own file while you're in
> here?

Done.
There's some TODOs with your name attached for refactoring (e.g.
sync_client.cc:53); are those TODOs satisfied by this refactoring (and can
therefor be removed)?

Also, one version prefers LOG(INFO) while the other uses VLOG(1). Any
preference? I've used LOG(INFO) for now since I assume people running these
tools want to see them by default.

https://codereview.chromium.org/11415049/diff/18/sync/tools/sync_listen_notif...
sync/tools/sync_listen_notifications.cc:128: task_runner->PostTask(FROM_HERE,
base::Bind(callback, ack_handles));
On 2012/11/28 00:11:42, akalin wrote:
> process return values (CHECK?)

Done.

[akalin, 2012-11-30 02:45:15]

https://codereview.chromium.org/11415049/diff/18/chrome/browser/sync/invalida...
File chrome/browser/sync/invalidations/invalidator_storage.cc (right):

https://codereview.chromium.org/11415049/diff/18/chrome/browser/sync/invalida...
chrome/browser/sync/invalidations/invalidator_storage.cc:63: // The ack handle
fields won't be set if upgrading from previous versions of
On 2012/11/30 01:42:54, dcheng wrote:
> On 2012/11/28 00:11:42, akalin wrote:
> > clear state first?
> 
> While this could be less error prone, it's also redundant (we pass in a
default
> constructed state which does all the initialization currently). I'm not sure
of
> a good way to defer that initialization here =/

i guess it's okay since this is a private function.

https://codereview.chromium.org/11415049/diff/18/sync/internal_api/public/bas...
File sync/internal_api/public/base/invalidation.h (right):

https://codereview.chromium.org/11415049/diff/18/sync/internal_api/public/bas...
sync/internal_api/public/base/invalidation.h:25: 
On 2012/11/30 01:42:54, dcheng wrote:
> On 2012/11/28 00:11:42, akalin wrote:
> > comment somewhere that default constructor creates an invalid ack handle
> 
> There is no default constructor =)

isn't there an implicit one?  or does the private constructor prevent it from
being instantiated?

Aren't we stuffing these in STL containers?

[dcheng, 2012-11-30 02:48:30]

https://codereview.chromium.org/11415049/diff/18/sync/internal_api/public/bas...
File sync/internal_api/public/base/invalidation.h (right):

https://codereview.chromium.org/11415049/diff/18/sync/internal_api/public/bas...
sync/internal_api/public/base/invalidation.h:25: 
On 2012/11/30 02:45:15, akalin wrote:
> On 2012/11/30 01:42:54, dcheng wrote:
> > On 2012/11/28 00:11:42, akalin wrote:
> > > comment somewhere that default constructor creates an invalid ack handle
> > 
> > There is no default constructor =)
> 
> isn't there an implicit one?  or does the private constructor prevent it from
> being instantiated?

We define a constructor which suppress the generation of the default
constructor.

> 
> Aren't we stuffing these in STL containers?

Yes, we do. That's why I use insert() instead of [].

[akalin, 2012-11-30 02:53:02]

few more

https://codereview.chromium.org/11415049/diff/9025/chrome/browser/sync/invali...
File chrome/browser/sync/invalidations/invalidator_storage.cc (right):

https://codereview.chromium.org/11415049/diff/9025/chrome/browser/sync/invali...
chrome/browser/sync/invalidations/invalidator_storage.cc:301:
ignore_result(task_runner->PostTask(FROM_HERE,
compiler_specific.h for ignore_result

https://codereview.chromium.org/11415049/diff/9025/chrome/browser/sync/invali...
File chrome/browser/sync/invalidations/invalidator_storage_unittest.cc (right):

https://codereview.chromium.org/11415049/diff/9025/chrome/browser/sync/invali...
chrome/browser/sync/invalidations/invalidator_storage_unittest.cc:272: // Tests
that deserializaing well-formed values when optional parameters are
deserializing

https://codereview.chromium.org/11415049/diff/9025/sync/test/null_invalidatio...
File sync/test/null_invalidation_state_tracker.cc (right):

https://codereview.chromium.org/11415049/diff/9025/sync/test/null_invalidatio...
sync/test/null_invalidation_state_tracker.cc:35: LOG(INFO) << "Forgetting
invalidation state for " << ObjectIdToString(*it);
don't we have ObjectIdSetToString somewhere?  If so, can you use it here?  If
not, nevermind

https://codereview.chromium.org/11415049/diff/9025/sync/test/null_invalidatio...
File sync/test/null_invalidation_state_tracker.h (right):

https://codereview.chromium.org/11415049/diff/9025/sync/test/null_invalidatio...
sync/test/null_invalidation_state_tracker.h:1: // Copyright (c) 2012 The
Chromium Authors. All rights reserved.
can you put this in tools/? (It's not really test-related)

Maybe make a new target for it, like sync_tools_helper or something

[akalin, 2012-11-30 02:54:31]

https://codereview.chromium.org/11415049/diff/18/sync/tools/sync_listen_notif...
File sync/tools/sync_listen_notifications.cc (right):

https://codereview.chromium.org/11415049/diff/18/sync/tools/sync_listen_notif...
sync/tools/sync_listen_notifications.cc:117: LOG(INFO) << "Setting bootstrap
data to: " << base64_data;
On 2012/11/30 01:42:54, dcheng wrote:
> On 2012/11/28 00:11:42, akalin wrote:
> > boo, duplicated code.  can you pull this out into its own file while you're
in
> > here?
> 
> Done.
> There's some TODOs with your name attached for refactoring (e.g.
> sync_client.cc:53); are those TODOs satisfied by this refactoring (and can
> therefor be removed)?

There's one more -- the MyTestURLRequestContext, etc..  So I think the TODO
should stay.

> 
> Also, one version prefers LOG(INFO) while the other uses VLOG(1). Any
> preference? I've used LOG(INFO) for now since I assume people running these
> tools want to see them by default.

You're right, LOG(INFO) is best.

[dcheng, 2012-11-30 23:44:54]

https://codereview.chromium.org/11415049/diff/9025/chrome/browser/sync/invali...
File chrome/browser/sync/invalidations/invalidator_storage.cc (right):

https://codereview.chromium.org/11415049/diff/9025/chrome/browser/sync/invali...
chrome/browser/sync/invalidations/invalidator_storage.cc:301:
ignore_result(task_runner->PostTask(FROM_HERE,
On 2012/11/30 02:53:02, akalin wrote:
> compiler_specific.h for ignore_result

This is defined in basictypes.h.

https://codereview.chromium.org/11415049/diff/9025/chrome/browser/sync/invali...
File chrome/browser/sync/invalidations/invalidator_storage_unittest.cc (right):

https://codereview.chromium.org/11415049/diff/9025/chrome/browser/sync/invali...
chrome/browser/sync/invalidations/invalidator_storage_unittest.cc:272: // Tests
that deserializaing well-formed values when optional parameters are
On 2012/11/30 02:53:02, akalin wrote:
> deserializing

Done.

https://codereview.chromium.org/11415049/diff/9025/sync/test/null_invalidatio...
File sync/test/null_invalidation_state_tracker.cc (right):

https://codereview.chromium.org/11415049/diff/9025/sync/test/null_invalidatio...
sync/test/null_invalidation_state_tracker.cc:35: LOG(INFO) << "Forgetting
invalidation state for " << ObjectIdToString(*it);
On 2012/11/30 02:53:02, akalin wrote:
> don't we have ObjectIdSetToString somewhere?  If so, can you use it here?  If
> not, nevermind

We do not =(

https://codereview.chromium.org/11415049/diff/9025/sync/test/null_invalidatio...
File sync/test/null_invalidation_state_tracker.h (right):

https://codereview.chromium.org/11415049/diff/9025/sync/test/null_invalidatio...
sync/test/null_invalidation_state_tracker.h:1: // Copyright (c) 2012 The
Chromium Authors. All rights reserved.
On 2012/11/30 02:53:02, akalin wrote:
> can you put this in tools/? (It's not really test-related)
> 
> Maybe make a new target for it, like sync_tools_helper or something

OK. I wasn't sure if you'd prefer this grouped with the existing Null*
implementations, but it's simple enough to pull it out.

[dcheng, 2012-12-04 18:05:59]

Ping!

[akalin, 2012-12-04 18:14:59]

lgtm!

https://codereview.chromium.org/11415049/diff/3054/sync/sync.gyp
File sync/sync.gyp (right):

https://codereview.chromium.org/11415049/diff/3054/sync/sync.gyp#newcode934
sync/sync.gyp:934:
'../third_party/cacheinvalidation/cacheinvalidation.gyp:cacheinvalidation',
depend on 'sync_notifier' instead (looks like it doesn't depend on
cacheinvalidation stuff directly?)

https://codereview.chromium.org/11415049/diff/3054/sync/sync.gyp#newcode938
sync/sync.gyp:938:
'../third_party/cacheinvalidation/cacheinvalidation.gyp:cacheinvalidation',
here too

https://codereview.chromium.org/11415049/diff/3054/sync/tools/null_invalidati...
File sync/tools/null_invalidation_state_tracker.cc (right):

https://codereview.chromium.org/11415049/diff/3054/sync/tools/null_invalidati...
sync/tools/null_invalidation_state_tracker.cc:12: #include
"sync/tools/null_invalidation_state_tracker.h"
should be first include (and separated with a newline from others)

https://codereview.chromium.org/11415049/diff/3054/sync/tools/null_invalidati...
File sync/tools/null_invalidation_state_tracker.h (right):

https://codereview.chromium.org/11415049/diff/3054/sync/tools/null_invalidati...
sync/tools/null_invalidation_state_tracker.h:23: int64 max_invalidation_version,
#include basictypes.h

https://codereview.chromium.org/11415049/diff/3054/sync/tools/null_invalidati...
sync/tools/null_invalidation_state_tracker.h:27: virtual std::string
GetBootstrapData() const OVERRIDE;
#include <string>

https://codereview.chromium.org/11415049/diff/3054/sync/tools/null_invalidati...
sync/tools/null_invalidation_state_tracker.h:32: const
scoped_refptr<base::TaskRunner>& task_runner,
#include ref_counted.h, forward-declare TaskRunner

https://codereview.chromium.org/11415049/diff/3054/sync/tools/null_invalidati...
sync/tools/null_invalidation_state_tracker.h:33: base::Callback<void(const
AckHandleMap&)> callback) OVERRIDE;
#include callback.h

[dcheng, 2012-12-04 19:10:50]

https://codereview.chromium.org/11415049/diff/3054/sync/sync.gyp
File sync/sync.gyp (right):

https://codereview.chromium.org/11415049/diff/3054/sync/sync.gyp#newcode934
sync/sync.gyp:934:
'../third_party/cacheinvalidation/cacheinvalidation.gyp:cacheinvalidation',
On 2012/12/04 18:15:00, akalin wrote:
> depend on 'sync_notifier' instead (looks like it doesn't depend on
> cacheinvalidation stuff directly?)

Done.

https://codereview.chromium.org/11415049/diff/3054/sync/sync.gyp#newcode938
sync/sync.gyp:938:
'../third_party/cacheinvalidation/cacheinvalidation.gyp:cacheinvalidation',
On 2012/12/04 18:15:00, akalin wrote:
> here too

Done.

https://codereview.chromium.org/11415049/diff/3054/sync/tools/null_invalidati...
File sync/tools/null_invalidation_state_tracker.cc (right):

https://codereview.chromium.org/11415049/diff/3054/sync/tools/null_invalidati...
sync/tools/null_invalidation_state_tracker.cc:12: #include
"sync/tools/null_invalidation_state_tracker.h"
On 2012/12/04 18:15:00, akalin wrote:
> should be first include (and separated with a newline from others)
Oops. Fixed.

https://codereview.chromium.org/11415049/diff/3054/sync/tools/null_invalidati...
File sync/tools/null_invalidation_state_tracker.h (right):

https://codereview.chromium.org/11415049/diff/3054/sync/tools/null_invalidati...
sync/tools/null_invalidation_state_tracker.h:23: int64 max_invalidation_version,
On 2012/12/04 18:15:00, akalin wrote:
> #include basictypes.h

I didn't include it because this should be included by the interface it's
implementing (and it is).

https://codereview.chromium.org/11415049/diff/3054/sync/tools/null_invalidati...
sync/tools/null_invalidation_state_tracker.h:27: virtual std::string
GetBootstrapData() const OVERRIDE;
On 2012/12/04 18:15:00, akalin wrote:
> #include <string>

See above.

https://codereview.chromium.org/11415049/diff/3054/sync/tools/null_invalidati...
sync/tools/null_invalidation_state_tracker.h:32: const
scoped_refptr<base::TaskRunner>& task_runner,
On 2012/12/04 18:15:00, akalin wrote:
> #include ref_counted.h, forward-declare TaskRunner

See above.

https://codereview.chromium.org/11415049/diff/3054/sync/tools/null_invalidati...
sync/tools/null_invalidation_state_tracker.h:33: base::Callback<void(const
AckHandleMap&)> callback) OVERRIDE;
On 2012/12/04 18:15:00, akalin wrote:
> #include callback.h

See above.

[commit-bot: I haz the power, 2012-12-04 19:21:59]

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

[commit-bot: I haz the power, 2012-12-04 19:30:48]

Step "update" is always a major failure.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2012-12-04 19:53:02]

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

[commit-bot: I haz the power, 2012-12-04 20:19:09]

Sorry for I got bad news for ya.
Compile failed with a clobber build on android_dbg.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...
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.

[commit-bot: I haz the power, 2012-12-04 20:52:17]

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

[commit-bot: I haz the power, 2012-12-04 21:27:27]

Sorry for I got bad news for ya.
Compile failed with a clobber build on android_dbg.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...
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.

[commit-bot: I haz the power, 2012-12-04 22:43:09]

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

[commit-bot: I haz the power, 2012-12-04 23:27:50]

Failed to trigger a try job on android_dbg
HTTP Error 400: Bad Request

[commit-bot: I haz the power, 2012-12-04 23:28:00]

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

[commit-bot: I haz the power, 2012-12-05 03:38:56]

Retried try job too often on linux_rel for step(s) content_unittests