[webkit-reviews] review granted: [Bug 132157] [iOS] Implement WebQuickLookHandleClient for WebKit2 : [Attachment 230215] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Andy Estes <aestes at apple.com>'s
request for review:
Bug 132157: [iOS] Implement WebQuickLookHandleClient for WebKit2
https://bugs.webkit.org/show_bug.cgi?id=132157

Attachment 230215: Patch
https://bugs.webkit.org/attachment.cgi?id=230215&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
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?