Bug 167306 – [WebRTC] Add support for WK2 libwebrtc endpoint

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[youenn fablet]

Add support for WK2 libwebrtc endpoint

[youenn fablet]

Created attachment 299515 [details]
Patch

[youenn fablet]

Created attachment 299606 [details]
Patch

[WebKit Commit Bot]

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


ERROR: Source/WebKit2/WebProcess/Network/webrtc/WebPacketSocket.h:48:  enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums.  [readability/enum_casing] [4]
ERROR: Source/WebKit2/NetworkProcess/webrtc/NetworkNetworkManager.h:46:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
ERROR: Source/WebKit2/WebProcess/Network/webrtc/WebPacketSocketFactory.h:63:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
ERROR: Source/WebKit2/Shared/RTCNetwork.cpp:38:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
ERROR: Source/WebKit2/Shared/RTCNetwork.h:50:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
Total errors found: 5 in 34 files


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

[youenn fablet]

Created attachment 299608 [details]
Patch

[WebKit Commit Bot]

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


ERROR: Source/WebKit2/WebProcess/Network/webrtc/WebPacketSocket.h:48:  enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums.  [readability/enum_casing] [4]
ERROR: Source/WebKit2/CMakeLists.txt:595:  There should be exactly one empty line instead of 0 between "NetworkProcess/NetworkResourceLoader.messages.in" and "NetworkProcess/webrtc/NetworkPacketSocket.messages.in".  [list/emptyline] [5]
ERROR: Source/WebKit2/NetworkProcess/webrtc/NetworkNetworkManager.h:46:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
ERROR: Source/WebKit2/WebProcess/Network/webrtc/WebPacketSocketFactory.h:63:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
ERROR: Source/WebKit2/Shared/RTCNetwork.cpp:38:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
ERROR: Source/WebKit2/Shared/RTCNetwork.h:52:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
Total errors found: 6 in 34 files


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

[youenn fablet]

Created attachment 299623 [details]
Patch

[WebKit Commit Bot]

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


ERROR: Source/WebKit2/WebProcess/Network/webrtc/WebPacketSocket.h:48:  enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums.  [readability/enum_casing] [4]
ERROR: Source/WebKit2/CMakeLists.txt:595:  There should be exactly one empty line instead of 0 between "NetworkProcess/NetworkResourceLoader.messages.in" and "NetworkProcess/webrtc/NetworkPacketSocket.messages.in".  [list/emptyline] [5]
ERROR: Source/WebKit2/NetworkProcess/webrtc/NetworkNetworkManager.h:46:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
ERROR: Source/WebKit2/WebProcess/Network/webrtc/WebPacketSocketFactory.h:63:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
ERROR: Source/WebKit2/Shared/RTCNetwork.cpp:40:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
ERROR: Source/WebKit2/Shared/RTCNetwork.h:52:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
Total errors found: 6 in 34 files


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

[youenn fablet]

Created attachment 299893 [details]
Patch

[WebKit Commit Bot]

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


ERROR: Source/WebKit2/WebProcess/Network/webrtc/WebPacketSocket.h:49:  enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums.  [readability/enum_casing] [4]
ERROR: Source/WebKit2/NetworkProcess/webrtc/NetworkNetworkManager.h:46:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
ERROR: Source/WebKit2/WebProcess/Network/webrtc/WebPacketSocketFactory.h:63:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
ERROR: Source/WebKit2/Shared/RTCNetwork.cpp:40:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
ERROR: Source/WebKit2/Shared/RTCNetwork.h:52:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
Total errors found: 5 in 34 files


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

[Alex Christensen]

Comment on attachment 299893 [details]
Patch

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

> Source/WebKit2/NetworkProcess/NetworkConnectionToWebProcess.h:154
> +    NetworkNetworkManager m_networkManager;

Neither of these are good names.

> Source/WebKit2/NetworkProcess/webrtc/NetworkPacketSocket.cpp:100
> +void NetworkPacketSocket::OnMessage(rtc::Message* message)
> +{
> +    ASSERT(message->message_id == 1);
> +    delete this;
> +}

This seems strange.

> Source/WebKit2/WebKit2.xcodeproj/project.pbxproj:10318
> +				HEADER_SEARCH_PATHS = (
> +					"$(BUILT_PRODUCTS_DIR)/usr/local/include",
> +					"\"$(WEBCORE_PRIVATE_HEADERS_DIR)/ForwardingHeaders\"",
> +					"\"$(WEBCORE_PRIVATE_HEADERS_DIR)/icu\"",

We shouldn't need to add any more WEBCORE include directories to WebKit2.

[youenn fablet]

Created attachment 299978 [details]
Patch

[WebKit Commit Bot]

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


ERROR: Source/WebKit2/WebProcess/Network/webrtc/LibWebRTCResolver.h:46:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
ERROR: Source/WebKit2/WebProcess/Network/webrtc/WebPacketSocketFactory.h:63:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
ERROR: Source/WebKit2/Shared/RTCNetwork.cpp:39:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
ERROR: Source/WebKit2/WebProcess/Network/webrtc/LibWebRTCPacketSocket.h:53:  enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums.  [readability/enum_casing] [4]
ERROR: Source/WebKit2/Shared/RTCNetwork.h:51:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
Total errors found: 5 in 36 files


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

[Radar WebKit Bug Importer]

<rdar://problem/30245426>

[youenn fablet]

Created attachment 300074 [details]
Patch

[WebKit Commit Bot]

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


ERROR: Source/WebKit2/WebProcess/Network/webrtc/LibWebRTCSocketFactory.cpp:34:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/WebKit2/WebProcess/Network/webrtc/LibWebRTCResolver.h:40:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
ERROR: Source/WebKit2/NetworkProcess/webrtc/NetworkRTCMonitor.h:46:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
ERROR: Source/WebKit2/WebProcess/Network/webrtc/WebRTCResolver.h:34:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
ERROR: Source/WebKit2/Shared/RTCNetwork.cpp:40:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
ERROR: Source/WebKit2/WebProcess/Network/webrtc/LibWebRTCSocket.h:52:  enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums.  [readability/enum_casing] [4]
ERROR: Source/WebKit2/Shared/RTCNetwork.h:52:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
Total errors found: 7 in 41 files


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

[Alex Christensen]

Comment on attachment 300074 [details]
Patch

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

partial review.  I'm not sure this is organized completely correctly.

> Source/WebKit2/NetworkProcess/NetworkConnectionToWebProcess.cpp:76
> +        m_rtcProvider->close();

Can this happen with a destructor instead of having to call close every time we might lose it?

> Source/WebKit2/NetworkProcess/NetworkConnectionToWebProcess.h:120
> +    RefPtr<NetworkRTCProvider> m_rtcProvider;

Does this need to be ref counted?  It seems to only have one or zero references.

> Source/WebKit2/NetworkProcess/webrtc/NetworkRTCMonitor.messages.in:25
> +messages -> NetworkRTCMonitor LegacyReceiver {

Could we not add LegacyReceiver to new IPC message types?

> Source/WebKit2/WebProcess/Network/webrtc/LibWebRTCSocket.cpp:56
> +    std::fill_n(m_options, MAX_SOCKET_OPTION, -1);

could we use memset and sizeof?  That would feel safer, although it's the same in this case.

> Source/WebKit2/WebProcess/Network/webrtc/WebRTCResolver.h:35
> +    class Decoder;

no indent

> Source/WebKit2/WebProcess/WebPage/WebPage.cpp:384
> +#if USE(LIBWEBRTC)
> +        makeUniqueRef<WebKit::LibWebRTCProvider>()
> +#else
>          makeUniqueRef<WebCore::LibWebRTCProvider>()
> +#endif

Could we do something similar and have WebKit::LibWebRTCProvider exist and do nothing if USE(LIBWEBRTC) is false?

> Source/WebKit2/WebProcess/WebProcess.h:171
> +    LibWebRTC& libWebRTC() { return *m_libWebRTC; }
> +    void initializeLibWebRTC();

The name LibWebRTC doesn't indicate what this class does.
Also, we requiring the caller to call initializeLibWebRTC before calling libWebRTC is strange.  Could we just check if it's null and if it is then create one?  Then we'd only need one function.

[youenn fablet]

Thanks for the initial review.

> View in context:
> https://bugs.webkit.org/attachment.cgi?id=300074&action=review
> 
> partial review.  I'm not sure this is organized completely correctly.
> 
> > Source/WebKit2/NetworkProcess/NetworkConnectionToWebProcess.cpp:76
> > +        m_rtcProvider->close();
> 
> Can this happen with a destructor instead of having to call close every time
> we might lose it?

m_rtcProvider may remain live while NetworkConnectionToWebProcess is gone.
close ensures m_rtcProvider will not use its deleted NetworkConnectionToWebProcess.

> > Source/WebKit2/NetworkProcess/NetworkConnectionToWebProcess.h:120
> > +    RefPtr<NetworkRTCProvider> m_rtcProvider;
> 
> Does this need to be ref counted?  It seems to only have one or zero
> references.

It is refcounted each time we go to/from the rtc thread/main thread.
In the future, we should find a path to go directly from web process rtc thread to network process rtc thread

> 
> > Source/WebKit2/NetworkProcess/webrtc/NetworkRTCMonitor.messages.in:25
> > +messages -> NetworkRTCMonitor LegacyReceiver {
> 
> Could we not add LegacyReceiver to new IPC message types?

Will try.

> > Source/WebKit2/WebProcess/Network/webrtc/LibWebRTCSocket.cpp:56
> > +    std::fill_n(m_options, MAX_SOCKET_OPTION, -1);
> 
> could we use memset and sizeof?  That would feel safer, although it's the
> same in this case.

OK

> > Source/WebKit2/WebProcess/Network/webrtc/WebRTCResolver.h:35
> > +    class Decoder;
> 
> no indent

OK

> > Source/WebKit2/WebProcess/WebPage/WebPage.cpp:384
> > +#if USE(LIBWEBRTC)
> > +        makeUniqueRef<WebKit::LibWebRTCProvider>()
> > +#else
> >          makeUniqueRef<WebCore::LibWebRTCProvider>()
> > +#endif
> 
> Could we do something similar and have WebKit::LibWebRTCProvider exist and
> do nothing if USE(LIBWEBRTC) is false?

OK

> > Source/WebKit2/WebProcess/WebProcess.h:171
> > +    LibWebRTC& libWebRTC() { return *m_libWebRTC; }
> > +    void initializeLibWebRTC();
> 
> The name LibWebRTC doesn't indicate what this class does.

Let's change it to LibWebRTCNetwork

> Also, we requiring the caller to call initializeLibWebRTC before calling
> libWebRTC is strange.  Could we just check if it's null and if it is then
> create one?  Then we'd only need one function.

Sure, that is really a left over.

[youenn fablet]

Created attachment 300177 [details]
Patch

[WebKit Commit Bot]

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


ERROR: Source/WebKit2/WebProcess/Network/webrtc/LibWebRTCResolver.h:40:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
ERROR: Source/WebKit2/NetworkProcess/webrtc/NetworkRTCMonitor.h:46:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
ERROR: Source/WebKit2/Shared/RTCNetwork.cpp:40:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
ERROR: Source/WebKit2/WebProcess/Network/webrtc/LibWebRTCSocket.h:52:  enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums.  [readability/enum_casing] [4]
ERROR: Source/WebKit2/Shared/RTCNetwork.h:52:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
Total errors found: 5 in 41 files


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

[youenn fablet]

Created attachment 300182 [details]
Patch

[WebKit Commit Bot]

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


ERROR: Source/WebKit2/WebProcess/Network/webrtc/LibWebRTCResolver.h:40:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
ERROR: Source/WebKit2/NetworkProcess/webrtc/NetworkRTCMonitor.h:46:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
ERROR: Source/WebKit2/Shared/RTCNetwork.cpp:40:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
ERROR: Source/WebKit2/WebProcess/Network/webrtc/LibWebRTCSocket.h:52:  enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums.  [readability/enum_casing] [4]
ERROR: Source/WebKit2/Shared/RTCNetwork.h:52:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
Total errors found: 5 in 41 files


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

[Build Bot]

Comment on attachment 300182 [details]
Patch

Attachment 300182 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/2978395

New failing tests:
css3/filters/backdrop/dynamic-with-clip-path.html

[Build Bot]

Created attachment 300198 [details]
Archive of layout-test-results from ews122 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews122  Port: ios-simulator-wk2  Platform: Mac OS X 10.11.6

[youenn fablet]

Comment on attachment 300182 [details]
Patch

Failure is unrelated to this patch

[Alex Christensen]

Comment on attachment 300182 [details]
Patch

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

> Source/WebKit2/ChangeLog:11
> +        1 Resolution of DNS names (RTCResikver)

RTCResolver

> Source/WebKit2/NetworkProcess/webrtc/LibWebRTCSocketClient.cpp:61
> +

extra space

> Source/WebKit2/NetworkProcess/webrtc/LibWebRTCSocketClient.cpp:91
> +    auto identifier = m_identifier;
> +    m_rtcProvider.sendFromMainThread([identifier, sentPacket](IPC::Connection& connection) {

You might be able to do identifier = m_identifier

> Source/WebKit2/NetworkProcess/webrtc/NetworkRTCProvider.cpp:133
> +    CFHostClientContext context;
> +    context.version = 0;
> +    context.info = resolver.get();
> +    context.copyDescription = nullptr;
> +    context.release = nullptr;
> +    context.retain = nullptr;

Let's use an initializer list.

> Source/WebKit2/WebProcess/Network/webrtc/LibWebRTCSocket.h:100
> +    size_t m_availableSendingBytes { 65536 };

This should be a named constant.

[youenn fablet]

Created attachment 300236 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 300236 [details]
Patch for landing

Clearing flags on attachment: 300236

Committed r211441: <http://trac.webkit.org/changeset/211441>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.