Bug 132157 – [iOS] Implement WebQuickLookHandleClient for WebKit2

• Component: New Bugs
• Status: RESOLVED FIXED

[Andy Estes]

[iOS] Implement WebQuickLookHandleClient for WebKit2

[Andy Estes]

Created attachment 230123 [details]
Patch

[Andy Estes]

Created attachment 230127 [details]
Patch

Comment on attachment 230127 [details]
Patch

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

> Source/WebKit2/Shared/ios/QuickLookDocumentData.cpp:85
> +    if (!decoder.decodeFixedLengthData(buffer, documentData.m_size, 1))
> +        return false;

Who releases the buffer if we return here?

> Source/WebKit2/UIProcess/API/Cocoa/WKNavigationDelegatePrivate.h:50
> +- (void)_webView:(WKWebView *)webView didStartLoadForQuickLookDocumentWithFileName:(NSString *)fileName uti:(NSString *)uti;
> +- (void)_webView:(WKWebView *)webView didFinishLoadForQuickLookDocument:(NSData *)documentData;

Would it make sense to add a WKNavigation as the a second parameter to these methods?

> Source/WebKit2/WebProcess/WebCoreSupport/ios/WebFrameLoaderClientIOS.mm:67
> +    handle.setClient(WebQuickLookHandleClient::create(handle, webPage->pageID()));

Is it OK that we don’t track which frame this is happening in? Can Quick Look happen in multiple frames in the same document? If so, how can the delegate in the UI process keep track of which callbacks go with which frame?

[Andy Estes]

(In reply to comment #3)
> (From update of attachment 230127 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=230127&action=review
> 
> > Source/WebKit2/Shared/ios/QuickLookDocumentData.cpp:85
> > +    if (!decoder.decodeFixedLengthData(buffer, documentData.m_size, 1))
> > +        return false;
> 
> Who releases the buffer if we return here?

NOBODY (OOPS!).

> 
> > Source/WebKit2/UIProcess/API/Cocoa/WKNavigationDelegatePrivate.h:50
> > +- (void)_webView:(WKWebView *)webView didStartLoadForQuickLookDocumentWithFileName:(NSString *)fileName uti:(NSString *)uti;
> > +- (void)_webView:(WKWebView *)webView didFinishLoadForQuickLookDocument:(NSData *)documentData;
> 
> Would it make sense to add a WKNavigation as the a second parameter to these methods?

I originally did this, but then removed it once I found I wasn't using it in the API client.

> 
> > Source/WebKit2/WebProcess/WebCoreSupport/ios/WebFrameLoaderClientIOS.mm:67
> > +    handle.setClient(WebQuickLookHandleClient::create(handle, webPage->pageID()));
> 
> Is it OK that we don’t track which frame this is happening in? Can Quick Look happen in multiple frames in the same document? If so, how can the delegate in the UI process keep track of which callbacks go with which frame?

Oops, it's not okay. I'll post a new patch to track the frame.

[Andy Estes]

(In reply to comment #4)
> (In reply to comment #3)
> > (From update of attachment 230127 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=230127&action=review
> > 
> > > Source/WebKit2/WebProcess/WebCoreSupport/ios/WebFrameLoaderClientIOS.mm:67
> > > +    handle.setClient(WebQuickLookHandleClient::create(handle, webPage->pageID()));
> > 
> > Is it OK that we don’t track which frame this is happening in? Can Quick Look happen in multiple frames in the same document? If so, how can the delegate in the UI process keep track of which callbacks go with which frame?
> 
> Oops, it's not okay. I'll post a new patch to track the frame.

Well, I guess it's actually somewhat okay. The client that will adopt this SPI only cares about documents loaded in the main frame. We could track the frame, but it's probably simpler just to not create a QuickLookHandleClient for non-main frames.

[Andy Estes]

Created attachment 230203 [details]
Patch

[Andy Estes]

Comment on attachment 230203 [details]
Patch

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

> Source/WebKit2/UIProcess/ios/WebPageProxyIOS.mm:580
> +void WebPageProxy::didStartLoadForQuickLookDocumentInMainFrame(const String& fileName, const String& uti)
> +{

I should probably sanitize fileName here to ensure it doesn't contain a relative path.

[Andy Estes]

Created attachment 230215 [details]
Patch

[Darin Adler]

Comment on attachment 230215 [details]
Patch

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

> Source/WebKit2/Shared/ios/QuickLookDocumentData.cpp:41
> +void QuickLookDocumentData::append(CFArrayRef dataArray)

I think this should just take one CFDataRef, and the iteration should be at the call site.

> Source/WebKit2/Shared/ios/QuickLookDocumentData.cpp:45
> +        ASSERT(CFGetTypeID(data) == CFDataGetTypeID());

Seems like this assertion should be on value instead of data. Casting after asserting makes more sense.

> Source/WebKit2/Shared/ios/QuickLookDocumentData.cpp:46
> +        QuickLookDocumentData* documentData = static_cast<QuickLookDocumentData*>(context);

Strange that we have to do this when a lambda is involved. You'd think the lambda could just capture this, but maybe that wouldn't make a function.

> Source/WebKit2/Shared/ios/QuickLookDocumentData.cpp:68
> +    encoder << m_size;

Seems a little strange that we keep m_size just so we don’t have to add up sizes here. I suggest we don’t store m_size, and just compute it here.

> Source/WebKit2/Shared/ios/QuickLookDocumentData.cpp:82
> +    uint8_t* const buffer = static_cast<uint8_t*>(CFAllocatorAllocate(kCFAllocatorDefault, documentData.m_size, 0));
> +    ASSERT(buffer);

Why aren’t we using CFDataCreateMutable instead?

> Source/WebKit2/Shared/ios/QuickLookDocumentData.h:40
> +class QuickLookDocumentData {

Can’t we use SharedBuffer for this? This seems a lot like that class.

> Source/WebKit2/Shared/ios/QuickLookDocumentData.h:41
> +WTF_MAKE_NONCOPYABLE(QuickLookDocumentData);

Not sure this needs to be non copyable. Seems to me it would copy just fine.

> Source/WebKit2/Shared/ios/QuickLookDocumentData.h:51
> +    Vector<RetainPtr<CFDataRef>> m_data;

Since it’s normally 1, should we do Vector<RetainPtr<CFDataRef>, 1>?

> Source/WebKit2/UIProcess/ios/WebPageProxyIOS.mm:583
> +    static_assert(notFound == static_cast<size_t>(-1), "The following line assumes WTF::notFound equals -1");

Not sure we should assert this everywhere we depend on it. I think it’s part of the design of the find/substring functions. But I guess it’s neat to assert it explicitly.

Would be better to assert what we really assume which is "notFound + 1 == 0".

> Source/WebKit2/WebProcess/WebCoreSupport/ios/WebQuickLookHandleClient.cpp:47
> +    m_data.append(dataArray);

Seems like iterating the array could be at this level rather than inside the QuickLookDocumentData class.

> Source/WebKit2/WebProcess/WebCoreSupport/ios/WebQuickLookHandleClient.h:44
> +class WebQuickLookHandleClient : public WebCore::QuickLookHandleClient {

Why not mark this class final?

[Andy Estes]

(In reply to comment #9)
> (From update of attachment 230215 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=230215&action=review
> 
> > Source/WebKit2/Shared/ios/QuickLookDocumentData.cpp:41
> > +void QuickLookDocumentData::append(CFArrayRef dataArray)
> 
> I think this should just take one CFDataRef, and the iteration should be at the call site.

Ok, I'll move the iteration up to WebQuickLookHandleClient.

> 
> > Source/WebKit2/Shared/ios/QuickLookDocumentData.cpp:45
> > +        ASSERT(CFGetTypeID(data) == CFDataGetTypeID());
> 
> Seems like this assertion should be on value instead of data. Casting after asserting makes more sense.

Ok.

> 
> > Source/WebKit2/Shared/ios/QuickLookDocumentData.cpp:46
> > +        QuickLookDocumentData* documentData = static_cast<QuickLookDocumentData*>(context);
> 
> Strange that we have to do this when a lambda is involved. You'd think the lambda could just capture this, but maybe that wouldn't make a function.

Right, capturing this would make the lambda no longer be an plain function.

> 
> > Source/WebKit2/Shared/ios/QuickLookDocumentData.cpp:68
> > +    encoder << m_size;
> 
> Seems a little strange that we keep m_size just so we don’t have to add up sizes here. I suggest we don’t store m_size, and just compute it here.

Ok, will do.

> 
> > Source/WebKit2/Shared/ios/QuickLookDocumentData.cpp:82
> > +    uint8_t* const buffer = static_cast<uint8_t*>(CFAllocatorAllocate(kCFAllocatorDefault, documentData.m_size, 0));
> > +    ASSERT(buffer);
> 
> Why aren’t we using CFDataCreateMutable instead?

Because the data shouldn't be mutable, and because I wished to avoid a zero-fill (although from reading code I don't believe CFDataCreateMutable() zero-fills).

> 
> > Source/WebKit2/Shared/ios/QuickLookDocumentData.h:40
> > +class QuickLookDocumentData {
> 
> Can’t we use SharedBuffer for this? This seems a lot like that class.

I don't believe we've taught WebKit2 to encode a SharedBuffer without first merging its data segments, and that's an extra cost I don't want to pay here. If we had already had this code I would've just used SharedBuffer.

It'd probably be good to do this at some point. I've found a few places where we unnecessarily flatten a SharedBuffer in order to encode it via a IPC::DataReference. I can look at fixing this as a follow-up.

> 
> > Source/WebKit2/Shared/ios/QuickLookDocumentData.h:41
> > +WTF_MAKE_NONCOPYABLE(QuickLookDocumentData);
> 
> Not sure this needs to be non copyable. Seems to me it would copy just fine.

Yeah, it would. I'll remove it.

> 
> > Source/WebKit2/Shared/ios/QuickLookDocumentData.h:51
> > +    Vector<RetainPtr<CFDataRef>> m_data;
> 
> Since it’s normally 1, should we do Vector<RetainPtr<CFDataRef>, 1>?

Yes, I'll do this.

> 
> > Source/WebKit2/UIProcess/ios/WebPageProxyIOS.mm:583
> > +    static_assert(notFound == static_cast<size_t>(-1), "The following line assumes WTF::notFound equals -1");
> 
> Not sure we should assert this everywhere we depend on it. I think it’s part of the design of the find/substring functions. But I guess it’s neat to assert it explicitly.
> 
> Would be better to assert what we really assume which is "notFound + 1 == 0".

Ok.

> 
> > Source/WebKit2/WebProcess/WebCoreSupport/ios/WebQuickLookHandleClient.cpp:47
> > +    m_data.append(dataArray);
> 
> Seems like iterating the array could be at this level rather than inside the QuickLookDocumentData class.

Ok.

> 
> > Source/WebKit2/WebProcess/WebCoreSupport/ios/WebQuickLookHandleClient.h:44
> > +class WebQuickLookHandleClient : public WebCore::QuickLookHandleClient {
> 
> Why not mark this class final?

No reason. I'll mark it final.

Thanks for the review!

[Andy Estes]

Committed r167901: <http://trac.webkit.org/changeset/167901>

[Andy Estes]

Fix a transcription error from r167901: <http://trac.webkit.org/changeset/167912>