Refactoring: move ownership of RtcEventLog from Call to PeerConnection

Refactoring: move ownership of RtcEventLog from Call to PeerConnection

This CL is a pure refactoring which should not result in any functinal
changes. It moves ownership of the RtcEventLog from webrtc::Call to the
webrtc::PeerConnection object.

This is done so that we can add RtcEventLog support for ICE events -
which will require the TransportController to have a pointer to the
RtcEventLog. PeerConnection is the closest common owner of both Call and
TransportController (through WebRtcSession).

BUG=webrtc:6393
Committed: https://crrev.com/11a9cbfa50631f9ca3e61f3f9d993450386983c1
Cr-Commit-Position: refs/heads/master@{#14578}

[skvlad, 2016-09-26 22:37:12]

Description was changed from

==========
Refactoring: lift RtcEventLog from Call to PeerConnection

This CL is a pure refactoring which should not result in any functinal
changes. It moves ownership of the RtcEventLog from webrtc::Call to the
webrtc::PeerConnection object.

This is done so that we can add RtcEventLog support for ICE events -
which will require the TransportController to have a pointer to the
RtcEventLog. PeerConnection is the closest common owner of both Call and
TransportController (through WebRtcSession).

BUG=webrtc:6393
==========

to

==========
Refactoring: lift RtcEventLog from Call to PeerConnection

This CL is a pure refactoring which should not result in any functinal
changes. It moves ownership of the RtcEventLog from webrtc::Call to the
webrtc::PeerConnection object.

This is done so that we can add RtcEventLog support for ICE events -
which will require the TransportController to have a pointer to the
RtcEventLog. PeerConnection is the closest common owner of both Call and
TransportController (through WebRtcSession).

BUG=webrtc:6393
==========

[skvlad, 2016-09-26 22:37:12]

skvlad@webrtc.org changed reviewers:
+ kjellander@webrtc.org, stefan@webrtc.org

[skvlad, 2016-09-26 22:37:34]

Description was changed from

==========
Refactoring: lift RtcEventLog from Call to PeerConnection

This CL is a pure refactoring which should not result in any functinal
changes. It moves ownership of the RtcEventLog from webrtc::Call to the
webrtc::PeerConnection object.

This is done so that we can add RtcEventLog support for ICE events -
which will require the TransportController to have a pointer to the
RtcEventLog. PeerConnection is the closest common owner of both Call and
TransportController (through WebRtcSession).

BUG=webrtc:6393
==========

to

==========
Refactoring: move ownership of RtcEventLog from Call to PeerConnection

This CL is a pure refactoring which should not result in any functinal
changes. It moves ownership of the RtcEventLog from webrtc::Call to the
webrtc::PeerConnection object.

This is done so that we can add RtcEventLog support for ICE events -
which will require the TransportController to have a pointer to the
RtcEventLog. PeerConnection is the closest common owner of both Call and
TransportController (through WebRtcSession).

BUG=webrtc:6393
==========

[skvlad, 2016-09-26 22:46:57]

kjellander@ - please pay extra attention to the checkdeps rules I've had to
modify. Is there another way around it?

Here is the reasoning behind the DEPS change:
- We need access to the interface file (rtc_event_log.h) from PeerConnection.cc
in order to create and control it.
- PeerConnection.cc is in api/, rtc_event_log.h is in call/ - so one of these
things should happen:
 a. rtc_event_log.{h, cc} should be moved to base/ or api/. Moving the
implementation is complicated as it uses the RTP parser from modules/rtp_rtcp.
 b. rtc_event_log.h is moved to base/ or api/, rtc_event_log.cc stays where it
is. This makes it more difficult to link, as headers and implementation are now
in different libraries. All existing users also need to be updated with the new
header path. api/ is inaccessible from modules/ (as far as I can tell).
 c. split rtc_event_log.h into a format agnostic part - LogEvent(rtclog::Event&)
- and a format dependent encoder  - rtclog::Event encode_rtp_event(char*, ...).
The encoder may live in the module. This makes it a lot more difficult to use,
as now we need to have both the log and the encoder available. While this might
be the cleanest solution from the dependency structure point of view, it may
become unnecessarily complex for a logging API. Nobody will want to use it.
 d. turn off the check_deps rule for that file, similarly to how it's being done
for call.h. This is what I've done in this CL. Everything is compiling, linking
and working correctly, just the presubmit script wasn't happy until the rule was
disabled.

[skvlad, 2016-09-26 22:59:33]

Patchset #2 (id:20001) has been deleted

[honghaiz3, 2016-09-27 17:49:26]

honghaiz@webrtc.org changed reviewers:
+ honghaiz@webrtc.org

[honghaiz3, 2016-09-27 17:49:27]

https://codereview.webrtc.org/2353033005/diff/80001/webrtc/call/call.cc
File webrtc/call/call.cc (right):

https://codereview.webrtc.org/2353033005/diff/80001/webrtc/call/call.cc#newco...
webrtc/call/call.cc:238: event_log_(config.event_log ? config.event_log :
null_event_log_.get()),
Is there a way to avoid having a null_event_log_ here?
Its whole purpose is to be a owner of event_log_ when that is not provided from
outside (in the test). 
Possible options I can think of: 
1. Let all downstream users to be able to handle a null_ptr event_log_. 
2. Let CallConfig hold a NullEventLog in the test. 
How complicated are these two options? If both of them are too complicated, I am
OK with this approach.

[skvlad, 2016-09-27 18:30:38]

On 2016/09/27 17:49:27, honghaiz3 wrote:
> Possible options I can think of: 
> 1. Let all downstream users to be able to handle a null_ptr event_log_.
I think the original intention was to use null object implementation and avoid
null checks everywhere, but there are already a few places where it is already
done
(https://cs.chromium.org/chromium/src/third_party/webrtc/voice_engine/channel....)
in place of using the null implementation. Maybe that's what we should do.

> 2. Let CallConfig hold a NullEventLog in the test. 
This is a little harder as there are multiple places where a Call (and a
CallConfig) are created in tests. These configs are currently empty, and now the
test will need to own its own RtcEventLog with a unique_ptr, and create the
CallConfig appropriately. It needs to set the last parameter in a large struct,
so it needs to do it in the constructor body as opposed to the field initializer
list, and that means switching a bunch of fields from const to non-const. It's a
fairly mechanical change, but it touches a lot more code than it needs to.

> How complicated are these two options? If both of them are too complicated, I
am
> OK with this approach.
I was thinking of a third option - which is to have a single global instance of
the null implementation, and use that if nothing was provided. Unfortunately,
this goes against the Chromium rules of not having non-POD global variables.
Even if the constructor is trivial (and it is), the RtcEventLog has a virtual
destructor - so some compilers generate initialization code to register it. I'll
try with lazy_instance<> instead.

[honghaiz3, 2016-09-27 19:07:17]

On 2016/09/27 18:30:38, skvlad wrote:
> On 2016/09/27 17:49:27, honghaiz3 wrote:
> > Possible options I can think of: 
> > 1. Let all downstream users to be able to handle a null_ptr event_log_.
> I think the original intention was to use null object implementation and avoid
> null checks everywhere, but there are already a few places where it is already
> done
>
(https://cs.chromium.org/chromium/src/third_party/webrtc/voice_engine/channel....)
> in place of using the null implementation. Maybe that's what we should do.
This sounds a good option to me. It doesn't have a lot of references of
event_log_ in the code (unless a different variable name has been used). 


> 
> > 2. Let CallConfig hold a NullEventLog in the test. 
> This is a little harder as there are multiple places where a Call (and a
> CallConfig) are created in tests. These configs are currently empty, and now
the
> test will need to own its own RtcEventLog with a unique_ptr, and create the
> CallConfig appropriately. It needs to set the last parameter in a large
struct,
> so it needs to do it in the constructor body as opposed to the field
initializer
> list, and that means switching a bunch of fields from const to non-const. It's
a
> fairly mechanical change, but it touches a lot more code than it needs to.
> 
> > How complicated are these two options? If both of them are too complicated,
I
> am
> > OK with this approach.
> I was thinking of a third option - which is to have a single global instance
of
> the null implementation, and use that if nothing was provided. Unfortunately,
> this goes against the Chromium rules of not having non-POD global variables.
> Even if the constructor is trivial (and it is), the RtcEventLog has a virtual
> destructor - so some compilers generate initialization code to register it.
I'll
> try with lazy_instance<> instead.

[skvlad, 2016-09-27 21:31:57]

Patchset #5 (id:100001) has been deleted

[skvlad, 2016-09-27 21:32:49]

On 2016/09/27 19:07:17, honghaiz3 wrote:
> This sounds a good option to me. It doesn't have a lot of references of
> event_log_ in the code (unless a different variable name has been used). 

Done. I have removed the null object implementation and added null checks where
they were missing.

[skvlad, 2016-09-27 21:37:06]

skvlad@webrtc.org changed reviewers:
+ terelius@webrtc.org

[skvlad, 2016-09-27 21:38:39]

Description was changed from

==========
Refactoring: move ownership of RtcEventLog from Call to PeerConnection

This CL is a pure refactoring which should not result in any functinal
changes. It moves ownership of the RtcEventLog from webrtc::Call to the
webrtc::PeerConnection object.

This is done so that we can add RtcEventLog support for ICE events -
which will require the TransportController to have a pointer to the
RtcEventLog. PeerConnection is the closest common owner of both Call and
TransportController (through WebRtcSession).

BUG=webrtc:6393
==========

to

==========
Refactoring: move ownership of RtcEventLog from Call to PeerConnection

This CL is a pure refactoring which should not result in any functinal
changes. It moves ownership of the RtcEventLog from webrtc::Call to the
webrtc::PeerConnection object.

This is done so that we can add RtcEventLog support for ICE events -
which will require the TransportController to have a pointer to the
RtcEventLog. PeerConnection is the closest common owner of both Call and
TransportController (through WebRtcSession).

As part of this refactoring, I've removed the null object implementation
(RtcEventLogNullImpl) and replaced it with explicit null checks at the
point of use, as the difficulty of managing the lifetime of the null
implementation in tests was outweighing the benefits it provided.

BUG=webrtc:6393
==========

[honghaiz3, 2016-09-27 23:06:59]

lgtm

[kjellander_webrtc, 2016-09-28 07:48:23]

I only looked at the DEPS change.

https://codereview.webrtc.org/2353033005/diff/160001/webrtc/DEPS
File webrtc/DEPS (right):

https://codereview.webrtc.org/2353033005/diff/160001/webrtc/DEPS#newcode14
webrtc/DEPS:14: "+webrtc/call/rtc_event_log.h",
I suggest you add "+webrtc/call" instead for the directories that need to access
this header.
These per-file entries are only needed since we cannot add "+webrtc/" on the
top-level since it would make the whole check deps check pretty useless
(everything would be allowed to depend on everything everywhere below webrtc/).

[skvlad, 2016-09-28 19:03:46]

terelius@, stefan@ - could one of you please take a look?

Thanks!

https://codereview.webrtc.org/2353033005/diff/160001/webrtc/DEPS
File webrtc/DEPS (right):

https://codereview.webrtc.org/2353033005/diff/160001/webrtc/DEPS#newcode14
webrtc/DEPS:14: "+webrtc/call/rtc_event_log.h",
On 2016/09/28 07:48:23, kjellander_webrtc wrote:
> I suggest you add "+webrtc/call" instead for the directories that need to
access
> this header.
> These per-file entries are only needed since we cannot add "+webrtc/" on the
> top-level since it would make the whole check deps check pretty useless
> (everything would be allowed to depend on everything everywhere below
webrtc/).

I've scoped the rule down to just that one file that needs to include it. PTAL.

[stefan-webrtc, 2016-09-29 07:56:24]

Personally, I prefer the null implementation over the nullptr checks. I see that
you looked into this and that it was a lot of boilerplate for the tests, but
could it be done if you created a null object in, e.g., BaseTest in call_test.h?
Most of the tests you're running into inherit from that.

https://codereview.webrtc.org/2353033005/diff/200001/webrtc/call/call.cc
File webrtc/call/call.cc (right):

https://codereview.webrtc.org/2353033005/diff/200001/webrtc/call/call.cc#newc...
webrtc/call/call.cc:454: if (event_log_) {
Remove {} here and below since it's a two line if.

[the sun, 2016-09-29 10:56:52]

On 2016/09/29 07:56:24, stefan-webrtc (holmer) wrote:
> Personally, I prefer the null implementation over the nullptr checks. I see
that
> you looked into this and that it was a lot of boilerplate for the tests, but
> could it be done if you created a null object in, e.g., BaseTest in
call_test.h?
> Most of the tests you're running into inherit from that.
> 
> https://codereview.webrtc.org/2353033005/diff/200001/webrtc/call/call.cc
> File webrtc/call/call.cc (right):
> 
>
https://codereview.webrtc.org/2353033005/diff/200001/webrtc/call/call.cc#newc...
> webrtc/call/call.cc:454: if (event_log_) {
> Remove {} here and below since it's a two line if.

I agree with Stefan about the null pointer checks. Think about it from a testing
perspective - if you allow the pointer to be null, with the associated runtime
logic, you introduce ambiguity and must increase the "area" that your tests
cover. The reason for the null implementation of RtcEventLog was to make the
calling code simpler.

[skvlad, 2016-09-29 18:10:18]

On 2016/09/29 10:56:52, the sun wrote:
> On 2016/09/29 07:56:24, stefan-webrtc (holmer) wrote:
> > Personally, I prefer the null implementation over the nullptr checks. I see
> that
> > you looked into this and that it was a lot of boilerplate for the tests, but
> > could it be done if you created a null object in, e.g., BaseTest in
> call_test.h?
> > Most of the tests you're running into inherit from that.
> > 
> > https://codereview.webrtc.org/2353033005/diff/200001/webrtc/call/call.cc
> > File webrtc/call/call.cc (right):
> > 
> >
>
https://codereview.webrtc.org/2353033005/diff/200001/webrtc/call/call.cc#newc...
> > webrtc/call/call.cc:454: if (event_log_) {
> > Remove {} here and below since it's a two line if.
> 
> I agree with Stefan about the null pointer checks. Think about it from a
testing
> perspective - if you allow the pointer to be null, with the associated runtime
> logic, you introduce ambiguity and must increase the "area" that your tests
> cover. The reason for the null implementation of RtcEventLog was to make the
> calling code simpler.

Agree that null objects make it easier on the user's code, and my own preference
is to use them whenever possible (especially after having used primarily
garbage-collected languages for many years.) However, it appears to not result
in a net benefit in this particular case:
 1. RtcEventLog is an interface that is implemented multiple times
(RtcEventLogImpl, RtcEventLogNullImpl, RtcEventLogProxy), but most methods will
only be called from one point. This means we trade one method in the null
implementation for one if statement elsewhere, which won't benefit readability
much.
 2. Managing the lifetime of the null implementation is tricky, especially when
there are multiple places where an RtcEventLog needs to be created
(PeerConnection/Call, VoEChannel, and about 6-7 top level unit test classes).
I've tried to use a static global instance, but that goes against the Chromium
rules of not having static initializers (the interface declares a destructor,
which needs to be registered with AtExit). I've tried creating owned instances
everywhere we need to, but this required changing a dozen or so files, and
reduced readability: we can no longer use the default constructor for
Call::Config, so its creation has to go into the constructor body, a few const
fields have to be changed to non-const, etc. - the change has a ripple effect
way beyond the benefit it gives. This problem doesn't exist in Java or C#, so
null objects are much more popular there.
 3. The null object is not a common pattern in WebRTC, so developers don't seem
to be famililar with it - in fact most users of RtcEventLog are already checking
it for null before calling, negating the benefit of reducing the number of
branches. Even RtcEventLogProxy is not using a null instance internally, but it
has a null check around each call instead - most likely because of issue #2,
lifetime management. I think I had to add the if check to only 2-3 call sites.

So in summary - I also wanted to use the null object, and spent about a day
trying different approaches to manage it. In the end I found out none of them
resulted in clearer code than simply checking for null. If you have specific
ideas for implementing it - feel free to suggest them.

Do you have other feedback on the change, other than the use of null objects?

[skvlad, 2016-10-04 07:57:33]

Ping. Please take a look when you have time.

[kjellander_webrtc, 2016-10-04 13:22:30]

https://codereview.webrtc.org/2353033005/diff/200001/webrtc/api/DEPS
File webrtc/api/DEPS (right):

https://codereview.webrtc.org/2353033005/diff/200001/webrtc/api/DEPS#newcode21
webrtc/api/DEPS:21: "+webrtc/call/rtc_event_log.h",
I think "+webrtc/call" should be allowed for all sources in webrtc/api.
I'm not sure why the special cases exist for the entries below. Maybe it's
because they're undesired dependencies... either way webrtc/call is a supported
API that shouldn't need to be limited.

[stefan-webrtc, 2016-10-04 14:47:19]

On 2016/09/29 18:10:18, skvlad wrote:
> On 2016/09/29 10:56:52, the sun wrote:
> > On 2016/09/29 07:56:24, stefan-webrtc (holmer) wrote:
> > > Personally, I prefer the null implementation over the nullptr checks. I
see
> > that
> > > you looked into this and that it was a lot of boilerplate for the tests,
but
> > > could it be done if you created a null object in, e.g., BaseTest in
> > call_test.h?
> > > Most of the tests you're running into inherit from that.
> > > 
> > > https://codereview.webrtc.org/2353033005/diff/200001/webrtc/call/call.cc
> > > File webrtc/call/call.cc (right):
> > > 
> > >
> >
>
https://codereview.webrtc.org/2353033005/diff/200001/webrtc/call/call.cc#newc...
> > > webrtc/call/call.cc:454: if (event_log_) {
> > > Remove {} here and below since it's a two line if.
> > 
> > I agree with Stefan about the null pointer checks. Think about it from a
> testing
> > perspective - if you allow the pointer to be null, with the associated
runtime
> > logic, you introduce ambiguity and must increase the "area" that your tests
> > cover. The reason for the null implementation of RtcEventLog was to make the
> > calling code simpler.
> 
> Agree that null objects make it easier on the user's code, and my own
preference
> is to use them whenever possible (especially after having used primarily
> garbage-collected languages for many years.) However, it appears to not result
> in a net benefit in this particular case:
>  1. RtcEventLog is an interface that is implemented multiple times
> (RtcEventLogImpl, RtcEventLogNullImpl, RtcEventLogProxy), but most methods
will
> only be called from one point. This means we trade one method in the null
> implementation for one if statement elsewhere, which won't benefit readability
> much.
>  2. Managing the lifetime of the null implementation is tricky, especially
when
> there are multiple places where an RtcEventLog needs to be created
> (PeerConnection/Call, VoEChannel, and about 6-7 top level unit test classes).
> I've tried to use a static global instance, but that goes against the Chromium
> rules of not having static initializers (the interface declares a destructor,
> which needs to be registered with AtExit). I've tried creating owned instances
> everywhere we need to, but this required changing a dozen or so files, and
> reduced readability: we can no longer use the default constructor for
> Call::Config, so its creation has to go into the constructor body, a few const
> fields have to be changed to non-const, etc. - the change has a ripple effect
> way beyond the benefit it gives. This problem doesn't exist in Java or C#, so
> null objects are much more popular there.
>  3. The null object is not a common pattern in WebRTC, so developers don't
seem
> to be famililar with it - in fact most users of RtcEventLog are already
checking
> it for null before calling, negating the benefit of reducing the number of
> branches. Even RtcEventLogProxy is not using a null instance internally, but
it
> has a null check around each call instead - most likely because of issue #2,
> lifetime management. I think I had to add the if check to only 2-3 call sites.

I looked at test changes in patch set 2, and I liked that better than the latest
patch set. If you also instantiate RtcEventLogNullImpl instead of a real
RtcEventLog you don't have to mention it in the initializer lists of your tests
either.

You may have to do a similar thing in BaseTest in call_tests.h as well, but it
shouldn't be many lines there either.

Is there more to it that makes you think it's becoming too ugly? I expect
RtcEventLog to be used more and more in webrtc, and as it does we'd end up
adding more and more if statements...

> 
> So in summary - I also wanted to use the null object, and spent about a day
> trying different approaches to manage it. In the end I found out none of them
> resulted in clearer code than simply checking for null. If you have specific
> ideas for implementing it - feel free to suggest them.
> 
> Do you have other feedback on the change, other than the use of null objects?

The rest looks good!

[terelius, 2016-10-04 15:04:32]

On 2016/10/04 14:47:19, stefan-webrtc (holmer) wrote:
> On 2016/09/29 18:10:18, skvlad wrote:
> > 
> > Do you have other feedback on the change, other than the use of null
objects?
> 
> The rest looks good!

I was about to say the same.

I think it looks good apart from the nullptr vs NullImpl issue.
(I'm on both sides of the fence when it comes to that issue, but
it'll be good to make a policy decision. If we go with the NullImpl
option and no nullptr checks at the call sites, then I'd like to
DCHECK the pointer in each constructor.)

[skvlad, 2016-10-05 03:03:40]

On 2016/10/04 14:47:19, stefan-webrtc (holmer) wrote:

> I looked at test changes in patch set 2, and I liked that better than the
latest
> patch set. If you also instantiate RtcEventLogNullImpl instead of a real
> RtcEventLog you don't have to mention it in the initializer lists of your
tests
> either.

Patch set 2 did not include fixes for Call unit tests, that is why it looked
much cleaner. This was my preferred approach too until I realized the extent of
the test changes.

> You may have to do a similar thing in BaseTest in call_tests.h as well, but it
> shouldn't be many lines there either.
> 
> Is there more to it that makes you think it's becoming too ugly? I expect
> RtcEventLog to be used more and more in webrtc, and as it does we'd end up
> adding more and more if statements...

I have uploaded a patch set that should take care of (hopefully) all the tests,
unless the bots find some more. As you can see, it's a lot more extensive than
just a few lines here and there, and touches almost every directory - because so
many unit tests find it convenient to create a Call directly.
PTAL and let me know if this is what you had in mind.

[stefan-webrtc, 2016-10-05 07:35:22]

https://codereview.webrtc.org/2353033005/diff/220001/webrtc/call/bitrate_esti...
File webrtc/call/bitrate_estimator_tests.cc (right):

https://codereview.webrtc.org/2353033005/diff/220001/webrtc/call/bitrate_esti...
webrtc/call/bitrate_estimator_tests.cc:256: webrtc::RtcEventLogNullImpl
event_log_;
This is a CallTest and should already have the event log.

https://codereview.webrtc.org/2353033005/diff/220001/webrtc/call/packet_injec...
File webrtc/call/packet_injection_tests.cc (right):

https://codereview.webrtc.org/2353033005/diff/220001/webrtc/call/packet_injec...
webrtc/call/packet_injection_tests.cc:41: webrtc::RtcEventLogNullImpl event_log;
This is a CallTest, so you already have an event_log_.

https://codereview.webrtc.org/2353033005/diff/220001/webrtc/test/call_test.h
File webrtc/test/call_test.h (right):

https://codereview.webrtc.org/2353033005/diff/220001/webrtc/test/call_test.h#...
webrtc/test/call_test.h:185: webrtc::RtcEventLogNullImpl event_log_;
Seems a bit odd that we need this both in BaseTest and CallTest. I think the
right thing to do would be to remove this line and change CallTest so that it
passes an RtcEventLog* into GetSenderCallConfig() and GetReceiverCallConfig().
WDYT?

An alternative would be to have CallTest always change the Call::Configs it gets
back and assign it the null event_log_. Maybe it could do that only if the
returned Call:Config has a nullptr assigned to its event_log? That'd remove all
the boilerplate you had to add to tests inheriting from BaseTest.

https://codereview.webrtc.org/2353033005/diff/220001/webrtc/video/end_to_end_...
File webrtc/video/end_to_end_tests.cc (right):

https://codereview.webrtc.org/2353033005/diff/220001/webrtc/video/end_to_end_...
webrtc/video/end_to_end_tests.cc:137: CreateCalls(config, config);
What do you think of having a constructor of Call::Config which takes an event
log so that we can do:

CreateCalls(Call::Config(&event_log_), Call::Config(&event_log_));

?

[skvlad, 2016-10-06 01:31:38]

https://codereview.webrtc.org/2353033005/diff/200001/webrtc/api/DEPS
File webrtc/api/DEPS (right):

https://codereview.webrtc.org/2353033005/diff/200001/webrtc/api/DEPS#newcode21
webrtc/api/DEPS:21: "+webrtc/call/rtc_event_log.h",
On 2016/10/04 13:22:30, kjellander_webrtc wrote:
> I think "+webrtc/call" should be allowed for all sources in webrtc/api.
> I'm not sure why the special cases exist for the entries below. Maybe it's
> because they're undesired dependencies... either way webrtc/call is a
supported
> API that shouldn't need to be limited.

Done.

https://codereview.webrtc.org/2353033005/diff/200001/webrtc/call/call.cc
File webrtc/call/call.cc (right):

https://codereview.webrtc.org/2353033005/diff/200001/webrtc/call/call.cc#newc...
webrtc/call/call.cc:454: if (event_log_) {
On 2016/09/29 07:56:24, stefan-webrtc (holmer) wrote:
> Remove {} here and below since it's a two line if.

Done.

https://codereview.webrtc.org/2353033005/diff/220001/webrtc/call/bitrate_esti...
File webrtc/call/bitrate_estimator_tests.cc (right):

https://codereview.webrtc.org/2353033005/diff/220001/webrtc/call/bitrate_esti...
webrtc/call/bitrate_estimator_tests.cc:256: webrtc::RtcEventLogNullImpl
event_log_;
On 2016/10/05 07:35:22, stefan-webrtc (holmer) wrote:
> This is a CallTest and should already have the event log.

Done.

https://codereview.webrtc.org/2353033005/diff/220001/webrtc/call/packet_injec...
File webrtc/call/packet_injection_tests.cc (right):

https://codereview.webrtc.org/2353033005/diff/220001/webrtc/call/packet_injec...
webrtc/call/packet_injection_tests.cc:41: webrtc::RtcEventLogNullImpl event_log;
On 2016/10/05 07:35:22, stefan-webrtc (holmer) wrote:
> This is a CallTest, so you already have an event_log_.

Removed from here and all other CallTest subclasses.

https://codereview.webrtc.org/2353033005/diff/220001/webrtc/test/call_test.h
File webrtc/test/call_test.h (right):

https://codereview.webrtc.org/2353033005/diff/220001/webrtc/test/call_test.h#...
webrtc/test/call_test.h:185: webrtc::RtcEventLogNullImpl event_log_;
On 2016/10/05 07:35:22, stefan-webrtc (holmer) wrote:
> Seems a bit odd that we need this both in BaseTest and CallTest. I think the
> right thing to do would be to remove this line and change CallTest so that it
> passes an RtcEventLog* into GetSenderCallConfig() and GetReceiverCallConfig().
> WDYT?
> 
> An alternative would be to have CallTest always change the Call::Configs it
gets
> back and assign it the null event_log_. Maybe it could do that only if the
> returned Call:Config has a nullptr assigned to its event_log? That'd remove
all
> the boilerplate you had to add to tests inheriting from BaseTest.
This would make the tests even more tricky to follow, as you need to be
constantly aware that the null log is substituted for you in some tests but not
in others. It will also make it impossible to require an RtcEventLog in the
config constructor.

https://codereview.webrtc.org/2353033005/diff/220001/webrtc/video/end_to_end_...
File webrtc/video/end_to_end_tests.cc (right):

https://codereview.webrtc.org/2353033005/diff/220001/webrtc/video/end_to_end_...
webrtc/video/end_to_end_tests.cc:137: CreateCalls(config, config);
On 2016/10/05 07:35:22, stefan-webrtc (holmer) wrote:
> What do you think of having a constructor of Call::Config which takes an event
> log so that we can do:
> 
> CreateCalls(Call::Config(&event_log_), Call::Config(&event_log_));
> 
> ?
I think this would be adding test-only methods to production code, which is not
desirable. But then again, I also think that doing these extensive changes just
to get rid of ~5 "if" statements is not worth the time and effort.
I've added the constructor and started using it. Since we're already at it, I
made it the only available constructor and added a DCHECK to make sure it's not
null.

[stefan-webrtc, 2016-10-06 08:53:14]

https://codereview.webrtc.org/2353033005/diff/220001/webrtc/test/call_test.h
File webrtc/test/call_test.h (right):

https://codereview.webrtc.org/2353033005/diff/220001/webrtc/test/call_test.h#...
webrtc/test/call_test.h:185: webrtc::RtcEventLogNullImpl event_log_;
On 2016/10/06 01:31:38, skvlad wrote:
> On 2016/10/05 07:35:22, stefan-webrtc (holmer) wrote:
> > Seems a bit odd that we need this both in BaseTest and CallTest. I think the
> > right thing to do would be to remove this line and change CallTest so that
it
> > passes an RtcEventLog* into GetSenderCallConfig() and
GetReceiverCallConfig().
> > WDYT?
> > 
> > An alternative would be to have CallTest always change the Call::Configs it
> gets
> > back and assign it the null event_log_. Maybe it could do that only if the
> > returned Call:Config has a nullptr assigned to its event_log? That'd remove
> all
> > the boilerplate you had to add to tests inheriting from BaseTest.
> This would make the tests even more tricky to follow, as you need to be
> constantly aware that the null log is substituted for you in some tests but
not
> in others. It will also make it impossible to require an RtcEventLog in the
> config constructor.

Agree, it's far from ideal.

I'm still not sure about having this event_log_ in both CallTest and BaseTest,
but I think that's an artifact of some tests only use CallTest while others are
based on BaseTest. I think ideally all those tests should be based on
BaseTest...

WDYT of passing event_log into GetSenderCallConfig() and GetReceiverCallConfig()
instead of having it as a member in BaseTest? I don't feel very strongly about
it, so I'm fine as is too.

https://codereview.webrtc.org/2353033005/diff/240001/webrtc/call.h
File webrtc/call.h (right):

https://codereview.webrtc.org/2353033005/diff/240001/webrtc/call.h#newcode100
webrtc/call.h:100: }
Move this to the top as methods and ctors usually go before members.

[stefan-webrtc, 2016-10-06 08:55:53]

https://codereview.webrtc.org/2353033005/diff/220001/webrtc/video/end_to_end_...
File webrtc/video/end_to_end_tests.cc (right):

https://codereview.webrtc.org/2353033005/diff/220001/webrtc/video/end_to_end_...
webrtc/video/end_to_end_tests.cc:137: CreateCalls(config, config);
On 2016/10/06 01:31:38, skvlad wrote:
> On 2016/10/05 07:35:22, stefan-webrtc (holmer) wrote:
> > What do you think of having a constructor of Call::Config which takes an
event
> > log so that we can do:
> > 
> > CreateCalls(Call::Config(&event_log_), Call::Config(&event_log_));
> > 
> > ?
> I think this would be adding test-only methods to production code, which is
not
> desirable. But then again, I also think that doing these extensive changes
just
> to get rid of ~5 "if" statements is not worth the time and effort.
> I've added the constructor and started using it. Since we're already at it, I
> made it the only available constructor and added a DCHECK to make sure it's
not
> null.

If you prefer, you could have a helper method which does this instead:

Call::Config config = CreateConfigWithEventLog(event_log_);
CreateCalls(config, config);

It has the downside of not being able to DCHECK though.

[stefan-webrtc, 2016-10-06 09:18:06]

https://codereview.webrtc.org/2353033005/diff/220001/webrtc/video/end_to_end_...
File webrtc/video/end_to_end_tests.cc (right):

https://codereview.webrtc.org/2353033005/diff/220001/webrtc/video/end_to_end_...
webrtc/video/end_to_end_tests.cc:137: CreateCalls(config, config);
On 2016/10/06 08:55:53, stefan-webrtc (holmer) wrote:
> On 2016/10/06 01:31:38, skvlad wrote:
> > On 2016/10/05 07:35:22, stefan-webrtc (holmer) wrote:
> > > What do you think of having a constructor of Call::Config which takes an
> event
> > > log so that we can do:
> > > 
> > > CreateCalls(Call::Config(&event_log_), Call::Config(&event_log_));
> > > 
> > > ?
> > I think this would be adding test-only methods to production code, which is
> not
> > desirable. But then again, I also think that doing these extensive changes
> just
> > to get rid of ~5 "if" statements is not worth the time and effort.
> > I've added the constructor and started using it. Since we're already at it,
I
> > made it the only available constructor and added a DCHECK to make sure it's
> not
> > null.
> 
> If you prefer, you could have a helper method which does this instead:
> 
> Call::Config config = CreateConfigWithEventLog(event_log_);
> CreateCalls(config, config);
> 
> It has the downside of not being able to DCHECK though.

terelius pointed out that DCHECK is still possible, although we'd put it in the
Call ctor instead of the Call::Config ctor. Maybe that's a better option then?

I'm fine with both solutions.

[terelius, 2016-10-06 14:20:49]

lgtm

If we really want to construct configs without having to specify an event log,
we could consider something like this:

RtcEventLog* GetNullRtcEventLog() {
  static RtcEventLogNullImpl null_event_log;
  return &null_event_log;
}

Config::Config() {
  event_log = GetNullRtcEventLog();
}

Of course, this requires that the event log only is used after static
initialization and before static destruction. (Typically, this means between
entry and exit from main().)

[skvlad, 2016-10-07 01:20:48]

On 2016/10/06 14:20:49, terelius wrote:
> lgtm
> 
> If we really want to construct configs without having to specify an event log,
> we could consider something like this:
> 
> RtcEventLog* GetNullRtcEventLog() {
>   static RtcEventLogNullImpl null_event_log;
>   return &null_event_log;
> }
> 
> Config::Config() {
>   event_log = GetNullRtcEventLog();
> }
> 
> Of course, this requires that the event log only is used after static
> initialization and before static destruction. (Typically, this means between
> entry and exit from main().)

Static locals are not thread safe in Chrome (unlike standard C++): 
https://www.chromium.org/developers/coding-style/cpp-dos-and-donts#TOC-Static...
https://groups.google.com/a/chromium.org/forum/#!msg/chromium-dev/p6h3HC8Wro4...

[skvlad, 2016-10-07 01:35:04]

Is it possible to agree on at least somewhat acceptable implementation (be it
with the null checks, null objects or something else), and create a bug to make
it really optimal?

I've been trying to add a bunch of network events to RtcEventLogs, and for about
2 weeks now we've been going back and forth on what seems to be just personal
preferences. Can we all agree to check in some version of the change that
satisfies some minimal quality standards, so that we can move ahead with the
actual implementation? I'm sure everyone involved in this discussion has more
important things to do than to spend two weeks on what's effectively moving a
single field to a different class, or reviewing the changes necessary to do so.
If you have specific preferences about how something should be done, you are
welcome to send a separate CL making the necessary changes.

https://codereview.webrtc.org/2353033005/diff/220001/webrtc/video/end_to_end_...
File webrtc/video/end_to_end_tests.cc (right):

https://codereview.webrtc.org/2353033005/diff/220001/webrtc/video/end_to_end_...
webrtc/video/end_to_end_tests.cc:137: CreateCalls(config, config);
On 2016/10/06 08:55:53, stefan-webrtc (holmer) wrote:
> On 2016/10/06 01:31:38, skvlad wrote:
> > On 2016/10/05 07:35:22, stefan-webrtc (holmer) wrote:
> > > What do you think of having a constructor of Call::Config which takes an
> event
> > > log so that we can do:
> > > 
> > > CreateCalls(Call::Config(&event_log_), Call::Config(&event_log_));
> > > 
> > > ?
> > I think this would be adding test-only methods to production code, which is
> not
> > desirable. But then again, I also think that doing these extensive changes
> just
> > to get rid of ~5 "if" statements is not worth the time and effort.
> > I've added the constructor and started using it. Since we're already at it,
I
> > made it the only available constructor and added a DCHECK to make sure it's
> not
> > null.
> 
> If you prefer, you could have a helper method which does this instead:
> 
> Call::Config config = CreateConfigWithEventLog(event_log_);
> CreateCalls(config, config);
> 
> It has the downside of not being able to DCHECK though.

This was the approach I took in patch set #10. The helper method was defined in
fakewebrtccall.h.

https://codereview.webrtc.org/2353033005/diff/240001/webrtc/call.h
File webrtc/call.h (right):

https://codereview.webrtc.org/2353033005/diff/240001/webrtc/call.h#newcode100
webrtc/call.h:100: }
On 2016/10/06 08:53:14, stefan-webrtc (holmer) wrote:
> Move this to the top as methods and ctors usually go before members.

Done.

[terelius, 2016-10-07 08:07:01]

On 2016/10/07 01:20:48, skvlad wrote:
> On 2016/10/06 14:20:49, terelius wrote:
> > lgtm
> > 
> > If we really want to construct configs without having to specify an event
log,
> > we could consider something like this:
> > 
> > RtcEventLog* GetNullRtcEventLog() {
> >   static RtcEventLogNullImpl null_event_log;
> >   return &null_event_log;
> > }
> > 
> > Config::Config() {
> >   event_log = GetNullRtcEventLog();
> > }
> > 
> > Of course, this requires that the event log only is used after static
> > initialization and before static destruction. (Typically, this means between
> > entry and exit from main().)
> 
> Static locals are not thread safe in Chrome (unlike standard C++): 
>
https://www.chromium.org/developers/coding-style/cpp-dos-and-donts#TOC-Static...
>
https://groups.google.com/a/chromium.org/forum/#!msg/chromium-dev/p6h3HC8Wro4...

Ack.

[stefan-webrtc, 2016-10-07 08:38:05]

Didn't mean to delay this unnecessarily. The current solution is totally fine
with me, but you didn't seem happy with it, which is why I made some other
suggestions.

lgtm

On 2016/10/07 01:35:04, skvlad wrote:
> Is it possible to agree on at least somewhat acceptable implementation (be it
> with the null checks, null objects or something else), and create a bug to
make
> it really optimal?
> 
> I've been trying to add a bunch of network events to RtcEventLogs, and for
about
> 2 weeks now we've been going back and forth on what seems to be just personal
> preferences. Can we all agree to check in some version of the change that
> satisfies some minimal quality standards, so that we can move ahead with the
> actual implementation? I'm sure everyone involved in this discussion has more
> important things to do than to spend two weeks on what's effectively moving a
> single field to a different class, or reviewing the changes necessary to do
so.
> If you have specific preferences about how something should be done, you are
> welcome to send a separate CL making the necessary changes.
> 
>
https://codereview.webrtc.org/2353033005/diff/220001/webrtc/video/end_to_end_...
> File webrtc/video/end_to_end_tests.cc (right):
> 
>
https://codereview.webrtc.org/2353033005/diff/220001/webrtc/video/end_to_end_...
> webrtc/video/end_to_end_tests.cc:137: CreateCalls(config, config);
> On 2016/10/06 08:55:53, stefan-webrtc (holmer) wrote:
> > On 2016/10/06 01:31:38, skvlad wrote:
> > > On 2016/10/05 07:35:22, stefan-webrtc (holmer) wrote:
> > > > What do you think of having a constructor of Call::Config which takes an
> > event
> > > > log so that we can do:
> > > > 
> > > > CreateCalls(Call::Config(&event_log_), Call::Config(&event_log_));
> > > > 
> > > > ?
> > > I think this would be adding test-only methods to production code, which
is
> > not
> > > desirable. But then again, I also think that doing these extensive changes
> > just
> > > to get rid of ~5 "if" statements is not worth the time and effort.
> > > I've added the constructor and started using it. Since we're already at
it,
> I
> > > made it the only available constructor and added a DCHECK to make sure
it's
> > not
> > > null.
> > 
> > If you prefer, you could have a helper method which does this instead:
> > 
> > Call::Config config = CreateConfigWithEventLog(event_log_);
> > CreateCalls(config, config);
> > 
> > It has the downside of not being able to DCHECK though.
> 
> This was the approach I took in patch set #10. The helper method was defined
in
> fakewebrtccall.h.
> 
> https://codereview.webrtc.org/2353033005/diff/240001/webrtc/call.h
> File webrtc/call.h (right):
> 
> https://codereview.webrtc.org/2353033005/diff/240001/webrtc/call.h#newcode100
> webrtc/call.h:100: }
> On 2016/10/06 08:53:14, stefan-webrtc (holmer) wrote:
> > Move this to the top as methods and ctors usually go before members.
> 
> Done.

[stefan-webrtc, 2016-10-07 08:39:07]

Remember to update the description before you land.

[skvlad, 2016-10-07 18:04:23]

Description was changed from

==========
Refactoring: move ownership of RtcEventLog from Call to PeerConnection

This CL is a pure refactoring which should not result in any functinal
changes. It moves ownership of the RtcEventLog from webrtc::Call to the
webrtc::PeerConnection object.

This is done so that we can add RtcEventLog support for ICE events -
which will require the TransportController to have a pointer to the
RtcEventLog. PeerConnection is the closest common owner of both Call and
TransportController (through WebRtcSession).

As part of this refactoring, I've removed the null object implementation
(RtcEventLogNullImpl) and replaced it with explicit null checks at the
point of use, as the difficulty of managing the lifetime of the null
implementation in tests was outweighing the benefits it provided.

BUG=webrtc:6393
==========

to

==========
Refactoring: move ownership of RtcEventLog from Call to PeerConnection

This CL is a pure refactoring which should not result in any functinal
changes. It moves ownership of the RtcEventLog from webrtc::Call to the
webrtc::PeerConnection object.

This is done so that we can add RtcEventLog support for ICE events -
which will require the TransportController to have a pointer to the
RtcEventLog. PeerConnection is the closest common owner of both Call and
TransportController (through WebRtcSession).

BUG=webrtc:6393
==========

[skvlad, 2016-10-07 18:04:36]

The CQ bit was checked by skvlad@webrtc.org

[skvlad, 2016-10-07 18:04:36]

The patchset sent to the CQ was uploaded after l-g-t-m from honghaiz@webrtc.org,
terelius@webrtc.org
Link to the patchset: https://codereview.webrtc.org/2353033005/#ps260001 (title:
"Moved the constructor")

[commit-bot: I haz the power, 2016-10-07 18:04:50]

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, 2016-10-07 18:53:10]

Description was changed from

==========
Refactoring: move ownership of RtcEventLog from Call to PeerConnection

This CL is a pure refactoring which should not result in any functinal
changes. It moves ownership of the RtcEventLog from webrtc::Call to the
webrtc::PeerConnection object.

This is done so that we can add RtcEventLog support for ICE events -
which will require the TransportController to have a pointer to the
RtcEventLog. PeerConnection is the closest common owner of both Call and
TransportController (through WebRtcSession).

BUG=webrtc:6393
==========

to

==========
Refactoring: move ownership of RtcEventLog from Call to PeerConnection

This CL is a pure refactoring which should not result in any functinal
changes. It moves ownership of the RtcEventLog from webrtc::Call to the
webrtc::PeerConnection object.

This is done so that we can add RtcEventLog support for ICE events -
which will require the TransportController to have a pointer to the
RtcEventLog. PeerConnection is the closest common owner of both Call and
TransportController (through WebRtcSession).

BUG=webrtc:6393
==========

[commit-bot: I haz the power, 2016-10-07 18:53:11]

Committed patchset #12 (id:260001)

[commit-bot: I haz the power, 2016-10-07 18:53:19]

Description was changed from

==========
Refactoring: move ownership of RtcEventLog from Call to PeerConnection

This CL is a pure refactoring which should not result in any functinal
changes. It moves ownership of the RtcEventLog from webrtc::Call to the
webrtc::PeerConnection object.

This is done so that we can add RtcEventLog support for ICE events -
which will require the TransportController to have a pointer to the
RtcEventLog. PeerConnection is the closest common owner of both Call and
TransportController (through WebRtcSession).

BUG=webrtc:6393
==========

to

==========
Refactoring: move ownership of RtcEventLog from Call to PeerConnection

This CL is a pure refactoring which should not result in any functinal
changes. It moves ownership of the RtcEventLog from webrtc::Call to the
webrtc::PeerConnection object.

This is done so that we can add RtcEventLog support for ICE events -
which will require the TransportController to have a pointer to the
RtcEventLog. PeerConnection is the closest common owner of both Call and
TransportController (through WebRtcSession).

BUG=webrtc:6393

Committed: https://crrev.com/11a9cbfa50631f9ca3e61f3f9d993450386983c1
Cr-Commit-Position: refs/heads/master@{#14578}
==========

[commit-bot: I haz the power, 2016-10-07 18:53:21]

Patchset 12 (id:??) landed as
https://crrev.com/11a9cbfa50631f9ca3e61f3f9d993450386983c1
Cr-Commit-Position: refs/heads/master@{#14578}

[the sun, 2016-10-13 08:06:32]

On 2016/10/07 18:53:21, commit-bot: I haz the power wrote:
> Patchset 12 (id:??) landed as
> https://crrev.com/11a9cbfa50631f9ca3e61f3f9d993450386983c1
> Cr-Commit-Position: refs/heads/master@{#14578}

Sorry, dropped the ball and missed your ping. The solution lgtm!