Add data passing to the GetStats interface of RTCPeerConnection

[Harald Alvestrand]

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 [e-mail]

[Harald Alvestrand]

Created attachment 166641 [details]
Patch

[WebKit Review Bot]

Please wait for approval from [e-mail], [e-mail], [e-mail], [e-mail] or [e-mail] before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.

[Tommy Widenflycht]

Comment on attachment 166641 [details]
Patch

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

[Tommy Widenflycht]

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.

[Harald Alvestrand]

Created attachment 166676 [details]
Review comments addressed

[Adam Barth]

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.

[Adam Barth]

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.

[Harald Alvestrand]

Created attachment 166820 [details]
Adam Barth review comments addressed

[Adam Barth]

Comment on attachment 166820 [details]
Adam Barth review comments addressed

Thanks.

[Adam Barth]

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+.

[WebKit Review Bot]

Comment on attachment 166820 [details]
Adam Barth review comments addressed

Clearing flags on attachment: 166820

Committed r130260: <http://trac.webkit.org/changeset/130260>

[Harald Alvestrand]

The bot seems to not have closed this bug, but the patch is commmitted.
Marking as resolved/fixed.