[webkit-reviews] review denied: [Bug 81339] [chromium] MediaStream API (JSEP): Introducing WebSessionDescription and WebIceCandidate : [Attachment 132260] Patch

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Fri Mar 16 13:25:07 PDT 2012


Darin Fisher (:fishd, Google) <fishd at chromium.org> has denied Tommy Widenflycht
<tommyw at google.com>'s request for review:
Bug 81339: [chromium] MediaStream API (JSEP): Introducing WebSessionDescription
and WebIceCandidate
https://bugs.webkit.org/show_bug.cgi?id=81339

Attachment 132260: Patch
https://bugs.webkit.org/attachment.cgi?id=132260&action=review

------- Additional Comments from Darin Fisher (:fishd, Google)
<fishd at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=132260&action=review


> Source/WebKit/chromium/public/platform/WebIceCandidate.h:45
> +class WebIceCandidate {

"Ice" is really an acronym, right?  ICE = Internet Connectivity Establishment,
right?

In that case, "Ice" needs to be "ICE" if it appears mid-identifier or at the
start of
a type name, else "ice" if it appears at the start of an identifier, possibly
prefixed
with "m_":

  m_iceRequest
  WebICECandidate

etc.

see the "Names" section of http://www.webkit.org/coding/coding-style.html

> Source/WebKit/chromium/public/platform/WebIceCandidate.h:59
> +    WEBKIT_EXPORT void initialize(const WebString& label, const WebString&
candidateLine);

what is "candidateLine"?  maybe there should be some documentation?

> Source/WebKit/chromium/public/platform/WebSessionDescription.h:65
> +    size_t numberOfAddedCandidates() const;

don't these methods need to be exported too?

> Source/WebKit/chromium/public/platform/WebSessionDescription.h:67
> +    WebString initialSdp() const;

nit: initialSdp -> initialSDP

> Source/WebKit/chromium/public/platform/WebSessionDescription.h:76
> +    WebPrivatePtr<WebCore::SessionDescriptionDescriptor> m_private;

why is the WebCore type named "SessionDescriptionDescriptor", but the public
API is just WebSessionDescription?  seems like the WebCore one could also drop
the Descriptor suffix or else the public one should gain a Descriptor suffix.
what is the reasoning behind the Descriptor suffix?


More information about the webkit-reviews mailing list