Bug 95193

Summary: Implement the GetStats interface on PeerConnection
Product: WebKit Reporter: Harald Alvestrand <hta>
Component: WebCore Misc.Assignee: Harald Alvestrand <hta>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, dglazkov, eric.carlson, feature-media-reviews, fishd, gns, gtk-ews, gyuyoung.kim, jamesr, mrobinson, ojan, philn, rakuco, tkent+wkapi, tommyw, webkit.review.bot, xan.lopez
Priority: P2 Keywords: WebExposed
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Bug Depends on: 97702    
Bug Blocks: 56459    
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch
none
Patch - this version fixes buildbot-detected failures.
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch none

Description Harald Alvestrand 2012-08-28 06:26:44 PDT
Implement an interface to access statistics on the components of a PeerConnection.

Proposal, not yet incorporated into the PeerConnection API:

https://docs.google.com/a/google.com/document/d/1D71z6ylu39FF--F4JWqIIWFNKbQ3isSLd0wQ2ahvT_4/edit
Comment 1 Harald Alvestrand 2012-09-18 16:43:51 PDT
*** Bug 96852 has been marked as a duplicate of this bug. ***
Comment 2 Harald Alvestrand 2012-09-19 13:04:54 PDT
Created attachment 164764 [details]
Patch
Comment 3 WebKit Review Bot 2012-09-20 01:00:43 PDT
Please wait for approval from abarth@webkit.org, dglazkov@chromium.org, fishd@chromium.org, jamesr@chromium.org or tkent@chromium.org before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.
Comment 4 WebKit Review Bot 2012-09-20 01:11:32 PDT
Comment on attachment 164764 [details]
Patch

Attachment 164764 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13952108
Comment 5 Tommy Widenflycht 2012-09-20 01:17:34 PDT
Comment on attachment 164764 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=164764&action=review

> Source/Platform/ChangeLog:9
> +        Additional information of the change such as approach, rationale. Please add per-function descriptions below (OOPS!).

Fix or remove.

> Source/Platform/chromium/public/WebRTCPeerConnectionHandler.h:61
> +    virtual void getStats(const WebRTCStatsRequest&)

Make this a one-liner.

> Source/Platform/chromium/public/WebRTCStatsRequest.h:35
> +#include "WebNonCopyable.h"

Unused

> Source/Platform/chromium/public/WebRTCStatsRequest.h:46
> +

no empty line here

> Source/WebCore/Modules/mediastream/RTCPeerConnection.cpp:387
> +    // TODO: Add use of selector, either in the request or as argument.

TODO -> FIXME

I would prefer if the selector ends up in the request when you fix this.

> Source/WebCore/Modules/mediastream/RTCStatsElement.idl:37
> +        // readonly attribute Dictionary stats;

Firstly this is not possible, and secondly we never check in commented out code in webkit.

> Source/WebCore/Modules/mediastream/RTCStatsRequestImpl.cpp:50
> +      m_successCallback(callback)

the comma goes under the colon.

> Source/WebCore/Modules/mediastream/RTCStatsRequestImpl.cpp:61
> +        // TODO: Fill in content of stats parameter.

TODO -> FIXME

> Source/WebCore/Modules/mediastream/RTCStatsRequestImpl.h:46
> +class RTCStatsRequestImpl : public RTCStatsRequest, public ActiveDOMObject {

You need to override the stop method from ActiveDOMObject and in it clear the callback.

> Source/WebCore/Modules/mediastream/RTCStatsResponse.cpp:36
> +    // TODO: Pass and populate from incoming arg.

FIXME

> Source/WebCore/platform/mediastream/RTCPeerConnectionHandler.h:-68
> -

Leave the empty line.

> Source/WebCore/platform/mediastream/RTCStatsRequest.cpp:52
> +void RTCStatsRequest::requestSucceeded()

This is a pure virtual... Remove implementation.

> Source/WebCore/platform/mediastream/RTCStatsRequest.h:42
> +class RTCStatsRequest : public RefCounted<RTCStatsRequest> {

This class doesn't need a cpp file, and definitely shouldn't have a create method. Look at RTCSessionDescriptionRequest.

> Source/WebCore/platform/mediastream/RTCStatsRequest.h:50
> +

Remove empty line.
Comment 6 Tommy Widenflycht 2012-09-20 01:20:07 PDT
To make this compile under gtk you need to add the files to two more build files in Source/WebCore.
Grep for RTCSessionDescriptionRequest for example in that directory.
Comment 7 Tommy Widenflycht 2012-09-20 01:46:49 PDT
Also, you are using a mix of different licenses. Please use this one for all new files:
 
http://trac.webkit.org/browser/trunk/Source/WebCore/platform/graphics/chromium/cc/CCActiveAnimation.cpp
Comment 8 Adam Barth 2012-09-20 10:57:32 PDT
Comment on attachment 164764 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=164764&action=review

> Source/WebCore/Modules/mediastream/RTCPeerConnection.idl:75
> +        void getStats(in [Callback, Optional=DefaultIsUndefined] RTCStatsCallback successCallback, in [Optional=DefaultIsUndefined] MediaStreamTrack selector);

Is the successCallback really optional?  That seems sort of pointless.  :)

> Source/WebCore/Modules/mediastream/RTCStatsElementList.idl:33
> +    interface [
> +        Conditional=MEDIA_STREAM,
> +        IndexedGetter
> +    ] RTCStatsElementList {
> +        readonly attribute unsigned long length;
> +        RTCStatsElement item(in [IsIndex] unsigned long index);
> +    };

Why not just use Sequence<RTCStatsElement> ?  That's generally preferred these days so that we'll use a real JavaScript array rather than one of these fake DOM-objects-that-try-to-act-like-arrays

> Source/WebCore/Modules/mediastream/RTCStatsReport.h:42
> +private:
> +    RTCStatsReport();
> +};

This class has no storage?

> Source/WebCore/Modules/mediastream/RTCStatsRequestImpl.cpp:60
> +    if (m_successCallback) {

Prefer early return

> Source/WebCore/Modules/mediastream/RTCStatsResponse.idl:36
> +        readonly attribute RTCStatsReportList result;

sequence<RTCStatsReport> ?
Comment 9 Adam Barth 2012-09-20 10:58:28 PDT
Comment on attachment 164764 [details]
Patch

I didn't see RTCStatsReport.cpp.  Maybe you forgot to "svn add" the file?
Comment 10 Harald Alvestrand 2012-09-21 06:09:01 PDT
Created attachment 165126 [details]
Patch
Comment 11 WebKit Review Bot 2012-09-21 06:22:45 PDT
Comment on attachment 165126 [details]
Patch

Attachment 165126 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13951692
Comment 12 Adam Barth 2012-09-21 09:39:34 PDT
Comment on attachment 165126 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=165126&action=review

This basically looks good.  Is there a reason why you're not using sequence<Foo> instead of creating these fake DOM-based arrays?  Also, it looks like the patch doesn't compile, perhaps because it is missing the CPP file noted below.

> Source/Platform/chromium/public/WebRTCStatsRequest.h:60
> +    WEBKIT_EXPORT void requestSucceeded() const;

Don't we need to supply some stats somehow?

> Source/WebCore/Modules/mediastream/RTCPeerConnection.h:89
> +    void getStats(PassRefPtr<RTCStatsCallback>, PassRefPtr<MediaStreamTrack>);

I would have given these parameters names since it's not clear what they mean otherwise.

> Source/WebCore/Modules/mediastream/RTCPeerConnection.idl:75
> +        void getStats(in [Callback] RTCStatsCallback successCallback, in [Optional=DefaultIsUndefined] MediaStreamTrack selector);

Do you have a link to the spec handy?

> Source/WebCore/Modules/mediastream/RTCStatsElement.idl:31
> +        // name "value" causes an error

What does this comment mean?

> Source/WebCore/Modules/mediastream/RTCStatsElementList.h:39
> +    // DOM methods & attributes for RTCStatsElementList

This comment isn't needed.

> Source/WebCore/Modules/mediastream/RTCStatsReport.h:32
> +class RTCStatsReport : public RefCounted<RTCStatsReport> {

I still don't see RTCStatsReport.cpp in this patch.  Is that what's causing the compile error?
Comment 13 Harald Alvestrand 2012-09-23 23:38:37 PDT
Created attachment 165315 [details]
Patch
Comment 14 WebKit Review Bot 2012-09-24 00:54:13 PDT
Attachment 165315 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/fast..." exit_code: 1
Source/WebCore/Modules/mediastream/RTCStatsResponse.h:44:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
Total errors found: 1 in 31 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 15 Gyuyoung Kim 2012-09-24 00:58:30 PDT
Comment on attachment 165315 [details]
Patch

Attachment 165315 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/13988556
Comment 16 WebKit Review Bot 2012-09-24 01:02:05 PDT
Comment on attachment 165315 [details]
Patch

Attachment 165315 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13993555
Comment 17 Harald Alvestrand 2012-09-25 02:48:09 PDT
Comment on attachment 165126 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=165126&action=review

>> Source/Platform/chromium/public/WebRTCStatsRequest.h:60
>> +    WEBKIT_EXPORT void requestSucceeded() const;
> 
> Don't we need to supply some stats somehow?

Next patch. I've expanded this patch to supply the storage.

>> Source/WebCore/Modules/mediastream/RTCPeerConnection.h:89
>> +    void getStats(PassRefPtr<RTCStatsCallback>, PassRefPtr<MediaStreamTrack>);
> 
> I would have given these parameters names since it's not clear what they mean otherwise.

Done.

>> Source/WebCore/Modules/mediastream/RTCPeerConnection.idl:75
>> +        void getStats(in [Callback] RTCStatsCallback successCallback, in [Optional=DefaultIsUndefined] MediaStreamTrack selector);
> 
> Do you have a link to the spec handy?

Dated link: http://dev.w3.org/2011/webrtc/editor/webrtc-20120920.html
Should this be in the code somewhere? Perhaps just in the changelog?

>> Source/WebCore/Modules/mediastream/RTCStatsElement.idl:31
>> +        // name "value" causes an error
> 
> What does this comment mean?

Reminder to self: I can't call this function "value", because that causes an obscure error. So I call it "stat". Moved to personal notebook.

>> Source/WebCore/Modules/mediastream/RTCStatsElementList.h:39
>> +    // DOM methods & attributes for RTCStatsElementList
> 
> This comment isn't needed.

File gone.

>> Source/WebCore/Modules/mediastream/RTCStatsReport.h:32
>> +class RTCStatsReport : public RefCounted<RTCStatsReport> {
> 
> I still don't see RTCStatsReport.cpp in this patch.  Is that what's causing the compile error?

It seems that I can get away with not implementing classes that I don't use when I'm using a component build, but not in a bot build. Implementations added.
Comment 18 Harald Alvestrand 2012-09-25 03:12:48 PDT
Created attachment 165567 [details]
Patch
Comment 19 Gyuyoung Kim 2012-09-25 03:20:25 PDT
Comment on attachment 165567 [details]
Patch

Attachment 165567 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/14002697
Comment 20 kov's GTK+ EWS bot 2012-09-25 03:24:14 PDT
Comment on attachment 165567 [details]
Patch

Attachment 165567 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/14032005
Comment 21 WebKit Review Bot 2012-09-25 03:27:42 PDT
Comment on attachment 165567 [details]
Patch

Attachment 165567 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/14021053
Comment 22 Harald Alvestrand 2012-09-25 04:22:23 PDT
Created attachment 165578 [details]
Patch - this version fixes buildbot-detected failures.
Comment 23 Harald Alvestrand 2012-09-25 04:44:18 PDT
Created attachment 165582 [details]
Patch
Comment 24 kov's GTK+ EWS bot 2012-09-25 04:52:31 PDT
Comment on attachment 165582 [details]
Patch

Attachment 165582 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/14014011
Comment 25 Harald Alvestrand 2012-09-25 05:11:32 PDT
Created attachment 165585 [details]
Patch
Comment 26 Tommy Widenflycht 2012-09-25 07:23:37 PDT
I just built the gtk port locally and that works just fine. Have no idea why the bot fails :/
Comment 27 Adam Barth 2012-09-25 09:31:27 PDT
> > Do you have a link to the spec handy?
> 
> Dated link: http://dev.w3.org/2011/webrtc/editor/webrtc-20120920.html
> Should this be in the code somewhere? Perhaps just in the changelog?

ChangeLog is great.
Comment 28 Adam Barth 2012-09-25 09:40:06 PDT
Comment on attachment 165585 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=165585&action=review

> Source/Platform/ChangeLog:1
> +2012-09-25  Harald Tveit Alvestrand  <harald@alvestrand.no>

There's supposed to be a blank line after this line.

> Source/WebCore/ChangeLog:1
> +2012-09-25  Harald Tveit Alvestrand  <harald@alvestrand.no>

here too

> Source/WebCore/Modules/mediastream/RTCStatsElement.cpp:38
> +    RefPtr<RTCStatsElement> response = adoptRef(new RTCStatsElement(timestamp));
> +    return response.release();

You can merge these two lines an eliminate the |response| variable.

> Source/WebCore/Modules/mediastream/RTCStatsElement.h:37
> +    PassRefPtr<RTCStatsElement> create(long timestamp);

Shouldn't this function be static?

> Source/WebCore/Modules/mediastream/RTCStatsElement.h:40
> +private:

Please add a blank line above this line.

> Source/WebCore/Modules/mediastream/RTCStatsElement.h:41
> +    RTCStatsElement(long timestamp);

Almost all one-argument constructors should have the "explicit" keyword.

> Source/WebCore/Modules/mediastream/RTCStatsReport.cpp:36
> +    RefPtr<RTCStatsReport> response = adoptRef(new RTCStatsReport());
> +    return response.release();

This can be merged into one line.

> Source/WebCore/Modules/mediastream/RTCStatsReport.h:39
> +    virtual ~RTCStatsReport();

This function is declared by never implemented.  DOes this function need to be virtual?  Are you planning to subclass this class?

> Source/WebCore/Modules/mediastream/RTCStatsResponse.cpp:37
> +    RefPtr<RTCStatsResponse> response = adoptRef(new RTCStatsResponse());
> +    // FIXME: Pass an RTCStatsRequest argument and populate from it.
> +    return response.release();

I'd put this on one line and expand it in a subsequent patch, if needed.

> Tools/ChangeLog:1
> +2012-09-25  Harald Tveit Alvestrand  <harald@alvestrand.no>

blank line below this line, pls

> Tools/ChangeLog:6
> +        http://dev.w3.org/2011/webrtc/editor/webrtc-20120920.html

Thanks for adding the link to the specification.  You don't need to put it in every ChangeLog.  Typically folks put it in the WebCore ChangeLog because that's where the bulk of the spec-implementing code is.

> Tools/ChangeLog:8
> +

Looks like you've got an extra blank line here.
Comment 29 Adam Barth 2012-09-25 09:40:55 PDT
Comment on attachment 165585 [details]
Patch

This looks great.  Thanks for iterating on the patch.  It looks like we have a few minor style issues to clean up and then we'll be ready to land this patch.
Comment 30 Harald Alvestrand 2012-09-25 11:11:54 PDT
Comment on attachment 165585 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=165585&action=review

>> Source/Platform/ChangeLog:1
>> +2012-09-25  Harald Tveit Alvestrand  <harald@alvestrand.no>
> 
> There's supposed to be a blank line after this line.

Done.

>> Source/WebCore/ChangeLog:1
>> +2012-09-25  Harald Tveit Alvestrand  <harald@alvestrand.no>
> 
> here too

Done.

>> Source/WebCore/Modules/mediastream/RTCStatsElement.cpp:38
>> +    return response.release();
> 
> You can merge these two lines an eliminate the |response| variable.

Done. Can always unmerge later.

>> Source/WebCore/Modules/mediastream/RTCStatsElement.h:37
>> +    PassRefPtr<RTCStatsElement> create(long timestamp);
> 
> Shouldn't this function be static?

Oops. Yes.

>> Source/WebCore/Modules/mediastream/RTCStatsElement.h:40
>> +private:
> 
> Please add a blank line above this line.

Done.

>> Source/WebCore/Modules/mediastream/RTCStatsElement.h:41
>> +    RTCStatsElement(long timestamp);
> 
> Almost all one-argument constructors should have the "explicit" keyword.

Done.

>> Source/WebCore/Modules/mediastream/RTCStatsReport.cpp:36
>> +    return response.release();
> 
> This can be merged into one line.

Done.

>> Source/WebCore/Modules/mediastream/RTCStatsReport.h:39
>> +    virtual ~RTCStatsReport();
> 
> This function is declared by never implemented.  DOes this function need to be virtual?  Are you planning to subclass this class?

Deleted. Can always add it back if I need it.

>> Source/WebCore/Modules/mediastream/RTCStatsResponse.cpp:37
>> +    return response.release();
> 
> I'd put this on one line and expand it in a subsequent patch, if needed.

Done.

>> Tools/ChangeLog:1
>> +2012-09-25  Harald Tveit Alvestrand  <harald@alvestrand.no>
> 
> blank line below this line, pls

Done.

>> Tools/ChangeLog:6
>> +        http://dev.w3.org/2011/webrtc/editor/webrtc-20120920.html
> 
> Thanks for adding the link to the specification.  You don't need to put it in every ChangeLog.  Typically folks put it in the WebCore ChangeLog because that's where the bulk of the spec-implementing code is.

OK, deleted from others.

>> Tools/ChangeLog:8
>> +
> 
> Looks like you've got an extra blank line here.

Fixed.
Comment 31 Harald Alvestrand 2012-09-25 11:19:30 PDT
Created attachment 165644 [details]
Patch
Comment 32 Adam Barth 2012-09-25 15:44:03 PDT
(In reply to comment #26)
> I just built the gtk port locally and that works just fine. Have no idea why the bot fails :/

DerivedSources/WebCore/JSRTCPeerConnection.cpp: In function 'void* WebCore::jsRTCPeerConnectionPrototypeFunctionGetStats(JSC::ExecState*)':
DerivedSources/WebCore/JSRTCPeerConnection.cpp:711:48: error: 'JSRTCStatsCallback' has not been declared

It does look like a failure related to this patch.  Does DerivedSources/WebCore/JSRTCPeerConnection.cpp #include the right stuff?
Comment 33 Adam Barth 2012-09-25 15:46:02 PDT
Comment on attachment 165644 [details]
Patch

This looks good, but I worry that we're going to break the GTK port by landing this patch.
Comment 34 Adam Barth 2012-09-25 15:47:17 PDT
Maybe we can ask someone who is more familiar with GTK to help us diagnose the build problem.  mrobinson, would you be willing to take a look or suggest someone who could?
Comment 35 Harald Alvestrand 2012-09-26 00:20:55 PDT
Both Tommy and I have checked out fresh WebKit build environments and built the gtk port with the patch - and it compiles cleanly.

(checkout sources; apply patch; Tools/Scripts/build-webkit --gtk)

Definitely someone who understands how the gtk bot is different is going to have to look at this.
Comment 36 Martin Robinson 2012-09-26 06:01:02 PDT
Comment on attachment 165644 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=165644&action=review

> Source/WebCore/GNUmakefile.list.am:649
> +	DerivedSources/WebCore/JSRTCStatsCallback.cpp \
> +	DerivedSources/WebCore/JSRTCStatsCallback.h \
> +	DerivedSources/WebCore/JSRTCStatsElement.cpp \
> +	DerivedSources/WebCore/JSRTCStatsElement.h \
> +	DerivedSources/WebCore/JSRTCStatsReport.cpp \
> +	DerivedSources/WebCore/JSRTCStatsReport.h \
> +	DerivedSources/WebCore/JSRTCStatsResponse.cpp \
> +	DerivedSources/WebCore/JSRTCStatsResponse.h \

If you add new IDLs, you should also add them to the dom_binding_idls in this file. That's probably why the build is broken in this case.
Comment 37 Harald Alvestrand 2012-09-26 06:56:55 PDT
Created attachment 165791 [details]
Patch
Comment 38 Adam Barth 2012-09-26 08:47:17 PDT
Comment on attachment 165791 [details]
Patch

Looks like the gtk bot is happy now.  Thanks!!
Comment 39 WebKit Review Bot 2012-09-26 09:16:28 PDT
Comment on attachment 165791 [details]
Patch

Clearing flags on attachment: 165791

Committed r129654: <http://trac.webkit.org/changeset/129654>
Comment 40 WebKit Review Bot 2012-09-26 09:16:34 PDT
All reviewed patches have been landed.  Closing bug.
Comment 41 WebKit Review Bot 2012-09-26 10:58:48 PDT
Re-opened since this is blocked by 97702
Comment 42 Harald Alvestrand 2012-09-28 04:47:23 PDT
Created attachment 166208 [details]
Patch
Comment 43 Harald Alvestrand 2012-09-28 04:48:28 PDT
Comment on attachment 166208 [details]
Patch

Added one more #include.
This has been compiled with Chrome on Windows.
Comment 44 WebKit Review Bot 2012-09-28 09:25:30 PDT
Comment on attachment 166208 [details]
Patch

Clearing flags on attachment: 166208

Committed r129908: <http://trac.webkit.org/changeset/129908>
Comment 45 WebKit Review Bot 2012-09-28 09:25:37 PDT
All reviewed patches have been landed.  Closing bug.