Bug 211110

Created attachment 397779 [details] Patch

Created attachment 397857 [details] Patch

Created attachment 397859 [details] Patch

Created attachment 397979 [details] Patch

Created attachment 398063 [details] Patch

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)

Created attachment 398783 [details] Patch

Created attachment 398786 [details] Patch

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.

Created attachment 398909 [details] Patch

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'