[Sync] Make InvalidationNotifier use PushClient

[Sync] Make InvalidationNotifier use PushClient

This removes a lot of duplicated code and unifies the two main
SyncNotifier implementations (P2P and Invalidation) to use the same
underlying transport layer.

Replace CacheInvalidationPacketHandler and ChromeNetwork with PushClientChannel.

Add PushClient::CreateDefaultOnIOThread which creates
an XmppPushClient directly.

BUG=76764
TEST=


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

[akalin, 2012-05-24 00:04:12]

+rlarocque for everything
+ghc for PushClientChannel (which implements invalidation::NetworkChannel).

[rlarocque, 2012-05-24 00:44:49]

https://chromiumcodereview.appspot.com/10436013/diff/1/sync/notifier/chrome_i...
File sync/notifier/chrome_invalidation_client.h (right):

https://chromiumcodereview.appspot.com/10436013/diff/1/sync/notifier/chrome_i...
sync/notifier/chrome_invalidation_client.h:58: scoped_ptr<notifier::PushClient>
push_client);
A scoped_ptr as a constructor parameter looks strange to me.  Would it make more
sense to perform the Pass() before passing the pointer to the constructor?

https://chromiumcodereview.appspot.com/10436013/diff/1/sync/notifier/invalida...
File sync/notifier/invalidation_notifier.h (right):

https://chromiumcodereview.appspot.com/10436013/diff/1/sync/notifier/invalida...
sync/notifier/invalidation_notifier.h:44: scoped_ptr<notifier::PushClient>
push_client,
Same concern as before.  I'm not sure it makes sense to pass scoped_ptrs to a
constructor, since passing pointers to a constructor normally implies a transfer
of ownership.

[akalin, 2012-05-24 00:48:27]

https://chromiumcodereview.appspot.com/10436013/diff/1/sync/notifier/chrome_i...
File sync/notifier/chrome_invalidation_client.h (right):

https://chromiumcodereview.appspot.com/10436013/diff/1/sync/notifier/chrome_i...
sync/notifier/chrome_invalidation_client.h:58: scoped_ptr<notifier::PushClient>
push_client);
On 2012/05/24 00:44:49, rlarocque wrote:
> A scoped_ptr as a constructor parameter looks strange to me.  Would it make
more
> sense to perform the Pass() before passing the pointer to the constructor?

.Pass() still returns a scoped_ptr<> so that wouldn't change anything.

It does look strange, but scoped_ptr<> has been modified to support "moving":
see comments in scoped_ptr.h.

[rlarocque, 2012-05-24 01:15:25]

On 2012/05/24 00:48:27, akalin wrote:
>
https://chromiumcodereview.appspot.com/10436013/diff/1/sync/notifier/chrome_i...
> File sync/notifier/chrome_invalidation_client.h (right):
> 
>
https://chromiumcodereview.appspot.com/10436013/diff/1/sync/notifier/chrome_i...
> sync/notifier/chrome_invalidation_client.h:58:
scoped_ptr<notifier::PushClient>
> push_client);
> On 2012/05/24 00:44:49, rlarocque wrote:
> > A scoped_ptr as a constructor parameter looks strange to me.  Would it make
> more
> > sense to perform the Pass() before passing the pointer to the constructor?
> 
> .Pass() still returns a scoped_ptr<> so that wouldn't change anything.
> 
> It does look strange, but scoped_ptr<> has been modified to support "moving":
> see comments in scoped_ptr.h.

After much thinking, I think you're right.  

It bugs me that it's not obvious that the caller's scoped_ptr will be
invalidated, but I guess there's not much we can do about that.  

LGTM.

[akalin, 2012-05-24 23:21:45]

Greg, any comments?

On 2012/05/24 01:15:25, rlarocque wrote:
> On 2012/05/24 00:48:27, akalin wrote:
> >
>
https://chromiumcodereview.appspot.com/10436013/diff/1/sync/notifier/chrome_i...
> > File sync/notifier/chrome_invalidation_client.h (right):
> > 
> >
>
https://chromiumcodereview.appspot.com/10436013/diff/1/sync/notifier/chrome_i...
> > sync/notifier/chrome_invalidation_client.h:58:
> scoped_ptr<notifier::PushClient>
> > push_client);
> > On 2012/05/24 00:44:49, rlarocque wrote:
> > > A scoped_ptr as a constructor parameter looks strange to me.  Would it
make
> > more
> > > sense to perform the Pass() before passing the pointer to the constructor?
> > 
> > .Pass() still returns a scoped_ptr<> so that wouldn't change anything.
> > 
> > It does look strange, but scoped_ptr<> has been modified to support
"moving":
> > see comments in scoped_ptr.h.
> 
> After much thinking, I think you're right.  
> 
> It bugs me that it's not obvious that the caller's scoped_ptr will be
> invalidated, but I guess there's not much we can do about that.  
> 
> LGTM.

[ghc, 2012-05-29 14:23:03]

lgtm

[commit-bot: I haz the power, 2012-05-30 01:06:43]

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

[commit-bot: I haz the power, 2012-05-30 03:53:51]

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

[commit-bot: I haz the power, 2012-05-30 07:26:11]

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

[commit-bot: I haz the power, 2012-05-30 09:10:29]

Change committed as 139508