[webkit-reviews] review denied: [Bug 208891] WK2 Quicklook for attachments : [Attachment 396689] Patch
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Thu Apr 16 13:31:37 PDT 2020
Darin Adler <darin at apple.com> has denied Nikos Mouchtaris
<nmouchtaris at apple.com>'s request for review:
Bug 208891: WK2 Quicklook for attachments
https://bugs.webkit.org/show_bug.cgi?id=208891
Attachment 396689: Patch
https://bugs.webkit.org/attachment.cgi?id=396689&action=review
--- Comment #9 from Darin Adler <darin at apple.com> ---
Comment on attachment 396689
--> https://bugs.webkit.org/attachment.cgi?id=396689
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
> + at 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
> + at 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".
More information about the webkit-reviews
mailing list