MediaStream API: Add the local and remote description functionality to RTCPeerConnection

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Tommy Widenflycht]

This is the last large patch missing, just a few small ones still lined up.

[Tommy Widenflycht]

Created attachment 162217 [details]
Patch

[WebKit Review Bot]

Please wait for approval from [e-mail], [e-mail], [e-mail], [e-mail] or [e-mail] before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.

[kov's GTK+ EWS bot]

Comment on attachment 162217 [details]
Patch

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

[Tommy Widenflycht]

Created attachment 162233 [details]
Patch

[Adam Barth]

Comment on attachment 162233 [details]
Patch

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

It's a bit unfortunate that we need to build all this machinery just for WebRTC.  The void request seems like something that could potentially be shared between WebRTC and FileSystem, for example.  However, I don't think we need to worry about that at this stage.

Would you be willing to fix up the variable names in a followup patch?

> Source/WebCore/Modules/mediastream/RTCPeerConnection.cpp:220
> +    RefPtr<RTCSessionDescription> desc = RTCSessionDescription::create(descriptor.release());

desc -> please use complete words in variable names.

[WebKit Review Bot]

Comment on attachment 162233 [details]
Patch

Clearing flags on attachment: 162233

Committed r127612: <http://trac.webkit.org/changeset/127612>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Kenichi Ishibashi]

r127612 broke win build.
http://build.chromium.org/p/chromium.webkit/builders/Win%20Builder/builds/27608

33>  RTCPeerConnectionHandlerChromium.cpp
33>..\..\WTF\wtf/PassRefPtr.h(53): error C2027: use of undefined type 'WebCore::RTCVoidRequest'
33>          ..\platform\mediastream\RTCPeerConnectionHandler.h(48) : see declaration of 'WebCore::RTCVoidRequest'
33>          ..\..\WTF\wtf/PassRefPtr.h(68) : see reference to function template instantiation 'void WTF::derefIfNotNull<T>(T *)' being compiled
33>          with
33>          [
33>              T=WebCore::RTCVoidRequest
33>          ]
33>          ..\..\WTF\wtf/PassRefPtr.h(68) : while compiling class template member function 'WTF::PassRefPtr<T>::~PassRefPtr(void)'
33>          with
33>          [
33>              T=WebCore::RTCVoidRequest
33>          ]
33>          ..\platform\mediastream\chromium\RTCPeerConnectionHandlerChromium.cpp(86) : see reference to class template instantiation 'WTF::PassRefPtr<T>' being compiled
33>          with
33>          [
33>              T=WebCore::RTCVoidRequest
33>          ]
33>..\..\WTF\wtf/PassRefPtr.h(53): error C2227: left of '->deref' must point to class/struct/union/generic type
32>  V8AbstractEventListener.cpp
33>
33>Build FAILED.

[Kenichi Ishibashi]

I'm trying to fix it:
http://trac.webkit.org/changeset/127660

[Kenichi Ishibashi]

After some attempts, I gave up to fix it and I'm rolling out the patch. Even if builds succeeded, it seems that there are layout tests failures.
http://build.chromium.org/p/chromium.webkit/builders/Webkit%20Linux/builds/30510

[Tommy Widenflycht]

(In reply to comment #10)
> After some attempts, I gave up to fix it and I'm rolling out the patch. Even if builds succeeded, it seems that there are layout tests failures.
> http://build.chromium.org/p/chromium.webkit/builders/Webkit%20Linux/builds/30510

Yeah, your fix didn't do the right thing at all but thanks for trying.

 The problem is that I don't want a third machine under my desk and since cr-win is so different for some reason it should have its own build bot.

[Kenichi Ishibashi]

(In reply to comment #11)
> (In reply to comment #10)
> > After some attempts, I gave up to fix it and I'm rolling out the patch. Even if builds succeeded, it seems that there are layout tests failures.
> > http://build.chromium.org/p/chromium.webkit/builders/Webkit%20Linux/builds/30510
> 
> Yeah, your fix didn't do the right thing at all but thanks for trying.
> 
>  The problem is that I don't want a third machine under my desk and since cr-win is so different for some reason it should have its own build bot.

You may want to use try.
http://dev.chromium.org/developers/testing/try-server-usage

[Tommy Widenflycht]

> You may want to use try.
> http://dev.chromium.org/developers/testing/try-server-usage

That seems to require chromium committer status, which I don't have.

[Kenichi Ishibashi]

(In reply to comment #13)
> > You may want to use try.
> > http://dev.chromium.org/developers/testing/try-server-usage
> 
> That seems to require chromium committer status, which I don't have.

You can ask a committer to submit a try. See the bottom of the page.

[Tommy Widenflycht]

Created attachment 162462 [details]
Patch

[Tommy Widenflycht]

Created attachment 162464 [details]
Patch

[Tommy Widenflycht]

Comment on attachment 162233 [details]
Patch

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

>> Source/WebCore/Modules/mediastream/RTCPeerConnection.cpp:220
>> +    RefPtr<RTCSessionDescription> desc = RTCSessionDescription::create(descriptor.release());
> 
> desc -> please use complete words in variable names.

Fixed.

[Tommy Widenflycht]

Patch now confirmed compiling and working on Windows, thanks to hbono. The fix was to add a missing header file to RTCPeerConnectionHandlerChromium.cpp

What I don't understand is how the previous patch could compile under cr-linux and cr-mac.

[Adam Barth]

Comment on attachment 162464 [details]
Patch

Thanks.

[WebKit Review Bot]

Comment on attachment 162464 [details]
Patch

Clearing flags on attachment: 162464

Committed r127766: <http://trac.webkit.org/changeset/127766>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.