Get rid of unnecessary cast of FlexfecReceiveStreamImpl to FlexfecReceiveStream

Get rid of unnecessary cast of FlexfecReceiveStreamImpl to FlexfecReceiveStream

BUG=None

Review-Url: https://codereview.webrtc.org/2967913002
Cr-Commit-Position: refs/heads/master@{#19131}
Committed: https://chromium.googlesource.com/external/webrtc/+/42f44f9cf672eb4bc43616c11e625111f48b6366

[eladalon, 2017-07-04 09:54:54]

eladalon@webrtc.org changed reviewers:
+ danilchap@webrtc.org, nisse@webrtc.org, stefan@webrtc.org

[eladalon, 2017-07-04 09:54:55]

PTAL

[eladalon, 2017-07-04 09:56:27]

https://codereview.webrtc.org/2967913002/diff/1/webrtc/call/flexfec_receive_s...
File webrtc/call/flexfec_receive_stream_impl.h (right):

https://codereview.webrtc.org/2967913002/diff/1/webrtc/call/flexfec_receive_s...
webrtc/call/flexfec_receive_stream_impl.h:51: const Config& GetConfig() const
override;
Might also be a good idea to return a copy of the Config, rather than a
reference to it. Otherwise, we have no guarantee nobody would be using it past
its lifetime. (Separate issue for a separate CL, though.) What do you think?

[stefan-webrtc, 2017-07-06 15:31:47]

I think the purpose of this cast is to ensure that noone else can delete the
object except the one who created it (Call). If we change the pattern for
FlexFec, should we not change it for all streams in that case?

[eladalon, 2017-07-06 16:40:25]

On 2017/07/06 15:31:47, stefan-webrtc wrote:
> I think the purpose of this cast is to ensure that noone else can delete the
> object except the one who created it (Call). If we change the pattern for
> FlexFec, should we not change it for all streams in that case?

1. I think if deletion is achieved by first casting, then everybody can cast and
delete equally; it doesn't really make sure only Call can do that.
2. Yes, I agree that we should change it for the other streams, too. I'll
replicate the change for them, as well as making some function names more
standard - config() vs. GetConfig() - after this CL is accepted; I think doing
it in separate CLs would make it easier to review (you) and avoid mistakes (me).
3. By the way, about the TODO around changing GetConfig() to return a copy
instead of a reference - wdyt?

[holmer, 2017-07-07 09:16:36]

On 2017/07/06 16:40:25, eladalon wrote:
> On 2017/07/06 15:31:47, stefan-webrtc wrote:
> > I think the purpose of this cast is to ensure that noone else can delete the
> > object except the one who created it (Call). If we change the pattern for
> > FlexFec, should we not change it for all streams in that case?
> 
> 1. I think if deletion is achieved by first casting, then everybody can cast
and
> delete equally; it doesn't really make sure only Call can do that.

No, but the users of Call and Send/ReceiveStreams can't delete it since it's not
part of the api. You'd have to cast to internal::FlexFecReceiveStream, for
instance, which is a pretty strong indication that you're doing something you
shouldn't do.

> 2. Yes, I agree that we should change it for the other streams, too. I'll
> replicate the change for them, as well as making some function names more
> standard - config() vs. GetConfig() - after this CL is accepted; I think doing
> it in separate CLs would make it easier to review (you) and avoid mistakes
(me).
> 3. By the way, about the TODO around changing GetConfig() to return a copy
> instead of a reference - wdyt?

Changing from Config& GetConfig() to Config config() seems like pretty low
priority tasks to me, but if you want to improve it I think it should be moved
to the Impl and not be in the interface.

[eladalon, 2017-07-07 09:25:05]

eladalon@webrtc.org changed reviewers:
+ ossu@webrtc.org, solenberg@webrtc.org

[eladalon, 2017-07-07 09:25:06]

Adding solenberg@ and ossu@.

[eladalon, 2017-07-07 09:27:20]

eladalon@webrtc.org changed reviewers:
+ sprang@webrtc.org

[eladalon, 2017-07-07 09:27:21]

And Erik.

[danilchap, 2017-07-19 10:42:44]

lgtm

https://codereview.webrtc.org/2967913002/diff/20001/webrtc/call/flexfec_recei...
File webrtc/call/flexfec_receive_stream_impl.h (right):

https://codereview.webrtc.org/2967913002/diff/20001/webrtc/call/flexfec_recei...
webrtc/call/flexfec_receive_stream_impl.h:50: 
may be remove this empty line (since comment above applies to line below too)

[eladalon, 2017-07-21 12:33:32]

https://codereview.webrtc.org/2967913002/diff/20001/webrtc/call/flexfec_recei...
File webrtc/call/flexfec_receive_stream_impl.h (right):

https://codereview.webrtc.org/2967913002/diff/20001/webrtc/call/flexfec_recei...
webrtc/call/flexfec_receive_stream_impl.h:50: 
On 2017/07/19 10:42:44, danilchap wrote:
> may be remove this empty line (since comment above applies to line below too)

Okay, will do on next revision, once we get some more lgtm and/or comments.

[ossu, 2017-07-21 13:07:15]

lgtm % that little empty line

On 2017/07/06 15:31:47, stefan-webrtc wrote:
> I think the purpose of this cast is to ensure that noone else can delete the
> object except the one who created it (Call). If we change the pattern for
> FlexFec, should we not change it for all streams in that case?

<slight rant>
I think it's unfortunate that we've tried to enforce such ownership/lifetime
semantics in the interface. Regardless, getting a pointer to an object shouldn't
imply that you're free to delete it - that's what unique_ptr is for. Had we been
able to use references more freely, that would even more clearly indicate
objects you have access to but cannot delete.

Making a destructor non-public limits perfectly acceptable uses, like putting an
implementation in a unique_ptr<> of the base class. This was recently a problem
when we wanted to wrap a Transport (which has a protected destructor) inside of
AudioSendStream. Ideally, that wrapper could've been placed inside an anonymous
namespace in the CC file, but that would require us to have a
unique_ptr<Transport>, which cannot be since unique_ptr cannot delete a Tranport
object.
</slight rant>

https://codereview.webrtc.org/2967913002/diff/1/webrtc/call/flexfec_receive_s...
File webrtc/call/flexfec_receive_stream_impl.h (right):

https://codereview.webrtc.org/2967913002/diff/1/webrtc/call/flexfec_receive_s...
webrtc/call/flexfec_receive_stream_impl.h:51: const Config& GetConfig() const
override;
On 2017/07/04 09:56:26, eladalon wrote:
> Might also be a good idea to return a copy of the Config, rather than a
> reference to it. Otherwise, we have no guarantee nobody would be using it past
> its lifetime. (Separate issue for a separate CL, though.) What do you think?

IMO, there's no reason to return a copy unless access to the Config must be
serialized (thus necessitating a serialized copy in GetConfig()). Nobody should
keep references inside other objects around. If the caller needs to retain a
Config, it's super easy for them to copy it during the GetConfig() call (or
later).

https://codereview.webrtc.org/2967913002/diff/20001/webrtc/call/flexfec_recei...
File webrtc/call/flexfec_receive_stream_impl.h (right):

https://codereview.webrtc.org/2967913002/diff/20001/webrtc/call/flexfec_recei...
webrtc/call/flexfec_receive_stream_impl.h:50: 
On 2017/07/19 10:42:44, danilchap wrote:
> may be remove this empty line (since comment above applies to line below too)

+1

[eladalon, 2017-07-21 13:19:32]

I'll expand the CL to make the same change for other occurrences of this in this
file, then ask you guys to lgtm-ize again.

https://codereview.webrtc.org/2967913002/diff/1/webrtc/call/flexfec_receive_s...
File webrtc/call/flexfec_receive_stream_impl.h (right):

https://codereview.webrtc.org/2967913002/diff/1/webrtc/call/flexfec_receive_s...
webrtc/call/flexfec_receive_stream_impl.h:51: const Config& GetConfig() const
override;
On 2017/07/21 13:07:15, ossu wrote:
> On 2017/07/04 09:56:26, eladalon wrote:
> > Might also be a good idea to return a copy of the Config, rather than a
> > reference to it. Otherwise, we have no guarantee nobody would be using it
past
> > its lifetime. (Separate issue for a separate CL, though.) What do you think?
> 
> IMO, there's no reason to return a copy unless access to the Config must be
> serialized (thus necessitating a serialized copy in GetConfig()). Nobody
should
> keep references inside other objects around. If the caller needs to retain a
> Config, it's super easy for them to copy it during the GetConfig() call (or
> later).

I was thinking about fool-proofing it, but I guess you're right - if returning a
temporarily valid handle is good enough for the STL (std::string::c_str), it
should be good enough for me; copying comes at a price, after all.

[sprang_webrtc, 2017-07-24 14:32:08]

lgtm

[eladalon, 2017-07-25 12:37:56]

Actually, come to think of it, it's probably better to keep separate CLs, to
make it easier to review.

[eladalon, 2017-07-25 12:38:02]

The CQ bit was checked by eladalon@webrtc.org

[eladalon, 2017-07-25 12:38:03]

The patchset sent to the CQ was uploaded after l-g-t-m from ossu@webrtc.org,
danilchap@webrtc.org, sprang@webrtc.org
Link to the patchset: https://codereview.webrtc.org/2967913002/#ps40001 (title:
"Rebase + remove empty line.")

[commit-bot: I haz the power, 2017-07-25 12:38:13]

CQ is trying da patch.

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

[commit-bot: I haz the power, 2017-07-25 12:44:21]

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

[commit-bot: I haz the power, 2017-07-25 12:44:22]

Try jobs failed on following builders:
  presubmit on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/presubmit/builds/19577)

[the sun, 2017-07-25 13:35:45]

lgtm

[eladalon, 2017-07-25 13:36:45]

The CQ bit was checked by eladalon@webrtc.org

[commit-bot: I haz the power, 2017-07-25 13:36:53]

CQ is trying da patch.

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

[commit-bot: I haz the power, 2017-07-25 13:40:07]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1500989805559290,
"parent_rev": "59cac99c9a80668110ad8f25f711a0d1111d71d9", "commit_rev":
"42f44f9cf672eb4bc43616c11e625111f48b6366"}

[commit-bot: I haz the power, 2017-07-25 13:40:12]

Description was changed from

==========
Get rid of unnecessary cast of FlexfecReceiveStreamImpl to FlexfecReceiveStream

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

to

==========
Get rid of unnecessary cast of FlexfecReceiveStreamImpl to FlexfecReceiveStream

BUG=None

Review-Url: https://codereview.webrtc.org/2967913002
Cr-Commit-Position: refs/heads/master@{#19131}
Committed:
https://chromium.googlesource.com/external/webrtc/+/42f44f9cf672eb4bc43616c11...
==========

[commit-bot: I haz the power, 2017-07-25 13:40:13]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/external/webrtc/+/42f44f9cf672eb4bc43616c11...