[iOS] The raw bytes of an iWork document's PDF preview are displayed rather than the PDF itself
<rdar://problem/17869353>
Created attachment 236014 [details] Patch
Comment on attachment 236014 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=236014&action=review r=me > Source/WebCore/platform/network/ios/QuickLook.mm:488 > + [delegate setQuickLookHandle:quickLookHandle.get()]; None of the other QuickLookHandle::create() overloaded methods need this? > Source/WebKit2/WebProcess/Network/WebResourceLoader.cpp:128 > + if (!(m_quickLookHandle = QuickLookHandle::create(resourceLoader(), responseCopy.nsURLResponse()))) I think this would be easier to read as two lines: one where m_quickLookHandle is set, and one where its value is null-checked (as seen in removed code above). Either way is fine, though.
(In reply to comment #3) > (From update of attachment 236014 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=236014&action=review > > > Source/WebCore/platform/network/ios/QuickLook.mm:488 > > + [delegate setQuickLookHandle:quickLookHandle.get()]; > > None of the other QuickLookHandle::create() overloaded methods need this? This probably seems like a dumb question because it's the only method called by the code changes in the patch. I'm just curious why there are three different QuickLookHandle::create() methods in general, but that does not need an answer to land the fix.
(In reply to comment #3) > (From update of attachment 236014 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=236014&action=review > > r=me Thanks! > > > Source/WebKit2/WebProcess/Network/WebResourceLoader.cpp:128 > > + if (!(m_quickLookHandle = QuickLookHandle::create(resourceLoader(), responseCopy.nsURLResponse()))) > > I think this would be easier to read as two lines: one where m_quickLookHandle is set, and one where its value is null-checked (as seen in removed code above). > Either way is fine, though. I agree with you. I'll make this change.
(In reply to comment #4) > (In reply to comment #3) > > (From update of attachment 236014 [details] [details]) > > View in context: https://bugs.webkit.org/attachment.cgi?id=236014&action=review > > > > > Source/WebCore/platform/network/ios/QuickLook.mm:488 > > > + [delegate setQuickLookHandle:quickLookHandle.get()]; > > > > None of the other QuickLookHandle::create() overloaded methods need this? > > This probably seems like a dumb question because it's the only method called by the code changes in the patch. > > I'm just curious why there are three different QuickLookHandle::create() methods in general, but that does not need an answer to land the fix. Not a dumb question at all! The ways in which we create QuickLookHandles has unfortunately grown as we added support for different networking configurations. The original WebKit1 NSURLConnection code path, the USE(CFNETWORK) path, and the WebKit2 out-of-process networking path each have their own version of QuickLookHandle::create(). Since this bug doesn't occur in the WebKit1 configurations, I opted not to change their behavior, so that's why you only see changes in one of the three create() functions.
(In reply to comment #6) > (In reply to comment #4) > > (In reply to comment #3) > > > (From update of attachment 236014 [details] [details] [details]) > > > View in context: https://bugs.webkit.org/attachment.cgi?id=236014&action=review > > > > > > > Source/WebCore/platform/network/ios/QuickLook.mm:488 > > > > + [delegate setQuickLookHandle:quickLookHandle.get()]; > > > > > > None of the other QuickLookHandle::create() overloaded methods need this? > > > > This probably seems like a dumb question because it's the only method called by the code changes in the patch. > > > > I'm just curious why there are three different QuickLookHandle::create() methods in general, but that does not need an answer to land the fix. > > Not a dumb question at all! The ways in which we create QuickLookHandles has unfortunately grown as we added support for different networking configurations. The original WebKit1 NSURLConnection code path, the USE(CFNETWORK) path, and the WebKit2 out-of-process networking path each have their own version of QuickLookHandle::create(). > > Since this bug doesn't occur in the WebKit1 configurations, I opted not to change their behavior, so that's why you only see changes in one of the three create() functions. It all makes sense now. (Not needed for this patch, but either some function renames or adding salient comments above each method would help to make this clearer when looking at the code in the future.) Thanks!
Committed r172035: <http://trac.webkit.org/changeset/172035>