Switch soft-linking of QuickLook.framework to use QuickLookSoftLink.{mm,h}.
<rdar://problem/20625299>
Created attachment 251903 [details] Patch v1
Comment on attachment 251903 [details] Patch v1 View in context: https://bugs.webkit.org/attachment.cgi?id=251903&action=review > Source/WebCore/ChangeLog:18 > + * platform/ios/QuickLookSoftLink.h: Added. > + * platform/ios/QuickLookSoftLink.mm: Added. I slightly prefer QuickLookSoftLinking.* to QuickLookSoftLink.* (same for the other soft linking files), but I don't object to the current name. > Source/WebCore/WebCore.xcodeproj/project.pbxproj:18851 > + 443917FD1A91B2F8006E04F2 /* QuickLookSoftLink.mm */, > + 443917FE1A91B2F8006E04F2 /* QuickLookSoftLink.h */, This isn't sorted properly. > Source/WebCore/platform/ios/QuickLookSoftLink.h:26 > +#ifndef QuickLookSoftLink_h > +#define QuickLookSoftLink_h Since this header can only be included by Objective-C files, there's no need for this. > Source/WebCore/platform/ios/QuickLookSoftLink.mm:30 > +#include "config.h" > + > +#if USE(QUICK_LOOK) > + > +#include "QuickLookSPI.h" > +#include "SoftLinking.h" #include => #import > Source/WebCore/platform/network/ios/QuickLook.mm:151 > - RetainPtr<id> converter = adoptNS([allocQLPreviewConverterInstance() initWithData:data name:nil uti:uti.get() options:nil]); > + RetainPtr<id> converter = adoptNS([[QLPreviewConverter alloc] initWithData:data name:nil uti:uti.get() options:nil]); RetainPtr<id> => RetainPtr<QLPreviewConverter>
Comment on attachment 251903 [details] Patch v1 Oh, and you need semicolons at the end of all the SOFT_LINK_*()s. This is why iOS EWS is red.
(In reply to comment #4) > Comment on attachment 251903 [details] > Patch v1 > > Oh, and you need semicolons at the end of all the SOFT_LINK_*()s. This is > why iOS EWS is red. Actually, it's because I misspelled the macro in SoftLinking.h: SOFT_LINK_CLASS_FOR_SORUCE Should be: SOFT_LINK_CLASS_FOR_SOURCE
(In reply to comment #5) > (In reply to comment #4) > > Comment on attachment 251903 [details] > > Patch v1 > > > > Oh, and you need semicolons at the end of all the SOFT_LINK_*()s. This is > > why iOS EWS is red. > > Actually, it's because I misspelled the macro in SoftLinking.h: > > SOFT_LINK_CLASS_FOR_SORUCE > > Should be: > > SOFT_LINK_CLASS_FOR_SOURCE Oh! Well I think you should still add the semicolons so that these look like other statements.
(In reply to comment #3) > Comment on attachment 251903 [details] > Patch v1 > > View in context: > https://bugs.webkit.org/attachment.cgi?id=251903&action=review > > > Source/WebCore/ChangeLog:18 > > + * platform/ios/QuickLookSoftLink.h: Added. > > + * platform/ios/QuickLookSoftLink.mm: Added. > > I slightly prefer QuickLookSoftLinking.* to QuickLookSoftLink.* (same for > the other soft linking files), but I don't object to the current name. I do, too, but this made it easier to write rules for check-webkit-style since there weren't any other files that ended in "*SoftLink.h". I'll consider this for a future change as CoreMediaSoftLink.{cpp,h} and one other class now exists. > > Source/WebCore/WebCore.xcodeproj/project.pbxproj:18851 > > + 443917FD1A91B2F8006E04F2 /* QuickLookSoftLink.mm */, > > + 443917FE1A91B2F8006E04F2 /* QuickLookSoftLink.h */, > > This isn't sorted properly. Fixed! > > Source/WebCore/platform/ios/QuickLookSoftLink.h:26 > > +#ifndef QuickLookSoftLink_h > > +#define QuickLookSoftLink_h > > Since this header can only be included by Objective-C files, there's no need > for this. Fixed! > > Source/WebCore/platform/ios/QuickLookSoftLink.mm:30 > > +#include "config.h" > > + > > +#if USE(QUICK_LOOK) > > + > > +#include "QuickLookSPI.h" > > +#include "SoftLinking.h" > > #include => #import Fixed! > > Source/WebCore/platform/network/ios/QuickLook.mm:151 > > - RetainPtr<id> converter = adoptNS([allocQLPreviewConverterInstance() initWithData:data name:nil uti:uti.get() options:nil]); > > + RetainPtr<id> converter = adoptNS([[QLPreviewConverter alloc] initWithData:data name:nil uti:uti.get() options:nil]); > > RetainPtr<id> => RetainPtr<QLPreviewConverter> I don't think this will work with this macro definition will it?: #define QLPreviewConverter get_QuickLook_QLPreviewConverterClass() Is it too non-obvious that this happens in the rest of the code? Thanks!
(In reply to comment #6) > (In reply to comment #5) > > (In reply to comment #4) > > > Comment on attachment 251903 [details] > > > Patch v1 > > > > > > Oh, and you need semicolons at the end of all the SOFT_LINK_*()s. This is > > > why iOS EWS is red. > > > > Actually, it's because I misspelled the macro in SoftLinking.h: > > > > SOFT_LINK_CLASS_FOR_SORUCE > > > > Should be: > > > > SOFT_LINK_CLASS_FOR_SOURCE > > Oh! Well I think you should still add the semicolons so that these look like > other statements. In the name of consistency, I'm going to leave them off for now: $ grep -r 'SOFT_LINK_' Source/WebCore | grep ';$' | wc -l 69 $ grep -r 'SOFT_LINK_' Source/WebCore | grep '[^;]$' | wc -l 851 $ grep -r 'SOFT_LINK_' Source/WebCore | wc -l 920 Maybe we need a rule about this, though. (Macros that don't require a semi-colon should have one regardless?)
Created attachment 251917 [details] Patch for EWS
(In reply to comment #8) > (In reply to comment #6) > > (In reply to comment #5) > > > (In reply to comment #4) > > > > Comment on attachment 251903 [details] > > > > Patch v1 > > > > > > > > Oh, and you need semicolons at the end of all the SOFT_LINK_*()s. This is > > > > why iOS EWS is red. > > > > > > Actually, it's because I misspelled the macro in SoftLinking.h: > > > > > > SOFT_LINK_CLASS_FOR_SORUCE > > > > > > Should be: > > > > > > SOFT_LINK_CLASS_FOR_SOURCE > > > > Oh! Well I think you should still add the semicolons so that these look like > > other statements. > > In the name of consistency, I'm going to leave them off for now: > > $ grep -r 'SOFT_LINK_' Source/WebCore | grep ';$' | wc -l > 69 > $ grep -r 'SOFT_LINK_' Source/WebCore | grep '[^;]$' | wc -l > 851 > $ grep -r 'SOFT_LINK_' Source/WebCore | wc -l > 920 > > Maybe we need a rule about this, though. (Macros that don't require a > semi-colon should have one regardless?) That's fine. I was just expressing a personal preference for preprocessor macro invocations to look like regular C++ statements, but it's not a style guideline or requirement.
(In reply to comment #7) > (In reply to comment #3) > > Comment on attachment 251903 [details] > > Patch v1 > > > > View in context: > > https://bugs.webkit.org/attachment.cgi?id=251903&action=review > > > > > Source/WebCore/platform/network/ios/QuickLook.mm:151 > > > - RetainPtr<id> converter = adoptNS([allocQLPreviewConverterInstance() initWithData:data name:nil uti:uti.get() options:nil]); > > > + RetainPtr<id> converter = adoptNS([[QLPreviewConverter alloc] initWithData:data name:nil uti:uti.get() options:nil]); > > > > RetainPtr<id> => RetainPtr<QLPreviewConverter> > > I don't think this will work with this macro definition will it?: > > #define QLPreviewConverter get_QuickLook_QLPreviewConverterClass() > > Is it too non-obvious that this happens in the rest of the code? Oh, I missed that this was a #define, sorry! We seem to not be doing ourselves any favors by using this #define, though. Using id instead of the real type means we lose some compile-time type safety, and we have the information we need to use the real type even when soft-linking (by importing QuickLookSPI.h). We should look to improve this in a follow-up.
(In reply to comment #11) > (In reply to comment #7) > > (In reply to comment #3) > > > Comment on attachment 251903 [details] > > > Patch v1 > > > > > > View in context: > > > https://bugs.webkit.org/attachment.cgi?id=251903&action=review > > > > > > > Source/WebCore/platform/network/ios/QuickLook.mm:151 > > > > - RetainPtr<id> converter = adoptNS([allocQLPreviewConverterInstance() initWithData:data name:nil uti:uti.get() options:nil]); > > > > + RetainPtr<id> converter = adoptNS([[QLPreviewConverter alloc] initWithData:data name:nil uti:uti.get() options:nil]); > > > > > > RetainPtr<id> => RetainPtr<QLPreviewConverter> > > > > I don't think this will work with this macro definition will it?: > > > > #define QLPreviewConverter get_QuickLook_QLPreviewConverterClass() > > > > Is it too non-obvious that this happens in the rest of the code? > > Oh, I missed that this was a #define, sorry! We seem to not be doing > ourselves any favors by using this #define, though. Using id instead of the > real type means we lose some compile-time type safety, and we have the > information we need to use the real type even when soft-linking (by > importing QuickLookSPI.h). > > We should look to improve this in a follow-up. Just to clarify, I'm not saying we have no need for get_QuickLook_QLPreviewConverterClass(). I'm just saying that for the purposes of RetainPtr<>'s type parameter, we can use the real class name and get the compile-time benefits of importing its @interface even if we aren't linking against the framework.
Comment on attachment 251917 [details] Patch for EWS View in context: https://bugs.webkit.org/attachment.cgi?id=251917&action=review > Source/WebCore/platform/network/ios/QuickLook.mm:398 > - , m_converter(adoptNS([allocQLPreviewConverterInstance() initWithConnection:connection delegate:delegate response:nsResponse options:nil])) > + , m_converter(adoptNS([[QLPreviewConverter alloc] initWithConnection:connection delegate:delegate response:nsResponse options:nil])) We have specifically introduced the alloc…Instance() macros to avoid compiler errors when multiple methods with the same signature but different types exist. This seems like taking a step backwards.
Created attachment 251921 [details] Patch for EWS v2
(In reply to comment #12) > (In reply to comment #11) > > (In reply to comment #7) > > > (In reply to comment #3) > > > > Comment on attachment 251903 [details] > > > > Patch v1 > > > > > > > > View in context: > > > > https://bugs.webkit.org/attachment.cgi?id=251903&action=review > > > > > > > > > Source/WebCore/platform/network/ios/QuickLook.mm:151 > > > > > - RetainPtr<id> converter = adoptNS([allocQLPreviewConverterInstance() initWithData:data name:nil uti:uti.get() options:nil]); > > > > > + RetainPtr<id> converter = adoptNS([[QLPreviewConverter alloc] initWithData:data name:nil uti:uti.get() options:nil]); > > > > > > > > RetainPtr<id> => RetainPtr<QLPreviewConverter> > > > > > > I don't think this will work with this macro definition will it?: > > > > > > #define QLPreviewConverter get_QuickLook_QLPreviewConverterClass() > > > > > > Is it too non-obvious that this happens in the rest of the code? > > > > Oh, I missed that this was a #define, sorry! We seem to not be doing > > ourselves any favors by using this #define, though. Using id instead of the > > real type means we lose some compile-time type safety, and we have the > > information we need to use the real type even when soft-linking (by > > importing QuickLookSPI.h). > > > > We should look to improve this in a follow-up. > > Just to clarify, I'm not saying we have no need for > get_QuickLook_QLPreviewConverterClass(). I'm just saying that for the > purposes of RetainPtr<>'s type parameter, we can use the real class name and > get the compile-time benefits of importing its @interface even if we aren't > linking against the framework. Okay, will fix before landing.
(In reply to comment #13) > Comment on attachment 251917 [details] > Patch for EWS > > View in context: > https://bugs.webkit.org/attachment.cgi?id=251917&action=review > > > Source/WebCore/platform/network/ios/QuickLook.mm:398 > > - , m_converter(adoptNS([allocQLPreviewConverterInstance() initWithConnection:connection delegate:delegate response:nsResponse options:nil])) > > + , m_converter(adoptNS([[QLPreviewConverter alloc] initWithConnection:connection delegate:delegate response:nsResponse options:nil])) > > We have specifically introduced the alloc…Instance() macros to avoid > compiler errors when multiple methods with the same signature but different > types exist. This seems like taking a step backwards. I will re-instate the alloc...Instance() method before landing. Thanks for the background; I was not aware this was done for a specific reason.
Created attachment 251952 [details] Patch for EWS v3
Attachment 251952 [details] did not pass style-queue: ERROR: Source/WebCore/platform/mac/SoftLinking.h:326: Extra space before [ [whitespace/braces] [5] Total errors found: 1 in 7 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 251952 [details] Patch for EWS v3 Forgot to remove the #define!
Created attachment 251955 [details] Patch for EWS v4
Attachment 251955 [details] did not pass style-queue: ERROR: Source/WebCore/platform/mac/SoftLinking.h:326: Extra space before [ [whitespace/braces] [5] Total errors found: 1 in 7 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 251957 [details] Patch for EWS v5 / Landing
Comment on attachment 251955 [details] Patch for EWS v4 Need to declare function pointer get_##framework##_##className##Class() as extern.
Attachment 251957 [details] did not pass style-queue: ERROR: Source/WebCore/platform/mac/SoftLinking.h:327: Extra space before [ [whitespace/braces] [5] Total errors found: 1 in 7 files If any of these errors are false positives, please file a bug against check-webkit-style.
Committed r183553: <http://trac.webkit.org/changeset/183553>
Re-opened since this is blocked by bug 144406
Here was the iOS build error: /Volumes/Data/BuildSlave/Monarch-WebKit/build/OpenSource/Source/WebCore/platform/ios/QuickLookSoftLink.mm:41:51: error: redefinition of 'QLPreviewScheme' with a different type: 'const NSString *' vs 'NSString *const' SOFT_LINK_CONSTANT_FOR_SOURCE(WebCore, QuickLook, QLPreviewScheme, NSString *) In file included from /Volumes/Data/BuildSlave/Monarch-WebKit/build/OpenSource/Source/WebCore/platform/ios/QuickLookSoftLink.mm:30: /Volumes/Data/BuildSlave/Monarch-WebKit/build/OpenSource/Source/WebCore/platform/mac/SoftLinking.h:366:31: note: expanded from macro 'SOFT_LINK_CONSTANT_FOR_SOURCE' In file included from /Volumes/Data/BuildSlave/Monarch-WebKit/build/OpenSource/Source/WebCore/platform/ios/QuickLookSoftLink.mm:29: In file included from /Volumes/Data/BuildSlave/Monarch-WebKit/build/OpenSource/Source/WebCore/platform/spi/ios/QuickLookSPI.h:30: /Applications/Monarch/Xcode.app/Contents/Developer/Platforms/iPhoneOS.platform/Developer/SDKs/iPhoneOS9.0.Internal.sdk/System/Library/Frameworks/QuickLook.framework/PrivateHeaders/QuickLookPrivate.h:17:27: note: previous definition is here QL_EXPORT NSString* const QLPreviewScheme; 1 error generated.
(In reply to comment #27) > Here was the iOS build error: > > /Volumes/Data/BuildSlave/Monarch-WebKit/build/OpenSource/Source/WebCore/ > platform/ios/QuickLookSoftLink.mm:41:51: error: redefinition of > 'QLPreviewScheme' with a different type: 'const NSString *' vs 'NSString > *const' > SOFT_LINK_CONSTANT_FOR_SOURCE(WebCore, QuickLook, QLPreviewScheme, NSString > *) > In file included from > /Volumes/Data/BuildSlave/Monarch-WebKit/build/OpenSource/Source/WebCore/ > platform/ios/QuickLookSoftLink.mm:30: > /Volumes/Data/BuildSlave/Monarch-WebKit/build/OpenSource/Source/WebCore/ > platform/mac/SoftLinking.h:366:31: note: expanded from macro > 'SOFT_LINK_CONSTANT_FOR_SOURCE' > In file included from > /Volumes/Data/BuildSlave/Monarch-WebKit/build/OpenSource/Source/WebCore/ > platform/ios/QuickLookSoftLink.mm:29: > In file included from > /Volumes/Data/BuildSlave/Monarch-WebKit/build/OpenSource/Source/WebCore/ > platform/spi/ios/QuickLookSPI.h:30: > /Applications/Monarch/Xcode.app/Contents/Developer/Platforms/iPhoneOS. > platform/Developer/SDKs/iPhoneOS9.0.Internal.sdk/System/Library/Frameworks/ > QuickLook.framework/PrivateHeaders/QuickLookPrivate.h:17:27: note: previous > definition is here > QL_EXPORT NSString* const QLPreviewScheme; > 1 error generated. Didn’t http://trac.webkit.org/r183561 fix that?
> (In reply to comment #27) > > Here was the iOS build error: > > > > /Volumes/Data/BuildSlave/Monarch-WebKit/build/OpenSource/Source/WebCore/ > > platform/ios/QuickLookSoftLink.mm:41:51: error: redefinition of > > 'QLPreviewScheme' with a different type: 'const NSString *' vs 'NSString > > *const' > > SOFT_LINK_CONSTANT_FOR_SOURCE(WebCore, QuickLook, QLPreviewScheme, NSString > > *) > > In file included from > > /Volumes/Data/BuildSlave/Monarch-WebKit/build/OpenSource/Source/WebCore/ > > platform/ios/QuickLookSoftLink.mm:30: > > /Volumes/Data/BuildSlave/Monarch-WebKit/build/OpenSource/Source/WebCore/ > > platform/mac/SoftLinking.h:366:31: note: expanded from macro > > 'SOFT_LINK_CONSTANT_FOR_SOURCE' > > In file included from > > /Volumes/Data/BuildSlave/Monarch-WebKit/build/OpenSource/Source/WebCore/ > > platform/ios/QuickLookSoftLink.mm:29: > > In file included from > > /Volumes/Data/BuildSlave/Monarch-WebKit/build/OpenSource/Source/WebCore/ > > platform/spi/ios/QuickLookSPI.h:30: > > /Applications/Monarch/Xcode.app/Contents/Developer/Platforms/iPhoneOS. > > platform/Developer/SDKs/iPhoneOS9.0.Internal.sdk/System/Library/Frameworks/ > > QuickLook.framework/PrivateHeaders/QuickLookPrivate.h:17:27: note: previous > > definition is here > > QL_EXPORT NSString* const QLPreviewScheme; > > 1 error generated. > > Didn’t http://trac.webkit.org/r183561 fix that? That revision just turned the error into: error: redefinition of 'QLPreviewScheme' with a different type: 'const NSString *const' vs 'NSString *const'
Created attachment 251987 [details] Patch for EWS v6 / Landing #2
Comment on attachment 251957 [details] Patch for EWS v5 / Landing For the 'NSString *' extern variable, we needed SOFT_LINK_POINTER_FOR_{HEADER,SOURCE} macros to be implemented.
Committed r183598: <http://trac.webkit.org/changeset/183598>