[webkit-reviews] review granted: [Bug 131597] [QuickLook] Move file system-related code into WebKit : [Attachment 229245] Patch

[bugzilla-daemon at webkit.org]

mitz at webkit.org <mitz at webkit.org> has granted Andy Estes <aestes at apple.com>'s
request for review:
Bug 131597: [QuickLook] Move file system-related code into WebKit
https://bugs.webkit.org/show_bug.cgi?id=131597

Attachment 229245: Patch
https://bugs.webkit.org/attachment.cgi?id=229245&action=review

------- Additional Comments from mitz at webkit.org <mitz at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=229245&action=review


> Source/WebCore/ChangeLog:40
> +	   (WebCore::createTemporaryFileForQuickLook): Made non-static.

Why does this function need to live in WebCore?

> Source/WebCore/ChangeLog:45
> +	   (WebCore::QuickLookHandle::didReceiveDataArray): Removed file system
and code and called QuickLookHandleClient::didReceiveDataArray() instead.

“file system and code”?

> Source/WebCore/platform/network/ios/QuickLook.mm:365
> +    std::unique_ptr<QuickLookHandle> quickLookHandle(new
QuickLookHandle([handle->firstRequest().nsURLRequest(DoNotUpdateHTTPBody) URL],
connection, nsResponse, delegate));

Strange that we go through an NSURLRequest only to get an NSURL out of a
request. Strange, but not new to this patch.

> Source/WebCore/platform/network/ios/QuickLook.mm:451
> +    m_client = nullptr;

Why do we have to do this?

> Source/WebCore/platform/network/ios/QuickLookHandleClient.h:40
> +    virtual void didReceiveDataArray(CFArrayRef) { }
> +    virtual void didReceiveData(CFDataRef) { }

It’s fine, but a little strange to expose the client to these two different
ways the data may be coming in.

> Source/WebKit/mac/WebCoreSupport/WebFrameLoaderClient.mm:2423
> +		   // We must use the generated URL from m_converter's
NSURLRequest object

There’s no m_converter in the new context of this comment.

> Source/WebKit/mac/WebCoreSupport/WebFrameLoaderClient.mm:2451
> +	   void didReceiveDataArray(CFArrayRef dataArray)
> +	   {
> +	       if (m_fileHandle) {
> +		   for (NSData *data in (NSArray *)dataArray)
> +		       [m_fileHandle writeData:data];
> +	       }
> +	   }
> +
> +	   void didReceiveData(CFDataRef data)
> +	   {
> +	       if (m_fileHandle)
> +		   [m_fileHandle writeData:(NSData *)data];
> +	   }

Seeing this I definitely think a single client function (didReceiveData) is
enough. The QuickLookHandle can call it repeatedly in the data-array case.

> Source/WebKit2/WebProcess/ios/WebResourceLoaderIOS.mm:105
> -    RetainPtr<WKWebResourceQuickLookDelegate> delegate =
adoptNS([[WKWebResourceQuickLookDelegate alloc]
initWithWebResourceLoader:this]);
> -    m_quickLookHandle = QuickLookHandle::create(resourceLoader(),
response.nsURLResponse(), delegate.get());
> +    m_quickLookHandle = QuickLookHandle::create(resourceLoader(),
response.nsURLResponse(), [[[WKWebResourceQuickLookDelegate alloc]
initWithWebResourceLoader:this] autorelease]);

Not sure why this change was needed.