Bug 131597 – [QuickLook] Move file system-related code into WebKit

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Andy Estes]

[QuickLook] Move file system-related code into WebKit

[Andy Estes]

Created attachment 229245 [details]
Patch

Comment on attachment 229245 [details]
Patch

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.

Comment on attachment 229245 [details]
Patch

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

>> Source/WebKit/mac/WebCoreSupport/WebFrameLoaderClient.mm:2451
>> +        }
> 
> Seeing this I definitely think a single client function (didReceiveData) is enough. The QuickLookHandle can call it repeatedly in the data-array case.

Alternatively, if we think the array case is the common case, the client function can be the one that takes an array, and QuickLookHandle can make a single-object array in the plain data case.

[Andy Estes]

Thank you for the review!

(In reply to comment #2)
> (From update of attachment 229245 [details])
> 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?

In a future patch I'll need to call it from WebKit2.

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

Whoops, copy/paste fail. Each should read "Removed file system code and called ...".

> 
> > Source/WebCore/platform/network/ios/QuickLook.mm:451
> > +    m_client = nullptr;
> 
> Why do we have to do this?

Totally unnecessary in the destructor. Not sure why I typed that.

> 
> > 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.

It wasn't. I just prefer removing the local variable in this case. I don't feel strongly about it.

[Andy Estes]

(In reply to comment #3)
> (From update of attachment 229245 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=229245&action=review
> 
> >> Source/WebKit/mac/WebCoreSupport/WebFrameLoaderClient.mm:2451
> >> +        }
> > 
> > Seeing this I definitely think a single client function (didReceiveData) is enough. The QuickLookHandle can call it repeatedly in the data-array case.
> 
> Alternatively, if we think the array case is the common case, the client function can be the one that takes an array, and QuickLookHandle can make a single-object array in the plain data case.

I believe it is the common case. I'll make the change you suggest before landing. Thanks!

[Andy Estes]

(In reply to comment #2)
> (From update of attachment 229245 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=229245&action=review
> 
> > 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.

I changed the comment to read:

// previewRequestURL should match the URL removed from -[WebDataSource dealloc].

[Andy Estes]

Committed r167207: <http://trac.webkit.org/changeset/167207>