Bug 81339 - [chromium] MediaStream API (JSEP): Introducing WebSessionDescription and WebICECandidate
Summary: [chromium] MediaStream API (JSEP): Introducing WebSessionDescription and WebI...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit API (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Tommy Widenflycht
URL:
Keywords:
Depends on:
Blocks: 80589
  Show dependency treegraph
 
Reported: 2012-03-16 05:32 PDT by Tommy Widenflycht
Modified: 2012-03-21 10:55 PDT (History)
7 users (show)

See Also:


Attachments
Patch (16.53 KB, patch)
2012-03-16 05:39 PDT, Tommy Widenflycht
no flags Details | Formatted Diff | Diff
Patch (25.54 KB, patch)
2012-03-19 07:48 PDT, Tommy Widenflycht
no flags Details | Formatted Diff | Diff
Patch (25.54 KB, patch)
2012-03-19 12:24 PDT, Tommy Widenflycht
no flags Details | Formatted Diff | Diff
Patch (28.68 KB, patch)
2012-03-20 04:58 PDT, Tommy Widenflycht
no flags Details | Formatted Diff | Diff
Patch (26.31 KB, patch)
2012-03-21 05:26 PDT, Tommy Widenflycht
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Tommy Widenflycht 2012-03-16 05:32:28 PDT
Simple WebKit representations of the WebCore/platform versions.
Comment 1 Tommy Widenflycht 2012-03-16 05:39:25 PDT
Created attachment 132260 [details]
Patch
Comment 2 WebKit Review Bot 2012-03-16 05:49:35 PDT
Please wait for approval from fishd@chromium.org before submitting because this patch contains changes to the Chromium public API.
Comment 3 Adam Barth 2012-03-16 06:50:58 PDT
Comment on attachment 132260 [details]
Patch

This looks fine, but we need fishd to review.
Comment 4 Darin Fisher (:fishd, Google) 2012-03-16 13:25:07 PDT
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?
Comment 5 Adam Barth 2012-03-16 13:31:26 PDT
> 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 6 Tommy Widenflycht 2012-03-19 07:26:27 PDT
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 7 Tommy Widenflycht 2012-03-19 07:39:28 PDT
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.
Comment 8 Tommy Widenflycht 2012-03-19 07:48:10 PDT
Created attachment 132582 [details]
Patch
Comment 9 Adam Barth 2012-03-19 10:32:08 PDT
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.
Comment 10 Tommy Widenflycht 2012-03-19 11:12:31 PDT
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.
Comment 11 Tommy Widenflycht 2012-03-19 12:24:51 PDT
Created attachment 132627 [details]
Patch
Comment 12 Tommy Widenflycht 2012-03-19 12:27:30 PDT
I have now renamed WebIceCandidate to WebICECandidate, and will rename IceCandidateDescriptor in an follow-up patch.
Comment 13 Darin Fisher (:fishd, Google) 2012-03-19 14:03:29 PDT
(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.
Comment 14 Tommy Widenflycht 2012-03-20 04:58:33 PDT
Created attachment 132803 [details]
Patch
Comment 15 Tommy Widenflycht 2012-03-20 05:01:10 PDT
Added some comments.
Comment 16 Darin Fisher (:fishd, Google) 2012-03-20 20:42:27 PDT
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
Comment 17 Tommy Widenflycht 2012-03-21 05:26:34 PDT
Created attachment 133021 [details]
Patch
Comment 18 Tommy Widenflycht 2012-03-21 05:28:07 PDT
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.
Comment 19 WebKit Review Bot 2012-03-21 05:30:20 PDT
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 20 Adam Barth 2012-03-21 10:04:07 PDT
Comment on attachment 133021 [details]
Patch

Forwarding fishd's R+
Comment 21 WebKit Review Bot 2012-03-21 10:55:22 PDT
Comment on attachment 133021 [details]
Patch

Clearing flags on attachment: 133021

Committed r111577: <http://trac.webkit.org/changeset/111577>
Comment 22 WebKit Review Bot 2012-03-21 10:55:29 PDT
All reviewed patches have been landed.  Closing bug.