Remove base::SupportsWeakPtr from {Audio,Video}Sender

Remove base::SupportsWeakPtr from {Audio,Video}Sender

{Audio,Video}Sender already creates a WeakPtrFactory instance to
access its weak pointer. Therefore, it does not have to inherit
base::SupportsWeakPtr. This CL removes SupportsWeakPtr and exposes
its weak pointer outside the class via AsWeakPtr().

BUG=None

Review-Url: https://codereview.chromium.org/2867013003
Cr-Commit-Position: refs/heads/master@{#470454}
Committed: https://chromium.googlesource.com/chromium/src/+/28b78a893c03f9da7d84fbee4e1a652211b6b905

[Chandan, 2017-05-08 13:34:09]

The CQ bit was checked by c.padhi@samsung.com to run a CQ dry run

[commit-bot: I haz the power, 2017-05-08 13:34:23]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-08 16:09:09]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-08 16:09:10]

Dry run: This issue passed the CQ dry run.

[Chandan, 2017-05-08 16:12:07]

c.padhi@samsung.com changed reviewers:
+ hubbe@chromium.org, miu@chromium.org

[Chandan, 2017-05-08 16:12:08]

PTAL. Thank you.

[hubbe, 2017-05-08 17:19:19]

On 2017/05/08 16:12:08, Chandan wrote:
> PTAL. Thank you.

Don't we usually prefer composition over inheritance?

[Chandan, 2017-05-08 17:52:00]

On 2017/05/08 17:19:19, hubbe wrote:
> On 2017/05/08 16:12:08, Chandan wrote:
> > PTAL. Thank you.
> 
> Don't we usually prefer composition over inheritance?

Sure, we do. 
Weak pointers to {Audio,Video}Sender are used outside the class.
Hence, preferred removing WeakPtrFactory over SupportsWeakPtr(going by their
descriptions in weak_ptr.h).
In case, we choose removing SupportsWeakPtr, then we have to expose a weak ptr
getter, something like {Audio,Video}Sender::GetWeakPtr().
Do let me know your opinion.

[miu, 2017-05-08 20:04:31]

https://codereview.chromium.org/2867013003/diff/1/media/cast/sender/audio_sen...
File media/cast/sender/audio_sender.h (left):

https://codereview.chromium.org/2867013003/diff/1/media/cast/sender/audio_sen...
media/cast/sender/audio_sender.h:67: base::WeakPtrFactory<AudioSender>
weak_factory_;
We can't do this: When ~AudioSender() is called, the weak pointers need to be
invalidated before |audio_encoder_| is destroyed. I think, in general, Chromium
code should not inherit from base::SupportsWeakPtr, because the weak pointers
are invalidated after the class members are destroyed.

[Chandan, 2017-05-08 21:04:30]

https://codereview.chromium.org/2867013003/diff/1/media/cast/sender/audio_sen...
File media/cast/sender/audio_sender.h (left):

https://codereview.chromium.org/2867013003/diff/1/media/cast/sender/audio_sen...
media/cast/sender/audio_sender.h:67: base::WeakPtrFactory<AudioSender>
weak_factory_;
On 2017/05/08 20:04:31, miu wrote:
> We can't do this: When ~AudioSender() is called, the weak pointers need to be
> invalidated before |audio_encoder_| is destroyed. I think, in general,
Chromium
> code should not inherit from base::SupportsWeakPtr, because the weak pointers
> are invalidated after the class members are destroyed.

In that case, will remove SupportsWeakPtr instead of WeakPtrFactory and expose a
weak pointer getter to be used outside this class? Same for VideoSender as well.
Thanks.

[Chandan, 2017-05-09 09:05:25]

The CQ bit was checked by c.padhi@samsung.com to run a CQ dry run

[commit-bot: I haz the power, 2017-05-09 09:06:12]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Chandan, 2017-05-09 09:07:42]

Description was changed from

==========
Remove WeakPtrFactory from {Audio,Video}Sender

{Audio,Video}Sender already inherits SupportsWeakPtr.
Hence, instead of creating a WeakPtrFactory instance to
access its weak pointer, we can directly use AsWeakPtr().

BUG=None
==========

to

==========
Remove base::SupportsWeakPtr from {Audio,Video}Sender

{Audio,Video}Sender already creates a WeakPtrFactory instance to
access its weak pointer. Therefore, it does not have to inherit
base::SupportsWeakPtr. This CL removes SupportsWeakPtr and exposes
its weak pointer outside the class via AsWeakPtr().

BUG=None
==========

[commit-bot: I haz the power, 2017-05-09 11:01:04]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-09 11:01:05]

Dry run: This issue passed the CQ dry run.

[Chandan, 2017-05-09 11:06:23]

On 2017/05/08 21:04:30, Chandan wrote:
>
https://codereview.chromium.org/2867013003/diff/1/media/cast/sender/audio_sen...
> File media/cast/sender/audio_sender.h (left):
> 
>
https://codereview.chromium.org/2867013003/diff/1/media/cast/sender/audio_sen...
> media/cast/sender/audio_sender.h:67: base::WeakPtrFactory<AudioSender>
> weak_factory_;
> On 2017/05/08 20:04:31, miu wrote:
> > We can't do this: When ~AudioSender() is called, the weak pointers need to
be
> > invalidated before |audio_encoder_| is destroyed. I think, in general,
> Chromium
> > code should not inherit from base::SupportsWeakPtr, because the weak
pointers
> > are invalidated after the class members are destroyed.
> 
> In that case, will remove SupportsWeakPtr instead of WeakPtrFactory and expose
a
> weak pointer getter to be used outside this class? Same for VideoSender as
well.
> Thanks.

Uploaded patch set 2 that removes SupportsWeakPtr.
PTAL. Thanks.

[miu, 2017-05-09 19:54:48]

PS2 lgtm! :)

[Chandan, 2017-05-10 02:50:05]

The CQ bit was checked by c.padhi@samsung.com

[commit-bot: I haz the power, 2017-05-10 02:51:10]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-10 04:43:38]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1494384605852060,
"parent_rev": "8e4017253ecba8c9745dff3b363b63e4781cd4ef", "commit_rev":
"28b78a893c03f9da7d84fbee4e1a652211b6b905"}

[commit-bot: I haz the power, 2017-05-10 04:43:49]

Description was changed from

==========
Remove base::SupportsWeakPtr from {Audio,Video}Sender

{Audio,Video}Sender already creates a WeakPtrFactory instance to
access its weak pointer. Therefore, it does not have to inherit
base::SupportsWeakPtr. This CL removes SupportsWeakPtr and exposes
its weak pointer outside the class via AsWeakPtr().

BUG=None
==========

to

==========
Remove base::SupportsWeakPtr from {Audio,Video}Sender

{Audio,Video}Sender already creates a WeakPtrFactory instance to
access its weak pointer. Therefore, it does not have to inherit
base::SupportsWeakPtr. This CL removes SupportsWeakPtr and exposes
its weak pointer outside the class via AsWeakPtr().

BUG=None

Review-Url: https://codereview.chromium.org/2867013003
Cr-Commit-Position: refs/heads/master@{#470454}
Committed:
https://chromium.googlesource.com/chromium/src/+/28b78a893c03f9da7d84fbee4e1a...
==========

[commit-bot: I haz the power, 2017-05-10 04:43:50]

Committed patchset #2 (id:20001) as
https://chromium.googlesource.com/chromium/src/+/28b78a893c03f9da7d84fbee4e1a...