Replace iOS-only WebKitSystemInterface calls with SPI
Created attachment 264498 [details] Patch
Comment on attachment 264498 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=264498&action=review > Source/WebKit2/Shared/AccessibilityIOS.h:1 > +/* Shouldn’t this file go somewhere inside the platform directory? > Source/WebKit2/Shared/AccessibilityIOS.h:28 > +NSData *AXRemoteToken(CFUUIDRef); This doesn’t mirror any SPI. It’s strange to give this function a name that begins with AX, the prefix of the system Accessibility APIs, even though it’s in the WebKit namespace. How about “newAccessibilityRemoteToken” (or maybe change the return type to a RetainPtr and have the name begin with create)? > Source/WebKit2/UIProcess/ios/WKContentView.mm:433 > +static void AXStoreRemoteConnectionInformation(id element, pid_t pid, mach_port_t sendPort, CFUUIDRef uuidRef) Here, too, naming this as if it were an Accessibility API is strange. > Source/WebKit2/UIProcess/ios/WKContentView.mm:443 > RetainPtr<CFUUIDRef> uuid = adoptCF(CFUUIDCreate(kCFAllocatorDefault)); It’d make the code a bit cleaner if you changed everything (including the function in AccessibilityIOS.mm) to use NSUUID instead. There’s no reason to use the CF type.
Created attachment 264500 [details] Patch
Comment on attachment 264500 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=264500&action=review I’d prefer a version of this that does not add a Gestalt.h header. > Source/WebCore/page/NavigatorBase.cpp:75 > +static CFStringRef platformNameForNavigator() This function should return a const String&, not a CFStringRef. > Source/WebCore/page/NavigatorBase.cpp:77 > + static CFStringRef deviceName = static_cast<CFStringRef>(CFRetain(Gestalt::deviceName())); This should be a NeverDestroyed<String>, and we should not do a CFRetain here. > Source/WebCore/page/ios/UserAgentIOS.mm:42 > +static inline bool isClassic(void) No need for (void) in a .mm file; should just be (). > Source/WebCore/page/ios/UserAgentIOS.mm:47 > +static inline NSString *osNameForUserAgent(void) No need for (void) in a .mm file; should just be (). > Source/WebCore/page/ios/UserAgentIOS.mm:54 > +static inline NSString *deviceName(void) No need for (void) in a .mm file; should just be (). > Source/WebCore/page/ios/UserAgentIOS.mm:62 > + NSRange simulatorStringRange = [deviceName rangeOfString:@" Simulator" options:NSBackwardsSearch]; > + NSUInteger location = simulatorStringRange.location; No need for the local variable simulatorStringRange. > Source/WebCore/page/ios/UserAgentIOS.mm:83 > + return [NSString stringWithFormat:@"Mozilla/5.0 (%@; CPU %@ %@ like Mac OS X) AppleWebKit/%@ (KHTML, like Gecko)", deviceName(), osNameForUserAgent(), osMarketingVersionString, webKitVersion]; > + return [NSString stringWithFormat:@"Mozilla/5.0 (%@; CPU %@ %@ like Mac OS X) AppleWebKit/%@ (KHTML, like Gecko) %@", deviceName(), osNameForUserAgent(), osMarketingVersionString, webKitVersion, static_cast<NSString *>(applicationName)]; Not sure why we do all this with NSString. I suggest coming back here at some point and using WTF::String and/or WTF::StringBuilder instead. > Source/WebCore/page/mac/SettingsMac.mm:36 > +#if PLATFORM(IOS) > +#include "Gestalt.h" > +#include "SoftLinking.h" > +#include "UIKitSPI.h" > + > +SOFT_LINK_FRAMEWORK(UIKit) > +SOFT_LINK_CLASS(UIKit, UIApplication) > +#endif I’d slightly prefer that the includes and soft linking be in two separate paragraphs. I don’t think that SettingsMac should be compiled for iOS, so this should eventually be renamed to SettingsCocoa.mm or divided into multiple files. > Source/WebCore/platform/PlatformScreen.h:75 > + class FloatSize; This should go at the top of the namespace section, in a separate paragraph. Also, why not do it unconditionally instead of only for PLATFORM(IOS)? Seems harmless enough. > Source/WebCore/platform/ios/Gestalt.cpp:37 > + int deviceClass = MGGetSInt32Answer(kMGQDeviceClassNumber, MGDeviceClassInvalid); build failed with error: use of undeclared identifier 'kMGQDeviceClassNumber' > Source/WebCore/platform/ios/Gestalt.h:34 > +namespace Gestalt { Not sure why we put these functions into a namespace nor why we call the namespace and source file Gestalt. I understand that the underlying iOS feature’s internal name is Gestalt, but in WebKit we could just give this some sensible WebKit name with less iOS/Apple flavor. Maybe even put this all into SettingsIOS.h/cpp? Or perhaps put these functions in the files that use them unless they are really used in multiple source files. > Source/WebCore/platform/ios/PlatformScreenIOS.mm:125 > + return FloatSize(CGSizeMake(320, 480)); I don’t think we should make a CGSize just to convert it to a FloatSize. Just: return { 320, 480 }; > Source/WebCore/platform/ios/PlatformScreenIOS.mm:132 > + return FloatSize(CGSizeMake(320, 480)); I don’t think we should make a CGSize just to convert it to a FloatSize. Just: return { 320, 480 }; > Source/WebCore/platform/ios/PlatformScreenIOS.mm:135 > +#pragma clang diagnostic ignored "-Wdeprecated-declarations" > + return FloatSize([getUIScreenClass() mainScreen].applicationFrame.size); If this is deprecated, maybe we should do the non-deprecated version? > Source/WebCore/platform/ios/WebVideoFullscreenInterfaceAVKit.mm:1448 > +#if HAVE(AVKIT) && TARGET_OS_IOS The use of TARGET_IOS_IOS here in WebKit code is peculiar. Isn’t that just another way to say PLATFORM(IOS)? > Source/WebCore/platform/spi/ios/MobileGestaltSPI.h:56 > +typedef enum { > + MGDeviceClassInvalid = -1, > + /* 0 is intentionally not in this enum */ > + MGDeviceClassiPhone = 1, > + MGDeviceClassiPod = 2, > + MGDeviceClassiPad = 3, > + MGDeviceClassAppleTV = 4, > + /* 5 is intentionally not in this enum */ > + MGDeviceClassWatch = 6, > +} MGDeviceClass; I think we need to also add kMGQDeviceClassNumber here. > Source/WebKit2/Shared/AccessibilityIOS.h:28 > +NSData *AXRemoteToken(CFUUIDRef); I agree with Dan’s warning that we should *not* define functions in WebKit with prefixes that belong to system frameworks, even in our own namespace. I agree with Dan’s suggestion that we use NSUUID and not CFUUID. > Source/WebKit2/Shared/AccessibilityIOS.mm:40 > + NSDictionary *value = @{ @"ax-pid" : @(getpid()), @"ax-uuid" : uuidString, @"ax-register" : @YES }; Are the spaces before the colons our usual format for this? Not sure we need a local variable for this. Might read better as a one liner. > Source/WebKit2/UIProcess/ios/WKContentView.mm:433 > +static void AXStoreRemoteConnectionInformation(id element, pid_t pid, mach_port_t sendPort, CFUUIDRef uuidRef) I agree with Dan’s warning that we should *not* define functions in WebKit with prefixes that belong to system frameworks, even in our own namespace. I agree with Dan’s suggestion that we use NSUUID and not CFUUID. > Source/WebKit2/UIProcess/ios/WKContentView.mm:438 > + objc_setAssociatedObject(element, (void*)[@"ax-uuid" hash], (id)uuidRef, OBJC_ASSOCIATION_RETAIN_NONATOMIC); > + objc_setAssociatedObject(element, (void*)[@"ax-pid" hash], @(pid), OBJC_ASSOCIATION_RETAIN_NONATOMIC); > + objc_setAssociatedObject(element, (void*)[@"ax-machport" hash], @(sendPort), OBJC_ASSOCIATION_RETAIN_NONATOMIC); I don’t know where this design comes from, where we use a hash as the associated object key. I guess it must come from some other code, since I only see the setting part here, not the getting. This does *not* seem like a good idea. It’s easy to come up with unique pointers to use as associated object keys; I really don’t see why we’d use a hash cast as a pointer instead.
(In reply to comment #2) > Comment on attachment 264498 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=264498&action=review > > > Source/WebKit2/Shared/AccessibilityIOS.h:1 > > +/* > > Shouldn’t this file go somewhere inside the platform directory? Ok, moved to Platform/ios/. > > > Source/WebKit2/Shared/AccessibilityIOS.h:28 > > +NSData *AXRemoteToken(CFUUIDRef); > > This doesn’t mirror any SPI. It’s strange to give this function a name that > begins with AX, the prefix of the system Accessibility APIs, even though > it’s in the WebKit namespace. How about “newAccessibilityRemoteToken” (or > maybe change the return type to a RetainPtr and have the name begin with > create)? I renamed this to createAccessibilityRemoteToken. > > > Source/WebKit2/UIProcess/ios/WKContentView.mm:433 > > +static void AXStoreRemoteConnectionInformation(id element, pid_t pid, mach_port_t sendPort, CFUUIDRef uuidRef) > > Here, too, naming this as if it were an Accessibility API is strange. I renamed this to storeAccessibilityRemoteConnectionInformation. > > > Source/WebKit2/UIProcess/ios/WKContentView.mm:443 > > RetainPtr<CFUUIDRef> uuid = adoptCF(CFUUIDCreate(kCFAllocatorDefault)); > > It’d make the code a bit cleaner if you changed everything (including the > function in AccessibilityIOS.mm) to use NSUUID instead. There’s no reason to > use the CF type. Done. Thanks for the feedback!
(In reply to comment #4) > Comment on attachment 264500 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=264500&action=review > > I’d prefer a version of this that does not add a Gestalt.h header. > > > Source/WebCore/page/NavigatorBase.cpp:75 > > +static CFStringRef platformNameForNavigator() > > This function should return a const String&, not a CFStringRef. > > > Source/WebCore/page/NavigatorBase.cpp:77 > > + static CFStringRef deviceName = static_cast<CFStringRef>(CFRetain(Gestalt::deviceName())); > > This should be a NeverDestroyed<String>, and we should not do a CFRetain > here. > > > Source/WebCore/page/ios/UserAgentIOS.mm:42 > > +static inline bool isClassic(void) > > No need for (void) in a .mm file; should just be (). > > > Source/WebCore/page/ios/UserAgentIOS.mm:47 > > +static inline NSString *osNameForUserAgent(void) > > No need for (void) in a .mm file; should just be (). > > > Source/WebCore/page/ios/UserAgentIOS.mm:54 > > +static inline NSString *deviceName(void) > > No need for (void) in a .mm file; should just be (). > > > Source/WebCore/page/ios/UserAgentIOS.mm:62 > > + NSRange simulatorStringRange = [deviceName rangeOfString:@" Simulator" options:NSBackwardsSearch]; > > + NSUInteger location = simulatorStringRange.location; > > No need for the local variable simulatorStringRange. Fixed all of the above. > > > Source/WebCore/page/ios/UserAgentIOS.mm:83 > > + return [NSString stringWithFormat:@"Mozilla/5.0 (%@; CPU %@ %@ like Mac OS X) AppleWebKit/%@ (KHTML, like Gecko)", deviceName(), osNameForUserAgent(), osMarketingVersionString, webKitVersion]; > > + return [NSString stringWithFormat:@"Mozilla/5.0 (%@; CPU %@ %@ like Mac OS X) AppleWebKit/%@ (KHTML, like Gecko) %@", deviceName(), osNameForUserAgent(), osMarketingVersionString, webKitVersion, static_cast<NSString *>(applicationName)]; > > Not sure why we do all this with NSString. I suggest coming back here at > some point and using WTF::String and/or WTF::StringBuilder instead. I've added a FIXME. > > > Source/WebCore/page/mac/SettingsMac.mm:36 > > +#if PLATFORM(IOS) > > +#include "Gestalt.h" > > +#include "SoftLinking.h" > > +#include "UIKitSPI.h" > > + > > +SOFT_LINK_FRAMEWORK(UIKit) > > +SOFT_LINK_CLASS(UIKit, UIApplication) > > +#endif > > I’d slightly prefer that the includes and soft linking be in two separate > paragraphs. > > I don’t think that SettingsMac should be compiled for iOS, so this should > eventually be renamed to SettingsCocoa.mm or divided into multiple files. I renamed this to SettingsCocoa.mm and moved it to page/ios/. > > > Source/WebCore/platform/PlatformScreen.h:75 > > + class FloatSize; > > This should go at the top of the namespace section, in a separate paragraph. > Also, why not do it unconditionally instead of only for PLATFORM(IOS)? Seems > harmless enough. Ok. > > > Source/WebCore/platform/ios/Gestalt.cpp:37 > > + int deviceClass = MGGetSInt32Answer(kMGQDeviceClassNumber, MGDeviceClassInvalid); > > build failed with error: use of undeclared identifier 'kMGQDeviceClassNumber' Fixed. > > > Source/WebCore/platform/ios/Gestalt.h:34 > > +namespace Gestalt { > > Not sure why we put these functions into a namespace nor why we call the > namespace and source file Gestalt. I understand that the underlying iOS > feature’s internal name is Gestalt, but in WebKit we could just give this > some sensible WebKit name with less iOS/Apple flavor. Maybe even put this > all into SettingsIOS.h/cpp? Or perhaps put these functions in the files that > use them unless they are really used in multiple source files. I struggled with this. Three of the six functions in Gestalt are only used in one source file, so I've moved those functions to the files that use them. The other three are used in multiple places, but Settings does not seem like the right place for these. These are platform queries, not page settings to be configured by the client. The three shared functions are deviceClass(), deviceName(), and hasIPadCapability() (which is probably the same as deviceClass() == MGDeviceClassiPad). I think I'll put this in a file called platform/ios/Device.{cpp,h}. I'm open to naming suggestions. > > > Source/WebCore/platform/ios/PlatformScreenIOS.mm:125 > > + return FloatSize(CGSizeMake(320, 480)); > > I don’t think we should make a CGSize just to convert it to a FloatSize. > Just: > > return { 320, 480 }; > > > Source/WebCore/platform/ios/PlatformScreenIOS.mm:132 > > + return FloatSize(CGSizeMake(320, 480)); > > I don’t think we should make a CGSize just to convert it to a FloatSize. > Just: > > return { 320, 480 }; Fixed both of these. > > > Source/WebCore/platform/ios/PlatformScreenIOS.mm:135 > > +#pragma clang diagnostic ignored "-Wdeprecated-declarations" > > + return FloatSize([getUIScreenClass() mainScreen].applicationFrame.size); > > If this is deprecated, maybe we should do the non-deprecated version? Changed to use -[UIScreen bounds]. > > > Source/WebCore/platform/ios/WebVideoFullscreenInterfaceAVKit.mm:1448 > > +#if HAVE(AVKIT) && TARGET_OS_IOS > > The use of TARGET_IOS_IOS here in WebKit code is peculiar. Isn’t that just > another way to say PLATFORM(IOS)? TARGET_OS_IOS is only true on an iOS device, whereas PLATFORM(IOS) is true on devices and simulators. I'm actually suspicious that excluding the simulator here wasn't intentional, so I'll investigate. > > > Source/WebCore/platform/spi/ios/MobileGestaltSPI.h:56 > > +typedef enum { > > + MGDeviceClassInvalid = -1, > > + /* 0 is intentionally not in this enum */ > > + MGDeviceClassiPhone = 1, > > + MGDeviceClassiPod = 2, > > + MGDeviceClassiPad = 3, > > + MGDeviceClassAppleTV = 4, > > + /* 5 is intentionally not in this enum */ > > + MGDeviceClassWatch = 6, > > +} MGDeviceClass; > > I think we need to also add kMGQDeviceClassNumber here. > > > Source/WebKit2/Shared/AccessibilityIOS.h:28 > > +NSData *AXRemoteToken(CFUUIDRef); > > I agree with Dan’s warning that we should *not* define functions in WebKit > with prefixes that belong to system frameworks, even in our own namespace. > > I agree with Dan’s suggestion that we use NSUUID and not CFUUID. > > > Source/WebKit2/Shared/AccessibilityIOS.mm:40 > > + NSDictionary *value = @{ @"ax-pid" : @(getpid()), @"ax-uuid" : uuidString, @"ax-register" : @YES }; > > Are the spaces before the colons our usual format for this? Yes, I believe so. > > Not sure we need a local variable for this. Might read better as a one liner. Ok. > > > Source/WebKit2/UIProcess/ios/WKContentView.mm:433 > > +static void AXStoreRemoteConnectionInformation(id element, pid_t pid, mach_port_t sendPort, CFUUIDRef uuidRef) > > I agree with Dan’s warning that we should *not* define functions in WebKit > with prefixes that belong to system frameworks, even in our own namespace. > > I agree with Dan’s suggestion that we use NSUUID and not CFUUID. > > > Source/WebKit2/UIProcess/ios/WKContentView.mm:438 > > + objc_setAssociatedObject(element, (void*)[@"ax-uuid" hash], (id)uuidRef, OBJC_ASSOCIATION_RETAIN_NONATOMIC); > > + objc_setAssociatedObject(element, (void*)[@"ax-pid" hash], @(pid), OBJC_ASSOCIATION_RETAIN_NONATOMIC); > > + objc_setAssociatedObject(element, (void*)[@"ax-machport" hash], @(sendPort), OBJC_ASSOCIATION_RETAIN_NONATOMIC); > > I don’t know where this design comes from, where we use a hash as the > associated object key. I guess it must come from some other code, since I > only see the setting part here, not the getting. This does *not* seem like a > good idea. It’s easy to come up with unique pointers to use as associated > object keys; I really don’t see why we’d use a hash cast as a pointer > instead. The matching code is in the Accessibility framework. I'll file a radar about this. Thanks for all the feedback!
(In reply to comment #6) > (In reply to comment #4) > > Comment on attachment 264500 [details] > > Patch > > > > View in context: > > https://bugs.webkit.org/attachment.cgi?id=264500&action=review > > > > > Source/WebCore/platform/ios/WebVideoFullscreenInterfaceAVKit.mm:1448 > > > +#if HAVE(AVKIT) && TARGET_OS_IOS > > > > The use of TARGET_IOS_IOS here in WebKit code is peculiar. Isn’t that just > > another way to say PLATFORM(IOS)? > > TARGET_OS_IOS is only true on an iOS device, whereas PLATFORM(IOS) is true > on devices and simulators. I'm actually suspicious that excluding the > simulator here wasn't intentional, so I'll investigate. Oh, I had that wrong. The difference here is that TARGET_OS_IOS is false on watchOS and tvOS, unlike PLATFORM(IOS). I think we can just remove the TARGET_OS_IOS bit; HAVE(AVKIT) should be sufficient.
Created attachment 264556 [details] Patch
Created attachment 264557 [details] Patch
Comment on attachment 264557 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=264557&action=review > Source/WebCore/css/MediaQueryEvaluator.cpp:635 > + static MGDeviceClass theClass = deviceClass(); > + return theClass == MGDeviceClassiPhone || theClass == MGDeviceClassiPod; We should cache a boolean rather than caching the device class. We should avoid names like “theClass”; could just write: static auto deviceClass = WebCore::deviceClass(); > Source/WebCore/page/cocoa/SettingsCocoa.mm:61 > +#if PLATFORM(MAC) > +void Settings::initializeDefaultFontFamilies() Missing blank line here. > Source/WebCore/page/cocoa/SettingsCocoa.mm:128 > +} > +#endif Missing blank line here. > Source/WebCore/platform/PlatformScreen.h:79 > + WEBCORE_EXPORT float screenScaleFactor(UIScreen * = nil); In case a non-Objective-C C++ file includes this header, I think this needs to say nullptr instead of nil. But maybe we can get away with nil. > Source/WebCore/platform/ios/Device.cpp:39 > + case MGDeviceClassInvalid: How can this ever be returned? > Source/WebCore/platform/ios/Device.cpp:52 > +CFStringRef deviceName() Why not have this return a String instead of a CFStringRef? > Source/WebCore/platform/ios/PlatformScreenIOS.mm:46 > SOFT_LINK(UIKit, UIAccessibilityIsGrayscaleEnabled, bool, (void), ()) > SOFT_LINK(UIKit, UIAccessibilityIsInvertColorsEnabled, bool, (void), ()) I think these need to be BOOL rather than bool. > Source/WebKit2/Platform/ios/AccessibilityIOS.h:33 > +RetainPtr<NSData> createAccessibilityRemoteToken(NSUUID *); Could just return an NSData * since it’s already going to be autoreleased.
Committed r191902: <http://trac.webkit.org/changeset/191902>
(In reply to comment #10) > Comment on attachment 264557 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=264557&action=review > > > Source/WebCore/css/MediaQueryEvaluator.cpp:635 > > + static MGDeviceClass theClass = deviceClass(); > > + return theClass == MGDeviceClassiPhone || theClass == MGDeviceClassiPod; > > We should cache a boolean rather than caching the device class. > > We should avoid names like “theClass”; could just write: > > static auto deviceClass = WebCore::deviceClass(); I made deviceClass() return a static MGDeviceClass and then just cached the result of the boolean expression. > > > Source/WebCore/page/cocoa/SettingsCocoa.mm:61 > > +#if PLATFORM(MAC) > > +void Settings::initializeDefaultFontFamilies() > > Missing blank line here. > > > Source/WebCore/page/cocoa/SettingsCocoa.mm:128 > > +} > > +#endif > > Missing blank line here. > > > Source/WebCore/platform/PlatformScreen.h:79 > > + WEBCORE_EXPORT float screenScaleFactor(UIScreen * = nil); > > In case a non-Objective-C C++ file includes this header, I think this needs > to say nullptr instead of nil. But maybe we can get away with nil. Done. > > > Source/WebCore/platform/ios/Device.cpp:39 > > + case MGDeviceClassInvalid: > > How can this ever be returned? This is the default value if MGGetSInt32Answer() doesn't find the query. I don't know if it'd happen in practice, but it is a valid member of the enum, so might as well cover it. > > > Source/WebCore/platform/ios/Device.cpp:52 > > +CFStringRef deviceName() > > Why not have this return a String instead of a CFStringRef? Yeah, I did that, and moved the caching of the name to here, which simplifies deviceName()'s callers. > > > Source/WebCore/platform/ios/PlatformScreenIOS.mm:46 > > SOFT_LINK(UIKit, UIAccessibilityIsGrayscaleEnabled, bool, (void), ()) > > SOFT_LINK(UIKit, UIAccessibilityIsInvertColorsEnabled, bool, (void), ()) > > I think these need to be BOOL rather than bool. > > > Source/WebKit2/Platform/ios/AccessibilityIOS.h:33 > > +RetainPtr<NSData> createAccessibilityRemoteToken(NSUUID *); > > Could just return an NSData * since it’s already going to be autoreleased. Done. Thanks again!
Re-opened since this is blocked by bug 150811
Rolled out due to breaking iOS builds
Committed r191904: <http://trac.webkit.org/changeset/191904>
*** Bug 146130 has been marked as a duplicate of this bug. ***