[QuickLook] Move file system-related code into WebKit
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.
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.
(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!
(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].
Committed r167207: <http://trac.webkit.org/changeset/167207>