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
*** Bug 96852 has been marked as a duplicate of this bug. ***
Created attachment 164764 [details] Patch
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 on attachment 164764 [details] Patch Attachment 164764 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/13952108
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.
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.
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 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 on attachment 164764 [details] Patch I didn't see RTCStatsReport.cpp. Maybe you forgot to "svn add" the file?
Created attachment 165126 [details] Patch
Comment on attachment 165126 [details] Patch Attachment 165126 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/13951692
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?
Created attachment 165315 [details] Patch
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 on attachment 165315 [details] Patch Attachment 165315 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/13988556
Comment on attachment 165315 [details] Patch Attachment 165315 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/13993555
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.
Created attachment 165567 [details] Patch
Comment on attachment 165567 [details] Patch Attachment 165567 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/14002697
Comment on attachment 165567 [details] Patch Attachment 165567 [details] did not pass gtk-ews (gtk): Output: http://queues.webkit.org/results/14032005
Comment on attachment 165567 [details] Patch Attachment 165567 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/14021053
Created attachment 165578 [details] Patch - this version fixes buildbot-detected failures.
Created attachment 165582 [details] Patch
Comment on attachment 165582 [details] Patch Attachment 165582 [details] did not pass gtk-ews (gtk): Output: http://queues.webkit.org/results/14014011
Created attachment 165585 [details] Patch
I just built the gtk port locally and that works just fine. Have no idea why the bot fails :/
> > 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 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 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 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.
Created attachment 165644 [details] Patch
(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 on attachment 165644 [details] Patch This looks good, but I worry that we're going to break the GTK port by landing this patch.
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?
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 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.
Created attachment 165791 [details] Patch
Comment on attachment 165791 [details] Patch Looks like the gtk bot is happy now. Thanks!!
Comment on attachment 165791 [details] Patch Clearing flags on attachment: 165791 Committed r129654: <http://trac.webkit.org/changeset/129654>
All reviewed patches have been landed. Closing bug.
Re-opened since this is blocked by 97702
Created attachment 166208 [details] Patch
Comment on attachment 166208 [details] Patch Added one more #include. This has been compiled with Chrome on Windows.
Comment on attachment 166208 [details] Patch Clearing flags on attachment: 166208 Committed r129908: <http://trac.webkit.org/changeset/129908>