Implemented initial version of server-issued notification client.

Implemented initial version of server-issued notification client.

Added NOTIFICATION_SERVER notification method (use
--sync-notification-method=server to turn on).

BUG=34647
TEST=manually

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

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

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

[akalin, 2010-06-17 01:57:36]

+nick for sync stuff
+ghc for invalidation stuff

I left a lot of TODOs and messiness around, but the main point of this CL is to
get the code out of sync_listen_notifications and into the chrome code base
proper.  I'll try to take care of all comments, but I may have to punt on some
of them.

ghc, the files of most interest to you are probably just the new ones in
chrome/browser/sync/notifier.  I'll have some questions for you the next time we
meet re. semantics of registration.

nick, I'll have to discuss with you the implications of this w.r.t. Cosmo
traffic, too.

[ghc, 2010-06-18 21:30:50]

http://codereview.chromium.org/2827014/diff/20002/19002
File chrome/browser/sync/engine/syncapi.cc (right):

http://codereview.chromium.org/2827014/diff/20002/19002#newcode1274
chrome/browser/sync/engine/syncapi.cc:1274: notifier::MediatorThread*
mediator_thread =
comment for this block?

http://codereview.chromium.org/2827014/diff/20002/19002#newcode1772
chrome/browser/sync/engine/syncapi.cc:1772: // has_more_to_sync only notify
peers if a successful commit has
has_more_to_sync doesn't fit on the previous line?

http://codereview.chromium.org/2827014/diff/20002/19005
File chrome/browser/sync/notifier/cache_invalidation_packet_handler.cc (right):

http://codereview.chromium.org/2827014/diff/20002/19005#newcode20
chrome/browser/sync/notifier/cache_invalidation_packet_handler.cc:20: //
TODO(akalin): Minimize use of 'tango' name.
s/Minimize/Eliminate ? :)

This should probably be done sooner rather than later; let me know if you want
to discuss any naming changes in the xmpp messages, for example.

http://codereview.chromium.org/2827014/diff/20002/19006
File chrome/browser/sync/notifier/cache_invalidation_packet_handler.h (right):

http://codereview.chromium.org/2827014/diff/20002/19006#newcode30
chrome/browser/sync/notifier/cache_invalidation_packet_handler.h:30:
CacheInvalidationPacketHandler(
specs on variables, parameters, methods?

http://codereview.chromium.org/2827014/diff/20002/19007
File chrome/browser/sync/notifier/chrome_invalidation_client.cc (right):

http://codereview.chromium.org/2827014/diff/20002/19007#newcode42
chrome/browser/sync/notifier/chrome_invalidation_client.cc:42: app_name,
listener, ticl_config));
indentation in this function looks inconsistent

http://codereview.chromium.org/2827014/diff/20002/19007#newcode46
chrome/browser/sync/notifier/chrome_invalidation_client.cc:46: const buzz::Jid
to_jid("tango@bot.talk.google.com");
make a constant for tango@bot.talk.google.com?

[akalin, 2010-06-21 21:30:34]

Please take another look!

http://codereview.chromium.org/2827014/diff/20002/19002
File chrome/browser/sync/engine/syncapi.cc (right):

http://codereview.chromium.org/2827014/diff/20002/19002#newcode1274
chrome/browser/sync/engine/syncapi.cc:1274: notifier::MediatorThread*
mediator_thread =
On 2010/06/18 21:30:50, ghc wrote:
> comment for this block?

Done.

http://codereview.chromium.org/2827014/diff/20002/19002#newcode1772
chrome/browser/sync/engine/syncapi.cc:1772: // has_more_to_sync only notify
peers if a successful commit has
On 2010/06/18 21:30:50, ghc wrote:
> has_more_to_sync doesn't fit on the previous line?

Done.

http://codereview.chromium.org/2827014/diff/20002/19005
File chrome/browser/sync/notifier/cache_invalidation_packet_handler.cc (right):

http://codereview.chromium.org/2827014/diff/20002/19005#newcode20
chrome/browser/sync/notifier/cache_invalidation_packet_handler.cc:20: //
TODO(akalin): Minimize use of 'tango' name.
On 2010/06/18 21:30:50, ghc wrote:
> s/Minimize/Eliminate ? :)
> 
> This should probably be done sooner rather than later; let me know if you want
> to discuss any naming changes in the xmpp messages, for example.

Done.  Yeah, let's talk about XMPP naming changes after this CL.

http://codereview.chromium.org/2827014/diff/20002/19006
File chrome/browser/sync/notifier/cache_invalidation_packet_handler.h (right):

http://codereview.chromium.org/2827014/diff/20002/19006#newcode30
chrome/browser/sync/notifier/cache_invalidation_packet_handler.h:30:
CacheInvalidationPacketHandler(
On 2010/06/18 21:30:50, ghc wrote:
> specs on variables, parameters, methods?

Done, also moved more code into this class.

http://codereview.chromium.org/2827014/diff/20002/19007
File chrome/browser/sync/notifier/chrome_invalidation_client.cc (right):

http://codereview.chromium.org/2827014/diff/20002/19007#newcode42
chrome/browser/sync/notifier/chrome_invalidation_client.cc:42: app_name,
listener, ticl_config));
On 2010/06/18 21:30:50, ghc wrote:
> indentation in this function looks inconsistent

Done.

http://codereview.chromium.org/2827014/diff/20002/19007#newcode46
chrome/browser/sync/notifier/chrome_invalidation_client.cc:46: const buzz::Jid
to_jid("tango@bot.talk.google.com");
On 2010/06/18 21:30:50, ghc wrote:
> make a constant for tango@bot.talk.google.com?

Done.

[ncarter, 2010-06-21 22:38:29]

http://codereview.chromium.org/2827014/diff/20002/19004
File chrome/browser/sync/notification_method.h (right):

http://codereview.chromium.org/2827014/diff/20002/19004#newcode21
chrome/browser/sync/notification_method.h:21: //        +-------+
This is a great comment, and I'd like to see it kept current.  If it doesn't
make sense to add an S case to this diagram, please add some sentences below
explaining how Server notifications are expected to interact with old clients.

http://codereview.chromium.org/2827014/diff/20002/19004#newcode36
chrome/browser/sync/notification_method.h:36: // New notification method. 
Compatible only with transitional
"New" is a dangerous adjective (witness the New New New Tab Page).  Here, you
should revise this comment to indicate what it's "new" relative to.

http://codereview.chromium.org/2827014/diff/20002/19004#newcode43
chrome/browser/sync/notification_method.h:43: };
idea: consider ways to encapsulate the business logic of in one place.  E.g.,
what if NotificationMethod were an abstract class with 4 concrete
implementations, no data members, and virtual methods like:

bool shouldSendOldStyleNotifications();
bool shouldSendNewStyleNotifications();
bool shouldInitiateInvalidationServerConnection();

I'm not necessarily confident that this would actually pan out, just offering it
for discussion.

http://codereview.chromium.org/2827014/diff/20002/19006
File chrome/browser/sync/notifier/cache_invalidation_packet_handler.h (right):

http://codereview.chromium.org/2827014/diff/20002/19006#newcode48
chrome/browser/sync/notifier/cache_invalidation_packet_handler.h:48: }  //
sync_notifier
"namespace sync_notifier"

http://codereview.chromium.org/2827014/diff/22002/40004
File chrome/browser/sync/notifier/cache_invalidation_packet_handler.cc (right):

http://codereview.chromium.org/2827014/diff/22002/40004#newcode174
chrome/browser/sync/notifier/cache_invalidation_packet_handler.cc:174: buzz::Jid
to_jid_;
const, maybe?

http://codereview.chromium.org/2827014/diff/22002/40004#newcode231
chrome/browser/sync/notifier/cache_invalidation_packet_handler.cc:231:
CHECK(base::Base64Decode(packet, &decoded_message));
Could a server bug cause this CHECK to fail?  If so, can we handle the error?

http://codereview.chromium.org/2827014/diff/22002/40008
File chrome/browser/sync/notifier/chrome_system_resources.cc (right):

http://codereview.chromium.org/2827014/diff/22002/40008#newcode22
chrome/browser/sync/notifier/chrome_system_resources.cc:22:
CHECK(!scheduler_active_);
Do we really need to crash the browser?

http://codereview.chromium.org/2827014/diff/22002/40012
File chrome/browser/sync/notifier/server_notifier_thread.cc (right):

http://codereview.chromium.org/2827014/diff/22002/40012#newcode147
chrome/browser/sync/notifier/server_notifier_thread.cc:147:
data_types.push_back("PREFERENCE");
Interesting, but should be fine for now.

http://codereview.chromium.org/2827014/diff/22002/40016
File chrome/chrome.gyp (right):

http://codereview.chromium.org/2827014/diff/22002/40016#newcode1067
chrome/chrome.gyp:1067: 'target_name': 'sync_notifier',
I might have trouble keeping straight the distinction between 'notifier' and
'sync_notifier.  How do you classify the two?  A comment on each target might
help.

[akalin, 2010-06-21 23:39:42]

Addressed all nick's comments.  Please take another look!

http://codereview.chromium.org/2827014/diff/20002/19004
File chrome/browser/sync/notification_method.h (right):

http://codereview.chromium.org/2827014/diff/20002/19004#newcode21
chrome/browser/sync/notification_method.h:21: //        +-------+
On 2010/06/21 22:38:29, ncarter wrote:
> This is a great comment, and I'd like to see it kept current.  If it doesn't
> make sense to add an S case to this diagram, please add some sentences below
> explaining how Server notifications are expected to interact with old clients.

Done.

http://codereview.chromium.org/2827014/diff/20002/19004#newcode36
chrome/browser/sync/notification_method.h:36: // New notification method. 
Compatible only with transitional
On 2010/06/21 22:38:29, ncarter wrote:
> "New" is a dangerous adjective (witness the New New New Tab Page).  Here, you
> should revise this comment to indicate what it's "new" relative to.

Done.

http://codereview.chromium.org/2827014/diff/20002/19004#newcode43
chrome/browser/sync/notification_method.h:43: };
On 2010/06/21 22:38:29, ncarter wrote:
> idea: consider ways to encapsulate the business logic of in one place.  E.g.,
> what if NotificationMethod were an abstract class with 4 concrete
> implementations, no data members, and virtual methods like:
> 
> bool shouldSendOldStyleNotifications();
> bool shouldSendNewStyleNotifications();
> bool shouldInitiateInvalidationServerConnection();
> 
> I'm not necessarily confident that this would actually pan out, just offering
it
> for discussion.
> 

I don't think it's worth it; once we move to server-issued notifications by
default, we could just wipe out all the code for all the other notification
methods.

http://codereview.chromium.org/2827014/diff/20002/19006
File chrome/browser/sync/notifier/cache_invalidation_packet_handler.h (right):

http://codereview.chromium.org/2827014/diff/20002/19006#newcode48
chrome/browser/sync/notifier/cache_invalidation_packet_handler.h:48: }  //
sync_notifier
On 2010/06/21 22:38:29, ncarter wrote:
> "namespace sync_notifier"

Done.

http://codereview.chromium.org/2827014/diff/22002/40004
File chrome/browser/sync/notifier/cache_invalidation_packet_handler.cc (right):

http://codereview.chromium.org/2827014/diff/22002/40004#newcode174
chrome/browser/sync/notifier/cache_invalidation_packet_handler.cc:174: buzz::Jid
to_jid_;
On 2010/06/21 22:38:29, ncarter wrote:
> const, maybe?

Done.

http://codereview.chromium.org/2827014/diff/22002/40004#newcode231
chrome/browser/sync/notifier/cache_invalidation_packet_handler.cc:231:
CHECK(base::Base64Decode(packet, &decoded_message));
On 2010/06/21 22:38:29, ncarter wrote:
> Could a server bug cause this CHECK to fail?  If so, can we handle the error?

Done.

http://codereview.chromium.org/2827014/diff/22002/40008
File chrome/browser/sync/notifier/chrome_system_resources.cc (right):

http://codereview.chromium.org/2827014/diff/22002/40008#newcode22
chrome/browser/sync/notifier/chrome_system_resources.cc:22:
CHECK(!scheduler_active_);
On 2010/06/21 22:38:29, ncarter wrote:
> Do we really need to crash the browser?

Done.

http://codereview.chromium.org/2827014/diff/22002/40016
File chrome/chrome.gyp (right):

http://codereview.chromium.org/2827014/diff/22002/40016#newcode1067
chrome/chrome.gyp:1067: 'target_name': 'sync_notifier',
On 2010/06/21 22:38:29, ncarter wrote:
> I might have trouble keeping straight the distinction between 'notifier' and
> 'sync_notifier.  How do you classify the two?  A comment on each target might
> help.

Basically: 'notifier' = common P2P notifier code shared between sync and cloud
print; 'sync_notifier': sync-only notifier code.

Added comments.

[ncarter, 2010-06-21 23:43:17]

Thanks for the fixes.  LGTM

[akalin, 2010-06-22 17:15:55]

Greg is on vacation until tomorrow, so I'll just submit this today (since I
addressed all his comments) and I can address further comments of his in a
follow up.

On 2010/06/21 23:43:17, ncarter wrote:
> Thanks for the fixes.  LGTM