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