Remove weak_ptr from CopresenceManager.

Remove weak_ptr from CopresenceManager.

The WhispernetClient object provided by the CopresenceDelegate passed to the
CopresenceManager has a lifetime exceeding that of the CopresenceManager. We
explicitly want to decouple their lifetimes since we may have lifetime
restrictions imposed on WhispernetClient which may require it to outlive the
CopresenceManager. For this reason, when passing a callback to the
WhispernetClient from CopresenceManager, we were using a WeakPtr. This is
unnecessary since the same can be accomplished by passing a CancelableCallback
to the WhispernetClient and Canceling the callback in the CopresenceManager
destructor.

R=derat@chromium.org, willchan@chromium.org
BUG=None.

Committed: https://crrev.com/0e07df1bb2a7f00bf6430fe3a73d3f450e1ae3af
Cr-Commit-Position: refs/heads/master@{#292772}

[Daniel Erat, 2014-08-28 21:32:41]

https://codereview.chromium.org/517753002/diff/1/components/copresence/copres...
File components/copresence/copresence_manager.cc (right):

https://codereview.chromium.org/517753002/diff/1/components/copresence/copres...
components/copresence/copresence_manager.cc:28: manager->init_callback_.Reset(
i'm missing some background here, but what's the reason for this file? it seems
strange to have it accessing internals that are part of CopresenceManagerImpl.

https://codereview.chromium.org/517753002/diff/1/components/copresence/copres...
File components/copresence/copresence_manager_impl.cc (right):

https://codereview.chromium.org/517753002/diff/1/components/copresence/copres...
components/copresence/copresence_manager_impl.cc:26: init_callback_.Cancel();
looking at CancelableCallback, i don't think you need to call this. implicitly
destroying it should be sufficient since it'll invalidate the weak pointer.

https://codereview.chromium.org/517753002/diff/1/components/copresence/public...
File components/copresence/public/copresence_manager.h (right):

https://codereview.chromium.org/517753002/diff/1/components/copresence/public...
components/copresence/public/copresence_manager.h:8: #include
"base/cancelable_callback.h"
this doesn't look like it's referenced in this header. did you mean to add it to
copresence_manager_impl.h?

[rkc, 2014-08-28 21:51:50]

https://codereview.chromium.org/517753002/diff/1/components/copresence/copres...
File components/copresence/copresence_manager.cc (right):

https://codereview.chromium.org/517753002/diff/1/components/copresence/copres...
components/copresence/copresence_manager.cc:28: manager->init_callback_.Reset(
On 2014/08/28 21:32:41, Daniel Erat wrote:
> i'm missing some background here, but what's the reason for this file? it
seems
> strange to have it accessing internals that are part of CopresenceManagerImpl.

This was written by Charlie while I was OOO. I am fairly confused about this
too, but since he is on paternity leave for a few weeks, I'm leaving the code as
is till he's back, assuming that he had a good reason to do this.

https://codereview.chromium.org/517753002/diff/1/components/copresence/copres...
File components/copresence/copresence_manager_impl.cc (right):

https://codereview.chromium.org/517753002/diff/1/components/copresence/copres...
components/copresence/copresence_manager_impl.cc:26: init_callback_.Cancel();
On 2014/08/28 21:32:41, Daniel Erat wrote:
> looking at CancelableCallback, i don't think you need to call this. implicitly
> destroying it should be sufficient since it'll invalidate the weak pointer.

Done.

https://codereview.chromium.org/517753002/diff/1/components/copresence/public...
File components/copresence/public/copresence_manager.h (right):

https://codereview.chromium.org/517753002/diff/1/components/copresence/public...
components/copresence/public/copresence_manager.h:8: #include
"base/cancelable_callback.h"
On 2014/08/28 21:32:41, Daniel Erat wrote:
> this doesn't look like it's referenced in this header. did you mean to add it
to
> copresence_manager_impl.h?

Yep, meant to include it in the impl header.
Done.

[Daniel Erat, 2014-08-28 22:54:04]

looks okay to me, although i'm not sure how this is appreciably different from
using weak pointers. it's still using them but with an extra layer of
indirection now, right? was it will's suggestion to avoid using weak pointers
directly?

[rkc, 2014-08-28 23:21:06]

On 2014/08/28 22:54:04, Daniel Erat wrote:
> looks okay to me, although i'm not sure how this is appreciably different from
> using weak pointers. it's still using them but with an extra layer of
> indirection now, right? was it will's suggestion to avoid using weak pointers
> directly?

IIRC Will didn't want us to use a weak pointer since it made the lifetime of the
CopresenceManager ambiguous. This is the next best solution I could think of. I
am open to other better options :)
My main aim is to not tie the lifetime of WhispernetClient (or for that matter,
CopresenceDelegate) with the lifetime of CopresenceManager. Currently
CopresenceManager is only used from Chrome, but we intend to let it be used by
other products built on Brillo, where the lifetime requirements for the
WhispernetClient may be dictated by yet unknown factors.

[willchan no longer on Chromium, 2014-08-29 18:47:41]

Sorry, I don't have time to review :(

[Daniel Erat, 2014-08-29 21:15:26]

lgtm

[rkc, 2014-08-30 02:23:28]

The CQ bit was checked by rkc@chromium.org

[commit-bot: I haz the power, 2014-08-30 02:24:45]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/rkc@chromium.org/517753002/20001

[commit-bot: I haz the power, 2014-08-30 03:19:34]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  android_dbg_tests_recipe on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_dbg_tes...)

[commit-bot: I haz the power, 2014-08-30 05:19:06]

Committed patchset #2 (id:20001) as 952fcc4bfbb8a6369d8183384c7881fc5dad00cc

[commit-bot: I haz the power, 2014-09-10 03:13:44]

Patchset 2 (id:??) landed as
https://crrev.com/0e07df1bb2a7f00bf6430fe3a73d3f450e1ae3af
Cr-Commit-Position: refs/heads/master@{#292772}

[Charlie, 2014-10-17 22:18:09]

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

[Charlie, 2014-10-17 22:18:10]

Ben, can you clarify how you were thinking the Create() method should work?
derat@ points out that it's weird to have Foo::Create() doing all the internal
setup for FooImpl.

https://codereview.chromium.org/517753002/diff/1/components/copresence/copres...
File components/copresence/copresence_manager.cc (right):

https://codereview.chromium.org/517753002/diff/1/components/copresence/copres...
components/copresence/copresence_manager.cc:28: manager->init_callback_.Reset(
On 2014/08/28 21:51:50, Rahul Chaturvedi wrote:
> On 2014/08/28 21:32:41, Daniel Erat wrote:
> > i'm missing some background here, but what's the reason for this file? it
> seems
> > strange to have it accessing internals that are part of
CopresenceManagerImpl.
> 
> This was written by Charlie while I was OOO. I am fairly confused about this
> too, but since he is on paternity leave for a few weeks, I'm leaving the code
as
> is till he's back, assuming that he had a good reason to do this.

This was in response to kalman@'s suggestion:

https://codereview.chromium.org/441103002/diff/40001/components/copresence/pu...

But there may be a cleaner way to do this. One option is to keep the impl class
at file scope:

https://code.google.com/p/chromium/codesearch#chromium/src/chromeos/dbus/sms_...