Bug 95839

Summary: MediaStream API: Add the local and remote description functionality to RTCPeerConnection
Product: WebKit Reporter: Tommy Widenflycht <tommyw>
Component: WebCore Misc.Assignee: Tommy Widenflycht <tommyw>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, bashi, dglazkov, eric.carlson, feature-media-reviews, fishd, gns, gtk-ews, gyuyoung.kim, jamesr, ojan, rakuco, tkent+wkapi, webkit.review.bot, xan.lopez
Priority: P2 Keywords: WebExposed
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Bug Depends on: 95920    
Bug Blocks: 80589    
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch none

Description Tommy Widenflycht 2012-09-05 03:50:03 PDT
This is the last large patch missing, just a few small ones still lined up.
Comment 1 Tommy Widenflycht 2012-09-05 04:36:56 PDT
Created attachment 162217 [details]
Patch
Comment 2 WebKit Review Bot 2012-09-05 04:40:33 PDT
Please wait for approval from abarth@webkit.org, dglazkov@chromium.org, fishd@chromium.org, jamesr@chromium.org or tkent@chromium.org before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.
Comment 3 kov's GTK+ EWS bot 2012-09-05 05:42:52 PDT
Comment on attachment 162217 [details]
Patch

Attachment 162217 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/13745835
Comment 4 Tommy Widenflycht 2012-09-05 06:16:10 PDT
Created attachment 162233 [details]
Patch
Comment 5 Adam Barth 2012-09-05 10:29:41 PDT
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.
Comment 6 WebKit Review Bot 2012-09-05 11:00:51 PDT
Comment on attachment 162233 [details]
Patch

Clearing flags on attachment: 162233

Committed r127612: <http://trac.webkit.org/changeset/127612>
Comment 7 WebKit Review Bot 2012-09-05 11:00:57 PDT
All reviewed patches have been landed.  Closing bug.
Comment 8 Kenichi Ishibashi 2012-09-05 16:37:49 PDT
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.
Comment 9 Kenichi Ishibashi 2012-09-05 16:39:32 PDT
I'm trying to fix it:
http://trac.webkit.org/changeset/127660
Comment 10 Kenichi Ishibashi 2012-09-05 18:20:01 PDT
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
Comment 11 Tommy Widenflycht 2012-09-06 01:51:51 PDT
(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.
Comment 12 Kenichi Ishibashi 2012-09-06 01:57:52 PDT
(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
Comment 13 Tommy Widenflycht 2012-09-06 02:32:34 PDT
> 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.
Comment 14 Kenichi Ishibashi 2012-09-06 02:34:15 PDT
(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.
Comment 15 Tommy Widenflycht 2012-09-06 03:04:58 PDT
Created attachment 162462 [details]
Patch
Comment 16 Tommy Widenflycht 2012-09-06 03:08:24 PDT
Created attachment 162464 [details]
Patch
Comment 17 Tommy Widenflycht 2012-09-06 03:09:52 PDT
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.
Comment 18 Tommy Widenflycht 2012-09-06 03:12:11 PDT
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.
Comment 19 Adam Barth 2012-09-06 10:36:50 PDT
Comment on attachment 162464 [details]
Patch

Thanks.
Comment 20 WebKit Review Bot 2012-09-06 11:50:34 PDT
Comment on attachment 162464 [details]
Patch

Clearing flags on attachment: 162464

Committed r127766: <http://trac.webkit.org/changeset/127766>
Comment 21 WebKit Review Bot 2012-09-06 11:50:39 PDT
All reviewed patches have been landed.  Closing bug.