Bug 81657

Created attachment 132814 [details] Patch

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

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

Created attachment 132875 [details] Patch

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.

Created attachment 132879 [details] Patch

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

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 on attachment 132879 [details] Patch The build error has something to do with your Ice callback, not with the PeerConnection00 constructor.

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.

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.

Created attachment 133452 [details] Patch

Created attachment 133475 [details] Patch

Everything's ready for a proper review :)

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 on attachment 133475 [details] Patch Clearing flags on attachment: 133475 Committed r111876: <http://trac.webkit.org/changeset/111876>

All reviewed patches have been landed. Closing bug.

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.

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

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

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.

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.

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