Bug 81657 - MediaStream API (JSEP): Introducing PeerConnection00 and IceCallback
: MediaStream API (JSEP): Introducing PeerConnection00 and IceCallback
Status: RESOLVED FIXED
Product: WebKit
Classification: Unclassified
Component: WebCore JavaScript
: 528+ (Nightly build)
: All All
: P2 Normal
Assigned To: Tommy Widenflycht
:
Depends on: 82045 82046
Blocks: 80589
  Show dependency treegraph
 
Reported: 2012-03-20 06:31 PDT by Tommy Widenflycht
Modified: 2012-03-29 02:12 PDT (History)
9 users (show)

See Also:


Attachments
Patch (40.01 KB, patch)
2012-03-20 06:39 PDT, Tommy Widenflycht
no flags Details | Formatted Diff | Diff
Patch (43.64 KB, patch)
2012-03-20 12:14 PDT, Tommy Widenflycht
no flags Details | Formatted Diff | Diff
Patch (43.66 KB, patch)
2012-03-20 12:33 PDT, Tommy Widenflycht
no flags Details | Formatted Diff | Diff
Patch (44.01 KB, patch)
2012-03-23 03:31 PDT, Tommy Widenflycht
no flags Details | Formatted Diff | Diff
Patch (41.31 KB, patch)
2012-03-23 06:55 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-20 06:31:37 PDT
Last major WebCore patch for the JSEP PeerConnection, together with its associated IceCallback (they both depend on each other and IceCallback has very few lines of real code).
Comment 1 Tommy Widenflycht 2012-03-20 06:39:51 PDT
Created attachment 132814 [details]
Patch
Comment 2 Philippe Normand 2012-03-20 09:23:30 PDT
Comment on attachment 132814 [details]
Patch

Attachment 132814 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/12009232
Comment 3 Adam Barth 2012-03-20 11:39:39 PDT
Comment on attachment 132814 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=132814&action=review

> Source/WebCore/Modules/mediastream/PeerConnection00.cpp:294
> +    // FIXME(tommyw): When the spec says what the mediaStreamHints should look like send it down.

FIXME(tommyw) => FIXME

> Source/WebCore/Modules/mediastream/PeerConnection00.cpp:404
> +bool PeerConnection00::canSuspend() const
> +{
> +    return false;
> +}

Isn't this the default?  I don't think you need to override this function.

> Source/WebCore/Modules/mediastream/PeerConnection00.cpp:450
> +    default:
> +        ASSERT_NOT_REACHED();
> +        break;

Normally we leave off the default case so that the compiler will complain if you forget a case statement.

> Source/WebCore/Modules/mediastream/PeerConnection00.h:93
> +    PassRefPtr<SessionDescription> createOffer();
> +    PassRefPtr<SessionDescription> createOffer(const String& mediaHints);
> +    PassRefPtr<SessionDescription> createAnswer(const String& offer);
> +    PassRefPtr<SessionDescription> createAnswer(const String& offer, const String& mediaHints);

You can use default parameter values if you'd rather.

> Source/WebCore/Modules/mediastream/PeerConnection00.h:134
> +    virtual bool canSuspend() const;
> +    virtual void stop();

Consider using OVERRIDE when overriding virtual methods.  It's not required, but we've been using it more and more.

> Source/WebCore/Modules/mediastream/PeerConnection00.idl:40
> +        // FIXME(tommyw): Make mediaHints an object

FIXME(tommyw) => FIXME

For whatever reason, WebKit doesn't put user names in FIXME.
Comment 4 Tommy Widenflycht 2012-03-20 12:12:43 PDT
Comment on attachment 132814 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=132814&action=review

>> Source/WebCore/Modules/mediastream/PeerConnection00.cpp:294
>> +    // FIXME(tommyw): When the spec says what the mediaStreamHints should look like send it down.
> 
> FIXME(tommyw) => FIXME

Fixed.

>> Source/WebCore/Modules/mediastream/PeerConnection00.cpp:404
>> +}
> 
> Isn't this the default?  I don't think you need to override this function.

You are right, deleted function.

>> Source/WebCore/Modules/mediastream/PeerConnection00.cpp:450
>> +        break;
> 
> Normally we leave off the default case so that the compiler will complain if you forget a case statement.

Fixed.

>> Source/WebCore/Modules/mediastream/PeerConnection00.h:93
>> +    PassRefPtr<SessionDescription> createAnswer(const String& offer, const String& mediaHints);
> 
> You can use default parameter values if you'd rather.

OK, will do that once I switch the temp String type to "objects-as-maps"

>> Source/WebCore/Modules/mediastream/PeerConnection00.h:134
>> +    virtual void stop();
> 
> Consider using OVERRIDE when overriding virtual methods.  It's not required, but we've been using it more and more.

Good to know! Have added OVERRIDE.

>> Source/WebCore/Modules/mediastream/PeerConnection00.idl:40
>> +        // FIXME(tommyw): Make mediaHints an object
> 
> FIXME(tommyw) => FIXME
> 
> For whatever reason, WebKit doesn't put user names in FIXME.

Fixed.
Comment 5 Tommy Widenflycht 2012-03-20 12:14:09 PDT
Created attachment 132875 [details]
Patch
Comment 6 Tommy Widenflycht 2012-03-20 12:15:45 PDT
To fix the GTK build error it seems that one have to add a custom constructor for JS when callbacks are involved. DeprecatedPeerConnection has one so I did the same for PeerConenction00.
Comment 7 Tommy Widenflycht 2012-03-20 12:33:17 PDT
Created attachment 132879 [details]
Patch
Comment 8 Philippe Normand 2012-03-20 12:37:42 PDT
Comment on attachment 132879 [details]
Patch

Attachment 132879 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/12026002
Comment 9 Adam Barth 2012-03-20 12:55:43 PDT
Comment on attachment 132879 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=132879&action=review

> Source/WebCore/Modules/mediastream/PeerConnection00.idl:38
> +        JSCustomConstructor,

You shouldn't need a custom constructor...
Comment 10 Adam Barth 2012-03-20 12:57:40 PDT
Comment on attachment 132879 [details]
Patch

The build error has something to do with your Ice callback, not with the PeerConnection00 constructor.
Comment 11 Tommy Widenflycht 2012-03-20 13:00:53 PDT
Yeah, I noticed. Think the JS code generator does something wrong.
I am currently setting up an GTK build environment to find out.

(In reply to comment #10)
> (From update of attachment 132879 [details])
> The build error has something to do with your Ice callback, not with the PeerConnection00 constructor.
Comment 12 Tommy Widenflycht 2012-03-23 03:26:54 PDT
I found two issues with the JS code generator :/ and for verification purposes I will upload my patch again with my fixes. When I have verified that everything seems to work fine I'll create proper bugs for it.
Comment 13 Tommy Widenflycht 2012-03-23 03:31:25 PDT
Created attachment 133452 [details]
Patch
Comment 14 Tommy Widenflycht 2012-03-23 06:55:10 PDT
Created attachment 133475 [details]
Patch
Comment 15 Tommy Widenflycht 2012-03-23 07:57:01 PDT
Everything's ready for a proper review :)
Comment 16 Adam Barth 2012-03-23 09:30:24 PDT
Comment on attachment 133475 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=133475&action=review

> Source/WebCore/Modules/mediastream/PeerConnection00.cpp:101
> +        if (*i == "audio")

This are supposed to be case sensitive?

> Source/WebCore/Modules/mediastream/PeerConnection00.cpp:126
> +    return SessionDescription::create(descriptor);

Should this be descriptor.release()?  It looks like you're transferring ownership of descriptor into this object.

> Source/WebCore/Modules/mediastream/PeerConnection00.cpp:141
> +    return SessionDescription::create(descriptor);

Should this be descriptor.release()?  It looks like you're transferring ownership of descriptor into this object.

> Source/WebCore/Modules/mediastream/PeerConnection00.cpp:234
> +    else if (options == "no_relay")

Again, these are also supposed to be case sensitive?
Comment 17 WebKit Review Bot 2012-03-23 10:16:24 PDT
Comment on attachment 133475 [details]
Patch

Clearing flags on attachment: 133475

Committed r111876: <http://trac.webkit.org/changeset/111876>
Comment 18 WebKit Review Bot 2012-03-23 10:16:31 PDT
All reviewed patches have been landed.  Closing bug.
Comment 19 Tommy Widenflycht 2012-03-28 07:50:28 PDT
Comment on attachment 133475 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=133475&action=review

>> Source/WebCore/Modules/mediastream/PeerConnection00.cpp:101
>> +        if (*i == "audio")
> 
> This are supposed to be case sensitive?

yes, and it is a temporary simplification (the standard says that it should be objects like { audio: True, video: False }

>> Source/WebCore/Modules/mediastream/PeerConnection00.cpp:126
>> +    return SessionDescription::create(descriptor);
> 
> Should this be descriptor.release()?  It looks like you're transferring ownership of descriptor into this object.

Good catch, will fix.

>> Source/WebCore/Modules/mediastream/PeerConnection00.cpp:141
>> +    return SessionDescription::create(descriptor);
> 
> Should this be descriptor.release()?  It looks like you're transferring ownership of descriptor into this object.

Good catch, will fix.

>> Source/WebCore/Modules/mediastream/PeerConnection00.cpp:234
>> +    else if (options == "no_relay")
> 
> Again, these are also supposed to be case sensitive?

se above.
Comment 20 Adam Barth 2012-03-28 10:58:47 PDT
> yes, and it is a temporary simplification (the standard says that it should be objects like { audio: True, video: False }

There's support in the code generator for that sort of thing.  It should be easy to use if you want.
Comment 21 Tommy Widenflycht 2012-03-29 01:52:15 PDT
(In reply to comment #20)
> > yes, and it is a temporary simplification (the standard says that it should be objects like { audio: True, video: False }
> 
> There's support in the code generator for that sort of thing.  It should be easy to use if you want.

Great! Can you give me a hint where to start looking? When I bug through the code all usages of Objects-as-maps" used custom bindings where the custom code converts the object to a string and then the WebCore code parses that string.
Comment 22 Adam Barth 2012-03-29 01:59:28 PDT
Kentaro would know, but I think you just declare the type as Options or Dictionary or something and the type conversion happens automatically for you.  I can look in ore detail tomorrow.
Comment 23 Tommy Widenflycht 2012-03-29 02:10:40 PDT
Dictionary seems to be the ticket. Neat!

(In reply to comment #22)
> Kentaro would know, but I think you just declare the type as Options or Dictionary or something and the type conversion happens automatically for you.  I can look in ore detail tomorrow.
Comment 24 Kentaro Hara 2012-03-29 02:12:07 PDT
(In reply to comment #22)
> Kentaro would know, but I think you just declare the type as Options or Dictionary or something and the type conversion happens automatically for you.  I can look in ore detail tomorrow.

In V8, you can use Dictionary.get() (WebCore/bindings/v8/Dictionary.h). In JSC, we want to support Dictionary.get() (WebCore/bindings/js/Dictionary.h), but it is not yet implemented. At present, in JSC you need to use JSDictionary.tryGetProperty() (WebCore/bindings/js/JSDictionary.h).

Event constructors are using such dictionaries (e.g. new Event("type", { bubbles: true, cancelable: true })). You can look at the generated code for Event constructors (V8Event.cpp, JSEvent.cpp).