[webkit-reviews] review granted: [Bug 210814] Clean up QuickLookThumbnailLoader : [Attachment 397126] Patch v1

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted David Kilzer (:ddkilzer)
<ddkilzer at webkit.org>'s request for review:
Bug 210814: Clean up QuickLookThumbnailLoader
https://bugs.webkit.org/show_bug.cgi?id=210814

Attachment 397126: Patch v1

https://bugs.webkit.org/attachment.cgi?id=397126&action=review




--- Comment #2 from Darin Adler <darin at apple.com> ---
Comment on attachment 397126
  --> https://bugs.webkit.org/attachment.cgi?id=397126
Patch v1

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

> Source/WebKit/UIProcess/QuickLookThumbnailLoader.h:33
>  #if PLATFORM(IOS_FAMILY)
>  @class UIImage;
> +using PlatformQLImage = UIImage;
> +#elif PLATFORM(MAC)
> +using PlatformQLImage = NSImage;
>  #endif

I’d like to see this somewhere in PAL or WTF where we can share it for other
Cocoa-specific files. Along with other types that just need UIKit/AppKit
typedefs. Not sure at all that I like the word "Platform" in these type names.
PlatformQLImage does not seem like an improvement to me. I’m thinking it should
be this:

    #if USE(APPKIT)
    OBJC_CLASS(NSImage);
    using CocoaImage = NSImage;
    #else
    OBJC_CLASS(UIImage);
    using CocoaImage = UIImage;
    #endif

In some appropriate header file.

> Source/WebKit/UIProcess/QuickLookThumbnailLoader.h:37
> + at property (nonatomic, readonly, retain) NSOperationQueue
*qlThumbnailGenerationQueue;

This property name really doesn’t need the "ql" prefix. In fact, given the
class it’s in, maybe it should be named "queue".

> Source/WebKit/UIProcess/QuickLookThumbnailLoader.mm:46
> +    [_qlThumbnailGenerationQueue release];

!!!

I guess this is why you’re here?

> Source/WebKit/UIProcess/QuickLookThumbnailLoader.mm:64
> + at interface WKQLThumbnailLoadOperation ()
> + at property (atomic, readwrite, getter=isExecuting) BOOL executing;
> + at property (atomic, readwrite, getter=isFinished) BOOL finished;
> + at property (nonatomic, readwrite, copy) NSString *identifier;
> + at property (nonatomic, readwrite) BOOL shouldWrite;
> + at property (nonatomic, readwrite, retain) PlatformQLImage *thumbnail;
> + at end

I don’t understand this. Why does this all needed to be repeated?

> Source/WebKit/UIProcess/QuickLookThumbnailLoader.mm:98
> -    if (_shouldWrite) {
> +
> +    if (self.shouldWrite) {

This seems to make things worse, not better. Why would a class use a property
to access its own instance variables? Maybe if it’s an object that needs to be
retained/released, but outside of that it does not seem great.

> Source/WebKit/UIProcess/QuickLookThumbnailLoader.mm:125
>  #if PLATFORM(IOS_FAMILY)
> -	   _thumbnail = thumbnail.UIImage;
> +	   self.thumbnail = thumbnail.UIImage;
> +#elif PLATFORM(MAC)
> +	   self.thumbnail = thumbnail.NSImage;
>  #endif

I’d suggest USE(APPKIT) rather than PLATFORM(MAC) and PLATFORM(IOS_FAMILY).

> Source/WebKit/UIProcess/QuickLookThumbnailLoader.mm:141
> +    if (![_thumbnail isEqual:thumbnail])

This seems like a surprising thing to do. Is it really normal to compare the
content of images when setting them?

> Source/WebKit/UIProcess/QuickLookThumbnailLoader.mm:152
> +    if (![_identifier isEqualToString:identifier])

This seems OK, but is it necessary?