[Sync] Avoid unregistering object IDs on shutdown

[Sync] Avoid unregistering object IDs on shutdown

Add RegisterHandler() and UnregisterHandler(), which should be called before and after calls to UpdateRegisteredIds().  Use UnregisterHandler() on
shutdown instead of UpdateRegisteredIds(_, ObjectIdSet()).

Make SyncNotifierHelper non-thread-safe.  Fix test breakages that this revealed.  Also add GetAllRegisteredIds() instead of making it the return value of UpdateRegisteredIds().

Propagate UpdateRegisteredIds()/RegisterHandler()/UnregisterHandler() all
the way up to ProfileSyncService.

Make FakeSyncManager be created on the sync thread.

Clean up SyncBackendHost startup/shutdown behavior a bit.

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

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

[akalin, 2012-08-03 04:52:03]

+dcheng for sync/notifier stuff.
+tim for everything else.

+msw FYI.

[dcheng, 2012-08-03 06:05:29]

https://chromiumcodereview.appspot.com/10824161/diff/4002/sync/notifier/sync_...
File sync/notifier/sync_notifier_helper.cc (right):

https://chromiumcodereview.appspot.com/10824161/diff/4002/sync/notifier/sync_...
sync/notifier/sync_notifier_helper.cc:32: handlers_.RemoveObserver(old_handler);
I don't think you need this line, since it's always removed at line 25.

[akalin, 2012-08-03 18:44:10]

PTAL

http://codereview.chromium.org/10824161/diff/4002/sync/notifier/sync_notifier...
File sync/notifier/sync_notifier_helper.cc (right):

http://codereview.chromium.org/10824161/diff/4002/sync/notifier/sync_notifier...
sync/notifier/sync_notifier_helper.cc:32: handlers_.RemoveObserver(old_handler);
On 2012/08/03 06:05:30, dcheng wrote:
> I don't think you need this line, since it's always removed at line 25.

Done.  In fact, the entire else block can be moved to the first block.

[dcheng, 2012-08-03 20:34:14]

LGTM. Some super minor comments.

http://codereview.chromium.org/10824161/diff/6005/sync/notifier/sync_notifier...
File sync/notifier/sync_notifier_helper.cc (right):

http://codereview.chromium.org/10824161/diff/6005/sync/notifier/sync_notifier...
sync/notifier/sync_notifier_helper.cc:31: name_to_handler_map_[handler_name] =
handler;
Nit: Though insert is more verbose, I prefer it over [] when I don't need the
default ctor behavior of [].

http://codereview.chromium.org/10824161/diff/6005/sync/notifier/sync_notifier...
File sync/notifier/sync_notifier_helper.h (right):

http://codereview.chromium.org/10824161/diff/6005/sync/notifier/sync_notifier...
sync/notifier/sync_notifier_helper.h:25: // interface.  It helps keep track of
registered handlers and which
Super minor: most of Chrome uses one space between sentences in comments (for
example, the copyright header =).

[akalin, 2012-08-03 20:48:39]

Tim?

http://codereview.chromium.org/10824161/diff/6005/sync/notifier/sync_notifier...
File sync/notifier/sync_notifier_helper.cc (right):

http://codereview.chromium.org/10824161/diff/6005/sync/notifier/sync_notifier...
sync/notifier/sync_notifier_helper.cc:31: name_to_handler_map_[handler_name] =
handler;
On 2012/08/03 20:34:14, dcheng wrote:
> Nit: Though insert is more verbose, I prefer it over [] when I don't need the
> default ctor behavior of [].

Done.

http://codereview.chromium.org/10824161/diff/6005/sync/notifier/sync_notifier...
File sync/notifier/sync_notifier_helper.h (right):

http://codereview.chromium.org/10824161/diff/6005/sync/notifier/sync_notifier...
sync/notifier/sync_notifier_helper.h:25: // interface.  It helps keep track of
registered handlers and which
On 2012/08/03 20:34:14, dcheng wrote:
> Super minor: most of Chrome uses one space between sentences in comments (for
> example, the copyright header =).

Eh, I don't know if it's worth fixing.  The double-space presence is not
insignificant in chrome. :)

$ git grep  '\. [^ ]' | wc -l
168233
$ git grep  '\.  ' | wc -l
68232

[msw, 2012-08-03 23:30:46]

Lots of line-break and one-param-per-line nits, etc. (sorry if some are
duplicated with the other CL).

I highly recommend getting your IDE to show an 80 char guide line! It's nice
when comment lines break on separate thoughts or at the last possible word
break, it looks ugly when they break unnecessarily mid-sentence and especially
mid-clause.

http://codereview.chromium.org/10824161/diff/1034/chrome/browser/sync/glue/br...
File chrome/browser/sync/glue/bridged_sync_notifier_unittest.cc (right):

http://codereview.chromium.org/10824161/diff/1034/chrome/browser/sync/glue/br...
chrome/browser/sync/glue/bridged_sync_notifier_unittest.cc:61: // All tests just
verify that each call is passed through to the
nit: why line break earlier? Embrace the precious 80 chars!

http://codereview.chromium.org/10824161/diff/1034/chrome/browser/sync/glue/ch...
File chrome/browser/sync/glue/chrome_sync_notification_bridge.h (right):

http://codereview.chromium.org/10824161/diff/1034/chrome/browser/sync/glue/ch...
chrome/browser/sync/glue/chrome_sync_notification_bridge.h:39: // Must be called
on the UI thread while the sync task runner is
nit: line breaking; does your IDE show the 80 char guide?

http://codereview.chromium.org/10824161/diff/1034/chrome/browser/sync/glue/ch...
File chrome/browser/sync/glue/chrome_sync_notification_bridge_unittest.cc
(right):

http://codereview.chromium.org/10824161/diff/1034/chrome/browser/sync/glue/ch...
chrome/browser/sync/glue/chrome_sync_notification_bridge_unittest.cc:7: #include
<cstddef>
nit/question: is it better practice to include this or base/basictypes.h?

http://codereview.chromium.org/10824161/diff/1034/chrome/browser/sync/glue/sy...
File chrome/browser/sync/glue/sync_backend_host.cc (right):

http://codereview.chromium.org/10824161/diff/1034/chrome/browser/sync/glue/sy...
chrome/browser/sync/glue/sync_backend_host.cc:593: if
(chrome_sync_notification_bridge_.get()) {
nit: remove unnecessary {}

http://codereview.chromium.org/10824161/diff/1034/chrome/browser/sync/glue/sy...
File chrome/browser/sync/glue/sync_backend_host.h (right):

http://codereview.chromium.org/10824161/diff/1034/chrome/browser/sync/glue/sy...
chrome/browser/sync/glue/sync_backend_host.h:183: // underlying notifier.  This
lasts until StopSyncingForShutdown()
nit: line breaks :)

http://codereview.chromium.org/10824161/diff/1034/chrome/browser/sync/glue/sy...
chrome/browser/sync/glue/sync_backend_host.h:215: // Called on |frontend_loop_|
to kick off shutdown procedure. After
nit: line breaks :)

http://codereview.chromium.org/10824161/diff/1034/chrome/browser/sync/glue/sy...
chrome/browser/sync/glue/sync_backend_host.h:536: // Whether or not we propagate
notifications to invalidation
nit: line breaks :)

http://codereview.chromium.org/10824161/diff/1034/chrome/browser/sync/glue/sy...
File chrome/browser/sync/glue/sync_backend_host_unittest.cc (right):

http://codereview.chromium.org/10824161/diff/1034/chrome/browser/sync/glue/sy...
chrome/browser/sync/glue/sync_backend_host_unittest.cc:100: std::string name)
OVERRIDE {
nit: fits on line above

http://codereview.chromium.org/10824161/diff/1034/chrome/browser/sync/glue/sy...
chrome/browser/sync/glue/sync_backend_host_unittest.cc:123: FakeSyncManager**
manager_ptr_;
Why is this a double pointer? Also, I think our style guide discourages "_ptr".

http://codereview.chromium.org/10824161/diff/1034/chrome/browser/sync/glue/sy...
chrome/browser/sync/glue/sync_backend_host_unittest.cc:630: // Call
StopSyncingForShutdown() on the backend and fire some
nit: line breaks :)

http://codereview.chromium.org/10824161/diff/1034/chrome/browser/sync/profile...
File chrome/browser/sync/profile_sync_service.h (right):

http://codereview.chromium.org/10824161/diff/1034/chrome/browser/sync/profile...
chrome/browser/sync/profile_sync_service.h:554: // Sets the invalidation handler
for the given name.  Pass in NULL
nit: line breaks :)

http://codereview.chromium.org/10824161/diff/1034/chrome/browser/sync/profile...
chrome/browser/sync/profile_sync_service.h:564: // Updates the set of ObjectIds
associated with a given invalidation
nit: line breaks :)

http://codereview.chromium.org/10824161/diff/1034/chrome/browser/sync/profile...
chrome/browser/sync/profile_sync_service.h:568: // The handler name ->
registered ids map is persisted across
nit: line breaks :)

http://codereview.chromium.org/10824161/diff/1034/sync/internal_api/public/sy...
File sync/internal_api/public/sync_manager.h (right):

http://codereview.chromium.org/10824161/diff/1034/sync/internal_api/public/sy...
sync/internal_api/public/sync_manager.h:410: // SyncNotifier::SetHandler()).
nit: line breaks :)

http://codereview.chromium.org/10824161/diff/1034/sync/internal_api/public/sy...
sync/internal_api/public/sync_manager.h:412: const std::string& handler_name,
SyncNotifierObserver* handler) = 0;
one param per line

http://codereview.chromium.org/10824161/diff/1034/sync/internal_api/public/sy...
sync/internal_api/public/sync_manager.h:417: const std::string& handler_name,
const ObjectIdSet& ids) = 0;
one param per line

http://codereview.chromium.org/10824161/diff/1034/sync/internal_api/public/te...
File sync/internal_api/public/test/fake_sync_manager.h (right):

http://codereview.chromium.org/10824161/diff/1034/sync/internal_api/public/te...
sync/internal_api/public/test/fake_sync_manager.h:23: //
|initial_sync_ended_types|: The set of types that have
nit: line breaks :)

http://codereview.chromium.org/10824161/diff/1034/sync/internal_api/public/te...
sync/internal_api/public/test/fake_sync_manager.h:67: // Block until the sync
thread has finished processing any pending
nit: line breaks :)

http://codereview.chromium.org/10824161/diff/1034/sync/internal_api/sync_mana...
File sync/internal_api/sync_manager_impl.cc (right):

http://codereview.chromium.org/10824161/diff/1034/sync/internal_api/sync_mana...
sync/internal_api/sync_manager_impl.cc:401: if (sync_notifier_.get()) {
nit: remove unnecessary {}

http://codereview.chromium.org/10824161/diff/1034/sync/internal_api/sync_mana...
sync/internal_api/sync_manager_impl.cc:747: const std::string& handler_name,
SyncNotifierObserver* handler) {
one param per line

http://codereview.chromium.org/10824161/diff/1034/sync/internal_api/sync_mana...
sync/internal_api/sync_manager_impl.cc:753: const std::string& handler_name,
const ObjectIdSet& ids) {
one param per line

http://codereview.chromium.org/10824161/diff/1034/sync/internal_api/sync_mana...
File sync/internal_api/sync_manager_impl.h (right):

http://codereview.chromium.org/10824161/diff/1034/sync/internal_api/sync_mana...
sync/internal_api/sync_manager_impl.h:93: const std::string& handler_name, const
ObjectIdSet& ids) OVERRIDE;
one param per line.

http://codereview.chromium.org/10824161/diff/1034/sync/internal_api/test/fake...
File sync/internal_api/test/fake_sync_manager.cc (right):

http://codereview.chromium.org/10824161/diff/1034/sync/internal_api/test/fake...
sync/internal_api/test/fake_sync_manager.cc:88: void
FakeSyncManager::WaitForSyncThread() {
What is this doing? Add a comment as to why this is necessary.

http://codereview.chromium.org/10824161/diff/1034/sync/internal_api/test/fake...
sync/internal_api/test/fake_sync_manager.cc:165: const std::string&
handler_name, SyncNotifierObserver* handler) {
one param per line

http://codereview.chromium.org/10824161/diff/1034/sync/internal_api/test/fake...
sync/internal_api/test/fake_sync_manager.cc:170: const std::string&
handler_name, const ObjectIdSet& ids) {
one param per line

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

http://codereview.chromium.org/10824161/diff/1034/sync/notifier/invalidation_...
sync/notifier/invalidation_notifier_unittest.cc:38: // Constructs an
InvalidationNotifier and places it in
nit: line breaks :)

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

http://codereview.chromium.org/10824161/diff/1034/sync/notifier/non_blocking_...
sync/notifier/non_blocking_invalidation_notifier.cc:7: #include <cstddef>
ditto nit/question for basictypes.h

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

http://codereview.chromium.org/10824161/diff/1034/sync/notifier/p2p_notifier....
sync/notifier/p2p_notifier.cc:168: // TODO(akalin): Handle arbitrary object IDs
nit: line breaks :)

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

http://codereview.chromium.org/10824161/diff/1034/sync/notifier/sync_notifier...
sync/notifier/sync_notifier.h:25: // Sets the handler for the given name.  Pass
in NULL for |handler|
nit: line breaks :)

http://codereview.chromium.org/10824161/diff/1034/sync/notifier/sync_notifier...
File sync/notifier/sync_notifier_helper.cc (right):

http://codereview.chromium.org/10824161/diff/1034/sync/notifier/sync_notifier...
sync/notifier/sync_notifier_helper.cc:40: const std::string& handler_name, const
ObjectIdSet& ids) {
one param per line

http://codereview.chromium.org/10824161/diff/1034/sync/notifier/sync_notifier...
sync/notifier/sync_notifier_helper.cc:54: // Now add the entries for
|handler_name|. We keep track of the last
nit: line breaks :)

http://codereview.chromium.org/10824161/diff/1034/sync/notifier/sync_notifier...
sync/notifier/sync_notifier_helper.cc:73: if (handler) {
nit: remove unnecessary {}

http://codereview.chromium.org/10824161/diff/1034/sync/notifier/sync_notifier...
sync/notifier/sync_notifier_helper.cc:100: // to an handler, just drop it -- the
handler was unregistered
nit: "a handler" and line breaks :)

http://codereview.chromium.org/10824161/diff/1034/sync/notifier/sync_notifier...
sync/notifier/sync_notifier_helper.cc:102: if (!handler) {
nit: just perform the insert if (handler), or remove unnecessary {}

http://codereview.chromium.org/10824161/diff/1034/sync/notifier/sync_notifier...
sync/notifier/sync_notifier_helper.cc:122:
FOR_EACH_OBSERVER(SyncNotifierObserver, handlers_,
nit: line breaks :)

http://codereview.chromium.org/10824161/diff/1034/sync/notifier/sync_notifier...
File sync/notifier/sync_notifier_helper.h (right):

http://codereview.chromium.org/10824161/diff/1034/sync/notifier/sync_notifier...
sync/notifier/sync_notifier_helper.h:24: // A helper class for classes that want
to implement the SyncNotifier
nit: line breaks, this looks worse to me.

http://codereview.chromium.org/10824161/diff/1034/sync/notifier/sync_notifier...
sync/notifier/sync_notifier_helper.h:34: // Sets the handler for the given name.
 Pass in NULL for |handler|
nit: line breaks :)

http://codereview.chromium.org/10824161/diff/1034/sync/notifier/sync_notifier...
sync/notifier/sync_notifier_helper.h:49: // Helper that sorts incoming
invalidations into a bucket for each
nit: line breaks :)

http://codereview.chromium.org/10824161/diff/1034/sync/notifier/sync_notifier...
sync/notifier/sync_notifier_helper.h:55: // Calls the given handler method for
each handler that has
nit: line breaks, why did you change this?

http://codereview.chromium.org/10824161/diff/1034/sync/tools/sync_listen_noti...
File sync/tools/sync_listen_notifications.cc (right):

http://codereview.chromium.org/10824161/diff/1034/sync/tools/sync_listen_noti...
sync/tools/sync_listen_notifications.cc:5: #include <cstddef>
ditto same basictypes nit/q

[akalin, 2012-08-04 00:03:41]

On 2012/08/03 23:30:46, msw wrote:
> Lots of line-break and one-param-per-line nits, etc. (sorry if some are
> duplicated with the other CL).
> 
> I highly recommend getting your IDE to show an 80 char guide line! It's nice
> when comment lines break on separate thoughts or at the last possible word
> break, it looks ugly when they break unnecessarily mid-sentence and especially
> mid-clause.

Hmm.  I usually just auto-wrap with emacs' M-q, which defaults to 72, I think. 
I also tend to aim for 72 more than 80.  But I'll go back and re-indent if it
makes you feel better. :)

[msw, 2012-08-04 00:42:33]

On 2012/08/04 00:03:41, akalin wrote:
> On 2012/08/03 23:30:46, msw wrote:
> > Lots of line-break and one-param-per-line nits, etc. (sorry if some are
> > duplicated with the other CL).
> > 
> > I highly recommend getting your IDE to show an 80 char guide line! It's nice
> > when comment lines break on separate thoughts or at the last possible word
> > break, it looks ugly when they break unnecessarily mid-sentence and
especially
> > mid-clause.
> 
> Hmm.  I usually just auto-wrap with emacs' M-q, which defaults to 72, I think.

> I also tend to aim for 72 more than 80.  But I'll go back and re-indent if it
> makes you feel better. :)

Well, the codebase is generally 80 char lines, so I prefer that.
No big deal though; if 72 is OK with others I'll try to be quiet.

[tim (not reviewing), 2012-08-06 05:55:32]

http://codereview.chromium.org/10824161/diff/1034/chrome/browser/sync/glue/ch...
File chrome/browser/sync/glue/chrome_sync_notification_bridge.cc (right):

http://codereview.chromium.org/10824161/diff/1034/chrome/browser/sync/glue/ch...
chrome/browser/sync/glue/chrome_sync_notification_bridge.cc:31: void
DestroyOnSyncThread();
Is 'Destroy' the right name?  This won't necessarily result in calling the
destructor.

http://codereview.chromium.org/10824161/diff/1034/chrome/browser/sync/glue/sy...
File chrome/browser/sync/glue/sync_backend_host.h (right):

http://codereview.chromium.org/10824161/diff/1034/chrome/browser/sync/glue/sy...
chrome/browser/sync/glue/sync_backend_host.h:538: bool propagate_invalidations_;
Consider calling this in_shutdown_ or something to reflect when it is actually
set. Until I saw the propagate_invalidations_ = false in Stop...() I wasn't sure
if this was a feature enable/disable switch or something else.   Even
safe_to_propagate_invalidations_ might be clearer.

http://codereview.chromium.org/10824161/diff/1034/chrome/browser/sync/glue/sy...
File chrome/browser/sync/glue/sync_backend_host_unittest.cc (right):

http://codereview.chromium.org/10824161/diff/1034/chrome/browser/sync/glue/sy...
chrome/browser/sync/glue/sync_backend_host_unittest.cc:94: :
manager_ptr_(manager_ptr) {}
Hm. manager_ptr_ptr?

http://codereview.chromium.org/10824161/diff/1034/chrome/browser/sync/glue/sy...
chrome/browser/sync/glue/sync_backend_host_unittest.cc:632: // shouldn't
propagate to the frontend.
Ever?  On restart? Do we have tests that cover that?

http://codereview.chromium.org/10824161/diff/1034/chrome/browser/sync/glue/sy...
chrome/browser/sync/glue/sync_backend_host_unittest.cc:649:
fake_manager_->WaitForSyncThread();
Is this to flush all the tasks?  Does it need to be separate from stopping it?

http://codereview.chromium.org/10824161/diff/1034/sync/internal_api/sync_mana...
File sync/internal_api/sync_manager_impl.cc (right):

http://codereview.chromium.org/10824161/diff/1034/sync/internal_api/sync_mana...
sync/internal_api/sync_manager_impl.cc:746: void
SyncManagerImpl::SetInvalidationHandler(
DCHECK(initialized) too?

[akalin, 2012-08-07 07:25:19]

Addressed all comments.  PTAL.

http://codereview.chromium.org/10824161/diff/1034/chrome/browser/sync/glue/br...
File chrome/browser/sync/glue/bridged_sync_notifier_unittest.cc (right):

http://codereview.chromium.org/10824161/diff/1034/chrome/browser/sync/glue/br...
chrome/browser/sync/glue/bridged_sync_notifier_unittest.cc:61: // All tests just
verify that each call is passed through to the
On 2012/08/03 23:30:46, msw wrote:
> nit: why line break earlier? Embrace the precious 80 chars!

Done.

http://codereview.chromium.org/10824161/diff/1034/chrome/browser/sync/glue/ch...
File chrome/browser/sync/glue/chrome_sync_notification_bridge.cc (right):

http://codereview.chromium.org/10824161/diff/1034/chrome/browser/sync/glue/ch...
chrome/browser/sync/glue/chrome_sync_notification_bridge.cc:31: void
DestroyOnSyncThread();
On 2012/08/06 05:55:32, timsteele wrote:
> Is 'Destroy' the right name?  This won't necessarily result in calling the
> destructor. 

Done, use Cleanup

http://codereview.chromium.org/10824161/diff/1034/chrome/browser/sync/glue/ch...
File chrome/browser/sync/glue/chrome_sync_notification_bridge.h (right):

http://codereview.chromium.org/10824161/diff/1034/chrome/browser/sync/glue/ch...
chrome/browser/sync/glue/chrome_sync_notification_bridge.h:39: // Must be called
on the UI thread while the sync task runner is
On 2012/08/03 23:30:46, msw wrote:
> nit: line breaking; does your IDE show the 80 char guide?

Done.

http://codereview.chromium.org/10824161/diff/1034/chrome/browser/sync/glue/ch...
File chrome/browser/sync/glue/chrome_sync_notification_bridge_unittest.cc
(right):

http://codereview.chromium.org/10824161/diff/1034/chrome/browser/sync/glue/ch...
chrome/browser/sync/glue/chrome_sync_notification_bridge_unittest.cc:7: #include
<cstddef>
On 2012/08/03 23:30:46, msw wrote:
> nit/question: is it better practice to include this or base/basictypes.h?

i don't think basictypes.h makes any particular guarantee about defining NULL,
so I think it's better to include cstddef directly

http://codereview.chromium.org/10824161/diff/1034/chrome/browser/sync/glue/sy...
File chrome/browser/sync/glue/sync_backend_host.cc (right):

http://codereview.chromium.org/10824161/diff/1034/chrome/browser/sync/glue/sy...
chrome/browser/sync/glue/sync_backend_host.cc:593: if
(chrome_sync_notification_bridge_.get()) {
On 2012/08/03 23:30:46, msw wrote:
> nit: remove unnecessary {}

Done.

http://codereview.chromium.org/10824161/diff/1034/chrome/browser/sync/glue/sy...
File chrome/browser/sync/glue/sync_backend_host.h (right):

http://codereview.chromium.org/10824161/diff/1034/chrome/browser/sync/glue/sy...
chrome/browser/sync/glue/sync_backend_host.h:183: // underlying notifier.  This
lasts until StopSyncingForShutdown()
On 2012/08/03 23:30:46, msw wrote:
> nit: line breaks :)

Done.

http://codereview.chromium.org/10824161/diff/1034/chrome/browser/sync/glue/sy...
chrome/browser/sync/glue/sync_backend_host.h:215: // Called on |frontend_loop_|
to kick off shutdown procedure. After
On 2012/08/03 23:30:46, msw wrote:
> nit: line breaks :)

Done.

http://codereview.chromium.org/10824161/diff/1034/chrome/browser/sync/glue/sy...
chrome/browser/sync/glue/sync_backend_host.h:536: // Whether or not we propagate
notifications to invalidation
On 2012/08/03 23:30:46, msw wrote:
> nit: line breaks :)

moot

http://codereview.chromium.org/10824161/diff/1034/chrome/browser/sync/glue/sy...
chrome/browser/sync/glue/sync_backend_host.h:538: bool propagate_invalidations_;
On 2012/08/06 05:55:32, timsteele wrote:
> Consider calling this in_shutdown_ or something to reflect when it is actually
> set. Until I saw the propagate_invalidations_ = false in Stop...() I wasn't
sure
> if this was a feature enable/disable switch or something else.   Even
> safe_to_propagate_invalidations_ might be clearer.

As we discussed, used frontend_ and null it out early

http://codereview.chromium.org/10824161/diff/1034/chrome/browser/sync/glue/sy...
File chrome/browser/sync/glue/sync_backend_host_unittest.cc (right):

http://codereview.chromium.org/10824161/diff/1034/chrome/browser/sync/glue/sy...
chrome/browser/sync/glue/sync_backend_host_unittest.cc:94: :
manager_ptr_(manager_ptr) {}
On 2012/08/06 05:55:32, timsteele wrote:
> Hm. manager_ptr_ptr?

moot, just exposed an accessor

http://codereview.chromium.org/10824161/diff/1034/chrome/browser/sync/glue/sy...
chrome/browser/sync/glue/sync_backend_host_unittest.cc:100: std::string name)
OVERRIDE {
On 2012/08/03 23:30:46, msw wrote:
> nit: fits on line above

not quite -- since it's exactly 80 chars, it overflows on emacs :/

http://codereview.chromium.org/10824161/diff/1034/chrome/browser/sync/glue/sy...
chrome/browser/sync/glue/sync_backend_host_unittest.cc:123: FakeSyncManager**
manager_ptr_;
On 2012/08/03 23:30:46, msw wrote:
> Why is this a double pointer? Also, I think our style guide discourages
"_ptr".

moot

http://codereview.chromium.org/10824161/diff/1034/chrome/browser/sync/glue/sy...
chrome/browser/sync/glue/sync_backend_host_unittest.cc:630: // Call
StopSyncingForShutdown() on the backend and fire some
On 2012/08/03 23:30:46, msw wrote:
> nit: line breaks :)

Done.

http://codereview.chromium.org/10824161/diff/1034/chrome/browser/sync/glue/sy...
chrome/browser/sync/glue/sync_backend_host_unittest.cc:632: // shouldn't
propagate to the frontend.
On 2012/08/06 05:55:32, timsteele wrote:
> Ever?  On restart? Do we have tests that cover that?

yeah.  added teardown and startup.

http://codereview.chromium.org/10824161/diff/1034/chrome/browser/sync/glue/sy...
chrome/browser/sync/glue/sync_backend_host_unittest.cc:649:
fake_manager_->WaitForSyncThread();
On 2012/08/06 05:55:32, timsteele wrote:
> Is this to flush all the tasks?  Does it need to be separate from stopping it?

yes.  this is so that the test would fail deterministically if it does fail.

http://codereview.chromium.org/10824161/diff/1034/chrome/browser/sync/profile...
File chrome/browser/sync/profile_sync_service.h (right):

http://codereview.chromium.org/10824161/diff/1034/chrome/browser/sync/profile...
chrome/browser/sync/profile_sync_service.h:554: // Sets the invalidation handler
for the given name.  Pass in NULL
On 2012/08/03 23:30:46, msw wrote:
> nit: line breaks :)

Done.

http://codereview.chromium.org/10824161/diff/1034/chrome/browser/sync/profile...
chrome/browser/sync/profile_sync_service.h:564: // Updates the set of ObjectIds
associated with a given invalidation
On 2012/08/03 23:30:46, msw wrote:
> nit: line breaks :)

Done.

http://codereview.chromium.org/10824161/diff/1034/chrome/browser/sync/profile...
chrome/browser/sync/profile_sync_service.h:568: // The handler name ->
registered ids map is persisted across
On 2012/08/03 23:30:46, msw wrote:
> nit: line breaks :)

Done.

http://codereview.chromium.org/10824161/diff/1034/sync/internal_api/public/sy...
File sync/internal_api/public/sync_manager.h (right):

http://codereview.chromium.org/10824161/diff/1034/sync/internal_api/public/sy...
sync/internal_api/public/sync_manager.h:410: // SyncNotifier::SetHandler()).
On 2012/08/03 23:30:46, msw wrote:
> nit: line breaks :)

Done.

http://codereview.chromium.org/10824161/diff/1034/sync/internal_api/public/sy...
sync/internal_api/public/sync_manager.h:412: const std::string& handler_name,
SyncNotifierObserver* handler) = 0;
On 2012/08/03 23:30:46, msw wrote:
> one param per line

Done.

http://codereview.chromium.org/10824161/diff/1034/sync/internal_api/public/sy...
sync/internal_api/public/sync_manager.h:417: const std::string& handler_name,
const ObjectIdSet& ids) = 0;
On 2012/08/03 23:30:46, msw wrote:
> one param per line

Done.

http://codereview.chromium.org/10824161/diff/1034/sync/internal_api/public/te...
File sync/internal_api/public/test/fake_sync_manager.h (right):

http://codereview.chromium.org/10824161/diff/1034/sync/internal_api/public/te...
sync/internal_api/public/test/fake_sync_manager.h:23: //
|initial_sync_ended_types|: The set of types that have
On 2012/08/03 23:30:46, msw wrote:
> nit: line breaks :)

Done.

http://codereview.chromium.org/10824161/diff/1034/sync/internal_api/public/te...
sync/internal_api/public/test/fake_sync_manager.h:67: // Block until the sync
thread has finished processing any pending
On 2012/08/03 23:30:46, msw wrote:
> nit: line breaks :)

Done.

http://codereview.chromium.org/10824161/diff/1034/sync/internal_api/sync_mana...
File sync/internal_api/sync_manager_impl.cc (right):

http://codereview.chromium.org/10824161/diff/1034/sync/internal_api/sync_mana...
sync/internal_api/sync_manager_impl.cc:401: if (sync_notifier_.get()) {
On 2012/08/03 23:30:46, msw wrote:
> nit: remove unnecessary {}

Done.

http://codereview.chromium.org/10824161/diff/1034/sync/internal_api/sync_mana...
sync/internal_api/sync_manager_impl.cc:746: void
SyncManagerImpl::SetInvalidationHandler(
On 2012/08/06 05:55:32, timsteele wrote:
> DCHECK(initialized) too?

Can't.  Added comment why not. :/

http://codereview.chromium.org/10824161/diff/1034/sync/internal_api/sync_mana...
sync/internal_api/sync_manager_impl.cc:747: const std::string& handler_name,
SyncNotifierObserver* handler) {
On 2012/08/03 23:30:46, msw wrote:
> one param per line

Done.

http://codereview.chromium.org/10824161/diff/1034/sync/internal_api/sync_mana...
sync/internal_api/sync_manager_impl.cc:753: const std::string& handler_name,
const ObjectIdSet& ids) {
On 2012/08/03 23:30:46, msw wrote:
> one param per line

Done.

http://codereview.chromium.org/10824161/diff/1034/sync/internal_api/sync_mana...
File sync/internal_api/sync_manager_impl.h (right):

http://codereview.chromium.org/10824161/diff/1034/sync/internal_api/sync_mana...
sync/internal_api/sync_manager_impl.h:93: const std::string& handler_name, const
ObjectIdSet& ids) OVERRIDE;
On 2012/08/03 23:30:46, msw wrote:
> one param per line.

Done.

http://codereview.chromium.org/10824161/diff/1034/sync/internal_api/test/fake...
File sync/internal_api/test/fake_sync_manager.cc (right):

http://codereview.chromium.org/10824161/diff/1034/sync/internal_api/test/fake...
sync/internal_api/test/fake_sync_manager.cc:88: void
FakeSyncManager::WaitForSyncThread() {
On 2012/08/03 23:30:46, msw wrote:
> What is this doing? Add a comment as to why this is necessary.

Done.

http://codereview.chromium.org/10824161/diff/1034/sync/internal_api/test/fake...
sync/internal_api/test/fake_sync_manager.cc:165: const std::string&
handler_name, SyncNotifierObserver* handler) {
On 2012/08/03 23:30:46, msw wrote:
> one param per line

Done.

http://codereview.chromium.org/10824161/diff/1034/sync/internal_api/test/fake...
sync/internal_api/test/fake_sync_manager.cc:170: const std::string&
handler_name, const ObjectIdSet& ids) {
On 2012/08/03 23:30:46, msw wrote:
> one param per line

Done.

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

http://codereview.chromium.org/10824161/diff/1034/sync/notifier/invalidation_...
sync/notifier/invalidation_notifier_unittest.cc:38: // Constructs an
InvalidationNotifier and places it in
On 2012/08/03 23:30:46, msw wrote:
> nit: line breaks :)

Done.

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

http://codereview.chromium.org/10824161/diff/1034/sync/notifier/non_blocking_...
sync/notifier/non_blocking_invalidation_notifier.cc:7: #include <cstddef>
On 2012/08/03 23:30:46, msw wrote:
> ditto nit/question for basictypes.h

ditto

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

http://codereview.chromium.org/10824161/diff/1034/sync/notifier/p2p_notifier....
sync/notifier/p2p_notifier.cc:168: // TODO(akalin): Handle arbitrary object IDs
On 2012/08/03 23:30:46, msw wrote:
> nit: line breaks :)

Done.

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

http://codereview.chromium.org/10824161/diff/1034/sync/notifier/sync_notifier...
sync/notifier/sync_notifier.h:25: // Sets the handler for the given name.  Pass
in NULL for |handler|
On 2012/08/03 23:30:46, msw wrote:
> nit: line breaks :)

Done.

http://codereview.chromium.org/10824161/diff/1034/sync/notifier/sync_notifier...
File sync/notifier/sync_notifier_helper.cc (right):

http://codereview.chromium.org/10824161/diff/1034/sync/notifier/sync_notifier...
sync/notifier/sync_notifier_helper.cc:40: const std::string& handler_name, const
ObjectIdSet& ids) {
On 2012/08/03 23:30:46, msw wrote:
> one param per line

Done.

http://codereview.chromium.org/10824161/diff/1034/sync/notifier/sync_notifier...
sync/notifier/sync_notifier_helper.cc:54: // Now add the entries for
|handler_name|. We keep track of the last
On 2012/08/03 23:30:46, msw wrote:
> nit: line breaks :)

Done.

http://codereview.chromium.org/10824161/diff/1034/sync/notifier/sync_notifier...
sync/notifier/sync_notifier_helper.cc:73: if (handler) {
On 2012/08/03 23:30:46, msw wrote:
> nit: remove unnecessary {}

Done.

http://codereview.chromium.org/10824161/diff/1034/sync/notifier/sync_notifier...
sync/notifier/sync_notifier_helper.cc:100: // to an handler, just drop it -- the
handler was unregistered
On 2012/08/03 23:30:46, msw wrote:
> nit: "a handler" and line breaks :)

Done.

http://codereview.chromium.org/10824161/diff/1034/sync/notifier/sync_notifier...
sync/notifier/sync_notifier_helper.cc:102: if (!handler) {
On 2012/08/03 23:30:46, msw wrote:
> nit: just perform the insert if (handler), or remove unnecessary {}

Done.

http://codereview.chromium.org/10824161/diff/1034/sync/notifier/sync_notifier...
sync/notifier/sync_notifier_helper.cc:122:
FOR_EACH_OBSERVER(SyncNotifierObserver, handlers_,
On 2012/08/03 23:30:46, msw wrote:
> nit: line breaks :)

Done.

http://codereview.chromium.org/10824161/diff/1034/sync/notifier/sync_notifier...
File sync/notifier/sync_notifier_helper.h (right):

http://codereview.chromium.org/10824161/diff/1034/sync/notifier/sync_notifier...
sync/notifier/sync_notifier_helper.h:24: // A helper class for classes that want
to implement the SyncNotifier
On 2012/08/03 23:30:46, msw wrote:
> nit: line breaks, this looks worse to me.

Done.

http://codereview.chromium.org/10824161/diff/1034/sync/notifier/sync_notifier...
sync/notifier/sync_notifier_helper.h:34: // Sets the handler for the given name.
 Pass in NULL for |handler|
On 2012/08/03 23:30:46, msw wrote:
> nit: line breaks :)

Done.

http://codereview.chromium.org/10824161/diff/1034/sync/notifier/sync_notifier...
sync/notifier/sync_notifier_helper.h:49: // Helper that sorts incoming
invalidations into a bucket for each
On 2012/08/03 23:30:46, msw wrote:
> nit: line breaks :)

Done.

http://codereview.chromium.org/10824161/diff/1034/sync/notifier/sync_notifier...
sync/notifier/sync_notifier_helper.h:55: // Calls the given handler method for
each handler that has
On 2012/08/03 23:30:46, msw wrote:
> nit: line breaks, why did you change this?

Done.

http://codereview.chromium.org/10824161/diff/1034/sync/tools/sync_listen_noti...
File sync/tools/sync_listen_notifications.cc (right):

http://codereview.chromium.org/10824161/diff/1034/sync/tools/sync_listen_noti...
sync/tools/sync_listen_notifications.cc:5: #include <cstddef>
On 2012/08/03 23:30:46, msw wrote:
> ditto same basictypes nit/q

ditto.

[tim (not reviewing), 2012-08-07 16:39:38]

http://codereview.chromium.org/10824161/diff/1038/chrome/browser/sync/glue/sy...
File chrome/browser/sync/glue/sync_backend_host.cc (right):

http://codereview.chromium.org/10824161/diff/1038/chrome/browser/sync/glue/sy...
chrome/browser/sync/glue/sync_backend_host.cc:580:
DCHECK_EQ(initialization_state_, NOT_ATTEMPTED);
hm, so I've always followed the pattern from ASSERT/EXPECT in tests, where the
first parameter to the macro is "expected" and the second is "actual".

http://codereview.chromium.org/10824161/diff/1038/chrome/browser/sync/glue/sy...
File chrome/browser/sync/glue/sync_backend_host_unittest.cc (right):

http://codereview.chromium.org/10824161/diff/1038/chrome/browser/sync/glue/sy...
chrome/browser/sync/glue/sync_backend_host_unittest.cc:661: SetUp();
Hmm.. wait, so how does the client know that it's out of date?

[akalin, 2012-08-07 17:19:23]

http://codereview.chromium.org/10824161/diff/1038/chrome/browser/sync/glue/sy...
File chrome/browser/sync/glue/sync_backend_host.cc (right):

http://codereview.chromium.org/10824161/diff/1038/chrome/browser/sync/glue/sy...
chrome/browser/sync/glue/sync_backend_host.cc:580:
DCHECK_EQ(initialization_state_, NOT_ATTEMPTED);
On 2012/08/07 16:39:38, timsteele wrote:
> hm, so I've always followed the pattern from ASSERT/EXPECT in tests, where the
> first parameter to the macro is "expected" and the second is "actual".  

I think that's only because EXPECT makes a distinction between the two arguments
(expected is first).  Since DCHECK_EQ doesn't, there's no reason to not put the
arguments in natural order.  I think that's the accepted style.

http://codereview.chromium.org/10824161/diff/1038/chrome/browser/sync/glue/sy...
File chrome/browser/sync/glue/sync_backend_host_unittest.cc (right):

http://codereview.chromium.org/10824161/diff/1038/chrome/browser/sync/glue/sy...
chrome/browser/sync/glue/sync_backend_host_unittest.cc:661: SetUp();
On 2012/08/07 16:39:38, timsteele wrote:
> Hmm.. wait, so how does the client know that it's out of date?

Do you mean in the interval that sync is shut down?  If sync is shutting down,
that means the browser is either shutting down or the user is stopping sync.  In
the former case, we may drop invalidations; this has always been true, since we
ack immediately.  Once we add the ack API, then it'll invalidate again when sync
is turned on (since the client won't ack).  In the latter case, the user is
stopping sync, which nukes invalidation state, so when he/she re-enables it,
it'll act like a new client (and send an initial invalidation).  I think even
without the initial invalidation, the ack api will take care of this case, too.

[tim (not reviewing), 2012-08-07 17:22:12]

LGTM

http://codereview.chromium.org/10824161/diff/1038/chrome/browser/sync/glue/sy...
File chrome/browser/sync/glue/sync_backend_host_unittest.cc (right):

http://codereview.chromium.org/10824161/diff/1038/chrome/browser/sync/glue/sy...
chrome/browser/sync/glue/sync_backend_host_unittest.cc:661: SetUp();
On 2012/08/07 17:19:23, akalin wrote:
> On 2012/08/07 16:39:38, timsteele wrote:
> > Hmm.. wait, so how does the client know that it's out of date?
> 
> Do you mean in the interval that sync is shut down?  If sync is shutting down,
> that means the browser is either shutting down or the user is stopping sync. 
In
> the former case, we may drop invalidations; this has always been true, since
we
> ack immediately.  Once we add the ack API, then it'll invalidate again when
sync
> is turned on (since the client won't ack).  In the latter case, the user is
> stopping sync, which nukes invalidation state, so when he/she re-enables it,
> it'll act like a new client (and send an initial invalidation).  I think even
> without the initial invalidation, the ack api will take care of this case,
too.

Oh right.  This would be useful to have in a comment somewhere if it's not
already there (perhaps near the PSS invalidation related methods).

[akalin, 2012-08-07 18:11:29]

msw?

http://codereview.chromium.org/10824161/diff/1038/chrome/browser/sync/glue/sy...
File chrome/browser/sync/glue/sync_backend_host_unittest.cc (right):

http://codereview.chromium.org/10824161/diff/1038/chrome/browser/sync/glue/sy...
chrome/browser/sync/glue/sync_backend_host_unittest.cc:661: SetUp();
On 2012/08/07 17:22:12, timsteele wrote:
> On 2012/08/07 17:19:23, akalin wrote:
> > On 2012/08/07 16:39:38, timsteele wrote:
> > > Hmm.. wait, so how does the client know that it's out of date?
> > 
> > Do you mean in the interval that sync is shut down?  If sync is shutting
down,
> > that means the browser is either shutting down or the user is stopping sync.

> In
> > the former case, we may drop invalidations; this has always been true, since
> we
> > ack immediately.  Once we add the ack API, then it'll invalidate again when
> sync
> > is turned on (since the client won't ack).  In the latter case, the user is
> > stopping sync, which nukes invalidation state, so when he/she re-enables it,
> > it'll act like a new client (and send an initial invalidation).  I think
even
> > without the initial invalidation, the ack api will take care of this case,
> too.
> 
> Oh right.  This would be useful to have in a comment somewhere if it's not
> already there (perhaps near the PSS invalidation related methods).

Done.

[msw, 2012-08-07 20:09:30]

You all know this code a lot better than I do, so feel free to ignore my
comments/questions if they're stupid.

http://codereview.chromium.org/10824161/diff/11011/chrome/browser/sync/glue/b...
File chrome/browser/sync/glue/bridged_sync_notifier.h (right):

http://codereview.chromium.org/10824161/diff/11011/chrome/browser/sync/glue/b...
chrome/browser/sync/glue/bridged_sync_notifier.h:32: virtual void
SetHandler(const std::string& handler_name,
When a Handler wants to unregister itself in this paradigm, it'll call
SetHandler("Foo", NULL), but what if another handler has taken over "Foo"? Using
strings seems easily vulnerable to unintended actions like unregistering another
handler when you try to unregister yourself.

What's the reasoning behind this approach and are you concerned about this extra
level of complication/indirection?

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

http://codereview.chromium.org/10824161/diff/11011/chrome/browser/sync/glue/c...
chrome/browser/sync/glue/chrome_sync_notification_bridge.cc:59: helper_(NULL) {
nit: I think this is unnecessary for scoped_ptrs.

http://codereview.chromium.org/10824161/diff/11011/chrome/browser/sync/glue/s...
File chrome/browser/sync/glue/sync_backend_host.h (right):

http://codereview.chromium.org/10824161/diff/11011/chrome/browser/sync/glue/s...
chrome/browser/sync/glue/sync_backend_host.h:217: // Furthermore, no
notifications are sent to any invalidation handler.
nit: s/are/will be/

http://codereview.chromium.org/10824161/diff/11011/chrome/browser/sync/glue/s...
File chrome/browser/sync/glue/sync_backend_host_unittest.cc (right):

http://codereview.chromium.org/10824161/diff/11011/chrome/browser/sync/glue/s...
chrome/browser/sync/glue/sync_backend_host_unittest.cc:195: //
|fake_manager_factory_|'s fake_manager() is set on the sync
Maybe make a similar comment with fake_manager_'s declaration?
It's lifetime management isn't obvious from the code there.

http://codereview.chromium.org/10824161/diff/11011/chrome/browser/sync/glue/s...
chrome/browser/sync/glue/sync_backend_host_unittest.cc:647: // Should not
trigger anything.
Can you comment on how this is being ensured?

Is it the lack of an EXPECT_CALL(mock_frontend_,
OnIncomingNotification(id_payloads,
syncer::REMOTE_NOTIFICATION)).WillOnce(InvokeWithoutArgs(QuitMessageLoop)); ???

Can you call that with .Times(0) to make an explicit assertion?

http://codereview.chromium.org/10824161/diff/11011/chrome/browser/sync/glue/s...
chrome/browser/sync/glue/sync_backend_host_unittest.cc:655:
fake_manager_->WaitForSyncThread();
Can you use MessageLoop::RunAllPending or similar instead?

http://codereview.chromium.org/10824161/diff/11011/chrome/browser/sync/profil...
File chrome/browser/sync/profile_sync_service.h (right):

http://codereview.chromium.org/10824161/diff/11011/chrome/browser/sync/profil...
chrome/browser/sync/profile_sync_service.h:565: void
SetInvalidationHandler(const std::string& handler_name,
I'm wondering if using strings is a necessity to avoiding unregistering object
ids on shutdown. Wouldn't it make more sense to just add something like
InvalidationHandlerDestroying(syncer::SyncNotifierObserver* handler) called from
SyncNotifierObserver::~SyncNotifierObserver (or some new
SyncNotifierObserver::Disable/ShutDown function) that would disable the handler
without unregistering its ids?

That would seem to avoid your dangling object IDs concern, avoid the
string->handler map, and simplify client-side logic on destruction.

http://codereview.chromium.org/10824161/diff/11011/sync/internal_api/sync_man...
File sync/internal_api/sync_manager_impl_unittest.cc (right):

http://codereview.chromium.org/10824161/diff/11011/sync/internal_api/sync_man...
sync/internal_api/sync_manager_impl_unittest.cc:787:
sync_manager_.RemoveObserver(&observer_);
If you want to ensure that UpdateRegisteredIds(_, ObjectIdSet()) isn't called,
please restore the EXPECT_CALL with .Times(0).

http://codereview.chromium.org/10824161/diff/11011/sync/internal_api/sync_man...
sync/internal_api/sync_manager_impl_unittest.cc:969: const char kHandlerName[] =
"handler";
nit: move this to an anon namespace to share with
UpdateRegisteredInvalidationIds (and future similar tests).

http://codereview.chromium.org/10824161/diff/11011/sync/internal_api/test/fak...
File sync/internal_api/test/fake_sync_manager.cc (right):

http://codereview.chromium.org/10824161/diff/11011/sync/internal_api/test/fak...
sync/internal_api/test/fake_sync_manager.cc:87: void
FakeSyncManager::WaitForSyncThread() {
This pattern seems odd, is there precedent for it elsewhere?
Can you use MessageLoop::RunAllPending or similar instead?

http://codereview.chromium.org/10824161/diff/11011/sync/notifier/invalidation...
File sync/notifier/invalidation_notifier.cc (right):

http://codereview.chromium.org/10824161/diff/11011/sync/notifier/invalidation...
sync/notifier/invalidation_notifier.cc:48: const ObjectIdSet& all_registered_ids
= helper_.GetAllRegisteredIds();
nit: comment explaining when |ids| won't match 
|all_registered_ids|? (It's not obvious to me)

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

http://codereview.chromium.org/10824161/diff/11011/sync/notifier/non_blocking...
sync/notifier/non_blocking_invalidation_notifier.cc:202: const std::string&
handler_name, const ObjectIdSet& ids) {
one param per line

http://codereview.chromium.org/10824161/diff/11011/sync/notifier/non_blocking...
sync/notifier/non_blocking_invalidation_notifier.cc:205: const ObjectIdSet&
all_registered_ids = helper_.GetAllRegisteredIds();
ditto nit: comment on |ids| != |all_registered_ids|?

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

http://codereview.chromium.org/10824161/diff/11011/sync/notifier/p2p_notifier...
sync/notifier/p2p_notifier.cc:171: const ModelTypeSet enabled_types =
ditto nit: comment why |ids| != |enabled_types|?

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

http://codereview.chromium.org/10824161/diff/11011/sync/notifier/sync_notifie...
sync/notifier/sync_notifier_helper_unittest.cc:212:
helper.UpdateRegisteredIds(kHandlerName1, ObjectIdSet());
Should this be SetHandler(kHandlerName1, NULL), or should the comment explain
that this is unregistering the handler and all its object ids?

[akalin, 2012-08-07 21:14:04]

On 2012/08/07 20:09:30, msw wrote:
>
http://codereview.chromium.org/10824161/diff/11011/chrome/browser/sync/glue/b...
> chrome/browser/sync/glue/bridged_sync_notifier.h:32: virtual void
> SetHandler(const std::string& handler_name,
> When a Handler wants to unregister itself in this paradigm, it'll call
> SetHandler("Foo", NULL), but what if another handler has taken over "Foo"?
Using
> strings seems easily vulnerable to unintended actions like unregistering
another
> handler when you try to unregister yourself.
> 
> What's the reasoning behind this approach and are you concerned about this
extra
> level of complication/indirection?

This is a good point!

>
http://codereview.chromium.org/10824161/diff/11011/chrome/browser/sync/profil...
> chrome/browser/sync/profile_sync_service.h:565: void
> SetInvalidationHandler(const std::string& handler_name,
> I'm wondering if using strings is a necessity to avoiding unregistering object
> ids on shutdown. Wouldn't it make more sense to just add something like
> InvalidationHandlerDestroying(syncer::SyncNotifierObserver* handler) called
from
> SyncNotifierObserver::~SyncNotifierObserver (or some new
> SyncNotifierObserver::Disable/ShutDown function) that would disable the
handler
> without unregistering its ids?
>
> You all know this code a lot better than I do, so feel free to ignore my
> comments/questions if they're stupid.

Not at all; given that you guys are going to be the users of the API (and the
first ones at that!) I think your comments are valuable.

It seems like this API still isn't quite what we want.  I think we need to
figure out the API first and then I can code it up.

I'll send a separate e-mail to you guys.

[akalin, 2012-08-07 22:27:56]

As discussed, I'll implement the following API:

void AddHandler(SyncNotifierObserver* handler)
void UpdateRegisteredIds(SyncNotifierObserver* handler, set<ObjectId>)
void RemoveHandler(SyncNotifierObserver* handler)

[akalin, 2012-08-07 23:38:05]

Addressed non-API comments, some with replies and additional questions.

msw@, PTAL at my replies.

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

http://codereview.chromium.org/10824161/diff/11011/chrome/browser/sync/glue/c...
chrome/browser/sync/glue/chrome_sync_notification_bridge.cc:59: helper_(NULL) {
On 2012/08/07 20:09:30, msw wrote:
> nit: I think this is unnecessary for scoped_ptrs.

Done.

http://codereview.chromium.org/10824161/diff/11011/chrome/browser/sync/glue/s...
File chrome/browser/sync/glue/sync_backend_host.h (right):

http://codereview.chromium.org/10824161/diff/11011/chrome/browser/sync/glue/s...
chrome/browser/sync/glue/sync_backend_host.h:217: // Furthermore, no
notifications are sent to any invalidation handler.
On 2012/08/07 20:09:30, msw wrote:
> nit: s/are/will be/

Done.

http://codereview.chromium.org/10824161/diff/11011/chrome/browser/sync/glue/s...
File chrome/browser/sync/glue/sync_backend_host_unittest.cc (right):

http://codereview.chromium.org/10824161/diff/11011/chrome/browser/sync/glue/s...
chrome/browser/sync/glue/sync_backend_host_unittest.cc:195: //
|fake_manager_factory_|'s fake_manager() is set on the sync
On 2012/08/07 20:09:30, msw wrote:
> Maybe make a similar comment with fake_manager_'s declaration?
> It's lifetime management isn't obvious from the code there.

Done.

http://codereview.chromium.org/10824161/diff/11011/chrome/browser/sync/glue/s...
chrome/browser/sync/glue/sync_backend_host_unittest.cc:647: // Should not
trigger anything.
On 2012/08/07 20:09:30, msw wrote:
> Can you comment on how this is being ensured?
> 
> Is it the lack of an EXPECT_CALL(mock_frontend_,
> OnIncomingNotification(id_payloads,
> syncer::REMOTE_NOTIFICATION)).WillOnce(InvokeWithoutArgs(QuitMessageLoop));
???
> 
> Can you call that with .Times(0) to make an explicit assertion?

mock_frontend_ is declared as a StrictMock.  Doesn't that suffice?  I could add
the .Times(0), but then logically I'd have to add that for every mocked
function. :/

http://codereview.chromium.org/10824161/diff/11011/chrome/browser/sync/glue/s...
chrome/browser/sync/glue/sync_backend_host_unittest.cc:655:
fake_manager_->WaitForSyncThread();
On 2012/08/07 20:09:30, msw wrote:
> Can you use MessageLoop::RunAllPending or similar instead?

No, because nothing is actually posted to the UI thread (since no messages are
triggered).

http://codereview.chromium.org/10824161/diff/11011/sync/internal_api/sync_man...
File sync/internal_api/sync_manager_impl_unittest.cc (right):

http://codereview.chromium.org/10824161/diff/11011/sync/internal_api/sync_man...
sync/internal_api/sync_manager_impl_unittest.cc:969: const char kHandlerName[] =
"handler";
On 2012/08/07 20:09:30, msw wrote:
> nit: move this to an anon namespace to share with
> UpdateRegisteredInvalidationIds (and future similar tests).

this'll be rendered moot

http://codereview.chromium.org/10824161/diff/11011/sync/internal_api/sync_man...
sync/internal_api/sync_manager_impl_unittest.cc:969: const char kHandlerName[] =
"handler";
On 2012/08/07 20:09:30, msw wrote:
> nit: move this to an anon namespace to share with
> UpdateRegisteredInvalidationIds (and future similar tests).

moot

http://codereview.chromium.org/10824161/diff/11011/sync/internal_api/test/fak...
File sync/internal_api/test/fake_sync_manager.cc (right):

http://codereview.chromium.org/10824161/diff/11011/sync/internal_api/test/fak...
sync/internal_api/test/fake_sync_manager.cc:87: void
FakeSyncManager::WaitForSyncThread() {
On 2012/08/07 20:09:30, msw wrote:
> This pattern seems odd, is there precedent for it elsewhere?
> Can you use MessageLoop::RunAllPending or similar instead?

I don't think there's a precedent.  An alternate way to do it would be to post a
task to the sync thread that posts a quit task back to the main thread, and then
call Run().  But I think this is simpler.

RunAllPending won't work since you can't guarantee that the reply task will be
posted while you're running current pending tasks.  So you have to use Run() and
have the reply task be a quit.  But MessageLoop::QuitClosure() is deprecated in
favor of RunLoop::QuitClosure() (to avoid run nesting).

http://codereview.chromium.org/10824161/diff/11011/sync/notifier/invalidation...
File sync/notifier/invalidation_notifier.cc (right):

http://codereview.chromium.org/10824161/diff/11011/sync/notifier/invalidation...
sync/notifier/invalidation_notifier.cc:48: const ObjectIdSet& all_registered_ids
= helper_.GetAllRegisteredIds();
On 2012/08/07 20:09:30, msw wrote:
> nit: comment explaining when |ids| won't match 
> |all_registered_ids|? (It's not obvious to me)

ids is only for |handler_name|/|handler|, but all_registered_ids holds all IDs
regardless of handler.  Do you think this still needs a comment?  Is the comment
in SyncNotifierHelper::GetAllRegisteredIds() insufficient?

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

http://codereview.chromium.org/10824161/diff/11011/sync/notifier/non_blocking...
sync/notifier/non_blocking_invalidation_notifier.cc:202: const std::string&
handler_name, const ObjectIdSet& ids) {
On 2012/08/07 20:09:30, msw wrote:
> one param per line

Done.

http://codereview.chromium.org/10824161/diff/11011/sync/notifier/non_blocking...
sync/notifier/non_blocking_invalidation_notifier.cc:205: const ObjectIdSet&
all_registered_ids = helper_.GetAllRegisteredIds();
On 2012/08/07 20:09:30, msw wrote:
> ditto nit: comment on |ids| != |all_registered_ids|?

ditto.

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

http://codereview.chromium.org/10824161/diff/11011/sync/notifier/p2p_notifier...
sync/notifier/p2p_notifier.cc:171: const ModelTypeSet enabled_types =
On 2012/08/07 20:09:30, msw wrote:
> ditto nit: comment why |ids| != |enabled_types|?

ditto

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

http://codereview.chromium.org/10824161/diff/11011/sync/notifier/sync_notifie...
sync/notifier/sync_notifier_helper_unittest.cc:212:
helper.UpdateRegisteredIds(kHandlerName1, ObjectIdSet());
On 2012/08/07 20:09:30, msw wrote:
> Should this be SetHandler(kHandlerName1, NULL), or should the comment explain
> that this is unregistering the handler and all its object ids?

It should be the latter.  But I think I'll have to rewrite this test anyway for
the new API.

[msw, 2012-08-08 00:55:23]

Looking forward to the revised API :)

http://codereview.chromium.org/10824161/diff/11011/chrome/browser/sync/glue/s...
File chrome/browser/sync/glue/sync_backend_host_unittest.cc (right):

http://codereview.chromium.org/10824161/diff/11011/chrome/browser/sync/glue/s...
chrome/browser/sync/glue/sync_backend_host_unittest.cc:647: // Should not
trigger anything.
On 2012/08/07 23:38:05, akalin wrote:
> On 2012/08/07 20:09:30, msw wrote:
> > Can you comment on how this is being ensured?
> > 
> > Is it the lack of an EXPECT_CALL(mock_frontend_,
> > OnIncomingNotification(id_payloads,
> > syncer::REMOTE_NOTIFICATION)).WillOnce(InvokeWithoutArgs(QuitMessageLoop));
> ???
> > 
> > Can you call that with .Times(0) to make an explicit assertion?
> 
> mock_frontend_ is declared as a StrictMock.  Doesn't that suffice?  I could
add
> the .Times(0), but then logically I'd have to add that for every mocked
> function. :/

Ahh, I was unfamiliar with StrictMock and that behavior.
This is fine, if anything perhaps note that in the comment.

http://codereview.chromium.org/10824161/diff/11011/chrome/browser/sync/glue/s...
chrome/browser/sync/glue/sync_backend_host_unittest.cc:655:
fake_manager_->WaitForSyncThread();
On 2012/08/07 23:38:05, akalin wrote:
> On 2012/08/07 20:09:30, msw wrote:
> > Can you use MessageLoop::RunAllPending or similar instead?
> 
> No, because nothing is actually posted to the UI thread (since no messages are
> triggered).

I meant to RunAllPending on the sync thread, but as you noted, that's
insufficient here. I'll presume the other reviewers understand and vouch for
this :)

http://codereview.chromium.org/10824161/diff/11011/sync/internal_api/test/fak...
File sync/internal_api/test/fake_sync_manager.cc (right):

http://codereview.chromium.org/10824161/diff/11011/sync/internal_api/test/fak...
sync/internal_api/test/fake_sync_manager.cc:87: void
FakeSyncManager::WaitForSyncThread() {
On 2012/08/07 23:38:05, akalin wrote:
> On 2012/08/07 20:09:30, msw wrote:
> > This pattern seems odd, is there precedent for it elsewhere?
> > Can you use MessageLoop::RunAllPending or similar instead?
> 
> I don't think there's a precedent.  An alternate way to do it would be to post
a
> task to the sync thread that posts a quit task back to the main thread, and
then
> call Run().  But I think this is simpler.
> 
> RunAllPending won't work since you can't guarantee that the reply task will be
> posted while you're running current pending tasks.  So you have to use Run()
and
> have the reply task be a quit.  But MessageLoop::QuitClosure() is deprecated
in
> favor of RunLoop::QuitClosure() (to avoid run nesting).

Sorry, I don't fully understand what you're doing, but that's okay, because I
assume the other reviewers do :)

http://codereview.chromium.org/10824161/diff/11011/sync/notifier/invalidation...
File sync/notifier/invalidation_notifier.cc (right):

http://codereview.chromium.org/10824161/diff/11011/sync/notifier/invalidation...
sync/notifier/invalidation_notifier.cc:48: const ObjectIdSet& all_registered_ids
= helper_.GetAllRegisteredIds();
On 2012/08/07 23:38:05, akalin wrote:
> On 2012/08/07 20:09:30, msw wrote:
> > nit: comment explaining when |ids| won't match 
> > |all_registered_ids|? (It's not obvious to me)
> 
> ids is only for |handler_name|/|handler|, but all_registered_ids holds all IDs
> regardless of handler.  Do you think this still needs a comment?  Is the
comment
> in SyncNotifierHelper::GetAllRegisteredIds() insufficient?

Ok, I was simply confused. I think it's odd that the three notifier
implementations need to [re]register all IDs every time any ID changes, but I'll
just presume that's intended.

[akalin, 2012-08-08 22:32:22]

New patchset with new API!

Everyone should probably take another look, since stuff has changed quite a bit.

[msw, 2012-08-09 05:20:26]

LGTM with nits; thanks!

http://codereview.chromium.org/10824161/diff/12014/chrome/browser/sync/profil...
File chrome/browser/sync/profile_sync_service.h (right):

http://codereview.chromium.org/10824161/diff/12014/chrome/browser/sync/profil...
chrome/browser/sync/profile_sync_service.h:555: // Invalidation clients should
follow the pattern below:
This is AWESOME, thank you!

http://codereview.chromium.org/10824161/diff/12014/chrome/browser/sync/profil...
chrome/browser/sync/profile_sync_service.h:578: //  
pss->UnregisterInvalidationHandler(client_handler)
nit: omg won't compile! needs trailing semicolon! :)

http://codereview.chromium.org/10824161/diff/12014/chrome/browser/sync/profil...
chrome/browser/sync/profile_sync_service.h:591: // not be NULL, and must have
already been added.  An ID must be registered
nit: s/added/registered/

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

http://codereview.chromium.org/10824161/diff/12014/sync/internal_api/sync_man...
sync/internal_api/sync_manager_impl.cc:399: if (sync_notifier_.get())
Can this legitimately be NULL? It's not NULL checked in some other places.
If it's just NULL in tests, perhaps add a comment to that effect?

http://codereview.chromium.org/10824161/diff/12014/sync/internal_api/sync_man...
sync/internal_api/sync_manager_impl.cc:1237: if (sync_notifier_.get()) {
ditto on NULL question; and nit: remove {}

http://codereview.chromium.org/10824161/diff/12014/sync/internal_api/sync_man...
File sync/internal_api/sync_manager_impl_unittest.cc (right):

http://codereview.chromium.org/10824161/diff/12014/sync/internal_api/sync_man...
sync/internal_api/sync_manager_impl_unittest.cc:789:
sync_manager_.ShutdownOnSyncThread();
nit: comment that StrictMock ensures that the current IDs aren't unregistered?
I didn't nit similarly elsewhere, but here doesn't even have a comment like:
"Ensure nothing happens..."

http://codereview.chromium.org/10824161/diff/12014/sync/notifier/invalidation...
File sync/notifier/invalidation_notifier.cc (right):

http://codereview.chromium.org/10824161/diff/12014/sync/notifier/invalidation...
sync/notifier/invalidation_notifier.cc:47:
invalidation_client_.RegisterIds(all_registered_ids);
nit: pass registrar_.GetAllRegisteredIds() and nix |all_registered_ids|

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

http://codereview.chromium.org/10824161/diff/12014/sync/notifier/non_blocking...
sync/notifier/non_blocking_invalidation_notifier.cc:199: const ObjectIdSet&
all_registered_ids = registrar_.GetAllRegisteredIds();
nit: pass directly to Bind, nix local?

http://codereview.chromium.org/10824161/diff/12014/sync/notifier/non_blocking...
File sync/notifier/non_blocking_invalidation_notifier.h (right):

http://codereview.chromium.org/10824161/diff/12014/sync/notifier/non_blocking...
sync/notifier/non_blocking_invalidation_notifier.h:30: // Generalize to
NonBlockingSyncNotifier (http://crbug.com/140409).
nit: prepend TODO(akalin): or similar?

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

http://codereview.chromium.org/10824161/diff/12014/sync/notifier/sync_notifie...
sync/notifier/sync_notifier.h:25: // Clients should follow the pattern below:
nit: I wonder if it's good to have this comment in both places, perhaps so.

http://codereview.chromium.org/10824161/diff/12014/sync/notifier/sync_notifie...
sync/notifier/sync_notifier.h:48: //  
notifier->UnregisterHandler(client_handler)
ditto: add trailing semicolon.

http://codereview.chromium.org/10824161/diff/12014/sync/notifier/sync_notifie...
sync/notifier/sync_notifier.h:61: // it must have been previously added.  Note
that this doesn't unregister the
ditto: s/added/registered/

http://codereview.chromium.org/10824161/diff/12014/sync/notifier/sync_notifie...
File sync/notifier/sync_notifier_registrar.cc (right):

http://codereview.chromium.org/10824161/diff/12014/sync/notifier/sync_notifie...
sync/notifier/sync_notifier_registrar.cc:91: if
(handlers_.might_have_observers()) {
nit: consider making this an early return up top, to avoid the work above.

http://codereview.chromium.org/10824161/diff/12014/sync/notifier/sync_notifie...
sync/notifier/sync_notifier_registrar.cc:96: if (dispatch_it !=
dispatch_map.end()) {
nit: remove unnecessary {}

http://codereview.chromium.org/10824161/diff/12014/sync/notifier/sync_notifie...
File sync/notifier/sync_notifier_registrar.h (right):

http://codereview.chromium.org/10824161/diff/12014/sync/notifier/sync_notifie...
sync/notifier/sync_notifier_registrar.h:37: // not be NULL, and must have
already been added.  An ID must be registered
nit: s/added/registered/

http://codereview.chromium.org/10824161/diff/12014/sync/notifier/sync_notifie...
sync/notifier/sync_notifier_registrar.h:48: // unregistered ones).
nit: s/ones/handlers/

http://codereview.chromium.org/10824161/diff/12014/sync/sync.gyp
File sync/sync.gyp (right):

http://codereview.chromium.org/10824161/diff/12014/sync/sync.gyp#newcode260
sync/sync.gyp:260: 'notifier/sync_notifier_registrar.cc',
nit: alphabetize _registrar after _observer

[dcheng, 2012-08-09 20:53:56]

I like the new name for SyncNotifierHelper a lot better.

One comment:
Once a handler calls SyncNotifierRegistrar::UnregisterHandler, all those IDs are
essentially orphaned right? As far as I can tell, there is no way to 're-adopt'
them with the current scheme.

[akalin, 2012-08-10 01:28:08]

Addressed all comments

http://codereview.chromium.org/10824161/diff/12014/chrome/browser/sync/profil...
File chrome/browser/sync/profile_sync_service.h (right):

http://codereview.chromium.org/10824161/diff/12014/chrome/browser/sync/profil...
chrome/browser/sync/profile_sync_service.h:578: //  
pss->UnregisterInvalidationHandler(client_handler)
On 2012/08/09 05:20:26, msw wrote:
> nit: omg won't compile! needs trailing semicolon! :)

Done.

http://codereview.chromium.org/10824161/diff/12014/chrome/browser/sync/profil...
chrome/browser/sync/profile_sync_service.h:591: // not be NULL, and must have
already been added.  An ID must be registered
On 2012/08/09 05:20:26, msw wrote:
> nit: s/added/registered/

done, also fixed other places

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

http://codereview.chromium.org/10824161/diff/12014/sync/internal_api/sync_man...
sync/internal_api/sync_manager_impl.cc:399: if (sync_notifier_.get())
On 2012/08/09 05:20:26, msw wrote:
> Can this legitimately be NULL? It's not NULL checked in some other places.
> If it's just NULL in tests, perhaps add a comment to that effect?

good catch.  this shouldn't be null.

http://codereview.chromium.org/10824161/diff/12014/sync/internal_api/sync_man...
sync/internal_api/sync_manager_impl.cc:1237: if (sync_notifier_.get()) {
On 2012/08/09 05:20:26, msw wrote:
> ditto on NULL question; and nit: remove {}

needed for tests.  added comment.

http://codereview.chromium.org/10824161/diff/12014/sync/internal_api/sync_man...
File sync/internal_api/sync_manager_impl_unittest.cc (right):

http://codereview.chromium.org/10824161/diff/12014/sync/internal_api/sync_man...
sync/internal_api/sync_manager_impl_unittest.cc:789:
sync_manager_.ShutdownOnSyncThread();
On 2012/08/09 05:20:26, msw wrote:
> nit: comment that StrictMock ensures that the current IDs aren't unregistered?
> I didn't nit similarly elsewhere, but here doesn't even have a comment like:
> "Ensure nothing happens..." 

Done.

http://codereview.chromium.org/10824161/diff/12014/sync/notifier/invalidation...
File sync/notifier/invalidation_notifier.cc (right):

http://codereview.chromium.org/10824161/diff/12014/sync/notifier/invalidation...
sync/notifier/invalidation_notifier.cc:47:
invalidation_client_.RegisterIds(all_registered_ids);
On 2012/08/09 05:20:26, msw wrote:
> nit: pass registrar_.GetAllRegisteredIds() and nix |all_registered_ids|

Done.

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

http://codereview.chromium.org/10824161/diff/12014/sync/notifier/non_blocking...
sync/notifier/non_blocking_invalidation_notifier.cc:199: const ObjectIdSet&
all_registered_ids = registrar_.GetAllRegisteredIds();
On 2012/08/09 05:20:26, msw wrote:
> nit: pass directly to Bind, nix local?

Done.

http://codereview.chromium.org/10824161/diff/12014/sync/notifier/non_blocking...
File sync/notifier/non_blocking_invalidation_notifier.h (right):

http://codereview.chromium.org/10824161/diff/12014/sync/notifier/non_blocking...
sync/notifier/non_blocking_invalidation_notifier.h:30: // Generalize to
NonBlockingSyncNotifier (http://crbug.com/140409).
On 2012/08/09 05:20:26, msw wrote:
> nit: prepend TODO(akalin): or similar?

Done.

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

http://codereview.chromium.org/10824161/diff/12014/sync/notifier/sync_notifie...
sync/notifier/sync_notifier.h:25: // Clients should follow the pattern below:
On 2012/08/09 05:20:26, msw wrote:
> nit: I wonder if it's good to have this comment in both places, perhaps so.

I think it's good to have it in both places for now.  Eventually, we might have
a common interface, which would make a convenient single place to put the
comments.

http://codereview.chromium.org/10824161/diff/12014/sync/notifier/sync_notifie...
sync/notifier/sync_notifier.h:48: //  
notifier->UnregisterHandler(client_handler)
On 2012/08/09 05:20:26, msw wrote:
> ditto: add trailing semicolon.

Done.

http://codereview.chromium.org/10824161/diff/12014/sync/notifier/sync_notifie...
sync/notifier/sync_notifier.h:61: // it must have been previously added.  Note
that this doesn't unregister the
On 2012/08/09 05:20:26, msw wrote:
> ditto: s/added/registered/

Done.

http://codereview.chromium.org/10824161/diff/12014/sync/notifier/sync_notifie...
File sync/notifier/sync_notifier_registrar.cc (right):

http://codereview.chromium.org/10824161/diff/12014/sync/notifier/sync_notifie...
sync/notifier/sync_notifier_registrar.cc:91: if
(handlers_.might_have_observers()) {
On 2012/08/09 05:20:26, msw wrote:
> nit: consider making this an early return up top, to avoid the work above.

Done.

http://codereview.chromium.org/10824161/diff/12014/sync/notifier/sync_notifie...
sync/notifier/sync_notifier_registrar.cc:96: if (dispatch_it !=
dispatch_map.end()) {
On 2012/08/09 05:20:26, msw wrote:
> nit: remove unnecessary {}

Done.

http://codereview.chromium.org/10824161/diff/12014/sync/notifier/sync_notifie...
File sync/notifier/sync_notifier_registrar.h (right):

http://codereview.chromium.org/10824161/diff/12014/sync/notifier/sync_notifie...
sync/notifier/sync_notifier_registrar.h:37: // not be NULL, and must have
already been added.  An ID must be registered
On 2012/08/09 05:20:26, msw wrote:
> nit: s/added/registered/

Done.

http://codereview.chromium.org/10824161/diff/12014/sync/notifier/sync_notifie...
sync/notifier/sync_notifier_registrar.h:48: // unregistered ones).
On 2012/08/09 05:20:26, msw wrote:
> nit: s/ones/handlers/

Done.

http://codereview.chromium.org/10824161/diff/12014/sync/sync.gyp
File sync/sync.gyp (right):

http://codereview.chromium.org/10824161/diff/12014/sync/sync.gyp#newcode260
sync/sync.gyp:260: 'notifier/sync_notifier_registrar.cc',
On 2012/08/09 05:20:26, msw wrote:
> nit: alphabetize _registrar after _observer

Done.

[akalin, 2012-08-10 01:28:55]

> One comment:
> Once a handler calls SyncNotifierRegistrar::UnregisterHandler, all those IDs
are
> essentially orphaned right? As far as I can tell, there is no way to
're-adopt'
> them with the current scheme.

Yes, that was my concern.  So you should only call UnregisterHandler without
calling UpdateRegisteredIds(_, ObjectIdSet()) first during shutdown.

[akalin, 2012-08-10 01:29:35]

I'm going to assume dcheng's and tim's LGTM still stands unless I hear otherwise
before the tryjobs finish.  Then I'll commit!

[commit-bot: I haz the power, 2012-08-10 02:58:23]

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

[commit-bot: I haz the power, 2012-08-10 04:07:21]

Change committed as 150990

[akalin, 2012-08-10 04:47:35]

sigh, looks like it made linux tests fail

[msw, 2012-08-10 06:46:36]

Might it help to split the API change from the titular behavior change that
removes calls to notifier->UpdateRegisteredIds(client_handler, ObjectIdSet());
on shutdown?

[akalin, 2012-08-10 19:01:47]

On 2012/08/10 06:46:36, msw wrote:
> Might it help to split the API change from the titular behavior change that
> removes calls to notifier->UpdateRegisteredIds(client_handler, ObjectIdSet());
> on shutdown?

No need, the fix for the test is pretty simple.

Tim, you said you were going to take another look?

[akalin, 2012-08-10 20:22:57]

On 2012/08/10 19:01:47, akalin wrote:
> On 2012/08/10 06:46:36, msw wrote:
> > Might it help to split the API change from the titular behavior change that
> > removes calls to notifier->UpdateRegisteredIds(client_handler,
ObjectIdSet());
> > on shutdown?
> 
> No need, the fix for the test is pretty simple.
> 
> Tim, you said you were going to take another look?

Looks like this is sticking.  Yay!