Summary: | [WebRTC] Introduce libwebrtc abstraction for WK1/WK2 implementations | ||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | youenn fablet <youennf> | ||||||||||||||||||||||||||
Component: | WebCore Misc. | Assignee: | youenn fablet <youennf> | ||||||||||||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||||||||||||
Severity: | Normal | CC: | achristensen, beidson, buildbot, commit-queue, ossy, rniwa | ||||||||||||||||||||||||||
Priority: | P2 | ||||||||||||||||||||||||||||
Version: | WebKit Nightly Build | ||||||||||||||||||||||||||||
Hardware: | Unspecified | ||||||||||||||||||||||||||||
OS: | Unspecified | ||||||||||||||||||||||||||||
Bug Depends on: | |||||||||||||||||||||||||||||
Bug Blocks: | 167289, 167305, 167306 | ||||||||||||||||||||||||||||
Attachments: |
|
Description
youenn fablet
2017-01-22 11:16:54 PST
Created attachment 299474 [details]
Patch
Created attachment 299555 [details]
Patch
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.
Created attachment 299556 [details]
Adding PageConfiguration integration
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 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) Created attachment 299603 [details]
Patch
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 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. Created attachment 299647 [details]
Patch
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.
Created attachment 299648 [details]
Patch
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.
Created attachment 299651 [details]
Patch
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.
Created attachment 299704 [details]
Patch
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 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 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
Created attachment 299711 [details]
Trying to fix EFL/GTK builds
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 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? Created attachment 299718 [details]
Patch
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.
Created attachment 299720 [details]
Patch
(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. I found it, here you are - https://trac.webkit.org/changeset/211208 Thanks, Ossy! |