[Sync] Address msw's comments for r149747

[Sync] Address msw's comments for r149747

See http://codereview.chromium.org/10805002/.

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

[akalin, 2012-08-03 20:08:49]

PTAL

https://chromiumcodereview.appspot.com/10828158/diff/1/chrome/browser/sync/pr...
File chrome/browser/sync/profile_sync_service.h (right):

https://chromiumcodereview.appspot.com/10828158/diff/1/chrome/browser/sync/pr...
chrome/browser/sync/profile_sync_service.h:554: // Updates the set of ObjectIds
associated with a given |handler|.
On 2012/08/03 05:14:21, msw wrote:
> nit: break lines at sentences or ~80 chars.

Done.

https://chromiumcodereview.appspot.com/10828158/diff/1/chrome/browser/sync/pr...
File chrome/browser/sync/profile_sync_service_unittest.cc (right):

https://chromiumcodereview.appspot.com/10828158/diff/1/chrome/browser/sync/pr...
chrome/browser/sync/profile_sync_service_unittest.cc:399:
backend->EmitOnNotificationsDisabled(syncer::TRANSIENT_NOTIFICATION_ERROR);
On 2012/08/03 05:14:21, msw wrote:
> nit: It seems odd to test emitting notifications after disabling... (same
below)

Done.

https://chromiumcodereview.appspot.com/10828158/diff/1/chrome/browser/sync/pr...
chrome/browser/sync/profile_sync_service_unittest.cc:423: EXPECT_CALL(observer,
OnIncomingNotification(
On 2012/08/03 05:14:21, msw wrote:
> Perhaps I'm confused, but this test doesn't appear to actually ensure that the
> invalidation messages are received by the observer. Shouldn't there be an
> "EXPECT_CALL(observer, OnIncomingNotification(...)"?

Done.

https://chromiumcodereview.appspot.com/10828158/diff/1/sync/internal_api/publ...
File sync/internal_api/public/sync_manager.h (left):

https://chromiumcodereview.appspot.com/10828158/diff/1/sync/internal_api/publ...
sync/internal_api/public/sync_manager.h:412: SyncNotifierObserver* handler,
const ObjectIdSet& ids) = 0;
On 2012/08/03 05:14:21, msw wrote:
> The style guide dictates that these parameters should be on their own lines.

Done.

https://chromiumcodereview.appspot.com/10828158/diff/1/sync/internal_api/publ...
File sync/internal_api/public/sync_manager.h (right):

https://chromiumcodereview.appspot.com/10828158/diff/1/sync/internal_api/publ...
sync/internal_api/public/sync_manager.h:33: 
On 2012/08/03 05:14:21, msw wrote:
> nit: if the syncer namespace contains more than just these forward decls, it
> should probably have a blank line directly below it.

Done.

https://chromiumcodereview.appspot.com/10828158/diff/1/sync/internal_api/publ...
File sync/internal_api/public/test/fake_sync_manager.h (right):

https://chromiumcodereview.appspot.com/10828158/diff/1/sync/internal_api/publ...
sync/internal_api/public/test/fake_sync_manager.h:57: void Invalidate(const
ObjectIdPayloadMap& id_payloads,
On 2012/08/03 05:14:21, msw wrote:
> nit: same suggestion to move this up a line and indent to match below.

Done. (InvalidateOnSyncThread doesn't fit)

https://chromiumcodereview.appspot.com/10828158/diff/1/sync/internal_api/sync...
File sync/internal_api/sync_manager_impl.h (right):

https://chromiumcodereview.appspot.com/10828158/diff/1/sync/internal_api/sync...
sync/internal_api/sync_manager_impl.h:90: SyncNotifierObserver* handler,
On 2012/08/03 05:14:21, msw wrote:
> One parameter per line.

Done.

https://chromiumcodereview.appspot.com/10828158/diff/1/sync/internal_api/test...
File sync/internal_api/test/fake_sync_manager.cc (right):

https://chromiumcodereview.appspot.com/10828158/diff/1/sync/internal_api/test...
sync/internal_api/test/fake_sync_manager.cc:232: if
(!sync_task_runner_->PostTask(FROM_HERE, callback)) {
On 2012/08/03 05:14:21, msw wrote:
> nit: remove unnecessary {}

Prefer to be consistent with the other posts.

https://chromiumcodereview.appspot.com/10828158/diff/1/sync/notifier/object_i...
File sync/notifier/object_id_payload_map.cc (right):

https://chromiumcodereview.appspot.com/10828158/diff/1/sync/notifier/object_i...
sync/notifier/object_id_payload_map.cc:19: ObjectIdPayloadMap
ObjectIdSetToPayloadMap(ObjectIdSet ids,
On 2012/08/03 05:14:21, msw wrote:
> One param per line

Done.

https://chromiumcodereview.appspot.com/10828158/diff/1/sync/notifier/object_i...
sync/notifier/object_id_payload_map.cc:22: for (ObjectIdSet::const_iterator it =
ids.begin(); it != ids.end(); ++it) {
On 2012/08/03 05:14:21, msw wrote:
> nit: remove unnecessary {}

I don't think that applies to for loops, does it?

https://chromiumcodereview.appspot.com/10828158/diff/1/sync/notifier/object_i...
File sync/notifier/object_id_payload_map.h (right):

https://chromiumcodereview.appspot.com/10828158/diff/1/sync/notifier/object_i...
sync/notifier/object_id_payload_map.h:23: ObjectIdPayloadMap
ObjectIdSetToPayloadMap(ObjectIdSet ids,
On 2012/08/03 05:14:21, msw wrote:
> One param per line.

Done.

https://chromiumcodereview.appspot.com/10828158/diff/1/sync/notifier/sync_not...
File sync/notifier/sync_notifier.h (right):

https://chromiumcodereview.appspot.com/10828158/diff/1/sync/notifier/sync_not...
sync/notifier/sync_notifier.h:25: // Updates the set of ObjectIds associated
with a given |handler|.
On 2012/08/03 05:14:21, msw wrote:
> nit: consider the same line breaking that I suggested earlier.

Done.

[msw, 2012-08-03 22:57:16]

LGTM with minor nits; thanks! Perhaps TBR tim and/or thestig?

https://chromiumcodereview.appspot.com/10828158/diff/1/chrome/browser/sync/pr...
File chrome/browser/sync/profile_sync_service.h (right):

https://chromiumcodereview.appspot.com/10828158/diff/1/chrome/browser/sync/pr...
chrome/browser/sync/profile_sync_service.h:555: // Passing an empty ObjectIdSet
will unregister |handler|. There
nit: Line break after |handler|.

https://chromiumcodereview.appspot.com/10828158/diff/1/sync/notifier/sync_not...
File sync/notifier/sync_notifier.h (right):

https://chromiumcodereview.appspot.com/10828158/diff/1/sync/notifier/sync_not...
sync/notifier/sync_notifier.h:26: // Passing an empty ObjectIdSet will
unregister |handler|. There
nit: Line break after |handler|.

[akalin, 2012-08-04 00:02:05]

Done, committing

http://codereview.chromium.org/10828158/diff/1/chrome/browser/sync/profile_sy...
File chrome/browser/sync/profile_sync_service.h (right):

http://codereview.chromium.org/10828158/diff/1/chrome/browser/sync/profile_sy...
chrome/browser/sync/profile_sync_service.h:555: // Passing an empty ObjectIdSet
will unregister |handler|. There
On 2012/08/03 22:57:16, msw wrote:
> nit: Line break after |handler|.

Done.

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

http://codereview.chromium.org/10828158/diff/1/sync/notifier/sync_notifier.h#...
sync/notifier/sync_notifier.h:26: // Passing an empty ObjectIdSet will
unregister |handler|. There
On 2012/08/03 22:57:16, msw wrote:
> nit: Line break after |handler|.

Done.

[commit-bot: I haz the power, 2012-08-04 00:02:48]

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

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

Change committed as 150008