Moving webrtc/stats/ into webrtc/api/ to resolve a circular dependency

Moving webrtc/stats/ into webrtc/api/ to resolve a circular dependency
introduced when adding PeerConnection[Interface]::GetRTCStats.

BUG=chromium:627816

[hbos, 2016-09-12 18:28:08]

hbos@webrtc.org changed reviewers:
+ kjellander@webrtc.org, nisse@webrtc.org

[hbos, 2016-09-12 18:28:09]

Please take a look kjellander and nisse. Trying to avoid a circular dependency
which is blocking me.

I don't like merging these two targets, but I don't see a clean alternative
solution. I hope I'm missing something?

We have RTCStatsCollector depending on everything in webrtc/stats/, and also
depending on PeerConnection (webrtc/api/) from where it is gathering stats.
Wanting PeerConnection[Interface] to have a GetRTCStats method (which would
invoke RTCStatsCollector to get stats), this introduces a dependency from
PeerConnection back to RTCStatsCollector resulting in a circular dependency.
As such, I merged the two targets.

Alternatives considered:

1. Make PeerConnection depend on an RTCStatsCollectorInterface, not the actual
implementation. We injecting the RTCStatsCollector implementation on or after
initializing the PeerConnection, in a different target.

Problems:
- Initialize is called from the factory, which is part of the same target as
  PeerConnection. Creating and injecting the implementation here is still a
  circular dependency.
- Wiring up the collector from outside code is not nice because it is then not
  fully initialized when returned from the factory.
- RTCStatsCollector depends on PeerConnection*, not PeerConnectionInterface*[1].

2. Skip adding PeerConnection::GetRTCStats. All it does is invoke an
RTCStatsCollector, so when you want stats, use your own standalone
RTCStatsCollector instead. This is less spec-like and you'd have to watch out
not to have multiple collectors for the same PeerConnection but should work.

Problems:
- The RTCStatsCollector depends on PeerConnection*, not
  PeerConnectionInterface*[1].

Problem with [1]: Most references to a peer connection are
PeerConnectionInterface* pointers (to PeerConnectionProxy objects which
reference a PeerConnection). We'd have to reverse the abstraction and dig out
PeerConnection*. Alternatively we would have to surface PeerConnection
implementation details to the PeerConnectionInterface so that RTCStatsCollector
can rely on the interface instead.

[hbos, 2016-09-12 18:31:08]

PS: Red bots and a new unittest for GetRTCStats will be addressed but first I
want to settle on if merging this target is the right thing to do or not.

[nisse-webrtc, 2016-09-13 07:47:04]

On 2016/09/12 18:28:09, hbos wrote:
> Please take a look kjellander and nisse. Trying to avoid a circular dependency
> which is blocking me.

To be clear, you're talking a bout a link dependency, not a include dependency?

If the new stats is an advertised feature of a PeerConnection, then it makes a
lot of sense to me to include it in the libjingle_peerconnection library. And
that's what you're doing, right?

[hbos, 2016-09-13 07:53:39]

On 2016/09/13 07:47:04, nisse-webrtc wrote:
> On 2016/09/12 18:28:09, hbos wrote:
> > Please take a look kjellander and nisse. Trying to avoid a circular
dependency
> > which is blocking me.
> 
> To be clear, you're talking a bout a link dependency, not a include
dependency?
> 
> If the new stats is an advertised feature of a PeerConnection, then it makes a
> lot of sense to me to include it in the libjingle_peerconnection library. And
> that's what you're doing, right?

Yes, a link dependency. And yes, the stats collection is a feature of a
PeerConnection.

[nisse-webrtc, 2016-09-13 08:16:40]

I'm afraid I've not understood the rules for what belongs in api/.

If we view the stats classes as an implementation detail, I guess it would be
preferable to keep them out of api. But we'd still need to arrange that the
libjingle_peerconnection library includes the corresponding object files from
stats/, preferably via a dependency. But I know too little about gyp and gn to
say what's the right arrangement.

https://codereview.webrtc.org/2335553006/diff/1/webrtc/api/peerconnection.cc
File webrtc/api/peerconnection.cc (right):

https://codereview.webrtc.org/2335553006/diff/1/webrtc/api/peerconnection.cc#...
webrtc/api/peerconnection.cc:894: RTC_DCHECK(stats_collector_);
Does stats_collector_ have to be a member, created above in Initialize? Is the
callback invoked asynchronously, or can you create a temporary stats collector
just for the duration of this call?

https://codereview.webrtc.org/2335553006/diff/1/webrtc/api/peerconnection.h
File webrtc/api/peerconnection.h (right):

https://codereview.webrtc.org/2335553006/diff/1/webrtc/api/peerconnection.h#n...
webrtc/api/peerconnection.h:107: const
rtc::scoped_refptr<RTCStatsCollectorCallback>& callback) override;
Passing references to "smart pointers" is no longer in accordance with the
coding standard. https://www.chromium.org/developers/smart-pointer-guidelines.

https://codereview.webrtc.org/2335553006/diff/1/webrtc/api/peerconnectioninte...
File webrtc/api/peerconnectioninterface.h (right):

https://codereview.webrtc.org/2335553006/diff/1/webrtc/api/peerconnectioninte...
webrtc/api/peerconnectioninterface.h:445: const
rtc::scoped_refptr<RTCStatsCollectorCallback>& callback) = 0;
Ditto, no references to smart pointers.

https://codereview.webrtc.org/2335553006/diff/1/webrtc/api/rtcstatscollector.cc
File webrtc/api/rtcstatscollector.cc (right):

https://codereview.webrtc.org/2335553006/diff/1/webrtc/api/rtcstatscollector....
webrtc/api/rtcstatscollector.cc:10: 
Hmm, is this the move you're concerned about?

I've never quite understood our directory structure, hope kjellander can say if
this is right or not.

[hbos, 2016-09-13 08:50:38]

On 2016/09/13 08:16:40, nisse-webrtc wrote:
> I'm afraid I've not understood the rules for what belongs in api/.
> 
> If we view the stats classes as an implementation detail, I guess it would be
> preferable to keep them out of api. But we'd still need to arrange that the
> libjingle_peerconnection library includes the corresponding object files from
> stats/, preferably via a dependency. But I know too little about gyp and gn to
> say what's the right arrangement.

I view the stats collector as an implementation detail of PeerConnection, but
the stats callback, stats report and stats are all public api.

https://codereview.webrtc.org/2335553006/diff/1/webrtc/api/peerconnection.cc
File webrtc/api/peerconnection.cc (right):

https://codereview.webrtc.org/2335553006/diff/1/webrtc/api/peerconnection.cc#...
webrtc/api/peerconnection.cc:894: RTC_DCHECK(stats_collector_);
On 2016/09/13 08:16:40, nisse-webrtc wrote:
> Does stats_collector_ have to be a member, created above in Initialize? Is the
> callback invoked asynchronously, or can you create a temporary stats collector
> just for the duration of this call?

You could create it temporary for just a call and it would work but then you
don't get the benefits of caching.

https://codereview.webrtc.org/2335553006/diff/1/webrtc/api/rtcstatscollector.cc
File webrtc/api/rtcstatscollector.cc (right):

https://codereview.webrtc.org/2335553006/diff/1/webrtc/api/rtcstatscollector....
webrtc/api/rtcstatscollector.cc:10: 
On 2016/09/13 08:16:40, nisse-webrtc wrote:
> Hmm, is this the move you're concerned about?
> 
> I've never quite understood our directory structure, hope kjellander can say
if
> this is right or not.

Yes, the stats -> api move (of not just that file but everything under
webrtc/stats/),
There's a doc[1] describing wanting to split up webrtc in smaller modules.
Putting stats in a separate folder and module made sense but then I hit this
dependency problem. Doing this move is going against splitting things up into
modules.

[1]
https://docs.google.com/document/d/1ZhugZLVk6ptbujZsnnpXUwqZS97L-gKM5VD9lGK_p...

[hbos, 2016-09-13 09:09:26]

hbos@webrtc.org changed reviewers:
+ hta@webrtc.org

[hbos, 2016-09-13 09:09:26]

+hta

On 2016/09/13 08:50:38, hbos wrote:
> On 2016/09/13 08:16:40, nisse-webrtc wrote:
> > I'm afraid I've not understood the rules for what belongs in api/.
> > 
> > If we view the stats classes as an implementation detail, I guess it would
be
> > preferable to keep them out of api. But we'd still need to arrange that the
> > libjingle_peerconnection library includes the corresponding object files
from
> > stats/, preferably via a dependency. But I know too little about gyp and gn
to
> > say what's the right arrangement.
> 
> I view the stats collector as an implementation detail of PeerConnection, but
> the stats callback, stats report and stats are all public api.

Maybe I can put rtcstatsreport.h/cc, rtcstats.h/cc and rtcstats_objects.h/cc in
target rtc_stats,
and rtcstatscollector.h/cc in target libjingle_peerconnection. The dependency
being just libjingle_peerconnection -> rtc_stats.

[hbos, 2016-09-13 09:29:45]

On 2016/09/13 09:09:26, hbos wrote:
> +hta
> 
> On 2016/09/13 08:50:38, hbos wrote:
> > On 2016/09/13 08:16:40, nisse-webrtc wrote:
> > > I'm afraid I've not understood the rules for what belongs in api/.
> > > 
> > > If we view the stats classes as an implementation detail, I guess it would
> be
> > > preferable to keep them out of api. But we'd still need to arrange that
the
> > > libjingle_peerconnection library includes the corresponding object files
> from
> > > stats/, preferably via a dependency. But I know too little about gyp and
gn
> to
> > > say what's the right arrangement.
> > 
> > I view the stats collector as an implementation detail of PeerConnection,
but
> > the stats callback, stats report and stats are all public api.
> 
> Maybe I can put rtcstatsreport.h/cc, rtcstats.h/cc and rtcstats_objects.h/cc
in
> target rtc_stats,
> and rtcstatscollector.h/cc in target libjingle_peerconnection. The dependency
> being just libjingle_peerconnection -> rtc_stats.

Started working on a CL like that.

[hbos, 2016-09-13 10:51:04]

On 2016/09/13 09:29:45, hbos wrote:
> Started working on a CL like that.

https://codereview.webrtc.org/2335553006/

[hbos, 2016-09-13 10:51:43]

On 2016/09/13 10:51:04, hbos wrote:
> On 2016/09/13 09:29:45, hbos wrote:
> > Started working on a CL like that.
> 
> https://codereview.webrtc.org/2335553006/

I meant https://codereview.webrtc.org/2331373004/

[kjellander_webrtc, 2016-09-15 10:18:04]

On 2016/09/13 10:51:43, hbos wrote:
> On 2016/09/13 10:51:04, hbos wrote:
> > On 2016/09/13 09:29:45, hbos wrote:
> > > Started working on a CL like that.
> > 
> > https://codereview.webrtc.org/2335553006/
> 
> I meant https://codereview.webrtc.org/2331373004/

will this CL be relevant after https://codereview.webrtc.org/2331373004/ is
landed?

[hbos, 2016-09-15 12:10:38]

On 2016/09/15 10:18:04, kjellander_webrtc wrote:
> On 2016/09/13 10:51:43, hbos wrote:
> > On 2016/09/13 10:51:04, hbos wrote:
> > > On 2016/09/13 09:29:45, hbos wrote:
> > > > Started working on a CL like that.
> > > 
> > > https://codereview.webrtc.org/2335553006/
> > 
> > I meant https://codereview.webrtc.org/2331373004/
> 
> will this CL be relevant after https://codereview.webrtc.org/2331373004/ is
> landed?

No, closing

[hbos, 2016-09-15 12:10:50]

Description was changed from

==========
Moving webrtc/stats/ into webrtc/api/ to resolve a circular dependency
introduced when adding PeerConnection[Interface]::GetRTCStats.

BUG=chromium:627816
==========

to

==========
Moving webrtc/stats/ into webrtc/api/ to resolve a circular dependency
introduced when adding PeerConnection[Interface]::GetRTCStats.

BUG=chromium:627816
==========