[Sync] Replace TalkMediator*/MediatorThread* with PushClient

[Sync] Replace TalkMediator*/MediatorThread* with PushClient

Streamline methods of PushClient and its Observer subclass.

Remove sync/protocol/service_constants.h and consolidate use of each
constant in one place.  Remove duplicate constant in cloud print code.

BUG=76764
TEST=
TBR=estade@chromium.org

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

[akalin, 2012-05-16 02:04:53]

+gene for cloud print stuff
+rlarocque for everything else

[rlarocque, 2012-05-16 17:46:00]

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

https://chromiumcodereview.appspot.com/10398051/diff/1/jingle/notifier/listen...
jingle/notifier/listener/push_client.cc:27: class PushClient::Core
There's a lot of scary thread code here.  Is this class new, and therefore
worthy of a thorough review?  Or is it recycled from one of the classes you
deleted and known to work reasonably well already?

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

https://chromiumcodereview.appspot.com/10398051/diff/1/jingle/notifier/listen...
jingle/notifier/listener/push_client.h:70: void
SimulateOnNotificationReceivedForTest(
Is there no way to move these into a test class?

[akalin, 2012-05-16 17:53:15]

I missed the P2PNotifier unit tests.  I will fix those.

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

https://chromiumcodereview.appspot.com/10398051/diff/1/jingle/notifier/listen...
jingle/notifier/listener/push_client.cc:27: class PushClient::Core
On 2012/05/16 17:46:00, rlarocque wrote:
> There's a lot of scary thread code here.  Is this class new, and therefore
> worthy of a thorough review?  Or is it recycled from one of the classes you
> deleted and known to work reasonably well already?

This was recycled from MediatorThreadImpl, but git didn't detect it as a move. 
I'll see if I can tweak the settings to get it to do so.

It follows a common pattern for objects that live on two threads, though. 
PushClient lives entirely on one thread (the calling thread), so it has the
ref-counted Core class that handles the IO thread stuff.  All the PushClient
methods simply post the equivalent method to Core on the IO thread.  Whenever
the Core wants to post to the observers, it simply does so directly, since it
stores a ThreadSafeObserverList.

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

https://chromiumcodereview.appspot.com/10398051/diff/1/jingle/notifier/listen...
jingle/notifier/listener/push_client.h:70: void
SimulateOnNotificationReceivedForTest(
On 2012/05/16 17:46:00, rlarocque wrote:
> Is there no way to move these into a test class?

What do you mean?  I couldn't think of a better way to poke holes in the
interface for the unit tests.

[rlarocque, 2012-05-16 19:07:49]

LGTM, with one request (the DCHECK).

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

https://chromiumcodereview.appspot.com/10398051/diff/1/jingle/notifier/listen...
jingle/notifier/listener/push_client.cc:27: class PushClient::Core
On 2012/05/16 17:53:16, akalin wrote:
> On 2012/05/16 17:46:00, rlarocque wrote:
> > There's a lot of scary thread code here.  Is this class new, and therefore
> > worthy of a thorough review?  Or is it recycled from one of the classes you
> > deleted and known to work reasonably well already?
> 
> This was recycled from MediatorThreadImpl, but git didn't detect it as a move.

> I'll see if I can tweak the settings to get it to do so.

Don't worry about it.  I doubt the code review site would be able to display the
changes very well either way.  

> It follows a common pattern for objects that live on two threads, though. 
> PushClient lives entirely on one thread (the calling thread), so it has the
> ref-counted Core class that handles the IO thread stuff.  All the PushClient
> methods simply post the equivalent method to Core on the IO thread.  Whenever
> the Core wants to post to the observers, it simply does so directly, since it
> stores a ThreadSafeObserverList.

https://chromiumcodereview.appspot.com/10398051/diff/1/jingle/notifier/listen...
jingle/notifier/listener/push_client.cc:222:
DCHECK(parent_message_loop_proxy_->BelongsToCurrentThread());
Can we DCHECK in this function that the core's observer list is empty?

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

https://chromiumcodereview.appspot.com/10398051/diff/1/jingle/notifier/listen...
jingle/notifier/listener/push_client.h:70: void
SimulateOnNotificationReceivedForTest(
On 2012/05/16 17:53:16, akalin wrote:
> On 2012/05/16 17:46:00, rlarocque wrote:
> > Is there no way to move these into a test class?
> 
> What do you mean?  I couldn't think of a better way to poke holes in the
> interface for the unit tests.

I see it now.  Moving these functions out of this class conflicts with the goal
of hiding PushClient::Core.  

Personally, I'd prefer moving the implementation of PushClient::Core into a .h
file, but I can see why you implemented this the way you did.

[akalin, 2012-05-16 22:47:41]

All comments addressed and fixed unittests.

http://codereview.chromium.org/10398051/diff/1/jingle/notifier/listener/push_...
File jingle/notifier/listener/push_client.cc (right):

http://codereview.chromium.org/10398051/diff/1/jingle/notifier/listener/push_...
jingle/notifier/listener/push_client.cc:222:
DCHECK(parent_message_loop_proxy_->BelongsToCurrentThread());
On 2012/05/16 19:07:50, rlarocque wrote:
> Can we DCHECK in this function that the core's observer list is empty?  

Done.  Not here, but up in ~Core()

http://codereview.chromium.org/10398051/diff/1/jingle/notifier/listener/push_...
File jingle/notifier/listener/push_client.h (right):

http://codereview.chromium.org/10398051/diff/1/jingle/notifier/listener/push_...
jingle/notifier/listener/push_client.h:70: void
SimulateOnNotificationReceivedForTest(
> Personally, I'd prefer moving the implementation of PushClient::Core into a .h
> file, but I can see why you implemented this the way you did.  

The logic in ::Core will be moved out into another class in the next CL!

[gene, 2012-05-16 22:54:51]

lgtm for cloudprint

[rlarocque, 2012-05-16 23:04:31]

http://codereview.chromium.org/10398051/diff/1/jingle/notifier/listener/push_...
File jingle/notifier/listener/push_client.cc (right):

http://codereview.chromium.org/10398051/diff/1/jingle/notifier/listener/push_...
jingle/notifier/listener/push_client.cc:222:
DCHECK(parent_message_loop_proxy_->BelongsToCurrentThread());
On 2012/05/16 22:47:42, akalin wrote:
> On 2012/05/16 19:07:50, rlarocque wrote:
> > Can we DCHECK in this function that the core's observer list is empty?  
> 
> Done.  Not here, but up in ~Core()

Actually, I was hoping it could be done here.  I seem to remember that classes
managed with DestroyOnIOThread don't have their destructors called reliably in
all shutdown scenarios.  

I'd be willing to let this slide for now if your future CLs are planning to move
the observer list onto this thread, though.

[akalin, 2012-05-16 23:06:52]

On 2012/05/16 23:04:31, rlarocque wrote:
> Actually, I was hoping it could be done here.  I seem to remember that classes
> managed with DestroyOnIOThread don't have their destructors called reliably in
> all shutdown scenarios.  

Ah, you may be thinking of the DeleteOnUIThread helper classes that you can pass
to RefCountedThreadSafe.  This is a bit different, but suffers from a similar
problem, theoretically.  But probably not in practice, since the IO thread is
still around when the sync thread dies (I think).

> I'd be willing to let this slide for now if your future CLs are planning to
move
> the observer list onto this thread, though.

Yeah, it'll eventually go away.

Thanks.

[rlarocque, 2012-05-16 23:08:23]

On 2012/05/16 23:06:52, akalin wrote:
> On 2012/05/16 23:04:31, rlarocque wrote:
> > Actually, I was hoping it could be done here.  I seem to remember that
classes
> > managed with DestroyOnIOThread don't have their destructors called reliably
in
> > all shutdown scenarios.  
> 
> Ah, you may be thinking of the DeleteOnUIThread helper classes that you can
pass
> to RefCountedThreadSafe.  This is a bit different, but suffers from a similar
> problem, theoretically.  But probably not in practice, since the IO thread is
> still around when the sync thread dies (I think).
> 
> > I'd be willing to let this slide for now if your future CLs are planning to
> move
> > the observer list onto this thread, though.
> 
> Yeah, it'll eventually go away.
> 
> Thanks.

Sounds good.  LGTM.

[commit-bot: I haz the power, 2012-05-17 00:45:37]

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

[commit-bot: I haz the power, 2012-05-17 00:45:57]

Presubmit check for 10398051-11001 failed and returned exit status 1.

Running presubmit commit checks ...

** Presubmit Messages **
You might be calling functions intended only for testing from
production code.  It is OK to ignore this warning if you know what
you are doing, as the heuristics used to detect the situation are
not perfect.  The commit queue will not block on this warning.
Email joi@chromium.org if you have questions.
  jingle/notifier/listener/push_client.cc:61
    void ReflectSentNotificationsForTest(); \
  sync/notifier/p2p_notifier.cc:270
    push_client_.SimulateConnectAndSubscribeForTest(base_task); \
  sync/notifier/p2p_notifier.cc:275
    push_client_.ReflectSentNotificationsForTest(); \
  sync/syncable/syncable.cc:2157
    EntryKernel* entry = GetPossibleLastChildForTest(lock, parent_id);

If this change requires manual test instructions to QA team, add
TEST=[instructions].

** Presubmit ERRORS **
Missing LGTM from an OWNER for files in these directories:
    chrome/browser/ui/webui

Presubmit checks took 3.7s to calculate.

[akalin, 2012-05-17 00:47:21]

tbring estade for trivial changes in webui

[commit-bot: I haz the power, 2012-05-17 00:49:56]

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