Summary: | Implement ID for RTCStatsReport object | ||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Harald Alvestrand <hta> | ||||||||||||||||
Component: | WebKit API | Assignee: | Harald Alvestrand <hta> | ||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||
Severity: | Normal | CC: | abarth, berto, cgarcia, dglazkov, eric.carlson, esprehn+autocc, feature-media-reviews, fishd, gyuyoung.kim, jamesr, jansson, jer.noble, jochen, kjellander, ojan.autocc, peter+ews, rakuco, tkent+wkapi, tommyw, webkit.review.bot | ||||||||||||||||
Priority: | P2 | ||||||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||||||
Hardware: | Unspecified | ||||||||||||||||||
OS: | Unspecified | ||||||||||||||||||
Bug Depends on: | |||||||||||||||||||
Bug Blocks: | 110332 | ||||||||||||||||||
Attachments: |
|
Description
Harald Alvestrand
2013-02-20 05:42:08 PST
Created attachment 189300 [details]
Patch
Created attachment 189729 [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. The API update that this patch is the start on is here: http://dev.w3.org/2011/webrtc/editor/archives/20130222/webrtc.html 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. 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 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 Created attachment 192210 [details]
This version of the patch should be ready. The previous ones were basically trybot probes.
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 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 Created attachment 192212 [details]
Patch
Comment on attachment 192212 [details] Patch Attachment 192212 [details] did not pass efl-ews (efl): Output: http://webkit-commit-queue.appspot.com/results/17047242 Created attachment 192218 [details]
Patch
Comment on attachment 192218 [details] Patch Attachment 192218 [details] did not pass efl-ews (efl): Output: http://webkit-commit-queue.appspot.com/results/17077318 Created attachment 192224 [details]
Patch
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. Created attachment 192293 [details]
Patch
Comment on attachment 192293 [details]
Patch
Thanks.
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. Comment on attachment 192293 [details] Patch Clearing flags on attachment: 192293 Committed r145279: <http://trac.webkit.org/changeset/145279> All reviewed patches have been landed. Closing bug. 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)' ... 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) (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. (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 |