Bug 95839

Created attachment 162217 [details] Patch

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 on attachment 162217 [details] Patch Attachment 162217 [details] did not pass gtk-ews (gtk): Output: http://queues.webkit.org/results/13745835

Created attachment 162233 [details] Patch

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

All reviewed patches have been landed. Closing bug.

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.

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

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

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

(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

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

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

Created attachment 162462 [details] Patch

Created attachment 162464 [details] Patch

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.

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 on attachment 162464 [details] Patch Thanks.

Comment on attachment 162464 [details] Patch Clearing flags on attachment: 162464 Committed r127766: <http://trac.webkit.org/changeset/127766>

All reviewed patches have been landed. Closing bug.