Bug 210894 – Soft link QuickLookThumbnailing framework

• Component: New Bugs
• Status: RESOLVED FIXED

[Nikos Mouchtaris]

Soft link QuickLookThumbnailing framework

[Nikos Mouchtaris]

Created attachment 397306 [details]
Patch

[Tim Horton]

Comment on attachment 397306 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=397306&action=review

> Source/WebKit/Configurations/WebKit.xcconfig:130
> +//WK_QUICKLOOK_THUMBNAILING_LDFLAGS_macosx = $(WK_QUICKLOOK_THUMBNAILING_LDFLAGS$(WK_MACOS_1015));
> +//WK_QUICKLOOK_THUMBNAILING_LDFLAGS_MACOS_SINCE_1015 = -framework QuickLookThumbnailing;
> +//WK_QUICKLOOK_THUMBNAILING_LDFLAGS_iphoneos = -framework QuickLookThumbnailing;

You'll want to actually delete this, not just comment it out

> Source/WebKit/UIProcess/QuickLookThumbnailLoader.mm:79
> +        _filePath = adoptNS([[NSURL fileURLWithPath:fileURL] copy]);

No need for the adopt OR the copy: you can just assign an autoreleased NSURL (as returned by -fileURLWithPath) to a RetainPtr. It will retain it, and all will be well. The previous code was OK

> Source/WebKit/UIProcess/QuickLookThumbnailLoader.mm:95
> -        auto fileURLPath = adoptNS([NSURL fileURLWithPath:filePath]);
> +        auto fileURLPath = adoptNS([[NSURL fileURLWithPath:filePath] copy]);

THIS one was wrong, and will crash. Remove the adoptNS(), and don't add a copy (you only need adopt if you have a +1 object, returned by a -new or -alloc or -create -- NOT for an autoreleased object like here).

> Source/WebKit/UIProcess/QuickLookThumbnailLoader.mm:104
> +    req.get().iconMode = YES;

we generally prefer [req setIconMode:YES]; (normally we'd rather dot notation, but the .get(). is SO UGLY)

> Source/WebKit/UIProcess/QuickLookThumbnailingSPI.h:26
> +#if HAVE(QUICKLOOK_THUMBNAILING)

You don't actually need this SPI header! QuickLookThumbnailing is API, you can include the real header in QuickLookThumbnailingSoftLink, no problem. Soft linking is only about linking, it has no impact on whether or not you can include headers from the framework.

[Nikos Mouchtaris]

Created attachment 397309 [details]
Patch

[Nikos Mouchtaris]

Created attachment 397310 [details]
Patch

[EWS]

Committed r260553: <https://trac.webkit.org/changeset/260553>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 397310 [details].

[Radar WebKit Bug Importer]

<rdar://problem/62229893>

[Ryan Haddad]

This change broke iOS and Catalina builds:
/Volumes/Data/worker/iOS-13-Build-EWS/build/Source/WebKit/UIProcess/QuickLookThumbnailLoader.mm:97:46: error: property 'get' not found on object of type 'NSURL *'

https://build.webkit.org/builders/Apple-Catalina-Release-Build/builds/5902/steps/compile-webkit/logs/stdio

https://build.webkit.org/builders/Apple%20iOS%2013%20Simulator%20Release%20%28Build%29/builds/6353/steps/compile-webkit/logs/stdio

[Ryan Haddad]

Reverted r260553 for reason:

Broke iOS and Catalina builds.

Committed r260558: <https://trac.webkit.org/changeset/260558>

[Nikos Mouchtaris]

Created attachment 397356 [details]
Patch

[EWS]

Committed r260581: <https://trac.webkit.org/changeset/260581>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 397356 [details].