Bug 81339 – [chromium] MediaStream API (JSEP): Introducing WebSessionDescription and WebICECandidate

• Component: WebKit API
• Status: RESOLVED FIXED

[Tommy Widenflycht]

Simple WebKit representations of the WebCore/platform versions.

[Tommy Widenflycht]

Created attachment 132260 [details]
Patch

[WebKit Review Bot]

Please wait for approval from [e-mail] before submitting because this patch contains changes to the Chromium public API.

[Adam Barth]

Comment on attachment 132260 [details]
Patch

This looks fine, but we need fishd to review.

[Darin Fisher (:fishd, Google)]

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?

[Adam Barth]

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

[Tommy Widenflycht]

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

[Tommy Widenflycht]

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.

[Tommy Widenflycht]

Created attachment 132582 [details]
Patch

[Adam Barth]

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.

[Tommy Widenflycht]

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.

[Tommy Widenflycht]

Created attachment 132627 [details]
Patch

[Tommy Widenflycht]

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

[Darin Fisher (:fishd, Google)]

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

[Tommy Widenflycht]

Created attachment 132803 [details]
Patch

[Tommy Widenflycht]

Added some comments.

[Darin Fisher (:fishd, Google)]

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

[Tommy Widenflycht]

Created attachment 133021 [details]
Patch

[Tommy Widenflycht]

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.

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

[Adam Barth]

Comment on attachment 133021 [details]
Patch

Forwarding fishd's R+

[WebKit Review Bot]

Comment on attachment 133021 [details]
Patch

Clearing flags on attachment: 133021

Committed r111577: <http://trac.webkit.org/changeset/111577>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.