Description
youenn fablet
2017-01-22 10:39:39 PST
Created attachment 299472 [details]
Patch
Created attachment 299481 [details]
Patch
Comment on attachment 299481 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=299481&action=review I think this should be USE(LIBWEBRTC) instead. > Source/JavaScriptCore/Configurations/FeatureDefines.xcconfig:168 > ENABLE_WEB_RTC = ENABLE_WEB_RTC; > +ENABLE_LIBWEBRTC_ENDPOINT = ; It is nice to have these two together, but I think the intent is that this list be alphabetically sorted. Created attachment 300087 [details]
Misc patch
Created attachment 300865 [details]
Patch
Created attachment 300930 [details]
Patch
Comment on attachment 300930 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=300930&action=review > Source/WebKit2/WebProcess/com.apple.WebProcess.sb.in:434 > (with-filter (extension "com.apple.webkit.microphone") > (allow device-microphone)) > > +(allow device-microphone) This doesn't look right. Why do we allow microphone both conditionally and unconditionally? Also, what tracks removing the unconditional rule? Comment on attachment 300930 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=300930&action=review > Source/WebKit2/NetworkProcess/mac/com.apple.WebKit.NetworkProcess.sb.in:201 > +(allow network*) Yikes! >> Source/WebKit2/WebProcess/com.apple.WebProcess.sb.in:434 >> +(allow device-microphone) > > This doesn't look right. Why do we allow microphone both conditionally and unconditionally? > > Also, what tracks removing the unconditional rule? This shouldn't be needed after Bug 167669. (In reply to comment #8) > Comment on attachment 300930 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=300930&action=review > > > Source/WebKit2/WebProcess/com.apple.WebProcess.sb.in:434 > > (with-filter (extension "com.apple.webkit.microphone") > > (allow device-microphone)) > > > > +(allow device-microphone) > > This doesn't look right. Why do we allow microphone both conditionally and > unconditionally? > > Also, what tracks removing the unconditional rule? This patch is not intended to be landed as is. But it allows testing/demoing WebKit with webrtc. Comment on attachment 300930 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=300930&action=review >> Source/WebKit2/NetworkProcess/mac/com.apple.WebKit.NetworkProcess.sb.in:201 >> +(allow network*) > > Yikes! This shouldn't be needed after Bug 168010. Created attachment 300980 [details]
Patch
Created attachment 301070 [details]
Patch
Created attachment 301119 [details]
Patch
Comment on attachment 301119 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=301119&action=review > Source/WebKit2/Configurations/WebKit.xcconfig:47 > +OTHER_LDFLAGS = $(inherited) $(UNEXPORTED_SYMBOL_LDFLAGS) $(ASAN_OTHER_LDFLAGS) $(FRAMEWORK_AND_LIBRARY_LDFLAGS) $(OTHER_LDFLAGS_PLATFORM) -framework WebKitLegacy -sub_umbrella WebKitLegacy -lboringssl -lwebrtc; Why do these static libraries need to be linked into both WebCore and WebKit? WebKit already links against WebCore, and it seems like this either already does or can easily lead to redundancy. (In reply to comment #16) > Comment on attachment 301119 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=301119&action=review > > > Source/WebKit2/Configurations/WebKit.xcconfig:47 > > +OTHER_LDFLAGS = $(inherited) $(UNEXPORTED_SYMBOL_LDFLAGS) $(ASAN_OTHER_LDFLAGS) $(FRAMEWORK_AND_LIBRARY_LDFLAGS) $(OTHER_LDFLAGS_PLATFORM) -framework WebKitLegacy -sub_umbrella WebKitLegacy -lboringssl -lwebrtc; > > Why do these static libraries need to be linked into both WebCore and > WebKit? WebKit already links against WebCore, and it seems like this either > already does or can easily lead to redundancy. We'll work on removing these for WebKit and WebKit2. Rolled out in https://trac.webkit.org/r212042 Seems to have caused failures? https://build.webkit.org/results/Apple%20Sierra%20Release%20WK2%20(Tests)/r212040%20(3463)/results.html And it broke a bunch of internal build stuff. Comment on attachment 301119 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=301119&action=review > Source/WTF/wtf/Platform.h:1225 > +#if PLATFORM(COCOA) 32-bit El Capitan will not work because of ARC. We should probably disable it for El Capitan, for 32-bit, or for both. (In reply to comment #21) > Comment on attachment 301119 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=301119&action=review > > > Source/WTF/wtf/Platform.h:1225 > > +#if PLATFORM(COCOA) > > 32-bit El Capitan will not work because of ARC. We should probably disable > it for El Capitan, for 32-bit, or for both. We should try to remove all files requiring -fobj-arc for mac builds. For iOS builds, we might need to keep a few of them. Reopening to attach new patch. Created attachment 301199 [details]
Patch
Created attachment 301202 [details]
Patch
Comment on attachment 301202 [details] Patch Attachment 301202 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/3065236 New failing tests: fast/mediastream/RTCPeerConnection-icecandidate-event.html Created attachment 301219 [details]
Archive of layout-test-results from ews105 for mac-elcapitan-wk2
The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews105 Port: mac-elcapitan-wk2 Platform: Mac OS X 10.11.6
Comment on attachment 301202 [details] Patch Attachment 301202 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: http://webkit-queues.webkit.org/results/3065256 New failing tests: imported/w3c/web-platform-tests/webrtc/datachannel-emptystring.html imported/w3c/web-platform-tests/webrtc/no-media-call.html imported/w3c/web-platform-tests/webrtc/promises-call.html Created attachment 301221 [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 301202 [details] Patch Attachment 301202 [details] did not pass mac-debug-ews (mac): Output: http://webkit-queues.webkit.org/results/3065288 New failing tests: imported/w3c/web-platform-tests/webrtc/rtcpeerconnection/rtcpeerconnection-idl.html imported/w3c/web-platform-tests/webrtc/datachannel-emptystring.html imported/w3c/web-platform-tests/webrtc/no-media-call.html imported/w3c/web-platform-tests/webrtc/promises-call.html imported/w3c/web-platform-tests/webrtc/RTCDataChannelEvent-constructor.html imported/w3c/web-platform-tests/webrtc/rtcpeerconnection/rtcpeerconnection-constructor.html Created attachment 301226 [details]
Archive of layout-test-results from ews114 for mac-elcapitan
The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews114 Port: mac-elcapitan Platform: Mac OS X 10.11.6
Created attachment 301702 [details]
Trying the dylib way
Created attachment 301708 [details]
Adding some more exports
Comment on attachment 301708 [details] Adding some more exports View in context: https://bugs.webkit.org/attachment.cgi?id=301708&action=review > Source/WebCore/Configurations/WebCore.xcconfig:69 > +LIBWEBRTC_LDFLAGS = -lwebrtc -framework CoreMedia -framework VideoToolbox; Instead of -lwebrtc let's do -weak_library <full path to libwebrtc from macros> Comment on attachment 301708 [details] Adding some more exports Attachment 301708 [details] did not pass mac-debug-ews (mac): Output: http://webkit-queues.webkit.org/results/3126647 New failing tests: imported/w3c/web-platform-tests/webrtc/rtcpeerconnection/rtcpeerconnection-idl.html imported/w3c/web-platform-tests/webrtc/datachannel-emptystring.html imported/w3c/web-platform-tests/webrtc/no-media-call.html imported/w3c/web-platform-tests/webrtc/promises-call.html imported/w3c/web-platform-tests/webrtc/RTCDataChannelEvent-constructor.html imported/w3c/web-platform-tests/webrtc/rtcpeerconnection/rtcpeerconnection-constructor.html Created attachment 301718 [details]
Archive of layout-test-results from ews117 for mac-elcapitan
The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews117 Port: mac-elcapitan Platform: Mac OS X 10.11.6
Comment on attachment 301708 [details] Adding some more exports Attachment 301708 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/3132449 New failing tests: fast/mediastream/RTCPeerConnection-icecandidate-event.html Created attachment 301738 [details]
Archive of layout-test-results from ews107 for mac-elcapitan-wk2
The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews107 Port: mac-elcapitan-wk2 Platform: Mac OS X 10.11.6
Comment on attachment 301708 [details] Adding some more exports Attachment 301708 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: http://webkit-queues.webkit.org/results/3132824 New failing tests: imported/w3c/web-platform-tests/webrtc/datachannel-emptystring.html imported/w3c/web-platform-tests/webrtc/no-media-call.html imported/w3c/web-platform-tests/webrtc/promises-call.html Created attachment 301751 [details]
Archive of layout-test-results from ews121 for ios-simulator-wk2
The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews121 Port: ios-simulator-wk2 Platform: Mac OS X 10.11.6
Comment on attachment 301708 [details] Adding some more exports Attachment 301708 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: http://webkit-queues.webkit.org/results/3132662 New failing tests: imported/w3c/web-platform-tests/webrtc/datachannel-emptystring.html imported/w3c/web-platform-tests/webrtc/no-media-call.html fast/scrolling/ios/scroll-events-back-forward.html imported/w3c/web-platform-tests/webrtc/promises-call.html Created attachment 301766 [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
(In reply to comment #34) > Comment on attachment 301708 [details] > Adding some more exports > > View in context: > https://bugs.webkit.org/attachment.cgi?id=301708&action=review > > > Source/WebCore/Configurations/WebCore.xcconfig:69 > > +LIBWEBRTC_LDFLAGS = -lwebrtc -framework CoreMedia -framework VideoToolbox; > > Instead of -lwebrtc let's do -weak_library <full path to libwebrtc from > macros> Right. The question is where should we put the libwebrtc library, at the root folder as currently done or within a Framework subfolder like done for other dylib? I am not sure I have the right settings currently. Mitz, any idea? Or maybe use -weak-lwebrtc for the linking (In reply to comment #44) > Or maybe use -weak-lwebrtc for the linking Yeah, that’d be better. But independently of that you still have to decide the install path and install name for the dylib in all build configurations. I think in iOS builds the install path and install name can be /System/Library/PrivateFrameworks/WebCore.framework/Frameworks/libwebrtc.dylib In normal macOS builds (when WK_USE_OVERRIDE_FRAMEWORKS_DIR is not set to YES) the install path can be /System/Library/Frameworks/WebKit.framework/Versions/A/Frameworks/WebCore.framework/Versions/A/Frameworks/libwebrtc.dylib which can also be the install name. In macOS builds when WK_USE_OVERRIDE_FRAMEWORKS_DIR is set to YES, the install path can be $(WK_OVERRIDE_FRAMEWORKS_DIR) and the install name can be @loader_path/../../../libwebrtc.dylib (which is the path from the WebCore binary to the dylib), assuming the use of @loader_path is compatible with Apple’s combination of use of restricted entitlements and library validation on the various executables in Safari Technology Preview and in its Safari updates. The libwebrtc headers can probably go into a webrtc subdirectory of WebCore’s PrivateHeaders directory (WebCore’s configuration computes where that ends up being). Created attachment 301967 [details]
Patch
Comment on attachment 301967 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=301967&action=review ! > Source/ThirdParty/libwebrtc/Configurations/libwebrtc.xcconfig:23 > -EXCLUDED_SOURCE_FILE_NAMES[sdk=macosx*] = audio_device_ios.mm voice_processing_audio_unit.mm audio_device_not_implemented_ios.mm RTCAudioSessionConfiguration.m RTCAudioSessionDelegateAdapter.mm RTCAudioSession.mm RTCAudioSession+Configuration.mm RTCUIApplication.mm; > +EXCLUDED_SOURCE_FILE_NAMES[sdk=macosx*] = voice_processing_audio_unit.mm; I remember excluding these because 32-bit El Capitan can't build with -fobjc-arc. Are these files needed now? > Source/WebKit2/UIProcess/WebPreferences.cpp:208 > + return true; return false if libwebrtc isn't there? Comment on attachment 301967 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=301967&action=review >> Source/ThirdParty/libwebrtc/Configurations/libwebrtc.xcconfig:23 >> +EXCLUDED_SOURCE_FILE_NAMES[sdk=macosx*] = voice_processing_audio_unit.mm; > > I remember excluding these because 32-bit El Capitan can't build with -fobjc-arc. Are these files needed now? These files are no longer part of the build system. >> Source/WebKit2/UIProcess/WebPreferences.cpp:208 >> + return true; > > return false if libwebrtc isn't there? If we are not using libwebrtc, we always activate WebRTC. This should allow taking care of GTK port. Comment on attachment 301967 [details] Patch Attachment 301967 [details] did not pass mac-ews (mac): Output: http://webkit-queues.webkit.org/results/3145011 New failing tests: editing/spelling/spellcheck-async.html Created attachment 301987 [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 on attachment 301967 [details]
Patch
Test failure is probably unrelated
And out again in http://trac.webkit.org/r212704 Created attachment 302353 [details]
Patch
Comment on attachment 302353 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=302353&action=review > Source/WTF/wtf/Platform.h:1218 > +asdf qwer Comment on attachment 302353 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=302353&action=review > Source/WebKit2/Configurations/WebKit.xcconfig:50 > +LIBWEBRTC_LDFLAGS_ =; Style: Normally we have a space between `=` and `;` for empty values. Sometimes we just leave it off entirely, but I like seeing it explicitly. > Tools/Scripts/webkitdirs.pm:2154 > + prependToEnvironmentVariableList("DYLD_LIBRARY_PATH", $dyldFrameworkPath); This looks identical to the line above it. Perhaps one of these was meant to be __XPC_DYLD_FRAMEWORK_PATH to cover the same 4 as above? Comment on attachment 302353 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=302353&action=review > Source/WTF/wtf/Platform.h:1216 > +#if PLATFORM(COCOA) && ENABLE(WEB_RTC) If we are going COCOA, then we should probably also link iOS/iOSsimulator webcore/webkit2 to libwebrtc. That would be useful. >> Tools/Scripts/webkitdirs.pm:2154 >> + prependToEnvironmentVariableList("DYLD_LIBRARY_PATH", $dyldFrameworkPath); > > This looks identical to the line above it. Perhaps one of these was meant to be __XPC_DYLD_FRAMEWORK_PATH to cover the same 4 as above? Right! Created attachment 302362 [details]
Patch
Comment on attachment 302362 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=302362&action=review > Tools/Scripts/webkitdirs.pm:2153 > + prependToEnvironmentVariableList("__XPC_DYLD_FRAMEWORK_PATH", $dyldFrameworkPath); What is this for? Patch has my name on it although Alex updated it so I am not sure whether I can r+. But anyway, r=me, let's land it and move forward! Created attachment 302364 [details]
Patch
Comment on attachment 302364 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=302364&action=review > Tools/Scripts/webkitdirs.pm:2153 > + prependToEnvironmentVariableList("__XPC_DYLD_FRAMEWORK_PATH", $dyldFrameworkPath); What is this for? Created attachment 302368 [details]
Patch
Comment on attachment 302368 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=302368&action=review > Source/JavaScriptCore/Configurations/FeatureDefines.xcconfig:171 > +ENABLE_WEB_RTC_Production = ; > +ENABLE_WEB_RTC_Debug = ENABLE_WEB_RTC; > +ENABLE_WEB_RTC_Release = ENABLE_WEB_RTC; This looks unusual. We don't differentiate between release and production builds when running tests, so how can we enable any tests when the feature is disabled in production builds? Created attachment 302370 [details]
Patch
(In reply to comment #64) > Comment on attachment 302368 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=302368&action=review > > > Source/JavaScriptCore/Configurations/FeatureDefines.xcconfig:171 > > +ENABLE_WEB_RTC_Production = ; > > +ENABLE_WEB_RTC_Debug = ENABLE_WEB_RTC; > > +ENABLE_WEB_RTC_Release = ENABLE_WEB_RTC; > > This looks unusual. We don't differentiate between release and production > builds when running tests, so how can we enable any tests when the feature > is disabled in production builds? The point of this patch is to get some of the bots building webrtc. For the purposes of this patch, we could skip all the webrtc tests in all release/production builds and I think this patch would still be a huge step in the right direction: enabling building on some bots to catch build regressions and running some tests (even if debug only) on some bots. We can improve from there, but we're having enough trouble getting something building somewhere I think this is the least of our worries. Created attachment 302372 [details]
Patch
Created attachment 302373 [details]
Patch
Comment on attachment 302373 [details] Patch http://trac.webkit.org/r212812 (In reply to comment #64) > We don't differentiate between release and production > builds when running tests, so how can we enable any tests when the feature > is disabled in production builds? Maybe we should skip all webrtc tests on all production/release builds until we get the production builds up and running. I think it would be quite useful to run them on debug bots until then. Comment on attachment 302373 [details] Patch Attachment 302373 [details] did not pass mac-ews (mac): Output: http://webkit-queues.webkit.org/results/3171237 New failing tests: editing/spelling/spellcheck-async.html Created attachment 302378 [details]
Archive of layout-test-results from ews100 for mac-elcapitan
The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews100 Port: mac-elcapitan Platform: Mac OS X 10.11.6
|