Bug 81339

Created attachment 132260 [details] Patch

Please wait for approval from fishd@chromium.org before submitting because this patch contains changes to the Chromium public API.

Comment on attachment 132260 [details] Patch This looks fine, but we need fishd to review.

Comment on attachment 132260 [details] Patch 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?

> What is the reasoning behind the Descriptor suffix? A bunch of the concepts in the JavaScript API turn up both as implementations of the API and as concepts in WebCore/platform/mediastream. Tommy seems to be using the Descriptor suffix to mean "the WebCore/platform/mediastream instantiation of the concept."

Adam is correct. This is the one drawback of the WebCore proper and WebCore platform split, ie needing an extra implementation class in between WebCore proper and WebKit. (In reply to comment #5) > > What is the reasoning behind the Descriptor suffix? > > A bunch of the concepts in the JavaScript API turn up both as implementations of the API and as concepts in WebCore/platform/mediastream. Tommy seems to be using the Descriptor suffix to mean "the WebCore/platform/mediastream instantiation of the concept."

Comment on attachment 132260 [details] Patch 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 I have deliberately used Ice as a name for the below reasons: The JavaScript API uses "Ice" everywhere as a key concept In my world Ice is used as a proper name ("breaking the ice") >> 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? Here I agree with Adams comments for my WebCore patches: that's what the standard is for. The JSEP-enabled PeerConnection is a lot more complicated the the deprecated ROAP based one, and a thorough knowledge of the standard (both PeerConnection and SDP) is necessary to be able to implement this in a browser. >> Source/WebKit/chromium/public/platform/WebSessionDescription.h:65 >> + size_t numberOfAddedCandidates() const; > > don't these methods need to be exported too? Thanks, fixed. >> Source/WebKit/chromium/public/platform/WebSessionDescription.h:67 >> + WebString initialSdp() const; > > nit: initialSdp -> initialSDP Fixed. >> 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? Silly of me but WebSessionDescriptionDescriptor just sounds awkward. So since the FooDescriptor classes are just platform representations of Foo I though WebFoo were appropriate but that just causes confusion. Fixed.

Created attachment 132582 [details] Patch

For what it's worth, we usually document the WebKit API a bit more than the IDL files. There's an obvious mapping from the IDL files to specifications, so it's easy to look up things that you don't know. For the WebKit API, the mapping is less clear. Maybe it would make sense to add comments that link the reader to the appropriate part of the specification? I know the spec is moving around a bit, so we'll likely need to update the links at some point.

How about I make a solemn oath to add documentation to the the WebFoo files once the JSEP stuff ends up in the in the specification? Right now any link would change for every change in the draft.

Created attachment 132627 [details] Patch

I have now renamed WebIceCandidate to WebICECandidate, and will rename IceCandidateDescriptor in an follow-up patch.

(In reply to comment #10) > How about I make a solemn oath to add documentation to the the WebFoo files once the JSEP stuff ends up in the in the specification? Right now any link would change for every change in the draft. OK, fair to leave out links. Perhaps you can just add some light documentation then.

Created attachment 132803 [details] Patch

Added some comments.

Comment on attachment 132803 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=132803&action=review > Source/WebKit/chromium/ChangeLog:263 > +<<<<<<< HEAD merge conflict. > Source/WebKit/chromium/public/platform/WebSessionDescriptionDescriptor.h:43 > + nit: no new line here

Created attachment 133021 [details] Patch

Comment on attachment 132803 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=132803&action=review >> Source/WebKit/chromium/ChangeLog:263 >> +<<<<<<< HEAD > > merge conflict. Ooops. Fixed. >> Source/WebKit/chromium/public/platform/WebSessionDescriptionDescriptor.h:43 >> + > > nit: no new line here OK, most existing files have one though. Will remember not to add it in the future.

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 133021 [details] Patch Forwarding fishd's R+

Comment on attachment 133021 [details] Patch Clearing flags on attachment: 133021 Committed r111577: <http://trac.webkit.org/changeset/111577>

All reviewed patches have been landed. Closing bug.