Bug 167294 – [WebRTC] Introduce libwebrtc abstraction for WK1/WK2 implementations

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[youenn fablet]

WK1 libwebrtc will do networking on its own while WK2 libwebrtc will delegate that to the network process.

[youenn fablet]

Created attachment 299474 [details]
Patch

[youenn fablet]

Created attachment 299555 [details]
Patch

[WebKit Commit Bot]

Attachment 299555 [details] did not pass style-queue:


ERROR: Source/WebCore/platform/mediastream/libwebrtc/LibWebRTCUtils.cpp:59:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
Total errors found: 1 in 5 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[youenn fablet]

Created attachment 299556 [details]
Adding PageConfiguration integration

[WebKit Commit Bot]

Attachment 299556 [details] did not pass style-queue:


ERROR: Source/WebCore/platform/mediastream/libwebrtc/LibWebRTCUtils.cpp:59:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
Total errors found: 1 in 8 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Alex Christensen]

Comment on attachment 299556 [details]
Adding PageConfiguration integration

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

> Source/WebCore/page/Page.cpp:205
> +    , m_libWebRTCClient(*pageConfiguration.libWebRTCClient)

When the PageConfiguration goes out of scope, this will be pointing to freed memory.

> Source/WebCore/page/Page.h:642
> +    LibWebRTCClient& m_libWebRTCClient;

Let's have the Page keep a UniqueRef<PeerConnectionProvider>

> Source/WebCore/platform/mediastream/libwebrtc/LibWebRTCClient.h:42
> +class WEBCORE_EXPORT LibWebRTCClient {
> +public:
> +    virtual rtc::scoped_refptr<webrtc::PeerConnectionInterface> createPeerConnection(webrtc::PeerConnectionObserver&) = 0;

I think this should be called a PeerConnectionProvider.  It should be inside of ENABLE(WEBRTC) except the createPeerConnection function, which should be inside a USE(LIBWEBRTC)

[youenn fablet]

Created attachment 299603 [details]
Patch

[WebKit Commit Bot]

Attachment 299603 [details] did not pass style-queue:


ERROR: Source/WebCore/platform/mediastream/libwebrtc/LibWebRTCUtils.cpp:59:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
Total errors found: 1 in 6 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Alex Christensen]

Comment on attachment 299603 [details]
Patch

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

> Source/WebCore/platform/mediastream/libwebrtc/LibWebRTCClient.h:40
> +class WEBCORE_EXPORT LibWebRTCClient {

My review feedback was not addressed.

[youenn fablet]

Created attachment 299647 [details]
Patch

[WebKit Commit Bot]

Attachment 299647 [details] did not pass style-queue:


ERROR: Source/WebCore/platform/mediastream/libwebrtc/LibWebRTCUtils.cpp:59:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
ERROR: Source/WebCore/page/PageConfiguration.cpp:53:  Wrong number of spaces before statement. (expected: 8)  [whitespace/indent] [4]
Total errors found: 2 in 15 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[youenn fablet]

Created attachment 299648 [details]
Patch

[WebKit Commit Bot]

Attachment 299648 [details] did not pass style-queue:


ERROR: Source/WebCore/platform/mediastream/libwebrtc/LibWebRTCUtils.cpp:59:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
ERROR: Source/WebCore/page/PageConfiguration.cpp:53:  Wrong number of spaces before statement. (expected: 8)  [whitespace/indent] [4]
Total errors found: 2 in 15 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[youenn fablet]

Created attachment 299651 [details]
Patch

[WebKit Commit Bot]

Attachment 299651 [details] did not pass style-queue:


ERROR: Source/WebCore/platform/mediastream/libwebrtc/LibWebRTCUtils.cpp:59:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
ERROR: Source/WebCore/page/PageConfiguration.cpp:53:  Wrong number of spaces before statement. (expected: 8)  [whitespace/indent] [4]
Total errors found: 2 in 15 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[youenn fablet]

Created attachment 299704 [details]
Patch

[WebKit Commit Bot]

Attachment 299704 [details] did not pass style-queue:


ERROR: Source/WebCore/platform/mediastream/libwebrtc/LibWebRTCUtils.cpp:59:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
ERROR: Source/WebCore/page/PageConfiguration.cpp:53:  Wrong number of spaces before statement. (expected: 8)  [whitespace/indent] [4]
Total errors found: 2 in 15 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Build Bot]

Comment on attachment 299704 [details]
Patch

Attachment 299704 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/2947146

New failing tests:
imported/w3c/web-platform-tests/html/browsers/history/the-location-interface/location-protocol-setter-non-broken.html

[Build Bot]

Created attachment 299709 [details]
Archive of layout-test-results from ews101 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews101  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[youenn fablet]

Created attachment 299711 [details]
Trying to fix EFL/GTK builds

[WebKit Commit Bot]

Attachment 299711 [details] did not pass style-queue:


ERROR: Source/WebCore/platform/mediastream/libwebrtc/LibWebRTCUtils.cpp:59:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
ERROR: Source/WebCore/page/PageConfiguration.cpp:53:  Wrong number of spaces before statement. (expected: 8)  [whitespace/indent] [4]
Total errors found: 2 in 19 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Alex Christensen]

Comment on attachment 299711 [details]
Trying to fix EFL/GTK builds

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

> Source/WebKit/mac/ChangeLog:3
> +        [WebRTC] Add support for WK1 libwebrtc endpoint

Duplicate ChangeLog entry.

> Source/WebCore/platform/mediastream/libwebrtc/LibWebRTCProvider.h:38
> +    virtual rtc::scoped_refptr<webrtc::PeerConnectionInterface> createPeerConnection(webrtc::PeerConnectionObserver&) = 0;

If we make this not purely virtual, then we can probably get rid of LibWebRTCEmptyProvider and the WebLibWebRTCProvider.h in Source/WebKit.

> Source/WebCore/platform/mediastream/libwebrtc/LibWebRTCProvider.h:43
> +class LibWebRTCEmptyProvider final : public LibWebRTCProvider {

I think this should be in EmptyClients.h/cpp like createEmptyEditorClient

> Source/WebCore/platform/mediastream/libwebrtc/LibWebRTCUtils.cpp:60
> +    Function<void()> function;

Maybe this should be called callback to be consistent.

> Source/WebKit2/WebProcess/WebPage/WebPage.cpp:379
> +        , makeUniqueRef<WebCore::LibWebRTCEmptyProvider>()

I assume the next step is to make this return a LibWebRTCProvider that returns something that makes connection proxies, right?

[Alex Christensen]

Created attachment 299718 [details]
Patch

[Alex Christensen]

Comment on attachment 299718 [details]
Patch

This version removes many of the USE(LIBWEBRTC) macros.  If you don't use libwebrtc, you will have a pointer on the Page to an object that does nothing.  I think that's worth having much cleaner code everywhere.

[Alex Christensen]

Created attachment 299720 [details]
Patch

[Alex Christensen]

http://trac.webkit.org/r211161

[Csaba Osztrogonác]

(In reply to comment #26)
> http://trac.webkit.org/r211161

It broke the Apple Mac cmake build:
/Volumes/Data/slave/elcapitan-cmake-debug/build/Source/WebKit/mac/WebView/WebView.mm:160:9: fatal error: 'WebCore/LibWebRTCProvider.h' file not found
#import <WebCore/LibWebRTCProvider.h>
        ^
1 warning and 1 error generated.

I don't know how Apple Mac cmake build generates this magic forwarding 
headers, I let you guys to fix the build you broke yourself.

[Csaba Osztrogonác]

I found it, here you are - https://trac.webkit.org/changeset/211208

[Alex Christensen]

Thanks, Ossy!