Expose Safe Browsing SPI
Created attachment 331644 [details] Patch
Comment on attachment 331644 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=331644&action=review > Source/WebKit/Configurations/WebKit.xcconfig:94 > +WK_SAFE_BROWSING_LDFLAGS[sdk=iphone*] = -framework SafariSafeBrowsing; > +WK_SAFE_BROWSING_LDFLAGS[sdk=iphone*10.*] = ; > +WK_SAFE_BROWSING_LDFLAGS[sdk=macosx*] = -weak_framework SafariSafeBrowsing; > +WK_SAFE_BROWSING_LDFLAGS[sdk=macosx10.11*] = ; > +WK_SAFE_BROWSING_LDFLAGS[sdk=macosx10.12*] = ; If we do want to do this differently for different operating system versions, I think it will need to use $(TARGET_MAC_OS_X_VERSION_MAJOR) like we do above. I'm not sure if this works. Also, why don't we just link on all operating systems?
Comment on attachment 331644 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=331644&action=review >> Source/WebKit/Configurations/WebKit.xcconfig:94 >> +WK_SAFE_BROWSING_LDFLAGS[sdk=macosx10.12*] = ; > > If we do want to do this differently for different operating system versions, I think it will need to use $(TARGET_MAC_OS_X_VERSION_MAJOR) like we do above. I'm not sure if this works. Also, why don't we just link on all operating systems? We could link against SafariSafeBrowsing framework on all operating systems we support, but we will only use Safe Browsing pieces on High Sierra and iOS 11 and above because the symbols were introduced then. This will use the SDK version that comes with Xcode, so it will behave differently based on the SDK version. Is that not ideal?
Comment on attachment 331644 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=331644&action=review >>> Source/WebKit/Configurations/WebKit.xcconfig:94 >>> +WK_SAFE_BROWSING_LDFLAGS[sdk=macosx10.12*] = ; >> >> If we do want to do this differently for different operating system versions, I think it will need to use $(TARGET_MAC_OS_X_VERSION_MAJOR) like we do above. I'm not sure if this works. Also, why don't we just link on all operating systems? > > We could link against SafariSafeBrowsing framework on all operating systems we support, but we will only use Safe Browsing pieces on High Sierra and iOS 11 and above because the symbols were introduced then. This will use the SDK version that comes with Xcode, so it will behave differently based on the SDK version. Is that not ideal? I'm not sure. I'm interested in what Dan thinks about this. I don't know this syntax or best practices well. > Source/WebKit/Platform/spi/Cocoa/SafeBrowsingSPI.h:28 > +#pragma once I think pragma once needs to be first. > Source/WebKit/Platform/spi/Cocoa/SafeBrowsingSPI.h:73 > +#endif // not USE(APPLE_INTERNAL_SDK) not should be removed
Comment on attachment 331644 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=331644&action=review >>>> Source/WebKit/Configurations/WebKit.xcconfig:94 >>>> +WK_SAFE_BROWSING_LDFLAGS[sdk=macosx10.12*] = ; >>> >>> If we do want to do this differently for different operating system versions, I think it will need to use $(TARGET_MAC_OS_X_VERSION_MAJOR) like we do above. I'm not sure if this works. Also, why don't we just link on all operating systems? >> >> We could link against SafariSafeBrowsing framework on all operating systems we support, but we will only use Safe Browsing pieces on High Sierra and iOS 11 and above because the symbols were introduced then. This will use the SDK version that comes with Xcode, so it will behave differently based on the SDK version. Is that not ideal? > > I'm not sure. I'm interested in what Dan thinks about this. I don't know this syntax or best practices well. The target macOS version is the right thing to use for linking (as well as for any code that uses the feature). As soon as the patch from bug 181803 lands, you should be able to write WK_SAFE_BROWSING_LDFLAGS[sdk=macosx*] = $(WK_SAFE_BROWSING_LDFLAGS$(WK_MACOS_1013)); WK_SAFE_BROWSING_LDFLAGS_MACOS_SINCE_1013 = -weak_framework SafariSafeBrowsing;
Comment on attachment 331644 [details] Patch Great! Please adopt Dan's new future-proof way of doing this.
(In reply to Alex Christensen from comment #4) > Comment on attachment 331644 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=331644&action=review > > >>> Source/WebKit/Configurations/WebKit.xcconfig:94 > >>> +WK_SAFE_BROWSING_LDFLAGS[sdk=macosx10.12*] = ; > >> > >> If we do want to do this differently for different operating system versions, I think it will need to use $(TARGET_MAC_OS_X_VERSION_MAJOR) like we do above. I'm not sure if this works. Also, why don't we just link on all operating systems? > > > > We could link against SafariSafeBrowsing framework on all operating systems we support, but we will only use Safe Browsing pieces on High Sierra and iOS 11 and above because the symbols were introduced then. This will use the SDK version that comes with Xcode, so it will behave differently based on the SDK version. Is that not ideal? > > I'm not sure. I'm interested in what Dan thinks about this. I don't know > this syntax or best practices well. > > > Source/WebKit/Platform/spi/Cocoa/SafeBrowsingSPI.h:28 > > +#pragma once > > I think pragma once needs to be first. Sure. > > > Source/WebKit/Platform/spi/Cocoa/SafeBrowsingSPI.h:73 > > +#endif // not USE(APPLE_INTERNAL_SDK) > > not should be removed Should it? Because this #endif is closer to the else statement which is when we are not using Apple internal SDK. But if this is the style WebKit uses, I will follow.
(In reply to mitz from comment #5) > Comment on attachment 331644 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=331644&action=review > > >>>> Source/WebKit/Configurations/WebKit.xcconfig:94 > >>>> +WK_SAFE_BROWSING_LDFLAGS[sdk=macosx10.12*] = ; > >>> > >>> If we do want to do this differently for different operating system versions, I think it will need to use $(TARGET_MAC_OS_X_VERSION_MAJOR) like we do above. I'm not sure if this works. Also, why don't we just link on all operating systems? > >> > >> We could link against SafariSafeBrowsing framework on all operating systems we support, but we will only use Safe Browsing pieces on High Sierra and iOS 11 and above because the symbols were introduced then. This will use the SDK version that comes with Xcode, so it will behave differently based on the SDK version. Is that not ideal? > > > > I'm not sure. I'm interested in what Dan thinks about this. I don't know this syntax or best practices well. > > The target macOS version is the right thing to use for linking (as well as > for any code that uses the feature). As soon as the patch from bug 181803 > lands, you should be able to write > WK_SAFE_BROWSING_LDFLAGS[sdk=macosx*] = > $(WK_SAFE_BROWSING_LDFLAGS$(WK_MACOS_1013)); > WK_SAFE_BROWSING_LDFLAGS_MACOS_SINCE_1013 = -weak_framework > SafariSafeBrowsing; Yay, thanks Dan for making it easier to use build settings.
Created attachment 331687 [details] Patch v2
Comment on attachment 331687 [details] Patch v2 Clearing flags on attachment: 331687 Committed r227211: <https://trac.webkit.org/changeset/227211>
All reviewed patches have been landed. Closing bug.
<rdar://problem/36626946>
(In reply to WebKit Commit Bot from comment #10) > Comment on attachment 331687 [details] > Patch v2 > > Clearing flags on attachment: 331687 > > Committed r227211: <https://trac.webkit.org/changeset/227211> This change breaks testing on iOS Simulator. WebKitTestRunnerApp crashes on launch, as seen here on EWS: https://webkit-queues.webkit.org/results/6136155
(In reply to Ryan Haddad from comment #13) > (In reply to WebKit Commit Bot from comment #10) > > Comment on attachment 331687 [details] > > Patch v2 > > > > Clearing flags on attachment: 331687 > > > > Committed r227211: <https://trac.webkit.org/changeset/227211> > This change breaks testing on iOS Simulator. WebKitTestRunnerApp crashes on > launch, as seen here on EWS: > https://webkit-queues.webkit.org/results/6136155 Crashed Thread: 0 Exception Type: EXC_CRASH (SIGABRT) Exception Codes: 0x0000000000000000, 0x0000000000000000 Exception Note: EXC_CORPSE_NOTIFY Termination Reason: DYLD, [0x3] Wrong version Application Specific Information: dyld: launch, loading dependent libraries DYLD_FRAMEWORK_PATH=/Volumes/Data/slave/ios-simulator-11-release-tests-wk2/build/WebKitBuild/Release-iphonesimulator DYLD_FALLBACK_LIBRARY_PATH=/Applications/Xcode.app/Contents/Developer/Platforms/iPhoneOS.platform/Developer/Library/CoreSimulator/Profiles/Runtimes/iOS.simruntime/Contents/Resources/RuntimeRoot/usr/lib DYLD_ROOT_PATH=/Applications/Xcode.app/Contents/Developer/Platforms/iPhoneOS.platform/Developer/Library/CoreSimulator/Profiles/Runtimes/iOS.simruntime/Contents/Resources/RuntimeRoot DYLD_FALLBACK_FRAMEWORK_PATH=/Applications/Xcode.app/Contents/Developer/Platforms/iPhoneOS.platform/Developer/Library/CoreSimulator/Profiles/Runtimes/iOS.simruntime/Contents/Resources/RuntimeRoot/System/Library/Frameworks DYLD_LIBRARY_PATH=/Volumes/Data/slave/ios-simulator-11-release-tests-wk2/build/WebKitBuild/Release-iphonesimulator Dyld Error Message: Library not loaded: /System/Library/PrivateFrameworks/SafariSafeBrowsing.framework/SafariSafeBrowsing Referenced from: /Volumes/VOLUME/*/WebKit.framework/WebKit Reason: Incompatible library version: WebKit requires version 1.0.0 or later, but SafariSafeBrowsing provides version 0.0.0
Reverted r227211 for reason: Breaks iOS Simulator tests. Committed r227224: <https://trac.webkit.org/changeset/227224>
Dan helped me figure out I should include "current-version: 0.0.0" in SafariSafeBrowsing.tbd file.
Created attachment 331784 [details] Patch v3
Comment on attachment 331784 [details] Patch v3 Attachment 331784 [details] did not pass mac-ews (mac): Output: http://webkit-queues.webkit.org/results/6142313 New failing tests: imported/w3c/web-platform-tests/media-source/mediasource-config-change-mp4-v-framerate.html
Created attachment 331811 [details] Archive of layout-test-results from ews102 for mac-sierra The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews102 Port: mac-sierra Platform: Mac OS X 10.12.6
Created attachment 331813 [details] Patch v3
Created attachment 331827 [details] Patch v3
Comment on attachment 331827 [details] Patch v3 Clearing flags on attachment: 331827 Committed r227265: <https://trac.webkit.org/changeset/227265>
Comment on attachment 331827 [details] Patch v3 View in context: https://bugs.webkit.org/attachment.cgi?id=331827&action=review > Source/WebKit/ChangeLog:15 > + and iOS 11 and above. Weak link against SafariSafeBrowsing framework > + because it is not present on the Base system. Maybe it should be present on the base system. Scammers target people looking for technical support.
Comment on attachment 331827 [details] Patch v3 View in context: https://bugs.webkit.org/attachment.cgi?id=331827&action=review > Source/WebKit/Configurations/WebKit.xcconfig:93 > +WK_SAFE_BROWSING_LDFLAGS_MACOS_SINCE_1013 = -weak_framework SafariSafeBrowsing; This should also be conditional on 64-bit, see rdar://problem/36964995. > WebKitLibraries/WebKitPrivateFrameworkStubs/iOS/11/SafariSafeBrowsing.framework/SafariSafeBrowsing.tbd:7 > + - armv7 > + - armv7s > + - arm64 > + - i386 > + - x86_64 This doesn't seem accurate.