Bug 99460 - Implement the Selector argument to RTCPeerConnection.getStats
Summary: Implement the Selector argument to RTCPeerConnection.getStats
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Harald Alvestrand
URL:
Keywords:
Depends on:
Blocks: 56459
  Show dependency treegraph
 
Reported: 2012-10-16 05:44 PDT by Harald Alvestrand
Modified: 2012-10-17 04:20 PDT (History)
10 users (show)

See Also:


Attachments
Patch (13.77 KB, patch)
2012-10-16 06:38 PDT, Harald Alvestrand
no flags Details | Formatted Diff | Diff
Patch (17.82 KB, patch)
2012-10-16 07:26 PDT, Harald Alvestrand
no flags Details | Formatted Diff | Diff
Patch (17.83 KB, patch)
2012-10-16 07:59 PDT, Harald Alvestrand
abarth: review+
abarth: commit-queue-
Details | Formatted Diff | Diff
Patch for landing (18.10 KB, patch)
2012-10-17 02:41 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-16 05:44:15 PDT
The Selector argument to RTCPeerConnection.getStats is supposed to give a single track that one wants to get statistics on, so that one does not have to fetch all stats all the time.
Comment 1 Harald Alvestrand 2012-10-16 06:38:22 PDT
Created attachment 168937 [details]
Patch
Comment 2 Tommy Widenflycht 2012-10-16 07:06:16 PDT
Comment on attachment 168937 [details]
Patch

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

> Source/WebCore/Modules/mediastream/RTCStatsRequestImpl.cpp:47
> +    , m_stream(selector?selector->streamDescriptor():0)

Space around operators

> Source/WebCore/Modules/mediastream/RTCStatsRequestImpl.cpp:66
> +PassRefPtr<MediaStreamDescriptor> RTCStatsRequestImpl::stream()

Only return a PassRefPtr is ownership is transfered, otherwise use raw pointers: return m_stream.get()
http://www.webkit.org/coding/RefPtr.html

> Source/WebCore/platform/mediastream/RTCStatsRequest.h:52
> +    virtual PassRefPtr<MediaStreamDescriptor> stream() = 0;

See the comment above.
Comment 3 Harald Alvestrand 2012-10-16 07:26:10 PDT
Created attachment 168942 [details]
Patch
Comment 4 Harald Alvestrand 2012-10-16 07:59:19 PDT
Created attachment 168946 [details]
Patch
Comment 5 Harald Alvestrand 2012-10-16 07:59:49 PDT
Comment on attachment 168937 [details]
Patch

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

>> Source/WebCore/Modules/mediastream/RTCStatsRequestImpl.cpp:47
>> +    , m_stream(selector?selector->streamDescriptor():0)
> 
> Space around operators

Done.

>> Source/WebCore/Modules/mediastream/RTCStatsRequestImpl.cpp:66
>> +PassRefPtr<MediaStreamDescriptor> RTCStatsRequestImpl::stream()
> 
> Only return a PassRefPtr is ownership is transfered, otherwise use raw pointers: return m_stream.get()
> http://www.webkit.org/coding/RefPtr.html

Done.

>> Source/WebCore/platform/mediastream/RTCStatsRequest.h:52
>> +    virtual PassRefPtr<MediaStreamDescriptor> stream() = 0;
> 
> See the comment above.

Done.
Comment 6 Tommy Widenflycht 2012-10-16 08:24:45 PDT
Comment on attachment 168946 [details]
Patch

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

> Source/WebCore/platform/chromium/support/WebRTCStatsRequest.cpp:68
>  

Nit: extra empty line

> Source/WebCore/platform/chromium/support/WebRTCStatsRequest.cpp:84
> +

Nit: extra empty line
Comment 7 Adam Barth 2012-10-16 09:17:20 PDT
Comment on attachment 168946 [details]
Patch

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

This patch looks good.  Tommy and I have some very minor style issues that we should clear up before landing.

> Source/Platform/chromium/public/WebRTCStatsRequest.h:89
> +    WEBKIT_EXPORT bool hasSelector() const;
> +
> +    WEBKIT_EXPORT const WebMediaStreamDescriptor stream() const;
> +
> +    WEBKIT_EXPORT const WebMediaStreamComponent component() const;

I might have added some comments explaining the relationship between these functions.
Comment 8 Rik Cabanier 2012-10-16 09:54:37 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 9 Harald Alvestrand 2012-10-17 02:41:51 PDT
Created attachment 169140 [details]
Patch for landing
Comment 10 Harald Alvestrand 2012-10-17 03:02:21 PDT
Comment on attachment 168946 [details]
Patch

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

>> Source/Platform/chromium/public/WebRTCStatsRequest.h:89
>> +    WEBKIT_EXPORT const WebMediaStreamComponent component() const;
> 
> I might have added some comments explaining the relationship between these functions.

Done.

> Source/WebCore/platform/chromium/support/WebRTCStatsRequest.cpp:68
>  

Fixed.

>> Source/WebCore/platform/chromium/support/WebRTCStatsRequest.cpp:84
>> +
> 
> Nit: extra empty line

Fixed.
Comment 11 WebKit Review Bot 2012-10-17 03:11:22 PDT
Comment on attachment 169140 [details]
Patch for landing

Rejecting attachment 169140 [details] from commit-queue.

hta@google.com does not have committer permissions according to http://trac.webkit.org/browser/trunk/Tools/Scripts/webkitpy/common/config/committers.py.

- If you do not have committer rights please read http://webkit.org/coding/contributing.html for instructions on how to use bugzilla flags.

- If you have committer rights please correct the error in Tools/Scripts/webkitpy/common/config/committers.py by adding yourself to the file (no review needed).  The commit-queue restarts itself every 2 hours.  After restart the commit-queue will correctly respect your committer rights.
Comment 12 WebKit Review Bot 2012-10-17 04:15:11 PDT
Comment on attachment 169140 [details]
Patch for landing

Clearing flags on attachment: 169140

Committed r131584: <http://trac.webkit.org/changeset/131584>