Bug 81657 - MediaStream API (JSEP): Introducing PeerConnection00 and IceCallback
: MediaStream API (JSEP): Introducing PeerConnection00 and IceCallback
Status: RESOLVED FIXED
: WebKit
WebCore JavaScript
: 528+ (Nightly build)
: All All
: P2 Normal
Assigned To:
:
:
: 82045 82046
: 80589
  Show dependency treegraph
 
Reported: 2012-03-20 06:31 PST by
Modified: 2012-03-29 02:12 PST (History)


Attachments
Patch (40.01 KB, patch)
2012-03-20 06:39 PST, Tommy Widenflycht
no flags Review Patch | Details | Formatted Diff | Diff
Patch (43.64 KB, patch)
2012-03-20 12:14 PST, Tommy Widenflycht
no flags Review Patch | Details | Formatted Diff | Diff
Patch (43.66 KB, patch)
2012-03-20 12:33 PST, Tommy Widenflycht
no flags Review Patch | Details | Formatted Diff | Diff
Patch (44.01 KB, patch)
2012-03-23 03:31 PST, Tommy Widenflycht
no flags Review Patch | Details | Formatted Diff | Diff
Patch (41.31 KB, patch)
2012-03-23 06:55 PST, Tommy Widenflycht
no flags Review Patch | Details | Formatted Diff | Diff


Note

You need to log in before you can comment on or make changes to this bug.


Description From 2012-03-20 06:31:37 PST
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 From 2012-03-20 06:39:51 PST -------
Created an attachment (id=132814) [details]
Patch
------- Comment #2 From 2012-03-20 09:23:30 PST -------
(From update of attachment 132814 [details])
Attachment 132814 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/12009232
------- Comment #3 From 2012-03-20 11:39:39 PST -------
(From update of attachment 132814 [details])
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 From 2012-03-20 12:12:43 PST -------
(From update of attachment 132814 [details])
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 From 2012-03-20 12:14:09 PST -------
Created an attachment (id=132875) [details]
Patch
------- Comment #6 From 2012-03-20 12:15:45 PST -------
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 From 2012-03-20 12:33:17 PST -------
Created an attachment (id=132879) [details]
Patch
------- Comment #8 From 2012-03-20 12:37:42 PST -------
(From update of attachment 132879 [details])
Attachment 132879 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/12026002
------- Comment #9 From 2012-03-20 12:55:43 PST -------
(From update of attachment 132879 [details])
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 From 2012-03-20 12:57:40 PST -------
(From update of attachment 132879 [details])
The build error has something to do with your Ice callback, not with the PeerConnection00 constructor.
------- Comment #11 From 2012-03-20 13:00:53 PST -------
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] [details])
> The build error has something to do with your Ice callback, not with the PeerConnection00 constructor.
------- Comment #12 From 2012-03-23 03:26:54 PST -------
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 From 2012-03-23 03:31:25 PST -------
Created an attachment (id=133452) [details]
Patch
------- Comment #14 From 2012-03-23 06:55:10 PST -------
Created an attachment (id=133475) [details]
Patch
------- Comment #15 From 2012-03-23 07:57:01 PST -------
Everything's ready for a proper review :)
------- Comment #16 From 2012-03-23 09:30:24 PST -------
(From update of attachment 133475 [details])
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 From 2012-03-23 10:16:24 PST -------
(From update of attachment 133475 [details])
Clearing flags on attachment: 133475

Committed r111876: <http://trac.webkit.org/changeset/111876>
------- Comment #18 From 2012-03-23 10:16:31 PST -------
All reviewed patches have been landed.  Closing bug.
------- Comment #19 From 2012-03-28 07:50:28 PST -------
(From update of attachment 133475 [details])
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 From 2012-03-28 10:58:47 PST -------
> 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 From 2012-03-29 01:52:15 PST -------
(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 From 2012-03-29 01:59:28 PST -------
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 From 2012-03-29 02:10:40 PST -------
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 From 2012-03-29 02:12:07 PST -------
(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).