Refactor sync-specific parts out of SyncNotifier/SyncNotifierObserver

Refactor sync-specific parts out of SyncNotifier/SyncNotifierObserver

Sort of. SendNotification() is still there. Perhaps we want to split the interfaces completely.

BUG=124149
TEST=tests should still pass, no observable behavior change

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

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

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

[dcheng, 2012-07-10 19:15:25]

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

http://codereview.chromium.org/10702074/diff/7001/sync/notifier/invalidation_...
sync/notifier/invalidation_notifier_unittest.cc:77: // clearing its object IDs.
I expect to put these new test cases in InvalidationNotifierBaseTest.

http://codereview.chromium.org/10702074/diff/7001/sync/notifier/non_blocking_...
File sync/notifier/non_blocking_invalidation_notifier.cc (right):

http://codereview.chromium.org/10702074/diff/7001/sync/notifier/non_blocking_...
sync/notifier/non_blocking_invalidation_notifier.cc:266: return ObjectIdSet();
I need to figure out why the unit test is passing with this method stubbed
out...

http://codereview.chromium.org/10702074/diff/7001/sync/sync.gyp
File sync/sync.gyp (right):

http://codereview.chromium.org/10702074/diff/7001/sync/sync.gyp#newcode36
sync/sync.gyp:36:
'../third_party/cacheinvalidation/cacheinvalidation.gyp:cacheinvalidation',
This is because model_type_payload_map.h defines the conversions between
ObjectIdPayloadMap and ModelTypePayloadMap, which requires us to pull in
google/cacheinvalidation/include/types.h in the header.

Note that we don't /have/ to put the cacheinvalidation include in
model_type_payload_map.h; however, this leads to adding a lot of random includes
in a bunch of .cc files and having to fix DEPS outside of sync as well..

[akalin, 2012-07-11 00:10:24]

Some high-level comments.  Also re. SendNotification, we don't have to take care
of it in this CL, but maybe have a separate interface, say,
SyncNotificationSender, and put SendNotification in there.  Then
*InvalidationNotifier can just implement SyncNotifier, and P2PNotifier can
implement SyncNotifier and SyncNotificationSender.  Then SyncFactory can expose
a getter that returns a SyncNotifier + SyncNotificationSender (although
ownership is a little murky...).

http://codereview.chromium.org/10702074/diff/7001/sync/sync.gyp
File sync/sync.gyp (right):

http://codereview.chromium.org/10702074/diff/7001/sync/sync.gyp#newcode36
sync/sync.gyp:36:
'../third_party/cacheinvalidation/cacheinvalidation.gyp:cacheinvalidation',
On 2012/07/10 19:15:25, dcheng wrote:
> This is because model_type_payload_map.h defines the conversions between
> ObjectIdPayloadMap and ModelTypePayloadMap, which requires us to pull in
> google/cacheinvalidation/include/types.h in the header.
> 
> Note that we don't /have/ to put the cacheinvalidation include in
> model_type_payload_map.h; however, this leads to adding a lot of random
includes
> in a bunch of .cc files and having to fix DEPS outside of sync as well..

I think the best solution would be to put the conversion function in
object_id_payload_map instead, since sync_notifier already depends on
cacheinvalidation (and it should be the only one that does).

http://codereview.chromium.org/10702074/diff/11001/sync/notifier/invalidation...
File sync/notifier/invalidation_notifier_base.cc (right):

http://codereview.chromium.org/10702074/diff/11001/sync/notifier/invalidation...
sync/notifier/invalidation_notifier_base.cc:25: if
(observers_.might_have_observers()) {
why avoid the FOR_EACH_OBSERVER macro here?

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

http://codereview.chromium.org/10702074/diff/11001/sync/notifier/invalidation...
sync/notifier/invalidation_notifier_base.h:41:
ObserverList<SyncNotifierObserver>& observers() { return observers_; }
should return a pointer

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

http://codereview.chromium.org/10702074/diff/11001/sync/notifier/p2p_notifier...
sync/notifier/p2p_notifier.h:85: class P2PNotifier : public
InvalidationNotifierBase,
i'm confused.  P2PNotifier isn't an InvalidationNotifier, so why does it inherit
from InvalidationNotifierBase?

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

http://codereview.chromium.org/10702074/diff/11001/sync/notifier/sync_notifie...
sync/notifier/sync_notifier.h:26: virtual void ReloadHandlers() = 0;
I know ReloadHandlers was in my design doc.  It was to batch registrations to
tango, but Daniel added a comment saying that we shouldn't have to worry about
it, since registrations are batched anyway.  So now I'm not sure if I still like
it.

We could have something like just:

virtual void AddHandler(ObjectId, Observer*);
virtual void RemoveHandler(ObjectId, Observer*);

instead.  Or maybe with set<ObjectId> instead.  Or map<ObjectId, Observer*>. 
What do you think?

[dcheng, 2012-07-11 05:45:28]

http://codereview.chromium.org/10702074/diff/7001/sync/sync.gyp
File sync/sync.gyp (right):

http://codereview.chromium.org/10702074/diff/7001/sync/sync.gyp#newcode36
sync/sync.gyp:36:
'../third_party/cacheinvalidation/cacheinvalidation.gyp:cacheinvalidation',
On 2012/07/11 00:10:24, akalin wrote:
> On 2012/07/10 19:15:25, dcheng wrote:
> > This is because model_type_payload_map.h defines the conversions between
> > ObjectIdPayloadMap and ModelTypePayloadMap, which requires us to pull in
> > google/cacheinvalidation/include/types.h in the header.
> > 
> > Note that we don't /have/ to put the cacheinvalidation include in
> > model_type_payload_map.h; however, this leads to adding a lot of random
> includes
> > in a bunch of .cc files and having to fix DEPS outside of sync as well..
> 
> I think the best solution would be to put the conversion function in
> object_id_payload_map instead, since sync_notifier already depends on
> cacheinvalidation (and it should be the only one that does).

Done. I think it makes more sense the other way though, since the sync stuff is
built on the invalidation stuff; maybe a long-term cleanup?

http://codereview.chromium.org/10702074/diff/11001/sync/notifier/invalidation...
File sync/notifier/invalidation_notifier_base.cc (right):

http://codereview.chromium.org/10702074/diff/11001/sync/notifier/invalidation...
sync/notifier/invalidation_notifier_base.cc:25: if
(observers_.might_have_observers()) {
On 2012/07/11 00:10:24, akalin wrote:
> why avoid the FOR_EACH_OBSERVER macro here?

We need the return values. It might be possible to use some really clever macro
expansion to get this to work with FOR_EACH_OBSERVER, but most other places that
need that just inline the logic.

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

http://codereview.chromium.org/10702074/diff/11001/sync/notifier/invalidation...
sync/notifier/invalidation_notifier_base.h:41:
ObserverList<SyncNotifierObserver>& observers() { return observers_; }
On 2012/07/11 00:10:24, akalin wrote:
> should return a pointer

Done.

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

http://codereview.chromium.org/10702074/diff/11001/sync/notifier/p2p_notifier...
sync/notifier/p2p_notifier.h:85: class P2PNotifier : public
InvalidationNotifierBase,
On 2012/07/11 00:10:24, akalin wrote:
> i'm confused.  P2PNotifier isn't an InvalidationNotifier, so why does it
inherit
> from InvalidationNotifierBase?

It probably needs a better name. It's to share the logic for ReloadHandlers()
and dispatching the (fake) invalidations in this case though.

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

http://codereview.chromium.org/10702074/diff/11001/sync/notifier/sync_notifie...
sync/notifier/sync_notifier.h:26: virtual void ReloadHandlers() = 0;
On 2012/07/11 00:10:24, akalin wrote:
> I know ReloadHandlers was in my design doc.  It was to batch registrations to
> tango, but Daniel added a comment saying that we shouldn't have to worry about
> it, since registrations are batched anyway.  So now I'm not sure if I still
like
> it.
> 
> We could have something like just:
> 
> virtual void AddHandler(ObjectId, Observer*);
> virtual void RemoveHandler(ObjectId, Observer*);
> 
> instead.  Or maybe with set<ObjectId> instead.  Or map<ObjectId, Observer*>. 
> What do you think?

Add/Remove would push some of the diffing logic to each of the handlers, which I
think would be cleaner to avoid. I'll see what I can hack together for something
like SyncNotifier::UpdateHandledIds(Observer*, ObjectIdSet) though.

[akalin, 2012-07-11 20:27:02]

http://codereview.chromium.org/10702074/diff/11001/sync/notifier/invalidation...
File sync/notifier/invalidation_notifier_base.cc (right):

http://codereview.chromium.org/10702074/diff/11001/sync/notifier/invalidation...
sync/notifier/invalidation_notifier_base.cc:25: if
(observers_.might_have_observers()) {
On 2012/07/11 05:45:28, dcheng wrote:
> On 2012/07/11 00:10:24, akalin wrote:
> > why avoid the FOR_EACH_OBSERVER macro here?
> 
> We need the return values. It might be possible to use some really clever
macro
> expansion to get this to work with FOR_EACH_OBSERVER, but most other places
that
> need that just inline the logic.

Ah, I missed that.  Yeah, that's another reason why that observer method is
problematic -- now you can't fully abstract away a SyncNotifier on another
thread (because you then have to require that that observer method is callable
from any thread).

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

http://codereview.chromium.org/10702074/diff/11001/sync/notifier/p2p_notifier...
sync/notifier/p2p_notifier.h:85: class P2PNotifier : public
InvalidationNotifierBase,
On 2012/07/11 05:45:28, dcheng wrote:
> On 2012/07/11 00:10:24, akalin wrote:
> > i'm confused.  P2PNotifier isn't an InvalidationNotifier, so why does it
> inherit
> > from InvalidationNotifierBase?
> 
> It probably needs a better name. It's to share the logic for ReloadHandlers()
> and dispatching the (fake) invalidations in this case though.

Would prefer a composed helper object rather than inheriting.

[akalin, 2012-07-11 20:29:22]

On 2012/07/11 05:45:28, dcheng wrote:
> Done. I think it makes more sense the other way though, since the sync stuff
is
> built on the invalidation stuff; maybe a long-term cleanup?

Hmm, yeah, maybe, once the notifier stuff is fully decoupled from sync.

> Add/Remove would push some of the diffing logic to each of the handlers, which
I
> think would be cleaner to avoid. I'll see what I can hack together for
something
> like SyncNotifier::UpdateHandledIds(Observer*, ObjectIdSet) though.

Can the SyncNotifier handle all the diffing?

Another option would be to simply just have Add/RemoveObserver, and blast all
notifications to all observers, letting them do the filtering.  They'd have to
filter themselves, anyway, as unregistrations don't take effect immediately
(esp. with multiple threads).  Also, they could filter by ObjectId type (or
whatever they call the bucketing) instead of having to filter on each individual
object id.

[dcheng, 2012-07-18 22:00:19]

PTAL. I've updated the change to use UpdateRegisteredIds() per our discussions.

There are two (hopefully) issues to resolve:
1) Unit tests for SyncNotifierHelper (in progress)
2) A better name for SyncNotifierHelper.

[akalin, 2012-07-19 00:42:06]

Looking good!  More comments

http://codereview.chromium.org/10702074/diff/25001/chrome/browser/sync/glue/c...
File chrome/browser/sync/glue/chrome_sync_notification_bridge.cc (right):

http://codereview.chromium.org/10702074/diff/25001/chrome/browser/sync/glue/c...
chrome/browser/sync/glue/chrome_sync_notification_bridge.cc:68:
helper_.DispatchInvalidationsToHandlers(
this won't work.  the invalidations must happen on the same thread as called by
UpdateRegisteredIds.  Ugh, this sucks, I forgot about this. :( Let me try to
think of a solution.

I may try to fix this in a separate patch in parallel.

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

http://codereview.chromium.org/10702074/diff/25001/sync/internal_api/sync_man...
sync/internal_api/sync_manager_impl.cc:2353: ModelTypePayloadMap type_payloads =
const ref

http://codereview.chromium.org/10702074/diff/25001/sync/internal_api/sync_man...
sync/internal_api/sync_manager_impl.cc:2486: ModelTypePayloadMap type_payloads =
const ref

http://codereview.chromium.org/10702074/diff/25001/sync/notifier/invalidation...
File sync/notifier/invalidation_notifier.h (right):

http://codereview.chromium.org/10702074/diff/25001/sync/notifier/invalidation...
sync/notifier/invalidation_notifier.h:80: ObjectIdSet registered_ids_;
registered_ids_ isn't needed is it?

http://codereview.chromium.org/10702074/diff/25001/sync/notifier/invalidation...
File sync/notifier/invalidation_util.cc (right):

http://codereview.chromium.org/10702074/diff/25001/sync/notifier/invalidation...
sync/notifier/invalidation_util.cc:41: for (ModelTypeSet::Iterator iter =
model_types.First(); iter.Good();
iter -> it?

http://codereview.chromium.org/10702074/diff/25001/sync/notifier/non_blocking...
File sync/notifier/non_blocking_invalidation_notifier.cc (right):

http://codereview.chromium.org/10702074/diff/25001/sync/notifier/non_blocking...
sync/notifier/non_blocking_invalidation_notifier.cc:109:
invalidation_notifier_->UpdateRegisteredIds(this, ids);
don't we need to union ids with the set of all registered ids?  since we're
replacing, right

http://codereview.chromium.org/10702074/diff/25001/sync/notifier/p2p_notifier.cc
File sync/notifier/p2p_notifier.cc (right):

http://codereview.chromium.org/10702074/diff/25001/sync/notifier/p2p_notifier...
sync/notifier/p2p_notifier.cc:162: const ModelTypeSet enabled_types =
ObjectIdSetToModelTypeSet(
const ref

http://codereview.chromium.org/10702074/diff/25001/sync/notifier/p2p_notifier...
sync/notifier/p2p_notifier.cc:164: const ModelTypeSet new_enabled_types =
const ref

http://codereview.chromium.org/10702074/diff/25001/sync/notifier/sync_notifie...
File sync/notifier/sync_notifier_helper.cc (right):

http://codereview.chromium.org/10702074/diff/25001/sync/notifier/sync_notifie...
sync/notifier/sync_notifier_helper.cc:14: if (ids.empty()) {
given that at most one handler can handle a given object ID, I think a better
data structure would be:

map<Observer*, ObjectIdSet> handler_to_ids_map_;
ObjectIdSet registered_ids_;

Then this function could just be:

ids_to_unregister = SetDiff(ids, handler_to_ids_map_[handler]);
if (ids.empty()) {
  handlers_to_ids_map_.erase(handler);
} else {
  handlers_to_ids_map_[handler] = ids;
}

registered_ids_ = SetDiff(Union(registered_ids_, ids), ids_to_unregister);
return registered_ids_;

And DispatchInvalidationsToHandlers can just be:

for ((handler, ids) in handlers_to_ids_map_) {
  <build id->payload map from id_payloads and ids>
  dispatch_map[handler] = id->payload map;
}

<dispatch>

http://codereview.chromium.org/10702074/diff/25001/sync/notifier/sync_notifie...
sync/notifier/sync_notifier_helper.cc:46: std::map<SyncNotifierObserver*,
ObjectIdPayloadMap> dispatch_map;
typedef the map to something? (can be scoped to this function)

http://codereview.chromium.org/10702074/diff/25001/sync/notifier/sync_notifie...
sync/notifier/sync_notifier_helper.cc:60:
DCHECK(handlers_.HasObserver(it->first));
this doesn't necessarily hold does it?  OnIncomingNotification can do anything,
including removing all other handlers.

http://codereview.chromium.org/10702074/diff/25001/sync/notifier/sync_notifie...
File sync/notifier/sync_notifier_helper.h (right):

http://codereview.chromium.org/10702074/diff/25001/sync/notifier/sync_notifie...
sync/notifier/sync_notifier_helper.h:18: class SyncNotifierHelper {
class-level comment

http://codereview.chromium.org/10702074/diff/25001/sync/notifier/sync_notifie...
sync/notifier/sync_notifier_helper.h:21: virtual ~SyncNotifierHelper();
doesn't need to be virtual

http://codereview.chromium.org/10702074/diff/25001/sync/notifier/sync_notifie...
sync/notifier/sync_notifier_helper.h:26: // registered values.
was momentarily confused -- maybe '...values.' -> 'values for all existing
handlers.'

http://codereview.chromium.org/10702074/diff/25001/sync/notifier/sync_notifie...
sync/notifier/sync_notifier_helper.h:26: // registered values.
Specify that at most one handler can handle a given object ID. (This is true,
right?)

http://codereview.chromium.org/10702074/diff/25001/sync/notifier/sync_notifie...
sync/notifier/sync_notifier_helper.h:35: // Note that this is non-const because
FOR_EACH_OBSERVER() may mutate the
Would rather replace this accessor with functions similar to
DispatchInvalidationToHandlers, since we have a small number of observer
methods. (Just applying Law of Demeter)

[dcheng, 2012-07-19 18:31:04]

Please ignore the incomplete test. I mainly addressed your review comments in
this patch, and I didn't have time to finish the test since I'm working on a
WebKit bug atm.
Also sorry for some unrelated lines showing up in the diffs. I meant to merge an
updated origin/master into another branch.

http://codereview.chromium.org/10702074/diff/25001/chrome/browser/sync/glue/c...
File chrome/browser/sync/glue/chrome_sync_notification_bridge.cc (right):

http://codereview.chromium.org/10702074/diff/25001/chrome/browser/sync/glue/c...
chrome/browser/sync/glue/chrome_sync_notification_bridge.cc:68:
helper_.DispatchInvalidationsToHandlers(
On 2012/07/19 00:42:06, akalin wrote:
> this won't work.  the invalidations must happen on the same thread as called
by
> UpdateRegisteredIds.  Ugh, this sucks, I forgot about this. :( Let me try to
> think of a solution.
> 
> I may try to fix this in a separate patch in parallel.

I don't quite understand the issue. Why isn't it a problem in the current code?

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

http://codereview.chromium.org/10702074/diff/25001/sync/internal_api/sync_man...
sync/internal_api/sync_manager_impl.cc:2353: ModelTypePayloadMap type_payloads =
On 2012/07/19 00:42:06, akalin wrote:
> const ref

Done.

As an aside, do you know if the style guide has some preference for binding
returned temporaries to const refs, or if this is something that is preferred in
sync code? AFAIK, it shouldn't actually make a difference in generated code due
to RVO/NRVO.

http://codereview.chromium.org/10702074/diff/25001/sync/internal_api/sync_man...
sync/internal_api/sync_manager_impl.cc:2486: ModelTypePayloadMap type_payloads =
On 2012/07/19 00:42:06, akalin wrote:
> const ref

Done.

http://codereview.chromium.org/10702074/diff/25001/sync/notifier/invalidation...
File sync/notifier/invalidation_notifier.h (right):

http://codereview.chromium.org/10702074/diff/25001/sync/notifier/invalidation...
sync/notifier/invalidation_notifier.h:80: ObjectIdSet registered_ids_;
On 2012/07/19 00:42:06, akalin wrote:
> registered_ids_ isn't needed is it?

Done.

http://codereview.chromium.org/10702074/diff/25001/sync/notifier/invalidation...
File sync/notifier/invalidation_util.cc (right):

http://codereview.chromium.org/10702074/diff/25001/sync/notifier/invalidation...
sync/notifier/invalidation_util.cc:41: for (ModelTypeSet::Iterator iter =
model_types.First(); iter.Good();
On 2012/07/19 00:42:06, akalin wrote:
> iter -> it?

Done.

http://codereview.chromium.org/10702074/diff/25001/sync/notifier/non_blocking...
File sync/notifier/non_blocking_invalidation_notifier.cc (right):

http://codereview.chromium.org/10702074/diff/25001/sync/notifier/non_blocking...
sync/notifier/non_blocking_invalidation_notifier.cc:109:
invalidation_notifier_->UpdateRegisteredIds(this, ids);
On 2012/07/19 00:42:06, akalin wrote:
> don't we need to union ids with the set of all registered ids?  since we're
> replacing, right

SyncNotifierHelper::UpdateRegisteredIds() conveniently returns the set of all
registered IDs when it's finished updating its internal map, and that's what
gets passed in here.

http://codereview.chromium.org/10702074/diff/25001/sync/notifier/p2p_notifier.cc
File sync/notifier/p2p_notifier.cc (right):

http://codereview.chromium.org/10702074/diff/25001/sync/notifier/p2p_notifier...
sync/notifier/p2p_notifier.cc:162: const ModelTypeSet enabled_types =
ObjectIdSetToModelTypeSet(
On 2012/07/19 00:42:06, akalin wrote:
> const ref

Done.

http://codereview.chromium.org/10702074/diff/25001/sync/notifier/p2p_notifier...
sync/notifier/p2p_notifier.cc:164: const ModelTypeSet new_enabled_types =
On 2012/07/19 00:42:07, akalin wrote:
> const ref

Done.

http://codereview.chromium.org/10702074/diff/25001/sync/notifier/sync_notifie...
File sync/notifier/sync_notifier_helper.cc (right):

http://codereview.chromium.org/10702074/diff/25001/sync/notifier/sync_notifie...
sync/notifier/sync_notifier_helper.cc:14: if (ids.empty()) {
On 2012/07/19 00:42:07, akalin wrote:
> given that at most one handler can handle a given object ID, I think a better
> data structure would be:
> 
> map<Observer*, ObjectIdSet> handler_to_ids_map_;
> ObjectIdSet registered_ids_;
> 
> Then this function could just be:
> 
> ids_to_unregister = SetDiff(ids, handler_to_ids_map_[handler]);
> if (ids.empty()) {
>   handlers_to_ids_map_.erase(handler);
> } else {
>   handlers_to_ids_map_[handler] = ids;
> }
> 
> registered_ids_ = SetDiff(Union(registered_ids_, ids), ids_to_unregister);
> return registered_ids_;
> 
> And DispatchInvalidationsToHandlers can just be:
> 
> for ((handler, ids) in handlers_to_ids_map_) {
>   <build id->payload map from id_payloads and ids>
>   dispatch_map[handler] = id->payload map;
> }
> 
> <dispatch>

I'm not a fan of the proposed scheme. It's more logical to go the other way and
build a directory of lookups for dispatching, rather than the proposed scheme of
scanning through all registered handlers and filtering the entire payload map
against each object ID set. You could use a counter to break out early, but
that'd just make the Dispatch method even longer.

http://codereview.chromium.org/10702074/diff/25001/sync/notifier/sync_notifie...
sync/notifier/sync_notifier_helper.cc:46: std::map<SyncNotifierObserver*,
ObjectIdPayloadMap> dispatch_map;
On 2012/07/19 00:42:07, akalin wrote:
> typedef the map to something? (can be scoped to this function)

Done.

http://codereview.chromium.org/10702074/diff/25001/sync/notifier/sync_notifie...
sync/notifier/sync_notifier_helper.cc:60:
DCHECK(handlers_.HasObserver(it->first));
On 2012/07/19 00:42:07, akalin wrote:
> this doesn't necessarily hold does it?  OnIncomingNotification can do
anything,
> including removing all other handlers.

That seems like an overreaching handler, and not a situation that we should
encourage. But yes, I guess it's possible =)

I'm happy to remove the DCHECK if you want though.

http://codereview.chromium.org/10702074/diff/25001/sync/notifier/sync_notifie...
File sync/notifier/sync_notifier_helper.h (right):

http://codereview.chromium.org/10702074/diff/25001/sync/notifier/sync_notifie...
sync/notifier/sync_notifier_helper.h:18: class SyncNotifierHelper {
On 2012/07/19 00:42:07, akalin wrote:
> class-level comment

Done.

http://codereview.chromium.org/10702074/diff/25001/sync/notifier/sync_notifie...
sync/notifier/sync_notifier_helper.h:21: virtual ~SyncNotifierHelper();
On 2012/07/19 00:42:07, akalin wrote:
> doesn't need to be virtual

Leftover from an earlier patch. We don't need it at all now.

http://codereview.chromium.org/10702074/diff/25001/sync/notifier/sync_notifie...
sync/notifier/sync_notifier_helper.h:26: // registered values.
On 2012/07/19 00:42:07, akalin wrote:
> Specify that at most one handler can handle a given object ID. (This is true,
> right?)

Done.

http://codereview.chromium.org/10702074/diff/25001/sync/notifier/sync_notifie...
sync/notifier/sync_notifier_helper.h:26: // registered values.
On 2012/07/19 00:42:07, akalin wrote:
> was momentarily confused -- maybe '...values.' -> 'values for all existing
> handlers.'

Done.

http://codereview.chromium.org/10702074/diff/25001/sync/notifier/sync_notifie...
sync/notifier/sync_notifier_helper.h:35: // Note that this is non-const because
FOR_EACH_OBSERVER() may mutate the
On 2012/07/19 00:42:07, akalin wrote:
> Would rather replace this accessor with functions similar to
> DispatchInvalidationToHandlers, since we have a small number of observer
> methods. (Just applying Law of Demeter)

Done.

[akalin, 2012-07-19 23:07:26]

replying to comments, will wait for your next patch

http://codereview.chromium.org/10702074/diff/25001/chrome/browser/sync/glue/c...
File chrome/browser/sync/glue/chrome_sync_notification_bridge.cc (right):

http://codereview.chromium.org/10702074/diff/25001/chrome/browser/sync/glue/c...
chrome/browser/sync/glue/chrome_sync_notification_bridge.cc:68:
helper_.DispatchInvalidationsToHandlers(
On 2012/07/19 18:31:05, dcheng wrote:
> On 2012/07/19 00:42:06, akalin wrote:
> > this won't work.  the invalidations must happen on the same thread as called
> by
> > UpdateRegisteredIds.  Ugh, this sucks, I forgot about this. :( Let me try to
> > think of a solution.
> > 
> > I may try to fix this in a separate patch in parallel.
> 
> I don't quite understand the issue. Why isn't it a problem in the current
code?

ObserverListThreadSafe guarantees that notifications are sent only on the same
thread as AddObserver, and only if RemoveObserver (which has to happen on the
same thread) hasn't been called yet.  Without using OLTS, you'd need to
guarantee that yourself (which is non-trivial).

FYI, my patch to fix this is https://chromiumcodereview.appspot.com/10808040/

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

http://codereview.chromium.org/10702074/diff/25001/sync/internal_api/sync_man...
sync/internal_api/sync_manager_impl.cc:2353: ModelTypePayloadMap type_payloads =
On 2012/07/19 18:31:05, dcheng wrote:
> On 2012/07/19 00:42:06, akalin wrote:
> > const ref
> 
> Done.
> 
> As an aside, do you know if the style guide has some preference for binding
> returned temporaries to const refs, or if this is something that is preferred
in
> sync code? AFAIK, it shouldn't actually make a difference in generated code
due
> to RVO/NRVO.

I don't think so -- I just prefer it myself.  I think it always avoids a copy,
even if RVO/NRVO doesn't apply.

http://codereview.chromium.org/10702074/diff/25001/sync/notifier/non_blocking...
File sync/notifier/non_blocking_invalidation_notifier.cc (right):

http://codereview.chromium.org/10702074/diff/25001/sync/notifier/non_blocking...
sync/notifier/non_blocking_invalidation_notifier.cc:109:
invalidation_notifier_->UpdateRegisteredIds(this, ids);
On 2012/07/19 18:31:05, dcheng wrote:
> On 2012/07/19 00:42:06, akalin wrote:
> > don't we need to union ids with the set of all registered ids?  since we're
> > replacing, right
> 
> SyncNotifierHelper::UpdateRegisteredIds() conveniently returns the set of all
> registered IDs when it's finished updating its internal map, and that's what
> gets passed in here.

Oh, I missed that.  That's easy to miss.  Maybe add a comment at the call-site
to highlight it?

http://codereview.chromium.org/10702074/diff/25001/sync/notifier/sync_notifie...
File sync/notifier/sync_notifier_helper.cc (right):

http://codereview.chromium.org/10702074/diff/25001/sync/notifier/sync_notifie...
sync/notifier/sync_notifier_helper.cc:14: if (ids.empty()) {
On 2012/07/19 18:31:05, dcheng wrote:
> On 2012/07/19 00:42:07, akalin wrote:
> > given that at most one handler can handle a given object ID, I think a
better
> > data structure would be:
> > 
> > map<Observer*, ObjectIdSet> handler_to_ids_map_;
> > ObjectIdSet registered_ids_;
> > 
> > Then this function could just be:
> > 
> > ids_to_unregister = SetDiff(ids, handler_to_ids_map_[handler]);
> > if (ids.empty()) {
> >   handlers_to_ids_map_.erase(handler);
> > } else {
> >   handlers_to_ids_map_[handler] = ids;
> > }
> > 
> > registered_ids_ = SetDiff(Union(registered_ids_, ids), ids_to_unregister);
> > return registered_ids_;
> > 
> > And DispatchInvalidationsToHandlers can just be:
> > 
> > for ((handler, ids) in handlers_to_ids_map_) {
> >   <build id->payload map from id_payloads and ids>
> >   dispatch_map[handler] = id->payload map;
> > }
> > 
> > <dispatch>
> 
> I'm not a fan of the proposed scheme. It's more logical to go the other way
and
> build a directory of lookups for dispatching, rather than the proposed scheme
of
> scanning through all registered handlers and filtering the entire payload map
> against each object ID set. You could use a counter to break out early, but
> that'd just make the Dispatch method even longer.

Ah, I see your point.  You're optimizing for the Dispatch method.  That makes
sense, but that does complicate UpdateRegisteredIds.  I guess that's okay,
although I'll try to think of a way to simplify this method.

http://codereview.chromium.org/10702074/diff/25001/sync/notifier/sync_notifie...
sync/notifier/sync_notifier_helper.cc:60:
DCHECK(handlers_.HasObserver(it->first));
On 2012/07/19 18:31:05, dcheng wrote:
> On 2012/07/19 00:42:07, akalin wrote:
> > this doesn't necessarily hold does it?  OnIncomingNotification can do
> anything,
> > including removing all other handlers.
> 
> That seems like an overreaching handler, and not a situation that we should
> encourage. But yes, I guess it's possible =)
> 
> I'm happy to remove the DCHECK if you want though.

Maybe add a comment somewhere mandating that observers should only touch
themselves ,then?

[dcheng, 2012-07-20 00:57:04]

Now with a test.

http://codereview.chromium.org/10702074/diff/25001/chrome/browser/sync/glue/c...
File chrome/browser/sync/glue/chrome_sync_notification_bridge.cc (right):

http://codereview.chromium.org/10702074/diff/25001/chrome/browser/sync/glue/c...
chrome/browser/sync/glue/chrome_sync_notification_bridge.cc:68:
helper_.DispatchInvalidationsToHandlers(
On 2012/07/19 23:07:26, akalin wrote:
> On 2012/07/19 18:31:05, dcheng wrote:
> > On 2012/07/19 00:42:06, akalin wrote:
> > > this won't work.  the invalidations must happen on the same thread as
called
> > by
> > > UpdateRegisteredIds.  Ugh, this sucks, I forgot about this. :( Let me try
to
> > > think of a solution.
> > > 
> > > I may try to fix this in a separate patch in parallel.
> > 
> > I don't quite understand the issue. Why isn't it a problem in the current
> code?
> 
> ObserverListThreadSafe guarantees that notifications are sent only on the same
> thread as AddObserver, and only if RemoveObserver (which has to happen on the
> same thread) hasn't been called yet.  Without using OLTS, you'd need to
> guarantee that yourself (which is non-trivial).
> 
> FYI, my patch to fix this is https://chromiumcodereview.appspot.com/10808040/

Ah. My fault for not looking at the implementation of OLTS. I assumed it was
pretty much just a bunch of locks =)

I'll wait for that patch to land then.

http://codereview.chromium.org/10702074/diff/25001/sync/notifier/non_blocking...
File sync/notifier/non_blocking_invalidation_notifier.cc (right):

http://codereview.chromium.org/10702074/diff/25001/sync/notifier/non_blocking...
sync/notifier/non_blocking_invalidation_notifier.cc:109:
invalidation_notifier_->UpdateRegisteredIds(this, ids);
On 2012/07/19 23:07:26, akalin wrote:
> On 2012/07/19 18:31:05, dcheng wrote:
> > On 2012/07/19 00:42:06, akalin wrote:
> > > don't we need to union ids with the set of all registered ids?  since
we're
> > > replacing, right
> > 
> > SyncNotifierHelper::UpdateRegisteredIds() conveniently returns the set of
all
> > registered IDs when it's finished updating its internal map, and that's what
> > gets passed in here.
> 
> Oh, I missed that.  That's easy to miss.  Maybe add a comment at the call-site
> to highlight it?

Done. Not sure my comment is helpful though...

http://codereview.chromium.org/10702074/diff/25001/sync/notifier/sync_notifie...
File sync/notifier/sync_notifier_helper.cc (right):

http://codereview.chromium.org/10702074/diff/25001/sync/notifier/sync_notifie...
sync/notifier/sync_notifier_helper.cc:60:
DCHECK(handlers_.HasObserver(it->first));
On 2012/07/19 23:07:26, akalin wrote:
> On 2012/07/19 18:31:05, dcheng wrote:
> > On 2012/07/19 00:42:07, akalin wrote:
> > > this doesn't necessarily hold does it?  OnIncomingNotification can do
> > anything,
> > > including removing all other handlers.
> > 
> > That seems like an overreaching handler, and not a situation that we should
> > encourage. But yes, I guess it's possible =)
> > 
> > I'm happy to remove the DCHECK if you want though.
> 
> Maybe add a comment somewhere mandating that observers should only touch
> themselves ,then?

After more thought, I just removed the DCHECK. I also updated the comments for
SyncNotifier::UpdateRegisteredIds and moved the comment about one handler per ID
there, since it's the public interface that gets used.

[akalin, 2012-07-20 19:01:40]

more SyncNotifierHelper comments

http://codereview.chromium.org/10702074/diff/28004/sync/notifier/sync_notifie...
File sync/notifier/sync_notifier_helper.cc (right):

http://codereview.chromium.org/10702074/diff/28004/sync/notifier/sync_notifie...
sync/notifier/sync_notifier_helper.cc:19: ObjectIdSet registered_ids;
why not just initialize registered_ids with ids?

http://codereview.chromium.org/10702074/diff/28004/sync/notifier/sync_notifie...
sync/notifier/sync_notifier_helper.cc:19: ObjectIdSet registered_ids;
Add a comment here like:

// Remove any existing entries for |handler| and fill |registered_ids| with the
rest.

http://codereview.chromium.org/10702074/diff/28004/sync/notifier/sync_notifie...
sync/notifier/sync_notifier_helper.cc:32: // Small optimization so we only
traverse the map once when inserting entries.
Then add a comment like:

// Now add the new entries for |handler|.

http://codereview.chromium.org/10702074/diff/28004/sync/notifier/sync_notifie...
sync/notifier/sync_notifier_helper.cc:32: // Small optimization so we only
traverse the map once when inserting entries.
it's a bit unclear what the small optimization is.  Perhaps:

// Keep track of the last insertion so we only ...

http://codereview.chromium.org/10702074/diff/28004/sync/notifier/sync_notifie...
sync/notifier/sync_notifier_helper.cc:35: insert_it =
id_to_handler_map_.insert(insert_it,
this is dangerous since if an entry for the id already exists (in violation of
the API contract) this insertion will silently no-op.

How about adding:

// Make sure there's no existing handler for an ID.
DCHECK_EQ(insert_it->second, handler);

?

Although ideally, we'd clobber instead of no-oping in release mode.  Yeah, let's
do that.  I think it suffices to do:

if (insert_id->second != handler) {
  NOTREACHED() << <some message>;
  insert_it->second = handler;
}

http://codereview.chromium.org/10702074/diff/28004/sync/notifier/sync_notifie...
sync/notifier/sync_notifier_helper.cc:39: return registered_ids;
add a DCHECK somewhere (or a test in an "if (DEBUG_MODE)" block) that makes sure
handlers_ is consistent with id_to_handler_map_.

http://codereview.chromium.org/10702074/diff/28004/sync/notifier/sync_notifie...
sync/notifier/sync_notifier_helper.cc:58: for (std::map<SyncNotifierObserver*,
ObjectIdPayloadMap>::const_iterator it =
i think it's worth it to do a FOR_EACH_OBSERVER-style loop here, actually, and
do a lookup on dispatch_map for each observer.  It's not as efficient, but I
think the consistency of behavior is worth it.

http://codereview.chromium.org/10702074/diff/28004/sync/notifier/sync_notifie...
sync/notifier/sync_notifier_helper.cc:58: for (std::map<SyncNotifierObserver*,
ObjectIdPayloadMap>::const_iterator it =
use the typedef here

http://codereview.chromium.org/10702074/diff/28004/sync/notifier/sync_notifie...
File sync/notifier/sync_notifier_helper.h (right):

http://codereview.chromium.org/10702074/diff/28004/sync/notifier/sync_notifie...
sync/notifier/sync_notifier_helper.h:37: void OnNotificationsEnabled();
rename to EmitOn...

http://codereview.chromium.org/10702074/diff/28004/sync/notifier/sync_notifie...
sync/notifier/sync_notifier_helper.h:37: void OnNotificationsEnabled();
Add comment like:

// Calls the given handler method for each handler that has registered IDs.

http://codereview.chromium.org/10702074/diff/28004/sync/notifier/sync_notifie...
sync/notifier/sync_notifier_helper.h:38: void
OnNotificationsDisabled(NotificationsDisabledReason reason);
rename to EmitOn...

[akalin, 2012-07-20 19:04:54]

Moar comments

http://codereview.chromium.org/10702074/diff/28004/sync/notifier/non_blocking...
File sync/notifier/non_blocking_invalidation_notifier.cc (right):

http://codereview.chromium.org/10702074/diff/28004/sync/notifier/non_blocking...
sync/notifier/non_blocking_invalidation_notifier.cc:199: //
SyncNotifierHelper::UpdateRegisteredIds updates its internal
Yeah, maybe the comment isn't that helpful.  On second thought, I think it would
be clearest to simply have an intermediate variable:

const ObjectIdSet& registered_ids = helper_...;
if (!...PostTask(..., registered_ids));

http://codereview.chromium.org/10702074/diff/28004/sync/notifier/sync_notifier.h
File sync/notifier/sync_notifier.h (right):

http://codereview.chromium.org/10702074/diff/28004/sync/notifier/sync_notifie...
sync/notifier/sync_notifier.h:27: // attempt to register for the same object ID,
the first registration wins.
oh, I see that you're already aware of the existing behavior.  I think we should
actually ban registering for the same ID (but have the behavior be clobber). 
What do you think?

[akalin, 2012-07-20 19:34:38]

http://codereview.chromium.org/10702074/diff/28004/sync/notifier/sync_notifie...
File sync/notifier/sync_notifier_helper.cc (right):

http://codereview.chromium.org/10702074/diff/28004/sync/notifier/sync_notifie...
sync/notifier/sync_notifier_helper.cc:35: insert_it =
id_to_handler_map_.insert(insert_it,
On 2012/07/20 19:01:40, akalin wrote:
> this is dangerous since if an entry for the id already exists (in violation of
> the API contract) this insertion will silently no-op.
> 
> How about adding:
> 
> // Make sure there's no existing handler for an ID.
> DCHECK_EQ(insert_it->second, handler);
> 
> ?
> 
> Although ideally, we'd clobber instead of no-oping in release mode.  Yeah,
let's
> do that.  I think it suffices to do:
> 
> if (insert_id->second != handler) {
>   NOTREACHED() << <some message>;
>   insert_it->second = handler;
> }

Actually, on second thought, I think we should just CHECK_EQ().

[akalin, 2012-07-20 19:46:51]

On 2012/07/20 19:34:38, akalin wrote:
>
http://codereview.chromium.org/10702074/diff/28004/sync/notifier/sync_notifie...
> File sync/notifier/sync_notifier_helper.cc (right):
> 
>
http://codereview.chromium.org/10702074/diff/28004/sync/notifier/sync_notifie...
> sync/notifier/sync_notifier_helper.cc:35: insert_it =
> id_to_handler_map_.insert(insert_it,
> On 2012/07/20 19:01:40, akalin wrote:
> > this is dangerous since if an entry for the id already exists (in violation
of
> > the API contract) this insertion will silently no-op.
> > 
> > How about adding:
> > 
> > // Make sure there's no existing handler for an ID.
> > DCHECK_EQ(insert_it->second, handler);
> > 
> > ?
> > 
> > Although ideally, we'd clobber instead of no-oping in release mode.  Yeah,
> let's
> > do that.  I think it suffices to do:
> > 
> > if (insert_id->second != handler) {
> >   NOTREACHED() << <some message>;
> >   insert_it->second = handler;
> > }
> 
> Actually, on second thought, I think we should just CHECK_EQ().

Also, this patch needs to be rebased. :/

[akalin, 2012-07-20 21:07:13]

My patch landed as 147717, so you should be able to rebase on that and get a
working patch. (Assuming I don't get reverted.)

[dcheng, 2012-07-20 21:13:33]

On 2012/07/20 21:07:13, akalin wrote:
> My patch landed as 147717, so you should be able to rebase on that and get a
> working patch. (Assuming I don't get reverted.)

I saw. Working on the first merge right now and cleaning up any leftover
syncer:: references that are unneeded. Then I'll merge in that patch and upload
before addressing the rest of the comments.

[dcheng, 2012-07-21 00:09:54]

http://codereview.chromium.org/10702074/diff/28004/sync/notifier/non_blocking...
File sync/notifier/non_blocking_invalidation_notifier.cc (right):

http://codereview.chromium.org/10702074/diff/28004/sync/notifier/non_blocking...
sync/notifier/non_blocking_invalidation_notifier.cc:199: //
SyncNotifierHelper::UpdateRegisteredIds updates its internal
On 2012/07/20 19:04:54, akalin wrote:
> Yeah, maybe the comment isn't that helpful.  On second thought, I think it
would
> be clearest to simply have an intermediate variable:
> 
> const ObjectIdSet& registered_ids = helper_...;
> if (!...PostTask(..., registered_ids));

Done.

http://codereview.chromium.org/10702074/diff/28004/sync/notifier/sync_notifier.h
File sync/notifier/sync_notifier.h (right):

http://codereview.chromium.org/10702074/diff/28004/sync/notifier/sync_notifie...
sync/notifier/sync_notifier.h:27: // attempt to register for the same object ID,
the first registration wins.
On 2012/07/20 19:04:54, akalin wrote:
> oh, I see that you're already aware of the existing behavior.  I think we
should
> actually ban registering for the same ID (but have the behavior be clobber). 
> What do you think?

That seems reasonable. I've updated the patch to be so.

http://codereview.chromium.org/10702074/diff/28004/sync/notifier/sync_notifie...
File sync/notifier/sync_notifier_helper.cc (right):

http://codereview.chromium.org/10702074/diff/28004/sync/notifier/sync_notifie...
sync/notifier/sync_notifier_helper.cc:19: ObjectIdSet registered_ids;
On 2012/07/20 19:01:40, akalin wrote:
> why not just initialize registered_ids with ids?

Done.

http://codereview.chromium.org/10702074/diff/28004/sync/notifier/sync_notifie...
sync/notifier/sync_notifier_helper.cc:19: ObjectIdSet registered_ids;
On 2012/07/20 19:01:40, akalin wrote:
> Add a comment here like:
> 
> // Remove any existing entries for |handler| and fill |registered_ids| with
the
> rest.

Done.

http://codereview.chromium.org/10702074/diff/28004/sync/notifier/sync_notifie...
sync/notifier/sync_notifier_helper.cc:32: // Small optimization so we only
traverse the map once when inserting entries.
On 2012/07/20 19:01:40, akalin wrote:
> it's a bit unclear what the small optimization is.  Perhaps:
> 
> // Keep track of the last insertion so we only ...

Done.

http://codereview.chromium.org/10702074/diff/28004/sync/notifier/sync_notifie...
sync/notifier/sync_notifier_helper.cc:32: // Small optimization so we only
traverse the map once when inserting entries.
On 2012/07/20 19:01:40, akalin wrote:
> Then add a comment like:
> 
> // Now add the new entries for |handler|.

Done.

http://codereview.chromium.org/10702074/diff/28004/sync/notifier/sync_notifie...
sync/notifier/sync_notifier_helper.cc:35: insert_it =
id_to_handler_map_.insert(insert_it,
On 2012/07/20 19:34:38, akalin wrote:
> On 2012/07/20 19:01:40, akalin wrote:
> > this is dangerous since if an entry for the id already exists (in violation
of
> > the API contract) this insertion will silently no-op.
> > 
> > How about adding:
> > 
> > // Make sure there's no existing handler for an ID.
> > DCHECK_EQ(insert_it->second, handler);
> > 
> > ?
> > 
> > Although ideally, we'd clobber instead of no-oping in release mode.  Yeah,
> let's
> > do that.  I think it suffices to do:
> > 
> > if (insert_id->second != handler) {
> >   NOTREACHED() << <some message>;
> >   insert_it->second = handler;
> > }
> 
> Actually, on second thought, I think we should just CHECK_EQ().

Is there a way to make CHECKS non-fatal for unit tests? Otherwise, I don't see
how I can add this without breaking the unti test.

http://codereview.chromium.org/10702074/diff/28004/sync/notifier/sync_notifie...
sync/notifier/sync_notifier_helper.cc:35: insert_it =
id_to_handler_map_.insert(insert_it,
On 2012/07/20 19:01:40, akalin wrote:
> this is dangerous since if an entry for the id already exists (in violation of
> the API contract) this insertion will silently no-op.
> 
> How about adding:
> 
> // Make sure there's no existing handler for an ID.
> DCHECK_EQ(insert_it->second, handler);
> 
> ?
> 
> Although ideally, we'd clobber instead of no-oping in release mode.  Yeah,
let's
> do that.  I think it suffices to do:
> 
> if (insert_id->second != handler) {
>   NOTREACHED() << <some message>;
>   insert_it->second = handler;
> }

Done with a variation.

http://codereview.chromium.org/10702074/diff/28004/sync/notifier/sync_notifie...
sync/notifier/sync_notifier_helper.cc:39: return registered_ids;
On 2012/07/20 19:01:40, akalin wrote:
> add a DCHECK somewhere (or a test in an "if (DEBUG_MODE)" block) that makes
sure
> handlers_ is consistent with id_to_handler_map_.

Done.

http://codereview.chromium.org/10702074/diff/28004/sync/notifier/sync_notifie...
sync/notifier/sync_notifier_helper.cc:58: for (std::map<SyncNotifierObserver*,
ObjectIdPayloadMap>::const_iterator it =
On 2012/07/20 19:01:40, akalin wrote:
> use the typedef here

Done.

http://codereview.chromium.org/10702074/diff/28004/sync/notifier/sync_notifie...
sync/notifier/sync_notifier_helper.cc:58: for (std::map<SyncNotifierObserver*,
ObjectIdPayloadMap>::const_iterator it =
On 2012/07/20 19:01:40, akalin wrote:
> i think it's worth it to do a FOR_EACH_OBSERVER-style loop here, actually, and
> do a lookup on dispatch_map for each observer.  It's not as efficient, but I
> think the consistency of behavior is worth it.

I'd prefer not to dispatch invalidations with no associated ObjectIdSet though.

http://codereview.chromium.org/10702074/diff/28004/sync/notifier/sync_notifie...
File sync/notifier/sync_notifier_helper.h (right):

http://codereview.chromium.org/10702074/diff/28004/sync/notifier/sync_notifie...
sync/notifier/sync_notifier_helper.h:37: void OnNotificationsEnabled();
On 2012/07/20 19:01:40, akalin wrote:
> rename to EmitOn...

Done.

http://codereview.chromium.org/10702074/diff/28004/sync/notifier/sync_notifie...
sync/notifier/sync_notifier_helper.h:37: void OnNotificationsEnabled();
On 2012/07/20 19:01:40, akalin wrote:
> Add comment like:
> 
> // Calls the given handler method for each handler that has registered IDs.

Done.

http://codereview.chromium.org/10702074/diff/28004/sync/notifier/sync_notifie...
sync/notifier/sync_notifier_helper.h:38: void
OnNotificationsDisabled(NotificationsDisabledReason reason);
On 2012/07/20 19:01:40, akalin wrote:
> rename to EmitOn...

Done.

[akalin, 2012-07-21 00:15:39]

On 2012/07/21 00:09:54, dcheng wrote:
> Is there a way to make CHECKS non-fatal for unit tests? Otherwise, I don't see
> how I can add this without breaking the unti test.

Hmm.  You could use ASSERT_DEATH/EXPECT_DEATH, maybe?

> I'd prefer not to dispatch invalidations with no associated ObjectIdSet
though.

Yes, I meant loop over the observer list, look up the handler in the dispatch
map, and emit an invalidation only if it's in there (which means its set is
non-empty, right).

[akalin, 2012-07-21 00:16:29]

http://codereview.chromium.org/10702074/diff/25062/sync/notifier/sync_notifie...
File sync/notifier/sync_notifier_helper.h (right):

http://codereview.chromium.org/10702074/diff/25062/sync/notifier/sync_notifie...
sync/notifier/sync_notifier_helper.h:25: 
clang is complaining saying that you need an explicit out-of-line destructor for
this class

[dcheng, 2012-07-21 00:20:14]

On 2012/07/21 00:15:39, akalin wrote:
> On 2012/07/21 00:09:54, dcheng wrote:
> > Is there a way to make CHECKS non-fatal for unit tests? Otherwise, I don't
see
> > how I can add this without breaking the unti test.
> 
> Hmm.  You could use ASSERT_DEATH/EXPECT_DEATH, maybe?
> 

Well, that doesn't quite accomplish what I want because I want to verify that we
still clobber the handler value... unless we prefer to just CHECK in this
scenario. Then it's easy with death tests.

> > I'd prefer not to dispatch invalidations with no associated ObjectIdSet
> though.
> 
> Yes, I meant loop over the observer list, look up the handler in the dispatch
> map, and emit an invalidation only if it's in there (which means its set is
> non-empty, right).

FOR_EACH_OBSERVER isn't capable of doing advanced things like that though... I'd
have to manually expand the macro. =/

http://codereview.chromium.org/10702074/diff/25062/sync/notifier/sync_notifie...
File sync/notifier/sync_notifier_helper.h (right):

http://codereview.chromium.org/10702074/diff/25062/sync/notifier/sync_notifie...
sync/notifier/sync_notifier_helper.h:25: 
On 2012/07/21 00:16:29, akalin wrote:
> clang is complaining saying that you need an explicit out-of-line destructor
for
> this class

Done.

[akalin, 2012-07-21 00:23:02]

On Fri, Jul 20, 2012 at 5:20 PM, <dcheng@chromium.org> wrote:

> On 2012/07/21 00:15:39, akalin wrote:
>
>> On 2012/07/21 00:09:54, dcheng wrote:
>> > Is there a way to make CHECKS non-fatal for unit tests? Otherwise, I
>> don't
>>
> see
>
>> > how I can add this without breaking the unti test.
>>
>
>  Hmm.  You could use ASSERT_DEATH/EXPECT_DEATH, maybe?
>>
>
>
> Well, that doesn't quite accomplish what I want because I want to verify
> that we
> still clobber the handler value... unless we prefer to just CHECK in this
> scenario. Then it's easy with death tests.


Right.  I think we should just CHECK and avoid the question of what the
"right" thing to do is.


> > I'd prefer not to dispatch invalidations with no associated ObjectIdSet
>> though.
>>
>
>  Yes, I meant loop over the observer list, look up the handler in the
>> dispatch
>> map, and emit an invalidation only if it's in there (which means its set
>> is
>> non-empty, right).
>>
>
> FOR_EACH_OBSERVER isn't capable of doing advanced things like that
> though... I'd
> have to manually expand the macro. =/


Right, that's what I meant.  It's ugly, but it's not so bad.  I'm mostly
concerned about getting the re-entrant-safe behavior of ObserverList.

[dcheng, 2012-07-21 00:32:29]

On 2012/07/21 00:23:02, akalin wrote:
> Right.  I think we should just CHECK and avoid the question of what the
> "right" thing to do is.

It's a CHECK_EQ and death test now.

> Right, that's what I meant.  It's ugly, but it's not so bad.  I'm mostly
> concerned about getting the re-entrant-safe behavior of ObserverList.

Our current behavior is already safe for reentrancy. The only thing
FOR_EACH_OBSERVERS gives is that notifications won't be dispatched if a handler
removes handlers that happen to run after it.

[akalin, 2012-07-21 00:35:40]

On Fri, Jul 20, 2012 at 5:32 PM, <dcheng@chromium.org> wrote:

> Our current behavior is already safe for reentrancy. The only thing
> FOR_EACH_OBSERVERS gives is that notifications won't be dispatched if a
> handler
> removes handlers that happen to run after it.
>

Right, but that's dangerous (I know we've talked about this before
already...).  If we call back a handler that's expected to be removed, it's
highly likely to do something bad.  I'd rather be defensive about it. :/

[dcheng, 2012-07-21 00:41:09]

On 2012/07/21 00:35:40, akalin wrote:
> On Fri, Jul 20, 2012 at 5:32 PM, <mailto:dcheng@chromium.org> wrote:
> 
> > Our current behavior is already safe for reentrancy. The only thing
> > FOR_EACH_OBSERVERS gives is that notifications won't be dispatched if a
> > handler
> > removes handlers that happen to run after it.
> >
> 
> Right, but that's dangerous (I know we've talked about this before
> already...).  If we call back a handler that's expected to be removed, it's
> highly likely to do something bad.  I'd rather be defensive about it. :/

OK... well, I don't really like encouraging the behavior, since with the
observer pattern, there really isn't supposed to be a deterministic ordering
between the observers, but done.

[akalin, 2012-07-21 00:42:35]

On 2012/07/21 00:41:09, dcheng wrote:
> OK... well, I don't really like encouraging the behavior, since with the
> observer pattern, there really isn't supposed to be a deterministic ordering
> between the observers, but done.

Thanks.  I don't like encouraging that behavior either, but the other methods
use the observer list, so I'd like to keep the behavior consistent.

Looking over everything...

[akalin, 2012-07-21 01:09:47]

LGTM after nits!!

http://codereview.chromium.org/10702074/diff/32090/chrome/browser/sync/glue/b...
File chrome/browser/sync/glue/bridged_sync_notifier_unittest.cc (right):

http://codereview.chromium.org/10702074/diff/32090/chrome/browser/sync/glue/b...
chrome/browser/sync/glue/bridged_sync_notifier_unittest.cc:78:
EXPECT_CALL(*mock_delegate_, UpdateRegisteredIds(
expect on the mock_bridge_ also?

http://codereview.chromium.org/10702074/diff/32090/chrome/browser/sync/glue/c...
File chrome/browser/sync/glue/chrome_sync_notification_bridge.cc (left):

http://codereview.chromium.org/10702074/diff/32090/chrome/browser/sync/glue/c...
chrome/browser/sync/glue/chrome_sync_notification_bridge.cc:44:
syncer::ModelTypeSet enabled_types_;
remove model-type-set include also?

http://codereview.chromium.org/10702074/diff/32090/chrome/browser/sync/glue/c...
chrome/browser/sync/glue/chrome_sync_notification_bridge.cc:45:
ObserverList<syncer::SyncNotifierObserver> observers_;
remove observerlist include

http://codereview.chromium.org/10702074/diff/32090/chrome/browser/sync/glue/c...
chrome/browser/sync/glue/chrome_sync_notification_bridge.cc:68:
DCHECK(sync_task_runner_->RunsTasksOnCurrentThread());
DCHECK(sync_task_runner_->RunsTasksOnCurrentThread());

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

http://codereview.chromium.org/10702074/diff/32090/chrome/browser/sync/glue/c...
chrome/browser/sync/glue/chrome_sync_notification_bridge_unittest.cc:56: const
syncer::ObjectIdSet& ids = syncer::ModelTypeSetToObjectIdSet(
not necessary for this CL, but surely we should have an ObjectIdPayloadMap ->
ObjectIdSet function instead of having to go through ModelType. :)  Add a TODO?

http://codereview.chromium.org/10702074/diff/32090/sync/internal_api/syncapi_...
File sync/internal_api/syncapi_unittest.cc (right):

http://codereview.chromium.org/10702074/diff/32090/sync/internal_api/syncapi_...
sync/internal_api/syncapi_unittest.cc:725: sync_manager_("Test sync manager") {}
init sync_notifier_mock_ to NULL

http://codereview.chromium.org/10702074/diff/32090/sync/notifier/invalidation...
File sync/notifier/invalidation_notifier_unittest.cc (right):

http://codereview.chromium.org/10702074/diff/32090/sync/notifier/invalidation...
sync/notifier/invalidation_notifier_unittest.cc:77: ModelTypeSet
models(PREFERENCES,
fit on one line?

http://codereview.chromium.org/10702074/diff/32090/sync/notifier/invalidation...
sync/notifier/invalidation_notifier_unittest.cc:83: ModelTypePayloadMap
type_payloads =
const ref

http://codereview.chromium.org/10702074/diff/32090/sync/notifier/non_blocking...
File sync/notifier/non_blocking_invalidation_notifier_unittest.cc (right):

http://codereview.chromium.org/10702074/diff/32090/sync/notifier/non_blocking...
sync/notifier/non_blocking_invalidation_notifier_unittest.cc:71: BOOKMARKS,
one line?

http://codereview.chromium.org/10702074/diff/32090/sync/notifier/non_blocking...
sync/notifier/non_blocking_invalidation_notifier_unittest.cc:76:
ModelTypePayloadMap type_payloads =
const ref

http://codereview.chromium.org/10702074/diff/32090/sync/notifier/p2p_notifier.cc
File sync/notifier/p2p_notifier.cc (right):

http://codereview.chromium.org/10702074/diff/32090/sync/notifier/p2p_notifier...
sync/notifier/p2p_notifier.cc:162: const ModelTypeSet& enabled_types =
ObjectIdSetToModelTypeSet(
don't const-ref &, just use const (since it fits in an int)

http://codereview.chromium.org/10702074/diff/32090/sync/notifier/sync_notifie...
File sync/notifier/sync_notifier_helper.cc (right):

http://codereview.chromium.org/10702074/diff/32090/sync/notifier/sync_notifie...
sync/notifier/sync_notifier_helper.cc:5: #include
"sync/notifier/sync_notifier_helper.h"
#include "base/logging.h"

http://codereview.chromium.org/10702074/diff/32090/sync/notifier/sync_notifie...
sync/notifier/sync_notifier_helper.cc:44: #ifndef NDEBUG
use:

if (logging::DEBUG_MODE) {
  ...
}

instead.

(there are subtle differences between it and NDEBUG, see logging.h)

http://codereview.chromium.org/10702074/diff/32090/sync/notifier/sync_notifie...
sync/notifier/sync_notifier_helper.cc:73: SyncNotifierObserver* handler;
= NULL;

http://codereview.chromium.org/10702074/diff/32090/sync/notifier/sync_notifie...
File sync/notifier/sync_notifier_helper.h (right):

http://codereview.chromium.org/10702074/diff/32090/sync/notifier/sync_notifie...
sync/notifier/sync_notifier_helper.h:14: #include
"sync/notifier/sync_notifier_observer.h"
can you forward-declare SyncNotifierObserver?

http://codereview.chromium.org/10702074/diff/32090/sync/notifier/sync_notifie...
sync/notifier/sync_notifier_helper.h:46: ObserverList<SyncNotifierObserver>
handlers_;
newline after typedef

http://codereview.chromium.org/10702074/diff/32090/sync/notifier/sync_notifie...
File sync/notifier/sync_notifier_helper_unittest.cc (right):

http://codereview.chromium.org/10702074/diff/32090/sync/notifier/sync_notifie...
sync/notifier/sync_notifier_helper_unittest.cc:15: const int kTestSourceId = 4; 
// Specified by Tango for testing.
is this in the tango headers anywhere?

http://codereview.chromium.org/10702074/diff/32090/sync/notifier/sync_notifie...
sync/notifier/sync_notifier_helper_unittest.cc:31:
TEST_F(SyncNotifierHelperTest, Basic) {
add test-level comments for each test in this file

http://codereview.chromium.org/10702074/diff/32090/sync/notifier/sync_notifie...
sync/notifier/sync_notifier_helper_unittest.cc:118:
EXPECT_DEATH({helper.UpdateRegisteredIds(&observer2, ids); },
space after {

[akalin, 2012-07-21 01:14:09]

forgot one

http://codereview.chromium.org/10702074/diff/32090/sync/notifier/non_blocking...
File sync/notifier/non_blocking_invalidation_notifier.cc (right):

http://codereview.chromium.org/10702074/diff/32090/sync/notifier/non_blocking...
sync/notifier/non_blocking_invalidation_notifier.cc:189: const ObjectIdSet&
registered_ids = helper_.UpdateRegisteredIds(handler, ids);
actually, rename this to "all_registered_ids"

[dcheng, 2012-07-21 14:06:53]

http://codereview.chromium.org/10702074/diff/32090/chrome/browser/sync/glue/b...
File chrome/browser/sync/glue/bridged_sync_notifier_unittest.cc (right):

http://codereview.chromium.org/10702074/diff/32090/chrome/browser/sync/glue/b...
chrome/browser/sync/glue/bridged_sync_notifier_unittest.cc:78:
EXPECT_CALL(*mock_delegate_, UpdateRegisteredIds(
On 2012/07/21 01:09:47, akalin wrote:
> expect on the mock_bridge_ also?

Done.

http://codereview.chromium.org/10702074/diff/32090/chrome/browser/sync/glue/c...
File chrome/browser/sync/glue/chrome_sync_notification_bridge.cc (left):

http://codereview.chromium.org/10702074/diff/32090/chrome/browser/sync/glue/c...
chrome/browser/sync/glue/chrome_sync_notification_bridge.cc:44:
syncer::ModelTypeSet enabled_types_;
On 2012/07/21 01:09:47, akalin wrote:
> remove model-type-set include also?

That's still used in UpdateEnabledTypes.

http://codereview.chromium.org/10702074/diff/32090/chrome/browser/sync/glue/c...
chrome/browser/sync/glue/chrome_sync_notification_bridge.cc:45:
ObserverList<syncer::SyncNotifierObserver> observers_;
On 2012/07/21 01:09:47, akalin wrote:
> remove observerlist include

Done.

http://codereview.chromium.org/10702074/diff/32090/chrome/browser/sync/glue/c...
chrome/browser/sync/glue/chrome_sync_notification_bridge.cc:68:
DCHECK(sync_task_runner_->RunsTasksOnCurrentThread());
On 2012/07/21 01:09:47, akalin wrote:
> DCHECK(sync_task_runner_->RunsTasksOnCurrentThread());

UpdateRegisteredIds has the corresponding DCHECK?

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

http://codereview.chromium.org/10702074/diff/32090/chrome/browser/sync/glue/c...
chrome/browser/sync/glue/chrome_sync_notification_bridge_unittest.cc:56: const
syncer::ObjectIdSet& ids = syncer::ModelTypeSetToObjectIdSet(
On 2012/07/21 01:09:47, akalin wrote:
> not necessary for this CL, but surely we should have an ObjectIdPayloadMap ->
> ObjectIdSet function instead of having to go through ModelType. :)  Add a
TODO?

Done.

http://codereview.chromium.org/10702074/diff/32090/sync/internal_api/syncapi_...
File sync/internal_api/syncapi_unittest.cc (right):

http://codereview.chromium.org/10702074/diff/32090/sync/internal_api/syncapi_...
sync/internal_api/syncapi_unittest.cc:725: sync_manager_("Test sync manager") {}
On 2012/07/21 01:09:47, akalin wrote:
> init sync_notifier_mock_ to NULL

Err... it is on line 724? =)
(I didn't add it, I just moved the definition from private to protected)

http://codereview.chromium.org/10702074/diff/32090/sync/notifier/invalidation...
File sync/notifier/invalidation_notifier_unittest.cc (right):

http://codereview.chromium.org/10702074/diff/32090/sync/notifier/invalidation...
sync/notifier/invalidation_notifier_unittest.cc:77: ModelTypeSet
models(PREFERENCES,
On 2012/07/21 01:09:47, akalin wrote:
> fit on one line?

Done.

http://codereview.chromium.org/10702074/diff/32090/sync/notifier/invalidation...
sync/notifier/invalidation_notifier_unittest.cc:83: ModelTypePayloadMap
type_payloads =
On 2012/07/21 01:09:47, akalin wrote:
> const ref

Done.

http://codereview.chromium.org/10702074/diff/32090/sync/notifier/non_blocking...
File sync/notifier/non_blocking_invalidation_notifier.cc (right):

http://codereview.chromium.org/10702074/diff/32090/sync/notifier/non_blocking...
sync/notifier/non_blocking_invalidation_notifier.cc:189: const ObjectIdSet&
registered_ids = helper_.UpdateRegisteredIds(handler, ids);
On 2012/07/21 01:14:10, akalin wrote:
> actually, rename this to "all_registered_ids"

Done.

http://codereview.chromium.org/10702074/diff/32090/sync/notifier/non_blocking...
File sync/notifier/non_blocking_invalidation_notifier_unittest.cc (right):

http://codereview.chromium.org/10702074/diff/32090/sync/notifier/non_blocking...
sync/notifier/non_blocking_invalidation_notifier_unittest.cc:71: BOOKMARKS,
On 2012/07/21 01:09:47, akalin wrote:
> one line?

Done.

http://codereview.chromium.org/10702074/diff/32090/sync/notifier/non_blocking...
sync/notifier/non_blocking_invalidation_notifier_unittest.cc:76:
ModelTypePayloadMap type_payloads =
On 2012/07/21 01:09:47, akalin wrote:
> const ref

Done.

http://codereview.chromium.org/10702074/diff/32090/sync/notifier/p2p_notifier.cc
File sync/notifier/p2p_notifier.cc (right):

http://codereview.chromium.org/10702074/diff/32090/sync/notifier/p2p_notifier...
sync/notifier/p2p_notifier.cc:162: const ModelTypeSet& enabled_types =
ObjectIdSetToModelTypeSet(
On 2012/07/21 01:09:47, akalin wrote:
> don't const-ref &, just use const (since it fits in an int)

Done.

http://codereview.chromium.org/10702074/diff/32090/sync/notifier/sync_notifie...
File sync/notifier/sync_notifier_helper.cc (right):

http://codereview.chromium.org/10702074/diff/32090/sync/notifier/sync_notifie...
sync/notifier/sync_notifier_helper.cc:5: #include
"sync/notifier/sync_notifier_helper.h"
On 2012/07/21 01:09:47, akalin wrote:
> #include "base/logging.h"

Done.

http://codereview.chromium.org/10702074/diff/32090/sync/notifier/sync_notifie...
sync/notifier/sync_notifier_helper.cc:44: #ifndef NDEBUG
On 2012/07/21 01:09:47, akalin wrote:
> use:
> 
> if (logging::DEBUG_MODE) {
>   ...
> }
> 
> instead.
> 
> (there are subtle differences between it and NDEBUG, see logging.h)

Done.

http://codereview.chromium.org/10702074/diff/32090/sync/notifier/sync_notifie...
sync/notifier/sync_notifier_helper.cc:73: SyncNotifierObserver* handler;
On 2012/07/21 01:09:47, akalin wrote:
> = NULL;

Done.

http://codereview.chromium.org/10702074/diff/32090/sync/notifier/sync_notifie...
File sync/notifier/sync_notifier_helper.h (right):

http://codereview.chromium.org/10702074/diff/32090/sync/notifier/sync_notifie...
sync/notifier/sync_notifier_helper.h:14: #include
"sync/notifier/sync_notifier_observer.h"
On 2012/07/21 01:09:47, akalin wrote:
> can you forward-declare SyncNotifierObserver?

The header is needed for IncomingNotificationSource.

http://codereview.chromium.org/10702074/diff/32090/sync/notifier/sync_notifie...
sync/notifier/sync_notifier_helper.h:46: ObserverList<SyncNotifierObserver>
handlers_;
On 2012/07/21 01:09:47, akalin wrote:
> newline after typedef

Done.

http://codereview.chromium.org/10702074/diff/32090/sync/notifier/sync_notifie...
File sync/notifier/sync_notifier_helper_unittest.cc (right):

http://codereview.chromium.org/10702074/diff/32090/sync/notifier/sync_notifie...
sync/notifier/sync_notifier_helper_unittest.cc:15: const int kTestSourceId = 4; 
// Specified by Tango for testing.
On 2012/07/21 01:09:47, akalin wrote:
> is this in the tango headers anywhere?

Done.

http://codereview.chromium.org/10702074/diff/32090/sync/notifier/sync_notifie...
sync/notifier/sync_notifier_helper_unittest.cc:31:
TEST_F(SyncNotifierHelperTest, Basic) {
On 2012/07/21 01:09:47, akalin wrote:
> add test-level comments for each test in this file

Done.

http://codereview.chromium.org/10702074/diff/32090/sync/notifier/sync_notifie...
sync/notifier/sync_notifier_helper_unittest.cc:118:
EXPECT_DEATH({helper.UpdateRegisteredIds(&observer2, ids); },
On 2012/07/21 01:09:47, akalin wrote:
> space after {

Done.

[commit-bot: I haz the power, 2012-07-21 14:07:18]

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

[commit-bot: I haz the power, 2012-07-21 14:38:52]

Try job failure for 10702074-29102 (retry) on android for steps "compile, build"
(clobber build).
It's a second try, previously, steps "compile, build" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android&nu...

[commit-bot: I haz the power, 2012-07-21 18:03:54]

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

[commit-bot: I haz the power, 2012-07-21 19:40:35]

Change committed as 147801

[akalin, 2012-07-24 03:31:51]

On 2012/07/21 19:40:35, I haz the power (commit-bot) wrote:
> Change committed as 147801

Rebase on top of https://chromiumcodereview.appspot.com/10817023/ and see if it
passes the sync integration test trybots.  If so, you can wait for it to land
and then reland this.

[dcheng, 2012-07-24 19:01:34]

I've rebased this on top of https://chromiumcodereview.appspot.com/10817023/ and
I'm sending it to the trybots. I've made some minor adjustments to
ChromeSyncNotificationBridge.

[akalin, 2012-07-25 19:56:50]

On 2012/07/24 19:01:34, dcheng wrote:
> I've rebased this on top of https://chromiumcodereview.appspot.com/10817023/
and
> I'm sending it to the trybots. I've made some minor adjustments to
> ChromeSyncNotificationBridge.

This should be good to land now.

[commit-bot: I haz the power, 2012-07-25 19:57:08]

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

[commit-bot: I haz the power, 2012-07-25 19:57:24]

Failed to apply patch for sync/notifier/p2p_notifier.cc:
While running patch -p1 --forward --force;
patching file sync/notifier/p2p_notifier.cc
Hunk #6 FAILED at 253.
1 out of 6 hunks FAILED -- saving rejects to file
sync/notifier/p2p_notifier.cc.rej

[commit-bot: I haz the power, 2012-07-25 22:00:22]

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

[commit-bot: I haz the power, 2012-07-25 23:14:37]

Try job failure for 10702074-42001 (retry) on linux_rel for step
"browser_tests".
It's a second try, previously, step "browser_tests" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...

[commit-bot: I haz the power, 2012-07-25 23:19:40]

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

[commit-bot: I haz the power, 2012-07-26 02:36:59]

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

[commit-bot: I haz the power, 2012-07-26 02:37:53]

Change committed as 148496