Bug 167294 - [WebRTC] Introduce libwebrtc abstraction for WK1/WK2 implementations
Summary: [WebRTC] Introduce libwebrtc abstraction for WK1/WK2 implementations
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: youenn fablet
URL:
Keywords:
Depends on:
Blocks: 167289 167305 167306
  Show dependency treegraph
 
Reported: 2017-01-22 11:16 PST by youenn fablet
Modified: 2017-01-26 11:12 PST (History)
6 users (show)

See Also:


Attachments
Patch (15.57 KB, patch)
2017-01-22 11:27 PST, youenn fablet
no flags Details | Formatted Diff | Diff
Patch (15.16 KB, patch)
2017-01-23 17:08 PST, youenn fablet
no flags Details | Formatted Diff | Diff
Adding PageConfiguration integration (18.43 KB, patch)
2017-01-23 17:16 PST, youenn fablet
no flags Details | Formatted Diff | Diff
Patch (21.95 KB, patch)
2017-01-24 08:43 PST, youenn fablet
no flags Details | Formatted Diff | Diff
Patch (33.19 KB, patch)
2017-01-24 16:16 PST, youenn fablet
no flags Details | Formatted Diff | Diff
Patch (33.19 KB, patch)
2017-01-24 16:23 PST, youenn fablet
no flags Details | Formatted Diff | Diff
Patch (33.19 KB, patch)
2017-01-24 16:28 PST, youenn fablet
no flags Details | Formatted Diff | Diff
Patch (32.48 KB, patch)
2017-01-25 07:54 PST, youenn fablet
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews101 for mac-elcapitan (855.69 KB, application/zip)
2017-01-25 08:56 PST, Build Bot
no flags Details
Trying to fix EFL/GTK builds (40.05 KB, patch)
2017-01-25 09:26 PST, youenn fablet
no flags Details | Formatted Diff | Diff
Patch (35.71 KB, patch)
2017-01-25 11:18 PST, Alex Christensen
no flags Details | Formatted Diff | Diff
Patch (35.91 KB, patch)
2017-01-25 11:34 PST, Alex Christensen
achristensen: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description youenn fablet 2017-01-22 11:16:54 PST
WK1 libwebrtc will do networking on its own while WK2 libwebrtc will delegate that to the network process.
Comment 1 youenn fablet 2017-01-22 11:27:53 PST
Created attachment 299474 [details]
Patch
Comment 2 youenn fablet 2017-01-23 17:08:27 PST
Created attachment 299555 [details]
Patch
Comment 3 WebKit Commit Bot 2017-01-23 17:10:10 PST
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.
Comment 4 youenn fablet 2017-01-23 17:16:20 PST
Created attachment 299556 [details]
Adding PageConfiguration integration
Comment 5 WebKit Commit Bot 2017-01-23 17:19:13 PST
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.
Comment 6 Alex Christensen 2017-01-23 23:54:58 PST
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)
Comment 7 youenn fablet 2017-01-24 08:43:42 PST
Created attachment 299603 [details]
Patch
Comment 8 WebKit Commit Bot 2017-01-24 08:46:07 PST
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.
Comment 9 Alex Christensen 2017-01-24 11:56:45 PST
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.
Comment 10 youenn fablet 2017-01-24 16:16:26 PST
Created attachment 299647 [details]
Patch
Comment 11 WebKit Commit Bot 2017-01-24 16:18:02 PST
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.
Comment 12 youenn fablet 2017-01-24 16:23:49 PST
Created attachment 299648 [details]
Patch
Comment 13 WebKit Commit Bot 2017-01-24 16:27:16 PST
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.
Comment 14 youenn fablet 2017-01-24 16:28:17 PST
Created attachment 299651 [details]
Patch
Comment 15 WebKit Commit Bot 2017-01-24 16:29:49 PST
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.
Comment 16 youenn fablet 2017-01-25 07:54:44 PST
Created attachment 299704 [details]
Patch
Comment 17 WebKit Commit Bot 2017-01-25 07:57:13 PST
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.
Comment 18 Build Bot 2017-01-25 08:56:43 PST
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
Comment 19 Build Bot 2017-01-25 08:56:46 PST
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
Comment 20 youenn fablet 2017-01-25 09:26:03 PST
Created attachment 299711 [details]
Trying to fix EFL/GTK builds
Comment 21 WebKit Commit Bot 2017-01-25 09:28:41 PST
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.
Comment 22 Alex Christensen 2017-01-25 10:43:57 PST
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?
Comment 23 Alex Christensen 2017-01-25 11:18:54 PST
Created attachment 299718 [details]
Patch
Comment 24 Alex Christensen 2017-01-25 11:20:52 PST
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.
Comment 25 Alex Christensen 2017-01-25 11:34:22 PST
Created attachment 299720 [details]
Patch
Comment 26 Alex Christensen 2017-01-25 13:01:41 PST
http://trac.webkit.org/r211161
Comment 27 Csaba Osztrogonác 2017-01-26 02:20:04 PST
(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.
Comment 28 Csaba Osztrogonác 2017-01-26 03:02:20 PST
I found it, here you are - https://trac.webkit.org/changeset/211208
Comment 29 Alex Christensen 2017-01-26 11:12:02 PST
Thanks, Ossy!