RpcHandlerTest cleanup

RpcHandlerTest cleanup

Committed: https://crrev.com/2941760c4affeed46eaf1252db4d0186e639fca9
Cr-Commit-Position: refs/heads/master@{#302676}

[Charlie, 2014-11-04 00:53:54]

ckehoe@chromium.org changed reviewers:
+ rkc@chromium.org

[rkc, 2014-11-04 19:35:27]

lgtm % comments

https://codereview.chromium.org/685983007/diff/20001/components/copresence/rp...
File components/copresence/rpc/rpc_handler.cc (right):

https://codereview.chromium.org/685983007/diff/20001/components/copresence/rp...
components/copresence/rpc/rpc_handler.cc:178: // TODO(ckehoe): Use
CancelableCallbacks instead.
We don't want to use CancelableCallbacks here. The *only* situation a cancelable
callback or weak pointer should be used in is if we *cannot* predict or control
the freeing of an object. This is true pretty much in one case till now (for
example, when sending a callback to the audio thread). This isn't one such
place; since we can clearly unregister our callbacks on our destruction.

https://codereview.chromium.org/685983007/diff/20001/components/copresence/te...
File components/copresence/test/stub_whispernet_client.h (right):

https://codereview.chromium.org/685983007/diff/20001/components/copresence/te...
components/copresence/test/stub_whispernet_client.h:15: StubWhispernetClient()
{}
override

[Charlie, 2014-11-04 19:57:04]

https://codereview.chromium.org/685983007/diff/20001/components/copresence/rp...
File components/copresence/rpc/rpc_handler.cc (right):

https://codereview.chromium.org/685983007/diff/20001/components/copresence/rp...
components/copresence/rpc/rpc_handler.cc:178: // TODO(ckehoe): Use
CancelableCallbacks instead.
On 2014/11/04 19:35:27, Rahul Chaturvedi wrote:
> We don't want to use CancelableCallbacks here. The *only* situation a
cancelable
> callback or weak pointer should be used in is if we *cannot* predict or
control
> the freeing of an object. This is true pretty much in one case till now (for
> example, when sending a callback to the audio thread). This isn't one such
> place; since we can clearly unregister our callbacks on our destruction.

Ok. But I still do not like registering them in one place and unregistering in
another. I suppose that is technically a separate issue.

https://codereview.chromium.org/685983007/diff/20001/components/copresence/te...
File components/copresence/test/stub_whispernet_client.h (right):

https://codereview.chromium.org/685983007/diff/20001/components/copresence/te...
components/copresence/test/stub_whispernet_client.h:15: StubWhispernetClient()
{}
On 2014/11/04 19:35:27, Rahul Chaturvedi wrote:
> override

Huh? You cannot override a constructor. And this class has no destructor.

[Charlie, 2014-11-04 19:58:22]

The CQ bit was checked by ckehoe@chromium.org

[commit-bot: I haz the power, 2014-11-04 19:59:13]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/685983007/40001

[rkc, 2014-11-04 20:02:57]

https://codereview.chromium.org/685983007/diff/20001/components/copresence/te...
File components/copresence/test/stub_whispernet_client.h (right):

https://codereview.chromium.org/685983007/diff/20001/components/copresence/te...
components/copresence/test/stub_whispernet_client.h:15: StubWhispernetClient()
{}
On 2014/11/04 19:57:04, Charlie wrote:
> On 2014/11/04 19:35:27, Rahul Chaturvedi wrote:
> > override
> 
> Huh? You cannot override a constructor. And this class has no destructor.

Oh nevermind, I can't read.

[chromium-reviews, 2014-11-04 20:09:24]

Yes, well, I believe you originally had at lease one of the callbacks
registered and unregistered in the same class after all. Then I separated
them, and then complained about it :-) So I suppose we're even.


On Tue Nov 04 2014 at 12:02:57 PM <rkc@chromium.org> wrote:

>
> https://codereview.chromium.org/685983007/diff/20001/
> components/copresence/test/stub_whispernet_client.h
> File components/copresence/test/stub_whispernet_client.h (right):
>
> https://codereview.chromium.org/685983007/diff/20001/
> components/copresence/test/stub_whispernet_client.h#newcode15
> components/copresence/test/stub_whispernet_client.h:15
>
<https://codereview.chromium.org/685983007/diff/20001/components/copresence/te...>
> :
> StubWhispernetClient() {}
> On 2014/11/04 19:57:04, Charlie wrote:
> > On 2014/11/04 19:35:27, Rahul Chaturvedi wrote:
> > > override
>
> > Huh? You cannot override a constructor. And this class has no
> destructor.
>
> Oh nevermind, I can't read.
>
> https://codereview.chromium.org/685983007/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[commit-bot: I haz the power, 2014-11-04 21:29:43]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2014-11-04 21:31:00]

Patchset 3 (id:??) landed as
https://crrev.com/2941760c4affeed46eaf1252db4d0186e639fca9
Cr-Commit-Position: refs/heads/master@{#302676}