Add version field to syncer::Invalidation.

Add version field to syncer::Invalidation.
BUG=260021
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=212448

[rlarocque, 2013-07-16 18:33:15]

That was a bit bigger than I would have expected.  Thanks for doing this.

You might need to get approval from some push_messaging OWNERs before you can
commit this.

https://codereview.chromium.org/19381005/diff/1/chrome/browser/extensions/api...
File chrome/browser/extensions/api/push_messaging/push_messaging_apitest.cc
(right):

https://codereview.chromium.org/19381005/diff/1/chrome/browser/extensions/api...
chrome/browser/extensions/api/push_messaging/push_messaging_apitest.cc:86:
syncer::Invalidation::kUnknownVersion,
I'm not sure this is right, though I don't think it will cause any harm.  I'm OK
with letting this through, if the push_messaging reviewer approves.

https://codereview.chromium.org/19381005/diff/1/chrome/browser/sync/test/inte...
File chrome/browser/sync/test/integration/sync_test.cc (right):

https://codereview.chromium.org/19381005/diff/1/chrome/browser/sync/test/inte...
chrome/browser/sync/test/integration/sync_test.cc:675:
syncer::Invalidation::UNKNOWN_VERSION,
kUnknownVersion?

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

https://codereview.chromium.org/19381005/diff/1/sync/internal_api/public/base...
sync/internal_api/public/base/invalidation.cc:67: const int64
Invalidation::kUnknownVersion = -1;
I wish this wasn't necessary.  Looking at the protobufs in
googlecacheinvalidation, I get the feeling that the protocol was intended to
allow any int64 to be a valid version.  There should be nothing special about
-1.

I'd prefer it if the API made these invalid versions explicit, like the
googlecacheinvalidation API does.  It has specific callbacks for unknown version
invalidations that don't pass back an Invalidation object.

This approach should be safe for now, since there no one has written an "if
(version == kInvalidVersion)" statement yet.  You can ignore this comment for
now.  I'll try to clean it up in a later CL.

https://codereview.chromium.org/19381005/diff/1/sync/internal_api/public/base...
sync/internal_api/public/base/invalidation.cc:91: std::string version_as_string;
nit: This function returns false when the parsing of other values fail. 
Although you can't return false if the version isn't there, maybe you could
return false if the StringToInt64 parsing fails?

https://codereview.chromium.org/19381005/diff/1/sync/notifier/object_id_inval...
File sync/notifier/object_id_invalidation_map.h (right):

https://codereview.chromium.org/19381005/diff/1/sync/notifier/object_id_inval...
sync/notifier/object_id_invalidation_map.h:55: #endif  //
HOME_DCHENG_SRC_CHROMIUM_SRC_SYNC_NOTIFIER_OBJECT_ID_STATE_MAP_H_
This is not related to your CL, but feel free to fix it while you're modifying
this file anyway.

[Steve Condie, 2013-07-16 21:23:06]

Added dcheng for push_messaging changes.

https://codereview.chromium.org/19381005/diff/1/chrome/browser/sync/test/inte...
File chrome/browser/sync/test/integration/sync_test.cc (right):

https://codereview.chromium.org/19381005/diff/1/chrome/browser/sync/test/inte...
chrome/browser/sync/test/integration/sync_test.cc:675:
syncer::Invalidation::UNKNOWN_VERSION,
Thanks. Missed this because I didn't build the sync integration tests. I have
now built and run sync_unit_tests, sync_integration_tests, and
sync_endtoend_tests. Is there anything else?

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

https://codereview.chromium.org/19381005/diff/1/sync/internal_api/public/base...
sync/internal_api/public/base/invalidation.cc:67: const int64
Invalidation::kUnknownVersion = -1;
Ok. I will be doing that comparison in my code, but my invalidation generating
code doesn't use negative numbers (as I assume is probably the normal case) so
it shouldn't pose a problem.

https://codereview.chromium.org/19381005/diff/1/sync/internal_api/public/base...
sync/internal_api/public/base/invalidation.cc:91: std::string version_as_string;
Done.

https://codereview.chromium.org/19381005/diff/1/sync/notifier/object_id_inval...
File sync/notifier/object_id_invalidation_map.h (right):

https://codereview.chromium.org/19381005/diff/1/sync/notifier/object_id_inval...
sync/notifier/object_id_invalidation_map.h:55: #endif  //
HOME_DCHENG_SRC_CHROMIUM_SRC_SYNC_NOTIFIER_OBJECT_ID_STATE_MAP_H_
Done.

[dcheng, 2013-07-16 21:27:46]

lgtm

[dcheng, 2013-07-16 21:28:15]

(forgot to send my comments)

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

https://codereview.chromium.org/19381005/diff/9001/chrome/browser/extensions/...
chrome/browser/extensions/api/push_messaging/push_messaging_invalidation_handler_unittest.cc:125:
ids,
Nit: wrapped lines should be indented 4 spaces.

[rlarocque, 2013-07-16 21:33:37]

LGTM

https://codereview.chromium.org/19381005/diff/1/chrome/browser/sync/test/inte...
File chrome/browser/sync/test/integration/sync_test.cc (right):

https://codereview.chromium.org/19381005/diff/1/chrome/browser/sync/test/inte...
chrome/browser/sync/test/integration/sync_test.cc:675:
syncer::Invalidation::UNKNOWN_VERSION,
On 2013/07/16 21:23:06, stepco wrote:
> Thanks. Missed this because I didn't build the sync integration tests. I have
> now built and run sync_unit_tests, sync_integration_tests, and
> sync_endtoend_tests. Is there anything else?

The existence of sync_endtoend_tests is news to me.

You might also want to try sync_client and sync_listen_notifications.  I don't
think there are any tests that require them to run, but they will break the
trybots if they don't compile

[Steve Condie, 2013-07-16 21:49:46]

https://codereview.chromium.org/19381005/diff/1/chrome/browser/sync/test/inte...
File chrome/browser/sync/test/integration/sync_test.cc (right):

https://codereview.chromium.org/19381005/diff/1/chrome/browser/sync/test/inte...
chrome/browser/sync/test/integration/sync_test.cc:675:
syncer::Invalidation::UNKNOWN_VERSION,
On 2013/07/16 21:33:37, rlarocque wrote:
> On 2013/07/16 21:23:06, stepco wrote:
> > Thanks. Missed this because I didn't build the sync integration tests. I
have
> > now built and run sync_unit_tests, sync_integration_tests, and
> > sync_endtoend_tests. Is there anything else?
> 
> The existence of sync_endtoend_tests is news to me.
> 
> You might also want to try sync_client and sync_listen_notifications.  I don't
> think there are any tests that require them to run, but they will break the
> trybots if they don't compile

Done.

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

https://codereview.chromium.org/19381005/diff/9001/chrome/browser/extensions/...
chrome/browser/extensions/api/push_messaging/push_messaging_invalidation_handler_unittest.cc:125:
ids,
On 2013/07/16 21:28:15, dcheng wrote:
> Nit: wrapped lines should be indented 4 spaces.

Done.

[Steve Condie, 2013-07-17 17:32:53]

Discovered I needed more changes to the Android-side code after running a try
job.

[rlarocque, 2013-07-18 17:50:33]

lgtm

[commit-bot: I haz the power, 2013-07-18 17:54:08]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/stepco@chromium.org/19381005/69001

[commit-bot: I haz the power, 2013-07-18 22:05:51]

Change committed as 212448