[webkit-reviews] review granted: [Bug 208891] Modern WebKit: QuickLook for attachment elements : [Attachment 396783] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Nikos Mouchtaris
<nmouchtaris at apple.com>'s request for review:
Bug 208891: Modern WebKit: QuickLook for attachment elements
https://bugs.webkit.org/show_bug.cgi?id=208891

Attachment 396783: Patch

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




--- Comment #12 from Darin Adler <darin at apple.com> ---
Comment on attachment 396783
  --> https://bugs.webkit.org/attachment.cgi?id=396783
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
> + at 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.