Bug 208891 – Modern WebKit: QuickLook for attachment elements

• Component: New Bugs
• Status: RESOLVED FIXED

[Nikos Mouchtaris]

WK2 Quicklook for attachments

[Nikos Mouchtaris]

Created attachment 395233 [details]
Patch

[Nikos Mouchtaris]

Created attachment 395234 [details]
Patch

[Tim Horton]

Comment on attachment 395234 [details]
Patch

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

> Source/WebCore/ChangeLog:7
> +

Both changelogs could do with more comments at the function level about what's going on and why.

> Source/WebCore/html/HTMLAttachmentElement.cpp:252
> +void HTMLAttachmentElement::updateThumbnailRepresentation(const RefPtr<Image> qlThumbnailRepresentation)

I think we should avoid referencing QuickLook at this platform-independent layer and just refer to it as a thumbnail or something

> Source/WebCore/rendering/RenderThemeIOS.mm:1826
> +            iconRect = FloatRect(FloatPoint((attachmentRect.width() / 2) - (65 / 2), 0), FloatSize(65, 65));

There's a spot for magic numbers up by the #if ENABLE(ATTACHMENT_ELEMENT) above

> Source/WebCore/rendering/RenderThemeIOS.mm:1832
> +            if (icon) {

I think all of the layout and painting code can be refactored so there's only one copy of the iconRect/yOffset computation and drawImage?

> Source/WebKit/UIProcess/API/Cocoa/WKWebView.mm:189
> +#import <QuickLookThumbnailing/QLThumbnailGenerator.h>
> +

I don't think this needs to be here?

> Source/WebKit/UIProcess/Cocoa/WebPageProxyCocoa.mm:35
> +#import "QLThumbnailLoad.h"

This is a weird name! Maybe expand it out a bit. QuickLookThumbnailLoader or something :)

> Source/WebKit/UIProcess/Cocoa/WebPageProxyCocoa.mm:366
> +static RefPtr<WebKit::ShareableBitmap> convertNSImageToBitmap(NSImage *image, const WebCore::IntSize& size)

Is there not code somewhere shared to do this already? (it might be two steps, NSImage->Image->ShareableBitmap?)

> Source/WebKit/UIProcess/Cocoa/WebPageProxyCocoa.mm:379
> +    ALLOW_DEPRECATED_DECLARATIONS_BEGIN

Not great to add /new/ code that uses deprecated API. Is there no way to do this without deprecated API?

> Source/WebKit/UIProcess/Cocoa/WebPageProxyCocoa.mm:410
> +void (^completionHandler)(BOOL success);

Having this just chilling in the WebKit namespace in WebPageProxyCocoa with the name "completionHandler" seems ripe for conflict. Also, I'm kind of confused what it is? You make a blockPtr from it below, but I don't see a definition anywhere? Something is not right here.

> Source/WebKit/UIProcess/Cocoa/WebPageProxyCocoa.mm:412
> +void WebPageProxy::getQLThumbnailForGenerationRequestion(WKQLThumbnailLoadOperation *operation)

"Requestion"!

> Source/WebKit/UIProcess/Cocoa/WebPageProxyCocoa.mm:418
> +            auto convertedImage = convertNSImageToBitmap(operation.qlThumbnailMac, WebCore::IntSize(1280, 1280));

That is a huuuuuuuuge thumbnail, for what ends up being a very small icon at its largest.

> Source/WebKit/UIProcess/Cocoa/WebPageProxyCocoa.mm:432
> +    [[WKQLThumbnailQueueManager sharedInstance].qlThumbnailGenerationQueue addOperation:operation];

Not sure about all the NSOperation stuff, will have to think about it. We don't often use it.

> Source/WebKit/UIProcess/Cocoa/WebPageProxyCocoa.mm:438
> +    WKQLThumbnailLoadOperation *operation = [[WKQLThumbnailLoadOperation alloc] initWithAttachment:fileWrapper identifier:identifier];

Who frees this?

> Source/WebKit/UIProcess/QLThumbnailLoad.h:1
> +// Copyright © 2020 ___ORGANIZATIONNAME___ All rights reserved.

Not the right copyright header again

> Source/WebKit/UIProcess/QLThumbnailLoad.h:25
> +#if PLATFORM(MAC)
> +@property (nonatomic, readwrite, assign) NSImage *qlThumbnailMac;
> +#endif
> +
> +#if PLATFORM(IOS_FAMILY)
> +@property (nonatomic, readwrite, assign) UIImage *qlThumbnailiOS;
> +#endif

You can have just one property that varies in type, instead of varying both the property name and type.

> Source/WebKit/UIProcess/QLThumbnailLoad.h:29
> +@property (nonatomic, readwrite, assign) NSString *contentType;

In general you don't have to say 'readwrite', that is the default. Also, 'assign' is not likely right (NSObjects should probably be retain or copy, depending on the ownership model)

> Source/WebKit/UIProcess/QLThumbnailLoad.h:38
> +- (id)initWithAttachment:(NSFileWrapper *)fileWrapper identifier: (NSString *)identifier;
> +- (id)initWithURL:(NSString *)fileUrl identifier: (NSString *)identifier;

No space after the :s

> Source/WebKit/UIProcess/QLThumbnailLoad.mm:1
> +// Copyright © 2020 ___ORGANIZATIONNAME___ All rights reserved.

Not the right copyright header again

> Source/WebKit/UIProcess/QLThumbnailLoad.mm:36
> +        _identifier = [identifier copy];

Who frees this?

> Source/WebKit/UIProcess/QLThumbnailLoad.mm:62
> +        _filePath = [[NSURL fileURLWithPath:fileUrl] copy];

Another leak as far as I can see

> Source/WebKit/UIProcess/WebPageProxy.h:1591
> +    void getQLThumbnailForAttachment(const String&, const String&);
> +    void getQLThumbnailForFileWrapper(NSFileWrapper*, const String&);
> +    void getQLThumbnailForGenerationRequestion(WKQLThumbnailLoadOperation*);

I think: drop the QL, and the names are much better. You could even imagine another platform implementing this using those names (but not if they mentioned "QL" or "QuickLook").

Also, the `get` prefix usually indicates an out argument. Maybe these should have a "request-" prefix instead?

[David Kilzer (:ddkilzer)]

Comment on attachment 395234 [details]
Patch

Marking r- based on Tim's comments.  Please post a new patch addressing review comments, and make sure Mac and (if possible) WinCario builds are fixed.

[Nikos Mouchtaris]

Created attachment 396563 [details]
Patch

[Nikos Mouchtaris]

Created attachment 396577 [details]
Patch

[Nikos Mouchtaris]

Created attachment 396689 [details]
Patch

[Tim Horton]

Comment on attachment 396689 [details]
Patch

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

> Source/WTF/wtf/PlatformEnableCocoa.h:289
> +#define ENABLE_QLTHUMBNAIL_ATTACHMENT 1

This could also be a HAVE macro ("I have the platform framework QuickLookThumbnailing" -> HAVE(QUICKLOOK_THUMBNAILING)).

An alternative name for the ENABLE form: ENABLE(ATTACHMENT_ELEMENT_THUMBNAILS)

> Source/WebCore/html/HTMLAttachmentElement.cpp:254
> +void HTMLAttachmentElement::updateThumbnailRepresentation(const RefPtr<Image> thumbnailRepresentation)
> +{
> +    m_thumbnailRepresentation = thumbnailRepresentation;

I wonder if the word "representation" really buys us anything. "updateThumbnail", "m_thumbnail", etc. all seem like they are sufficient to me. Thoughts?

> Source/WebCore/rendering/RenderThemeIOS.mm:1585
> +const CGFloat defaultThumbnailIconWidth = 65;

Remind me where this came from? (I'm surprised it's not the same as attachmentIconSize, but maybe it makes sense!).

> Source/WebKit/Configurations/WebKit.xcconfig:127
> +WK_QUICKLOOK_LDFLAGS = $(WK_QUICKLOOK_LDFLAGS_$(WK_PLATFORM_NAME));

Should be WK_QUICKLOOK_THUMBNAILING_LDFLAGS, IMO. QuickLook.framework is a different (but related) thing.

> Source/WebKit/UIProcess/Cocoa/WebPageProxyCocoa.mm:366
> +static RefPtr<WebKit::ShareableBitmap> convertNSImageToBitmap(NSImage *image, const WebCore::IntSize& size)

You could smush these together (convertPlatformImageToShareableBitmap, taking a PlatformImage typedef), and then only a few lines in the middle would have to be ifdeffed.

> Source/WebKit/UIProcess/Cocoa/WebPageProxyCocoa.mm:379
> +    ALLOW_DEPRECATED_DECLARATIONS_BEGIN

I still think you can find a non-deprecated way to do this. I think what you want is:

[NSGraphicsContext setCurrentContext:[NSGraphicsContext graphicsContextWithCGContext:graphicsContext->platformContext() flipped:YES]];

graphicsContextWithCGContext is available back to 10.10 so it should be no problem.

> Source/WebKit/UIProcess/Cocoa/WebPageProxyCocoa.mm:403
> +    [image drawInRect: CGRectMake(0, 0, bitmap->size().width(), bitmap->size().height())];

No space after :

> Source/WebKit/UIProcess/QuickLookThumbnailLoader.mm:139
> +-(UIImage *)getThumbnailiOS {

Braces are in the wrong place (should be alone on the next line), and this should not have a get- prefix because it does not have any out arguments.
Also, I would recommend naming both of these methods the same thing, just -thumbnail, and only having their return value differ. That way, the caller (above) doesn't have to have extraneous #ifdefs, and we don't have to say weird things like iOS and Mac in method names.

> Source/WebKit/UIProcess/QuickLookThumbnailLoader.mm:151
> +- (NSString *)getAttachmentIdentifier

Another get!

[Darin Adler]

Comment on attachment 396689 [details]
Patch

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

review- because this has at least one reference counting mistake that must be fixed

> Source/WebCore/ChangeLog:3
> +        WK2 Quicklook for attachments

Let's say modern WebKit if we need to distinguish from WebKitLegacy, and not "WK2". QuickLook and not "Quicklook".

> Source/WebCore/ChangeLog:9
> +        ql thumbnail. Added code to render this image on both ios and mac.

Let's say QuickLook and not "ql". Also, lets use iOS and Mac, not "ios" and "mac".

> Source/WebCore/ChangeLog:15
> +        Allow setting of thumbnial member.

typo: "thumbnail"

> Source/WebKit/ChangeLog:8
> +        Allow attachment elements to render quicklook thumbnail generated

Let's say QuickLook and not "quicklook".

> Source/WebCore/html/HTMLAttachmentElement.h:61
> +    WEBCORE_EXPORT void updateThumbnailRepresentation(const RefPtr<Image> thumbnailRepresentation);

This type doesn’t make sense; the use of const unusual here since it’s not so meaningful when applied to a type. Some possible types that would make more sense:

    Image*
    const RefPtr<Image>&
    Image&
    const Ref<Image>&

> Source/WebCore/rendering/RenderThemeIOS.mm:1585
> +const CGFloat defaultThumbnailIconWidth = 65;

In new code we want to use constexpr for things like this instead of const.

> Source/WebCore/rendering/RenderThemeIOS.mm:1854
> +    context.drawImage(*iconImage.get(), info.iconRect);

Should not need ".get()" here if we are using *.

> Source/WebCore/rendering/RenderThemeMac.mm:2781
> +    auto thumbnailIcon = attachment.attachmentElement().thumbnailRepresentation();
> +    if (thumbnailIcon) {

Better to define the local variable inside the if so it's scoped:

    if (auto thumbnailIcon = attachment.attachmentElement().thumbnailRepresentation()) {

> Source/WebCore/rendering/RenderThemeMac.mm:2782
> +        context.drawImage(*thumbnailIcon.get(), layout.iconRect);

Should not need ".get()" here if we are using const.

> Source/WebKit/UIProcess/Cocoa/WebPageProxyCocoa.mm:377
> +    RetainPtr<NSGraphicsContext> savedContext = [NSGraphicsContext currentContext];

I guess you just moved this code. How about writing it this way?

    auto savedContext = retainPtr([NSGraphicsContext currentContext]);

Also the LocalCurrentGraphicsContext seems to exist for this purpose. Too bad we can’t use that.

> Source/WebKit/UIProcess/Cocoa/WebPageProxyCocoa.mm:381
> +    ALLOW_DEPRECATED_DECLARATIONS_BEGIN
> +    [NSGraphicsContext setCurrentContext:[NSGraphicsContext graphicsContextWithGraphicsPort:graphicsContext->platformContext() flipped:YES]];
> +    ALLOW_DEPRECATED_DECLARATIONS_END

Seems unfortunate that we are using this deprecated declaration in newly written code. Is there a non-deprecated alternative available?

> Source/WebKit/UIProcess/Cocoa/WebPageProxyCocoa.mm:421
> +            if (convertedImage == nullptr)

WebKit coding style says:

    if (!convertedImage)
        return;

> Source/WebKit/UIProcess/QuickLookThumbnailLoader.h:25
> +*/
> +#if ENABLE(QLTHUMBNAIL_ATTACHMENT)

Missing blank line here.

> Source/WebKit/UIProcess/QuickLookThumbnailLoader.h:26
> +#import <Foundation/Foundation.h>

Seems highly unlikely this included is needed. Pretty sure Foundation.h is always included in any Objective-C compilation.

> Source/WebKit/UIProcess/QuickLookThumbnailLoader.h:29
> +#if PLATFORM(IOS_FAMILY)
> +#import <UIKit/UIKit.h>
> +#endif

Normally we’d use a forward declaration of the Objective-C class we are using rather than importing the UIKit header.

> Source/WebKit/UIProcess/QuickLookThumbnailLoader.h:30
> +@interface WKQLThumbnailQueueManager : NSObject

Missing blank line before this.

> Source/WebKit/UIProcess/QuickLookThumbnailLoader.h:33
> +- (id)init;

Typically don’t need to declare this since it’s inherited from NSObject. We can override it, of course.

> Source/WebKit/UIProcess/QuickLookThumbnailLoader.h:43
> +@property (nonatomic, retain) NSString *contentType;

Normally we’d use copy for an NSString, not retain.

> Source/WebKit/UIProcess/QuickLookThumbnailLoader.h:46
> +- (id)initWithAttachment:(NSFileWrapper *)fileWrapper identifier: (NSString *)identifier;

No space after the colon here.

> Source/WebKit/UIProcess/QuickLookThumbnailLoader.h:47
> +- (id)initWithURL:(NSString *)fileUrl identifier: (NSString *)identifier;

Should be fileURL, also no space after the colon.

> Source/WebKit/UIProcess/QuickLookThumbnailLoader.h:48
> +- (NSString *)getAttachmentIdentifier;

We don’t use get in these kinds of method names in WebKit coding style or even Cocoa coding style.

> Source/WebKit/UIProcess/QuickLookThumbnailLoader.h:50
> +-(UIImage *)getThumbnailiOS;

Ditto. Also, doesn’t need to mention the platform in the name.

> Source/WebKit/UIProcess/QuickLookThumbnailLoader.h:53
> +-(NSImage *)getThumbnailMac;

Ditto.

> Source/WebKit/UIProcess/QuickLookThumbnailLoader.mm:28
> +#import "config.h"
> +#import "QuickLookThumbnailLoader.h"

Typically we put these outside the #if.

> Source/WebKit/UIProcess/QuickLookThumbnailLoader.mm:39
> +        _qlThumbnailGenerationQueue = [[NSOperationQueue alloc] init];

I don’t think we need the prefix "ql" in this field name. Also I don’t see where it’s defined.

> Source/WebKit/UIProcess/QuickLookThumbnailLoader.mm:50
> +    static WKQLThumbnailQueueManager *sharedInstance = nil;
> +    static dispatch_once_t isDispatched;
> +
> +    dispatch_once(&isDispatched, ^{
> +        sharedInstance = [[WKQLThumbnailQueueManager alloc] init];
> +    });

If this is used from a single thread.

> Source/WebKit/UIProcess/QuickLookThumbnailLoader.mm:62
> +    RetainPtr<NSImage> _qlThumbnail;

Should just be named _thumbnail.

> Source/WebKit/UIProcess/QuickLookThumbnailLoader.mm:65
> +    RetainPtr<UIImage> _qlThumbnail;

Ditto.

> Source/WebKit/UIProcess/QuickLookThumbnailLoader.mm:72
> +        _fileWrapper =  adoptNS(fileWrapper);

This is wrong and will result in over-release. Do not call adoptNS.

> Source/WebKit/UIProcess/QuickLookThumbnailLoader.mm:75
> +        _qlThumbnail = nullptr;
> +        _filePath = nullptr;

Unnecessary. Objective-C starts all fields as null or NO.

> Source/WebKit/UIProcess/QuickLookThumbnailLoader.mm:84
> +        _fileWrapper = nullptr;

Unnecessary.

> Source/WebKit/UIProcess/QuickLookThumbnailLoader.mm:85
> +        _identifier =  adoptNS([identifier copy]);

Extra space here before adoptNS.

> Source/WebKit/UIProcess/QuickLookThumbnailLoader.mm:86
> +        _qlThumbnail = nullptr;

Unnecessary.

> Source/WebKit/UIProcess/QuickLookThumbnailLoader.mm:88
> +        _shouldWrite = NO;

Unnecessary.

> Source/WebKit/UIProcess/QuickLookThumbnailLoader.mm:96
> +    
> +    NSLog(@"Starting %@", self);

Please don’t log with this line.

> Source/WebKit/UIProcess/QuickLookThumbnailLoader.mm:114
> +    QLThumbnailGenerationRequest *req = [[QLThumbnailGenerationRequest alloc] initWithFileAtURL:_filePath.get() size:CGSizeMake(400, 400) scale:1 representationTypes:QLThumbnailGenerationRequestRepresentationTypeAll];

This is allocated with no release. We need to use adoptNS or release.

Also, we don’t use names like "req" in WebKit, instead "request".

> Source/WebKit/UIProcess/QuickLookThumbnailLoader.mm:139
> +-(UIImage *)getThumbnailiOS {

We put spaces after "-". We put a "{" on a separate line.

> Source/WebKit/UIProcess/QuickLookThumbnailLoader.mm:145
> +-(NSImage *)getThumbnailMac {

We put spaces after "-". We put a "{" on a separate line.

> Source/WebKit/WebProcess/WebPage/WebPage.cpp:6835
> +        if (RefPtr<ShareableBitmap> qlThumbnail = !qlThumbnailHandle.isNull() ? ShareableBitmap::create(qlThumbnailHandle) : nullptr)

Should just call this "thumbnail".

[Darin Adler]

Comment on attachment 396689 [details]
Patch

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

>> Source/WebKit/UIProcess/QuickLookThumbnailLoader.mm:50
>> +    });
> 
> If this is used from a single thread.

Sorry, I meant to say, if this is used from a single thread, don’t need the dispatch_once dance.

[Nikos Mouchtaris]

Created attachment 396783 [details]
Patch

[Darin Adler]

Comment on attachment 396783 [details]
Patch

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

Not sure I fully understand the threading concerns and whether the use of @synchronized is sufficient locking. Typically it’s not sufficient to just do individual locks around setting flags and it takes more. But I didn’t see anything I know is incorrect.

> Source/WebCore/rendering/RenderThemeIOS.mm:1810
> +            auto iconSizeBoth = thumbnailIcon ? FloatSize(attachmentIconSize, attachmentIconSize) : iconSize;

I don’t fully understand the word "both" in this name given that it’s size or the other. Is there some guarantee that this is the larger of the two? Maybe this should be something like "visibleIconSize" since only one is visible?

> Source/WebKit/UIProcess/Cocoa/WebPageProxyCocoa.mm:366
> +typedef NSImage *PlatformImage;

In newer code we try to use "using" instead of "typedef".

> Source/WebKit/UIProcess/Cocoa/WebPageProxyCocoa.mm:368
> +typedef UIImage *PlatformImage;

Ditto.

> Source/WebKit/UIProcess/QuickLookThumbnailLoader.h:25
> +#if HAVE(QUICKLOOK_THUMBNAILING)

One more blank line before this, please.

> Source/WebKit/UIProcess/QuickLookThumbnailLoader.h:57
> +#endif

One more blank line before this, please.

> Source/WebKit/UIProcess/QuickLookThumbnailLoader.mm:27
> +*/
> +
> +
> +#import "config.h"

Only need one blank line here, not two.

> Source/WebKit/UIProcess/QuickLookThumbnailLoader.mm:50
> +@end

Another blank line before this, please.

> Source/WebKit/UIProcess/QuickLookThumbnailLoader.mm:64
> +- (id)initWithAttachment:(NSFileWrapper *)fileWrapper identifier:(NSMutableString *)identifier

This should be NSString, not NSMutableString.

> Source/WebKit/UIProcess/QuickLookThumbnailLoader.mm:74
> +- (id)initWithURL:(NSMutableString *)fileURL identifier:(NSMutableString *)identifier

This should be NSString, not NSMutableString.

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

The adoptNS/copy aren’t needed here. Just:

    _filePath = [NSURL fileURLWithPath:fileURL];

> Source/WebKit/UIProcess/QuickLookThumbnailLoader.mm:86
> +{
> +    
> +    self.executing = YES;

Should get rid of this excess blank line.

> Source/WebKit/UIProcess/QuickLookThumbnailLoader.mm:89
> +        NSString *tempDirectory = FileSystem::createTemporaryDirectory(@"QLTempFileData");

WebKit coding style says to use words, not abbreviations. So this would be better named either "directory" or "temporaryDirectory".

> Source/WebKit/UIProcess/QuickLookThumbnailLoader.mm:98
> +        _filePath = fileURLPath;

Could save a little reference count churn here with WTFMove since fileURLPath is not used after this line:

    _filePath = WTFMove(fileURLPath);

> Source/WebKit/UIProcess/QuickLookThumbnailLoader.mm:119
> +            NSError *errorFile = nil;
> +            [[NSFileManager defaultManager] removeItemAtURL:_filePath.get() error:&errorFile];

If we are going to ignore the error, we can pass error:nullptr and there is no need for a local variable.

> Source/WebKit/UIProcess/QuickLookThumbnailLoader.mm:192
> +#endif

Should add another blank line before this.

[Nikos Mouchtaris]

Created attachment 397035 [details]
Patch

[EWS]

Committed r260407: <https://trac.webkit.org/changeset/260407>

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

[Radar WebKit Bug Importer]

<rdar://problem/62083967>