Bug 211110 – Test of rendering of QuickLook thumbnail for attachment element

• Status: NEW
• Component: New Bugs

[Nikos Mouchtaris]

Test of rendering of QuickLook thumbnail for attachment element

[Nikos Mouchtaris]

Created attachment 397779 [details]
Patch

[Nikos Mouchtaris]

Created attachment 397857 [details]
Patch

[Nikos Mouchtaris]

Created attachment 397859 [details]
Patch

[Nikos Mouchtaris]

Created attachment 397979 [details]
Patch

[Nikos Mouchtaris]

Created attachment 398063 [details]
Patch

[Tim Horton]

Comment on attachment 398063 [details]
Patch

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

> Source/WebCore/ChangeLog:10
> +        Add FileReaderLoaderWrapper to simplify getting file data. Also 

I can't tell from here or the code what makes FileReaderLoaderWrapper (whose name is a mouthful) better than just using FileReaderLoader (whose name is also a mouthful), or why you didn't just fix FileReaderLoader to be simpler.

> Source/WebCore/html/HTMLAttachmentElement.cpp:120
> +#if HAVE(QUICKLOOK_THUMBNAILING)

Is any of this code actually QLT-specific, or should it be unifdeffed?

> Source/WebCore/html/HTMLAttachmentElement.cpp:274
> +void HTMLAttachmentElement::updateCallback(const RefPtr<VoidCallback> callback)

This is not a great name, you can imagine HTMLAttachmentElement having other callbacks. Also maybe it's "set"? "setThumbnailLoadedCallback" or something? Also, what if there are multiple clients? Is this testing-only code? Maybe a -"ForTesting" suffix?

> Source/WebCore/html/HTMLAttachmentElement.cpp:278
> +#if HAVE(QUICKLOOK_THUMBNAILING)

Newline before this line. But also, this is another one where I'm not sure about the #ifdef.

> Source/WebCore/page/FileReaderLoaderWrapper.cpp:47
> +

Extraneous newlines in some of these empty functions.

> LayoutTests/platform/ios-wk2/TestExpectations:77
> +#QuickLook framework needed

At least: a space after the #
Ideally: a complete sentence, ending with punctuation (normal WebKit style, though this file is NOT a good example of good style)

[Nikos Mouchtaris]

Created attachment 398783 [details]
Patch

[Nikos Mouchtaris]

Created attachment 398786 [details]
Patch

[Darin Adler]

Comment on attachment 398786 [details]
Patch

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

> Source/WebCore/html/HTMLAttachmentElement.cpp:283
> +        RefPtr<SharedBuffer> sharedBuffer = SharedBuffer::create(static_cast<const unsigned char*>(arrayBuffer->data()), arrayBuffer->byteLength());

This should use auto for the type.

If the result was a pointer, RefPtr, then we need to check for null. If not, then it should be Ref<>, not RefPtr.

> Source/WebCore/html/HTMLAttachmentElement.h:63
>      WEBCORE_EXPORT void updateThumbnail(const RefPtr<Image>& thumbnail);

This existing argument type isn’t quite right. I think this should be Image* or Image&. Or maybe RefPtr<Image>&& or Ref<Image>&& if we always want to be handing in ownership.

> Source/WebCore/html/HTMLAttachmentElement.h:64
> +    WEBCORE_EXPORT void setThumbnailLoadedCallbackForTest(const RefPtr<VoidCallback>);

This argument type isn’t right. The "const" makes no sense and has no effect here. I think this should be VoidCallback* or VoidCallback&. Or it could be const RefPtr<VoidCallback>& or const Ref<VoidCallback>& (but not sure why that’s good), or RefPtr<VoidCallback>&& or Ref<VoidCallback>&& (not sure about this either).

> Source/WebCore/html/HTMLAttachmentElement.h:98
> +    std::unique_ptr<BlobLoader> m_pendingFileLoad;

It’s typically not a good pattern to cancel the load only when the element is deallocated. Since the element is a reference-counted object, there should be a more direct lifetime decision to cancel the load not based on the actual lifetime of the reference-counted object. Let's find the analogy in existing objects.

> Source/WebCore/page/ChromeClient.h:530
> +    virtual void registerAttachmentIdentifierFromFileObject(const String&, const String&, const RefPtr<SharedBuffer>&) { }

Type here is strange. Should be SharedBuffer* or SharedBuffer&.

> Source/WebCore/page/ChromeClient.h:531
> +#endif // ENABLE(ATTACHMENT_ELEMENT)

Should remove that comment.

> Source/WebCore/testing/Internals.cpp:4703
> +        return;
> +
> +    
> +    if (auto attachment = document->attachmentForIdentifier(identifier)) {

No need for two blank lines above. Maybe no blank line, but maximum of one.

> Source/WebCore/testing/Internals.h:716
> +#endif // ENABLE(ATTACHMENT_ELEMENT)

Shouldn’t include this comment.

> Source/WebKit/UIProcess/Cocoa/WebPageProxyCocoa.mm:438
> +    auto fileWrapper = adoptNS([pageClient().allocFileWrapperInstance() initRegularFileWithContents:data.buffer()->createNSData().autorelease()]);

There is no need for autorelease here. It should be get instead.

> Source/WebKit/WebProcess/WebCoreSupport/WebChromeClient.cpp:1390
> +    m_page.send(Messages::WebPageProxy::RegisterAttachmentIdentifierFromData(identifier, "", fileName, *data));

More efficient to use emptyString() rather than writing "".

> Source/WebKit/WebProcess/WebCoreSupport/WebChromeClient.h:401
> +    void registerAttachmentIdentifierFromFileObject(const String&, const String&, const RefPtr<WebCore::SharedBuffer>&);

Argument type here is wrong. This should be SharedBuffer&, since the code can’t handle null. Not helpful to use const Ref&.

> Source/WebKit/WebProcess/WebCoreSupport/WebChromeClient.h:402
> +#endif // ENABLE(ATTACHMENT_ELEMENT)

Shouldn’t include this comment.

[Nikos Mouchtaris]

Created attachment 398909 [details]
Patch

[Darin Adler]

Comment on attachment 398909 [details]
Patch

Build failure on wincairo seems to be caused by this patch. Should investigate and fix.

WebKit\WebProcess/WebCoreSupport/WebChromeClient.cpp(1394): error C2440: '<function-style-cast>': cannot convert from 'initializer list' to 'Messages::WebPageProxy::RegisterAttachmentIdentifierFromData'
WebKit\WebProcess/WebCoreSupport/WebChromeClient.cpp(1394): note: No constructor could take the source type, or constructor overload resolution was ambiguous
WebKit\WebProcess/WebCoreSupport/WebChromeClient.cpp(1394): error C2672: 'IPC::MessageSender::send': no matching overloaded function found
WebKit\WebProcess/WebCoreSupport/WebChromeClient.cpp(1394): error C2780: 'bool IPC::MessageSender::send(const U &,WTF::ObjectIdentifier<U>,WTF::OptionSet<IPC::SendOption>)': expects 3 arguments - 1 provided
WebKit\Platform\IPC\MessageSender.h(53): note: see declaration of 'IPC::MessageSender::send'
WebKit\WebProcess/WebCoreSupport/WebChromeClient.cpp(1394): error C2780: 'bool IPC::MessageSender::send(const U &,uint64_t,WTF::OptionSet<IPC::SendOption>)': expects 3 arguments - 1 provided
WebKit\Platform\IPC\MessageSender.h(42): note: see declaration of 'IPC::MessageSender::send'