Bug 158590 - [Cocoa] REGRESSION (r198177): Cannot paste an image when the pasteboard format is MIME type
Summary: [Cocoa] REGRESSION (r198177): Cannot paste an image when the pasteboard forma...
Alias: None
Product: WebKit
Classification: Unclassified
Component: HTML Editing (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Enrica Casucci
Depends on:
Reported: 2016-06-09 15:55 PDT by Enrica Casucci
Modified: 2016-06-10 17:19 PDT (History)
2 users (show)

See Also:

Patch (12.96 KB, patch)
2016-06-09 16:13 PDT, Enrica Casucci
darin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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.

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

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;


> 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.