Bug 110333 – Implement ID for RTCStatsReport object

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit API

[Harald Alvestrand]

Patch to implement ID support in RTCStatsReport.

- Add the ID property to RTCStatsReport
- Add the get-by-ID named getter to RTCStatsResponse
- Move the actual content of stats from RTCStatsElement to RTCStatsReport

Preserve RTCStatsElement API until Chrome has been updated, but don't bother preserving the local/remote distinction.

[Harald Alvestrand]

Created attachment 189300 [details]
Patch

[Harald Alvestrand]

Created attachment 189729 [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.

[Harald Alvestrand]

The API update that this patch is the start on is here:

http://dev.w3.org/2011/webrtc/editor/archives/20130222/webrtc.html

[Tommy Widenflycht]

Comment on attachment 189729 [details]
Patch

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

> Source/WebCore/Modules/mediastream/RTCStatsReport.cpp:57
> +        it != m_stats.end(); ++it) {

One line please

> Source/WebCore/platform/chromium/support/WebRTCStatsResponse.cpp:60
> +    return m_private->addReport(id, type, timestamp);

Please add ASSERT(m_private) first in these functions. Bettar safe than sorry.

[WebKit Review Bot]

Comment on attachment 189729 [details]
Patch

Attachment 189729 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/16760381

New failing tests:
fast/multicol/block-axis-vertical-lr.html

[Peter Beverloo (cr-android ews)]

Comment on attachment 189729 [details]
Patch

Attachment 189729 [details] did not pass cr-android-ews (chromium-android):
Output: http://webkit-commit-queue.appspot.com/results/16793281

[Harald Alvestrand]

Created attachment 192210 [details]
This version of the patch should be ready. The previous ones were basically trybot probes.

[Peter Beverloo (cr-android ews)]

Comment on attachment 192210 [details]
This version of the patch should be ready. The previous ones were basically trybot probes.

Attachment 192210 [details] did not pass cr-android-ews (chromium-android):
Output: http://webkit-commit-queue.appspot.com/results/17110056

[EFL EWS Bot]

Comment on attachment 192210 [details]
This version of the patch should be ready. The previous ones were basically trybot probes.

Attachment 192210 [details] did not pass efl-ews (efl):
Output: http://webkit-commit-queue.appspot.com/results/17016174

[Harald Alvestrand]

Created attachment 192212 [details]
Patch

[EFL EWS Bot]

Comment on attachment 192212 [details]
Patch

Attachment 192212 [details] did not pass efl-ews (efl):
Output: http://webkit-commit-queue.appspot.com/results/17047242

[Harald Alvestrand]

Created attachment 192218 [details]
Patch

[EFL EWS Bot]

Comment on attachment 192218 [details]
Patch

Attachment 192218 [details] did not pass efl-ews (efl):
Output: http://webkit-commit-queue.appspot.com/results/17077318

[Harald Alvestrand]

Created attachment 192224 [details]
Patch

[Adam Barth]

Comment on attachment 192224 [details]
Patch

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

> Source/WebCore/Modules/mediastream/RTCStatsReport.cpp:37
> -    return adoptRef(new RTCStatsReport());
> +    return adoptRef(new RTCStatsReport("inner fake ID", "inner fake type", 0));

What's up with these strings?  They seems like odd things to use in production code.  Should we use empty or null strings instead?

> Source/WebCore/Modules/mediastream/RTCStatsReport.cpp:45
> +RTCStatsReport::RTCStatsReport(String id, String type, double timestamp)

String -> const String&

Almost all the time you take a String as a parameter to a function, you want to use a const String& to avoid churning the refcount on the underlying StringImpl object.

> Source/WebCore/Modules/mediastream/RTCStatsResponse.cpp:31
> +

This addition seems spurious.

[Harald Alvestrand]

Created attachment 192293 [details]
Patch

[Adam Barth]

Comment on attachment 192293 [details]
Patch

Thanks.

[Harald Alvestrand]

Comment on attachment 192224 [details]
Patch

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

Thanks - fixed the comments.

>> Source/WebCore/Modules/mediastream/RTCStatsReport.cpp:37
>> +    return adoptRef(new RTCStatsReport("inner fake ID", "inner fake type", 0));
> 
> What's up with these strings?  They seems like odd things to use in production code.  Should we use empty or null strings instead?

This function is part of the //DEPRECATED stuff that I plan to remove week after next (after landing the Chromium patches that stop all usage of the deprecated interface). The new interface should never have null strings for these, so I want something that's easy to see and not invalid.

>> Source/WebCore/Modules/mediastream/RTCStatsReport.cpp:45
>> +RTCStatsReport::RTCStatsReport(String id, String type, double timestamp)
> 
> String -> const String&
> 
> Almost all the time you take a String as a parameter to a function, you want to use a const String& to avoid churning the refcount on the underlying StringImpl object.

Done!

>> Source/WebCore/Modules/mediastream/RTCStatsResponse.cpp:31
>> +
> 
> This addition seems spurious.

Fixed.

[WebKit Review Bot]

Comment on attachment 192293 [details]
Patch

Clearing flags on attachment: 192293

Committed r145279: <http://trac.webkit.org/changeset/145279>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Alberto Garcia]

Hey, I can't link the BlackBerry port after this patch.

DerivedSources/WebCore/JSRTCStatsResponse.cpp.o: In function `WebCore::JSRTCStatsResponse::getOwnPropertySlotByIndex(JSC::JSCell*, JSC::ExecState*, unsigned int, JSC::PropertySlot&)':
DerivedSources/WebCore/JSRTCStatsResponse.cpp:182: undefined reference to `WebCore::JSRTCStatsResponse::canGetItemsForName(JSC::ExecState*, WebCore::RTCStatsResponse*, JSC::PropertyName)'

DerivedSources/WebCore/JSRTCStatsResponse.cpp.o: In function `WTF::StringImpl::operator delete(void*)':
Source/WTF/wtf/text/StringImpl.h:135: undefined reference to `WebCore::JSRTCStatsResponse::nameGetter(JSC::ExecState*, JSC::JSValue, JSC::PropertyName)'

...

[Harald Alvestrand]

This looks like a reaction to the use of the "NamedGetter" control in RTCStatsResponse.idl.

In Chrome, it was enough to add the namedItem function in the IDL and implement it to have this function correctly. Can you check if you have the NamedGetter in any other IDL function that you currently compile, and if so, what functions there have to be to support it?

(dom/NodeList and html/HTMLCollection have the same shape to their interfaces)

[Alberto Garcia]

(In reply to comment #23)
> Can you check if you have the NamedGetter in any other IDL function that you currently compile, and if so, what functions there have to be to support it?

> (dom/NodeList and html/HTMLCollection have the same shape to their interfaces)

In those cases, JSNodeList::nameGetter() and JSNodeList::canGetItemsForName() are defined in Source/WebCore/bindings/js/

But there's no such implementation for RTCStatsResponse.

[Carlos Garcia Campos]

(In reply to comment #22)
> Hey, I can't link the BlackBerry port after this patch.
> 
> DerivedSources/WebCore/JSRTCStatsResponse.cpp.o: In function `WebCore::JSRTCStatsResponse::getOwnPropertySlotByIndex(JSC::JSCell*, JSC::ExecState*, unsigned int, JSC::PropertySlot&)':
> DerivedSources/WebCore/JSRTCStatsResponse.cpp:182: undefined reference to `WebCore::JSRTCStatsResponse::canGetItemsForName(JSC::ExecState*, WebCore::RTCStatsResponse*, JSC::PropertyName)'
> 
> DerivedSources/WebCore/JSRTCStatsResponse.cpp.o: In function `WTF::StringImpl::operator delete(void*)':
> Source/WTF/wtf/text/StringImpl.h:135: undefined reference to `WebCore::JSRTCStatsResponse::nameGetter(JSC::ExecState*, JSC::JSValue, JSC::PropertyName)'
> 
> ...

I don't think this is a BlackBerry specific build failure, but any port using JSC and building with media stream enabled would fail to build. I've opened a new bug:

https://bugs.webkit.org/show_bug.cgi?id=113294