Add support for WK2 libwebrtc endpoint
Created attachment 299515 [details] Patch
Created attachment 299606 [details] Patch
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.
Created attachment 299608 [details] Patch
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.
Created attachment 299623 [details] Patch
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.
Created attachment 299893 [details] Patch
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.
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.
Created attachment 299978 [details] Patch
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.
<rdar://problem/30245426>
Created attachment 300074 [details] Patch
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.
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.
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.
Created attachment 300177 [details] Patch
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.
Created attachment 300182 [details] Patch
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.
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
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
Comment on attachment 300182 [details] Patch Failure is unrelated to this patch
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.
Created attachment 300236 [details] Patch for landing
Comment on attachment 300236 [details] Patch for landing Clearing flags on attachment: 300236 Committed r211441: <http://trac.webkit.org/changeset/211441>
All reviewed patches have been landed. Closing bug.