Bug 81657 – MediaStream API (JSEP): Introducing PeerConnection00 and IceCallback

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore JavaScript

[Tommy Widenflycht]

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

[Tommy Widenflycht]

Created attachment 132814 [details]
Patch

[Philippe Normand]

Comment on attachment 132814 [details]
Patch

Attachment 132814 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/12009232

[Adam Barth]

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.

[Tommy Widenflycht]

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.

[Tommy Widenflycht]

Created attachment 132875 [details]
Patch

[Tommy Widenflycht]

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.

[Tommy Widenflycht]

Created attachment 132879 [details]
Patch

[Philippe Normand]

Comment on attachment 132879 [details]
Patch

Attachment 132879 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/12026002

[Adam Barth]

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

[Adam Barth]

Comment on attachment 132879 [details]
Patch

The build error has something to do with your Ice callback, not with the PeerConnection00 constructor.

[Tommy Widenflycht]

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.

[Tommy Widenflycht]

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.

[Tommy Widenflycht]

Created attachment 133452 [details]
Patch

[Tommy Widenflycht]

Created attachment 133475 [details]
Patch

[Tommy Widenflycht]

Everything's ready for a proper review :)

[Adam Barth]

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?

[WebKit Review Bot]

Comment on attachment 133475 [details]
Patch

Clearing flags on attachment: 133475

Committed r111876: <http://trac.webkit.org/changeset/111876>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Tommy Widenflycht]

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.

[Adam Barth]

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

[Tommy Widenflycht]

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

[Adam Barth]

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.

[Tommy Widenflycht]

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.

[Kentaro Hara]

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