[Sync] Rework Invalidator-related unit tests

[Sync] Rework Invalidator-related unit tests

Introduce invalidator_test_template.h, which contains tests that should be used for each Invalidator implementation.  Make all Invalidator implementations, and also InvalidatorRegistrar, instantiate that test template.  Move most InvalidatorRegistrar tests and some of the other Invalidator implementation tests to the template.

Make P2PInvalidator support arbitrary object IDs.  Also make SendInvalidation take an ObjectIdStateMap.

Simplify BridgedInvalidator a bit; it no longer requires a call to UpdateEnabledTypes (it just uses all registered types from its registrar).

Rework tests for BridgedInvalidator/ChromeSyncNotifierBridge to not require functions in ChromeSyncNotifierBridge to be marked virtual.

Add conversions to/from Values for InvalidationState, ObjectId, and ObjectIdStateMap.

BUG=141678
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=154538

[akalin, 2012-08-29 00:47:00]

And the lucky winner is +zea for review!

+dcheng fyi

[Nicolas Zea, 2012-08-29 23:41:05]

Whew, first round comments.

http://codereview.chromium.org/10874096/diff/11001/chrome/browser/sync/glue/b...
File chrome/browser/sync/glue/bridged_invalidator_unittest.cc (right):

http://codereview.chromium.org/10874096/diff/11001/chrome/browser/sync/glue/b...
chrome/browser/sync/glue/bridged_invalidator_unittest.cc:35: }  // namespace
remove anon namespace?

http://codereview.chromium.org/10874096/diff/11001/chrome/browser/sync/glue/b...
chrome/browser/sync/glue/bridged_invalidator_unittest.cc:54: const std::string&
initial_state,
Comment that the two params aren't needed for this implementation?

http://codereview.chromium.org/10874096/diff/11001/chrome/browser/sync/glue/b...
chrome/browser/sync/glue/bridged_invalidator_unittest.cc:110:
syncer::FakeInvalidator* fake_delegate_;
comment that this is owned by invalidator_

http://codereview.chromium.org/10874096/diff/11001/chrome/browser/sync/glue/c...
File chrome/browser/sync/glue/chrome_sync_notification_bridge_unittest.cc
(right):

http://codereview.chromium.org/10874096/diff/11001/chrome/browser/sync/glue/c...
chrome/browser/sync/glue/chrome_sync_notification_bridge_unittest.cc:38: //
safety issues within GTest/GMock.
I don't think this comment makes sense here anymore. PErhaps in
fake_invalidation_handler.h instead?

http://codereview.chromium.org/10874096/diff/11001/chrome/browser/sync/glue/c...
chrome/browser/sync/glue/chrome_sync_notification_bridge_unittest.cc:130: delete
sync_handler_;
make sync_handler_ a scoped_ptr?

http://codereview.chromium.org/10874096/diff/11001/chrome/browser/sync/glue/c...
chrome/browser/sync/glue/chrome_sync_notification_bridge_unittest.cc:154:
run_loop.QuitClosure()));
don't you need to do run_loop.run()?

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

http://codereview.chromium.org/10874096/diff/11001/sync/internal_api/public/b...
sync/internal_api/public/base/invalidation_state.h:25: base::Value* ToValue()
const;
have these return ScopedPtr's?

http://codereview.chromium.org/10874096/diff/11001/sync/notifier/invalidation...
File sync/notifier/invalidation_util.h (right):

http://codereview.chromium.org/10874096/diff/11001/sync/notifier/invalidation...
sync/notifier/invalidation_util.h:46: base::DictionaryValue* ObjectIdToValue(
return scoped ptr?

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

http://codereview.chromium.org/10874096/diff/11001/sync/notifier/invalidator....
sync/notifier/invalidator.h:16: #include "sync/notifier/object_id_state_map.h"
forward declare?

http://codereview.chromium.org/10874096/diff/11001/sync/notifier/invalidator_...
File sync/notifier/invalidator_registrar.cc (right):

http://codereview.chromium.org/10874096/diff/11001/sync/notifier/invalidator_...
sync/notifier/invalidator_registrar.cc:73: registered_ids.insert(it->first);
break after?

http://codereview.chromium.org/10874096/diff/11001/sync/notifier/invalidator_...
File sync/notifier/invalidator_registrar_unittest.cc (right):

http://codereview.chromium.org/10874096/diff/11001/sync/notifier/invalidator_...
sync/notifier/invalidator_registrar_unittest.cc:39: virtual void
UnregisterHandler(InvalidationHandler* handler) OVERRIDE {
newline before/after method?

http://codereview.chromium.org/10874096/diff/11001/sync/notifier/invalidator_...
File sync/notifier/invalidator_test_template.h (right):

http://codereview.chromium.org/10874096/diff/11001/sync/notifier/invalidator_...
sync/notifier/invalidator_test_template.h:115:
this->delegate_.CreateInvalidator("fake_initial_state",
why are all the "this->"'s necessary?

http://codereview.chromium.org/10874096/diff/11001/sync/notifier/invalidator_...
sync/notifier/invalidator_test_template.h:119: // TODO(tim): This call should be
a no-op. Remove once bug 124140 and
this bug is fixed, do we still need the call around?

http://codereview.chromium.org/10874096/diff/11001/sync/notifier/invalidator_...
sync/notifier/invalidator_test_template.h:374: this->fake_tracker_.AsWeakPtr());
fix indent

http://codereview.chromium.org/10874096/diff/11001/sync/notifier/non_blocking...
File sync/notifier/non_blocking_invalidator_unittest.cc (right):

http://codereview.chromium.org/10874096/diff/11001/sync/notifier/non_blocking...
sync/notifier/non_blocking_invalidator_unittest.cc:70: if
(!io_thread_.message_loop_proxy()->PostTaskAndReply(
ASSERT_TRUE?

http://codereview.chromium.org/10874096/diff/11001/sync/sync.gyp
File sync/sync.gyp (right):

http://codereview.chromium.org/10874096/diff/11001/sync/sync.gyp#newcode490
sync/sync.gyp:490: 'notifier/invalidator_test_template.h',
abc order

[akalin, 2012-08-30 22:20:38]

PTAL!

http://codereview.chromium.org/10874096/diff/11001/chrome/browser/sync/glue/b...
File chrome/browser/sync/glue/bridged_invalidator_unittest.cc (right):

http://codereview.chromium.org/10874096/diff/11001/chrome/browser/sync/glue/b...
chrome/browser/sync/glue/bridged_invalidator_unittest.cc:35: }  // namespace
On 2012/08/29 23:41:05, nzea wrote:
> remove anon namespace?

Done.

http://codereview.chromium.org/10874096/diff/11001/chrome/browser/sync/glue/b...
chrome/browser/sync/glue/bridged_invalidator_unittest.cc:54: const std::string&
initial_state,
On 2012/08/29 23:41:05, nzea wrote:
> Comment that the two params aren't needed for this implementation?

Done.

http://codereview.chromium.org/10874096/diff/11001/chrome/browser/sync/glue/b...
chrome/browser/sync/glue/bridged_invalidator_unittest.cc:110:
syncer::FakeInvalidator* fake_delegate_;
On 2012/08/29 23:41:05, nzea wrote:
> comment that this is owned by invalidator_

Done.

http://codereview.chromium.org/10874096/diff/11001/chrome/browser/sync/glue/c...
File chrome/browser/sync/glue/chrome_sync_notification_bridge_unittest.cc
(right):

http://codereview.chromium.org/10874096/diff/11001/chrome/browser/sync/glue/c...
chrome/browser/sync/glue/chrome_sync_notification_bridge_unittest.cc:38: //
safety issues within GTest/GMock.
On 2012/08/29 23:41:05, nzea wrote:
> I don't think this comment makes sense here anymore. PErhaps in
> fake_invalidation_handler.h instead?

Hmm actually I think the general idea belongs here -- rewrote the comment.

http://codereview.chromium.org/10874096/diff/11001/chrome/browser/sync/glue/c...
chrome/browser/sync/glue/chrome_sync_notification_bridge_unittest.cc:130: delete
sync_handler_;
On 2012/08/29 23:41:05, nzea wrote:
> make sync_handler_ a scoped_ptr?

Done.

http://codereview.chromium.org/10874096/diff/11001/chrome/browser/sync/glue/c...
chrome/browser/sync/glue/chrome_sync_notification_bridge_unittest.cc:154:
run_loop.QuitClosure()));
On 2012/08/29 23:41:05, nzea wrote:
> don't you need to do run_loop.run()?

Done.

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

http://codereview.chromium.org/10874096/diff/11001/sync/internal_api/public/b...
sync/internal_api/public/base/invalidation_state.h:25: base::Value* ToValue()
const;
On 2012/08/29 23:41:05, nzea wrote:
> have these return ScopedPtr's?

Done.

http://codereview.chromium.org/10874096/diff/11001/sync/notifier/invalidation...
File sync/notifier/invalidation_util.h (right):

http://codereview.chromium.org/10874096/diff/11001/sync/notifier/invalidation...
sync/notifier/invalidation_util.h:46: base::DictionaryValue* ObjectIdToValue(
On 2012/08/29 23:41:05, nzea wrote:
> return scoped ptr?

Done.

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

http://codereview.chromium.org/10874096/diff/11001/sync/notifier/invalidator....
sync/notifier/invalidator.h:16: #include "sync/notifier/object_id_state_map.h"
On 2012/08/29 23:41:05, nzea wrote:
> forward declare?

can't, it's a typedef

http://codereview.chromium.org/10874096/diff/11001/sync/notifier/invalidator_...
File sync/notifier/invalidator_registrar.cc (right):

http://codereview.chromium.org/10874096/diff/11001/sync/notifier/invalidator_...
sync/notifier/invalidator_registrar.cc:73: registered_ids.insert(it->first);
On 2012/08/29 23:41:05, nzea wrote:
> break after?

no, it has to find all registered IDs

http://codereview.chromium.org/10874096/diff/11001/sync/notifier/invalidator_...
File sync/notifier/invalidator_registrar_unittest.cc (right):

http://codereview.chromium.org/10874096/diff/11001/sync/notifier/invalidator_...
sync/notifier/invalidator_registrar_unittest.cc:39: virtual void
UnregisterHandler(InvalidationHandler* handler) OVERRIDE {
On 2012/08/29 23:41:05, nzea wrote:
> newline before/after method?

Done.

http://codereview.chromium.org/10874096/diff/11001/sync/notifier/invalidator_...
File sync/notifier/invalidator_test_template.h (right):

http://codereview.chromium.org/10874096/diff/11001/sync/notifier/invalidator_...
sync/notifier/invalidator_test_template.h:115:
this->delegate_.CreateInvalidator("fake_initial_state",
On 2012/08/29 23:41:05, nzea wrote:
> why are all the "this->"'s necessary?

Basically because they're type-parametrized tests and something about how it
can't disambiguate the reference or something.

http://codereview.chromium.org/10874096/diff/11001/sync/notifier/invalidator_...
sync/notifier/invalidator_test_template.h:119: // TODO(tim): This call should be
a no-op. Remove once bug 124140 and
On 2012/08/29 23:41:05, nzea wrote:
> this bug is fixed, do we still need the call around?

Actually, I think we still can't remove it yet?  I unfixed the bug (I fixed it
by mistake).

http://codereview.chromium.org/10874096/diff/11001/sync/notifier/invalidator_...
sync/notifier/invalidator_test_template.h:374: this->fake_tracker_.AsWeakPtr());
On 2012/08/29 23:41:05, nzea wrote:
> fix indent

Done.

http://codereview.chromium.org/10874096/diff/11001/sync/notifier/non_blocking...
File sync/notifier/non_blocking_invalidator_unittest.cc (right):

http://codereview.chromium.org/10874096/diff/11001/sync/notifier/non_blocking...
sync/notifier/non_blocking_invalidator_unittest.cc:70: if
(!io_thread_.message_loop_proxy()->PostTaskAndReply(
On 2012/08/29 23:41:05, nzea wrote:
> ASSERT_TRUE?

Done.

http://codereview.chromium.org/10874096/diff/11001/sync/sync.gyp
File sync/sync.gyp (right):

http://codereview.chromium.org/10874096/diff/11001/sync/sync.gyp#newcode490
sync/sync.gyp:490: 'notifier/invalidator_test_template.h',
On 2012/08/29 23:41:05, nzea wrote:
> abc order

Done.

[Nicolas Zea, 2012-08-31 17:44:25]

LGTM!

http://codereview.chromium.org/10874096/diff/18002/chrome/browser/sync/glue/c...
File chrome/browser/sync/glue/chrome_sync_notification_bridge_unittest.cc
(right):

http://codereview.chromium.org/10874096/diff/18002/chrome/browser/sync/glue/c...
chrome/browser/sync/glue/chrome_sync_notification_bridge_unittest.cc:36: //
Since all the interesting stuff happens on the sync thread, we have
Maybe just move it down to be right above the class declaration instead of the
DoNothing() call?

http://codereview.chromium.org/10874096/diff/18002/sync/notifier/non_blocking...
File sync/notifier/non_blocking_invalidator_unittest.cc (right):

http://codereview.chromium.org/10874096/diff/18002/sync/notifier/non_blocking...
sync/notifier/non_blocking_invalidator_unittest.cc:26: void DoNothing() {}
Comment that this is needed to block sync thread.

[akalin, 2012-08-31 21:06:33]

Committing via commit bot.

http://codereview.chromium.org/10874096/diff/18002/chrome/browser/sync/glue/c...
File chrome/browser/sync/glue/chrome_sync_notification_bridge_unittest.cc
(right):

http://codereview.chromium.org/10874096/diff/18002/chrome/browser/sync/glue/c...
chrome/browser/sync/glue/chrome_sync_notification_bridge_unittest.cc:36: //
Since all the interesting stuff happens on the sync thread, we have
On 2012/08/31 17:44:25, nzea wrote:
> Maybe just move it down to be right above the class declaration instead of the
> DoNothing() call?

Done.

http://codereview.chromium.org/10874096/diff/18002/sync/notifier/non_blocking...
File sync/notifier/non_blocking_invalidator_unittest.cc (right):

http://codereview.chromium.org/10874096/diff/18002/sync/notifier/non_blocking...
sync/notifier/non_blocking_invalidator_unittest.cc:26: void DoNothing() {}
On 2012/08/31 17:44:25, nzea wrote:
> Comment that this is needed to block sync thread.

Done.

[commit-bot: I haz the power, 2012-08-31 21:12:17]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/akalin@chromium.org/10874096/23008

[commit-bot: I haz the power, 2012-08-31 23:39:29]

Change committed as 154538