Bug 144362 – Switch QuickLook soft-linking to use QuickLookSoftLink.{h,mm}

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[David Kilzer (:ddkilzer)]

Switch soft-linking of QuickLook.framework to use QuickLookSoftLink.{mm,h}.

[David Kilzer (:ddkilzer)]

<rdar://problem/20625299>

[David Kilzer (:ddkilzer)]

Created attachment 251903 [details]
Patch v1

[Andy Estes]

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>

[Andy Estes]

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.

[David Kilzer (:ddkilzer)]

(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

[Andy Estes]

(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.

[David Kilzer (:ddkilzer)]

(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!

[David Kilzer (:ddkilzer)]

(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?)

[David Kilzer (:ddkilzer)]

Created attachment 251917 [details]
Patch for EWS

[Andy Estes]

(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.

[Andy Estes]

(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.

[Andy Estes]

(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.

[David Kilzer (:ddkilzer)]

Created attachment 251921 [details]
Patch for EWS v2

[David Kilzer (:ddkilzer)]

(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.

[David Kilzer (:ddkilzer)]

(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.

[David Kilzer (:ddkilzer)]

Created attachment 251952 [details]
Patch for EWS v3

[WebKit Commit Bot]

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.

[David Kilzer (:ddkilzer)]

Comment on attachment 251952 [details]
Patch for EWS v3

Forgot to remove the #define!

[David Kilzer (:ddkilzer)]

Created attachment 251955 [details]
Patch for EWS v4

[WebKit Commit Bot]

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.

[David Kilzer (:ddkilzer)]

Created attachment 251957 [details]
Patch for EWS v5 / Landing

[David Kilzer (:ddkilzer)]

Comment on attachment 251955 [details]
Patch for EWS v4

Need to declare function pointer get_##framework##_##className##Class() as extern.

[WebKit Commit Bot]

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.

[David Kilzer (:ddkilzer)]

Committed r183553: <http://trac.webkit.org/changeset/183553>

[WebKit Commit Bot]

Re-opened since this is blocked by bug 144406

[Andy Estes]

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?

[Andy Estes]

> (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'

[David Kilzer (:ddkilzer)]

Created attachment 251987 [details]
Patch for EWS v6 / Landing #2

[David Kilzer (:ddkilzer)]

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.

[David Kilzer (:ddkilzer)]

Committed r183598: <http://trac.webkit.org/changeset/183598>