Bug 98003

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

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

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.

Created attachment 166676 [details] Review comments addressed

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

Created attachment 166820 [details] Adam Barth review comments addressed

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

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 on attachment 166820 [details] Adam Barth review comments addressed Clearing flags on attachment: 166820 Committed r130260: <http://trac.webkit.org/changeset/130260>

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