[webkit-reviews] review granted: [Bug 185459] Download and present System Preview : [Attachment 340378] Patch

[bugzilla-daemon at webkit.org]

Tim Horton <thorton at apple.com> has granted Dean Jackson <dino at apple.com>'s
request for review:
Bug 185459: Download and present System Preview
https://bugs.webkit.org/show_bug.cgi?id=185459

Attachment 340378: Patch

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




--- Comment #4 from Tim Horton <thorton at apple.com> ---
Comment on attachment 340378
  --> https://bugs.webkit.org/attachment.cgi?id=340378
Patch

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

> Source/WebKit/UIProcess/Cocoa/DownloadClient.mm:88
> +#if USE(SYSTEM_PREVIEW)

I’m sure you could use an early return and avoid conditionalizing quite so much
chaos here.

> Source/WebKit/UIProcess/Cocoa/DownloadClient.mm:96
> +    if (downloadProxy.isSystemPreviewDownload()) {

I wonder if in all of these we could hide the logic away in some System Preview
class instead, make it conform to the normal delegate protocol, and just change
where we send the messages if it’s a system preview download? Can you get
enough information back out of the objects handed to the delegate?

> Source/WebKit/UIProcess/Cocoa/DownloadClient.mm:121
> +	   if (m_delegateMethods.downloadDidReceiveData)

I still think these should all be early returns

> Source/WebKit/UIProcess/Cocoa/DownloadClient.mm:208
> +#if USE(SYSTEM_PREVIEW)

Why is this in the else? If the delegate implements that method we should skip
the system preview path??

> Source/WebKit/UIProcess/Cocoa/SystemPreviewControllerCocoa.mm:86
> +	   [_itemProvider registerItemForTypeIdentifier:contentType
loadHandler:^(NSItemProviderCompletionHandler _Null_unspecified
completionHandler, Class _Null_unspecified __unsafe_unretained
expectedValueClass, NSDictionary * _Null_unspecified options) {

Get those _Null_unspecifieds out of there?

> Source/WebKit/UIProcess/Cocoa/SystemPreviewControllerCocoa.mm:171
> +	   [m_qlPreviewController.get() dismissViewControllerAnimated:(BOOL)YES
completion:nullptr];

(BOOL)YES WHAT?