[Sync] Turn notifier::PushClient into an interface

[Sync] Turn notifier::PushClient into an interface

Split the previous implementation into two pieces:
XmppPushClient and NonBlockingPushClient.

Add FakePushClient and FakePushClientObserver.

Remove use of ThreadSafeObserverList.

Add PushClient::CreateDefault() function, which creates a
NonBlockingPushClient for an XmppPushClient.

Dep-inject PushClient into P2PNotifier.

Add some helper functions to notification_defines.{h,cc}.

BUG=76764
TEST=

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

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

[akalin, 2012-05-18 22:27:42]

+rlarocque for review

[rlarocque, 2012-05-18 23:01:51]

I didn't spend a lot of time looking at the new files because I assume most of
their code is being moved from existing files.  Let me know if that's not the
case.  

All the rest is LGTM.

https://chromiumcodereview.appspot.com/10413014/diff/1/jingle/notifier/listen...
File jingle/notifier/listener/push_client.h (right):

https://chromiumcodereview.appspot.com/10413014/diff/1/jingle/notifier/listen...
jingle/notifier/listener/push_client.h:29: static scoped_ptr<PushClient>
CreateForCurrentThread(
What's the use case for this?  Shouldn't the caller be responsible for knowing
what thread we're on and constructing the right object accordingly?

[akalin, 2012-05-18 23:21:57]

committing after trybots

https://chromiumcodereview.appspot.com/10413014/diff/1/jingle/notifier/listen...
File jingle/notifier/listener/push_client.h (right):

https://chromiumcodereview.appspot.com/10413014/diff/1/jingle/notifier/listen...
jingle/notifier/listener/push_client.h:29: static scoped_ptr<PushClient>
CreateForCurrentThread(
On 2012/05/18 23:01:51, rlarocque wrote:
> What's the use case for this?  Shouldn't the caller be responsible for knowing
> what thread we're on and constructing the right object accordingly?

I thought it was a more convenient API, but you're right.  Changed to
CreateDefault().

[akalin, 2012-05-19 01:26:10]

mac_rel looks legit, investigating...

[akalin, 2012-05-21 16:55:20]

On 2012/05/19 01:26:10, akalin wrote:
> mac_rel looks legit, investigating...

Change destruction method to avoid DCHECK.  At worst the delegate object will
leak (if the IO thread doesn't exist anymore).

[akalin, 2012-05-21 18:56:28]

On 2012/05/21 16:55:20, akalin wrote:
> On 2012/05/19 01:26:10, akalin wrote:
> > mac_rel looks legit, investigating...
> 
> Change destruction method to avoid DCHECK.  At worst the delegate object will
> leak (if the IO thread doesn't exist anymore).

Whoa, memory corruption!  The plot thickens.

[akalin, 2012-05-22 01:36:57]

On 2012/05/21 18:56:28, akalin wrote:
> On 2012/05/21 16:55:20, akalin wrote:
> > On 2012/05/19 01:26:10, akalin wrote:
> > > mac_rel looks legit, investigating...
> > 
> > Change destruction method to avoid DCHECK.  At worst the delegate object
will
> > leak (if the IO thread doesn't exist anymore).
> 
> Whoa, memory corruption!  The plot thickens.

My attempted fix was buggy (wasn't calling RemoveObserver properly).

I figured out the reason for the original mac_rel failure.  Documented it in
~Core.

Committing after trybots.

[commit-bot: I haz the power, 2012-05-22 01:37:30]

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

[commit-bot: I haz the power, 2012-05-22 04:11:31]

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

[akalin, 2012-05-22 04:24:40]

On 2012/05/22 04:11:31, I haz the power (commit-bot) wrote:
> Try job failure for 10413014-8028 (retry) on win_rel for steps
"base_unittests,
> sync_unit_tests" (clobber build).
> It's a second try, previously, step "compile" failed.
>
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

I'm pretty sure the win_rel failures are unrelated.  I'm somewhat sure that the
linux_rel failures are also unrelated.

I will commit this manually and keep an eye on the tree.

[commit-bot: I haz the power, 2012-05-23 02:54:07]

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

[commit-bot: I haz the power, 2012-05-23 02:54:17]

Failed to apply patch for sync/notifier/p2p_notifier_unittest.cc:
While running patch -p1 --forward --force;
patching file sync/notifier/p2p_notifier_unittest.cc
Hunk #1 FAILED at 6.
Hunk #2 FAILED at 27.
Hunk #3 succeeded at 63 (offset 12 lines).
Hunk #4 succeeded at 96 (offset 12 lines).
Hunk #5 succeeded at 121 (offset 12 lines).
Hunk #6 succeeded at 139 (offset 12 lines).
Hunk #7 succeeded at 161 (offset 12 lines).
Hunk #8 succeeded at 174 (offset 12 lines).
Hunk #9 succeeded at 224 (offset 12 lines).
2 out of 9 hunks FAILED -- saving rejects to file
sync/notifier/p2p_notifier_unittest.cc.rej

[commit-bot: I haz the power, 2012-05-23 03:14:08]

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

[akalin, 2012-05-23 03:25:58]

On 2012/05/23 03:14:08, I haz the power (commit-bot) wrote:
> CQ is trying da patch. Follow status at
> https://chromium-status.appspot.com/cq/akalin%40chromium.org/10413014/7028

For posterity, I figured out the memory stomping.  It was because I was posting
a task with 'this' in Core's constructor.  Since Core is ref-counted, the task
could then finish executing and destroy the Core object before the constructor
finishes.  Blurgh!