Bug 158590

Summary: [Cocoa] REGRESSION (r198177): Cannot paste an image when the pasteboard format is MIME type
Product: WebKit Reporter: Enrica Casucci <enrica>
Component: HTML EditingAssignee: Enrica Casucci <enrica>
Status: RESOLVED FIXED    
Severity: Normal CC: beidson, darin
Priority: P2    
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch darin: review+

Description Enrica Casucci 2016-06-09 15:55:54 PDT
This regressed with http://trac.webkit.org/changeset/198177 where EditorIOS.mm and EditorMac.mm where modified inadvertently, postponing adding the resources to the document loader.

rdar://problem/25471371
Comment 1 Enrica Casucci 2016-06-09 16:13:30 PDT
Created attachment 280962 [details]
Patch
Comment 2 Darin Adler 2016-06-09 20:17:38 PDT
Comment on attachment 280962 [details]
Patch

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

Sorry about my part in letting this get broken!

> Source/WebCore/editing/ios/EditorIOS.mm:583
> +    String resourceUrl = [URL isFileURL] ? [URL absoluteString] : resource->url();

Should name this resourceURL, with capital letters for the acronym.

> Source/WebCore/editing/ios/EditorIOS.mm:588
> +    auto imageElement = m_frame.document()->createElement(HTMLNames::imgTag, false);

This is an unnecessarily roundabout way to create an image element. Should instead just write:

    auto imageElement = HTMLImageElement::create(*m_frame.document());

> Source/WebCore/editing/ios/EditorIOS.mm:589
> +    imageElement->setAttribute(HTMLNames::srcAttr, resourceUrl);

For slightly better performance this can use setAttributeWithoutSynchronization instead of setAttribute.

> Source/WebCore/editing/mac/EditorMac.mm:636
> +    String resourceUrl = resource->url().string();

Same comment as above.

> Source/WebCore/editing/mac/EditorMac.mm:641
> +    auto imageElement = document().createElement(HTMLNames::imgTag, false);
> +    imageElement->setAttribute(HTMLNames::srcAttr, resourceUrl);

Same comments as above.

But also, can we (at least eventually) find a way to share more of this copied and pasted code between iOS and Mac?

> Source/WebCore/page/Settings.h:147
> +    WEBCORE_EXPORT void setPreferMimeTypeForImages(bool);
> +    bool preferMimeTypeForImages() const { return m_preferMimeTypeForImages; }

I think it should be MIMEType rather than MimeType.

> Source/WebCore/page/Settings.h:326
> +    bool m_preferMimeTypeForImages : 1;

Ditto.

> Source/WebCore/testing/InternalSettings.idl:66
> +    [RaisesException] void setPreferMimeTypeForImages(boolean preferMimeTypeForImage);

I think it should be MIMEType rather than MimeType.

> LayoutTests/ChangeLog:9
> +        * editing/pasteboard/image-in-iframe-expected.txt: Added.

Can we make this a reference test instead of a render-tree-dumping test?
Comment 3 Enrica Casucci 2016-06-10 17:13:04 PDT
Thanks for the review. I've addressed all your comments.
I tried making a ref test, but I keep getting problems with the caret image.
I'd rather leave this as render tree test.
Comment 4 Enrica Casucci 2016-06-10 17:19:57 PDT
Committed revision 201956.