Bug 98003 - Add data passing to the GetStats interface of RTCPeerConnection
Summary: Add data passing to the GetStats interface of RTCPeerConnection
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Harald Alvestrand
Depends on:
Blocks: 56459
  Show dependency treegraph
Reported: 2012-10-01 01:58 PDT by Harald Alvestrand
Modified: 2012-10-09 04:48 PDT (History)
10 users (show)

See Also:

Patch (31.24 KB, patch)
2012-10-02 01:39 PDT, Harald Alvestrand
no flags Details | Formatted Diff | Diff
Review comments addressed (32.25 KB, patch)
2012-10-02 05:25 PDT, Harald Alvestrand
abarth: review+
abarth: commit-queue-
Details | Formatted Diff | Diff
Adam Barth review comments addressed (32.36 KB, patch)
2012-10-03 00:34 PDT, Harald Alvestrand
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Harald Alvestrand 2012-10-01 01:58:18 PDT
Bug 95193 added data types and control flow, but did not add a platform interface for adding statistics.
This patch will add functions for adding statistics to the data structure; these will be accessible only from the platform side, not from the Javascript side.

Please assign to hta@google.com
Comment 1 Harald Alvestrand 2012-10-02 01:39:43 PDT
Created attachment 166641 [details]
Comment 2 WebKit Review Bot 2012-10-02 01:42:21 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 3 Tommy Widenflycht 2012-10-02 01:57:15 PDT
Comment on attachment 166641 [details]

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

> Source/Platform/chromium/public/WebRTCStatsRequest.h:41

Since the stats feature introduces a lot of classes it would be great if you described how to use it here, with an example or two.

> Source/Platform/chromium/public/WebRTCStatsResponse.h:56
> +    WEBKIT_EXPORT void addStatistic(int report, bool isLocal, int element, WebString name, WebString value);

int -> size_t

> Source/WebCore/Modules/mediastream/RTCStatsElement.h:41
>  private:

I like an empty line before private:

> Source/WebCore/platform/chromium/support/WebRTCStatsResponse.cpp:62
> +int WebRTCStatsResponse::addElement(int report, bool isLocal, long timestamp)

missing empty line
Comment 4 Tommy Widenflycht 2012-10-02 02:01:32 PDT
For the reviewer:

This patch introduces a different pattern in WebCore/platform instead of the Descriptor pattern, with an abstract base class in WebCore/platform.
Comment 5 Harald Alvestrand 2012-10-02 05:25:24 PDT
Created attachment 166676 [details]
Review comments addressed
Comment 6 Adam Barth 2012-10-02 09:16:21 PDT
Comment on attachment 166676 [details]
Review comments addressed

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

> Source/Platform/chromium/public/WebRTCStatsRequest.h:59
> +//   size_t element_index = response.addElement(report_index, true, now_in_ms());

This sample code should probably be in WebKit style, so report_index -> reportIndex etc

> Source/Platform/chromium/public/WebRTCStatsRequest.h:85
> +    WEBKIT_EXPORT WebRTCStatsResponse response() const;

In the example, code, you called this function createResponse, which seems like a better name because it actually creates a new response object for you instead of just providing an accessor for an existing response.

> Source/WebCore/Modules/mediastream/RTCStatsReport.cpp:42
> +

Looks like you've got an extra blank line here.

> Source/WebCore/Modules/mediastream/RTCStatsReport.cpp:43
> +int RTCStatsReport::addElement(bool isLocal, long timestamp)

Should this return a size_t?

> Source/WebCore/Modules/mediastream/RTCStatsRequestImpl.cpp:57
> +PassRefPtr<RTCStatsResponseBase> RTCStatsRequestImpl::createResponse()
> +{
> +    return RTCStatsResponse::create();
> +}

If there's no connection between the request and the response, why do we need this create method?  I guess the reason is that we want to instantiate RTCStatsResponse rather than RTCStatsResponseBase.

> Source/WebCore/Modules/mediastream/RTCStatsRequestImpl.cpp:63
> +    m_successCallback->handleEvent(static_cast<RTCStatsResponse*>(response.get()));

I see, this static cast is here because of the factory pattern.

> Source/WebCore/Modules/mediastream/RTCStatsResponse.cpp:42
> +int RTCStatsResponse::addReport()

Same question re: size_t here.

> Source/WebCore/Modules/mediastream/RTCStatsResponse.h:49
> +    virtual int addReport();
> +    virtual int addElement(size_t report, bool isLocal, long timestamp);
> +    virtual void addStatistic(size_t report, bool isLocal, size_t element, String name, String value);

Should we add the OVERRIDE keyword to these functions?

> Tools/DumpRenderTree/chromium/MockWebRTCPeerConnectionHandler.cpp:219
> +        int repidx = response.addReport();

repidx -> please use complete words in variable names.  Perhaps reportIndex ?

> Tools/DumpRenderTree/chromium/MockWebRTCPeerConnectionHandler.cpp:220
> +        int elementidx = response.addElement(repidx, true, 12345);

elementidx -> elementIndex.  It seems likely these variables should be of type size_t because they're indexes into a Vector.
Comment 7 Adam Barth 2012-10-02 22:01:55 PDT
Comment on attachment 166676 [details]
Review comments addressed

This seems fine modulo the nits above.  I probably would have continued to use the descriptor pattern that we're using elsewhere in mediastream because it avoids the virtual function calls and the static_casts, but this way works too.
Comment 8 Harald Alvestrand 2012-10-03 00:34:48 PDT
Created attachment 166820 [details]
Adam Barth review comments addressed
Comment 9 Adam Barth 2012-10-03 00:36:51 PDT
Comment on attachment 166820 [details]
Adam Barth review comments addressed

Comment 10 Adam Barth 2012-10-03 00:38:02 PDT
Comment on attachment 166820 [details]
Adam Barth review comments addressed

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

> Source/Platform/ChangeLog:6
> +        Reviewed by NOBODY (OOPS!).

A reviewer needs to set the Review flag so that the bot knows whose name to fill in here.  Alternatively, you can fill it in yourself if you've already gotten an r+.
Comment 11 WebKit Review Bot 2012-10-03 01:01:15 PDT
Comment on attachment 166820 [details]
Adam Barth review comments addressed

Clearing flags on attachment: 166820

Committed r130260: <http://trac.webkit.org/changeset/130260>
Comment 12 Harald Alvestrand 2012-10-09 04:48:31 PDT
The bot seems to not have closed this bug, but the patch is commmitted.
Marking as resolved/fixed.