Bug 135596 – [iOS] The raw bytes of an iWork document's PDF preview are displayed rather than the PDF itself

• Component: New Bugs
• Status: RESOLVED FIXED

[Andy Estes]

[iOS] The raw bytes of an iWork document's PDF preview are displayed rather than the PDF itself

[Andy Estes]

<rdar://problem/17869353>

[Andy Estes]

Created attachment 236014 [details]
Patch

[David Kilzer (:ddkilzer)]

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.

[David Kilzer (:ddkilzer)]

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

[Andy Estes]

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

[Andy Estes]

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

[David Kilzer (:ddkilzer)]

(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!

[Andy Estes]

Committed r172035: <http://trac.webkit.org/changeset/172035>