Bug 236405 – Mail attachment switched to TIFF and balloons in size after markup.

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Megan Gardner]

Mail attachment switched to TIFF and baloons in size after markup.

[Megan Gardner]

Created attachment 451446 [details]
Patch

[Tim Horton]

Comment on attachment 451446 [details]
Patch

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

> Source/WebKit/ChangeLog:3
> +        Mail attachment switched to TIFF and baloons in size after markup.

"balloons"

> Source/WebKit/UIProcess/API/Cocoa/APIAttachmentCocoa.mm:157
> +NSData* Attachment::enclosingImageNSData() const

all your nsdata stars are on the wrong side

> Source/WebKit/UIProcess/API/Cocoa/APIAttachmentCocoa.mm:160
> +        return NULL;

nil in a variety of places

> Source/WebKit/UIProcess/mac/WebContextMenuProxyMac.mm:216
> +        auto attachment = page()->attachmentForIdentifier(m_context.controlledImageAttachmentID());
> +        auto itemProvider = adoptNS([[NSItemProvider alloc] initWithItem:attachment->enclosingImageNSData() typeIdentifier:attachment->utiType()]);

I think there might be cases where we don't have an API attachment backing a controlled image? but I'm not sure. Maybe fall back to the old thing if you fail to get the attachment? Or maybe Wenson will tell you I'm wrong.

[Wenson Hsieh]

Comment on attachment 451446 [details]
Patch

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

> Source/WebKit/UIProcess/API/Cocoa/APIAttachmentCocoa.mm:171
> +    NSData *data = [fileWrapper regularFileContents];
> +    if (!data)
> +        return NULL;
> +
> +    return data;

Nit - I would just write this `return fileWrapper.regularFileContents;`

>> Source/WebKit/UIProcess/mac/WebContextMenuProxyMac.mm:216
>> +        auto itemProvider = adoptNS([[NSItemProvider alloc] initWithItem:attachment->enclosingImageNSData() typeIdentifier:attachment->utiType()]);
> 
> I think there might be cases where we don't have an API attachment backing a controlled image? but I'm not sure. Maybe fall back to the old thing if you fail to get the attachment? Or maybe Wenson will tell you I'm wrong.

My understanding was that we would only allow mark up on images that are attachment-backed (i.e., backed by <object> in legacy WK1 compose).

I did verify that in legacy Mail compose, remote images cannot be marked up (by replying to an email with remote images).

But at any rate, using the image bitmap data as a fallback seems sensible!

[Megan Gardner]

Created attachment 451455 [details]
Patch

[Wenson Hsieh]

Comment on attachment 451455 [details]
Patch

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

> Source/WebKit/UIProcess/API/APIAttachment.cpp:106
> +NSData* Attachment::enclosingImageNSData() const

Nit - * is still on the wrong side here.

> Source/WebKit/UIProcess/API/APIAttachment.cpp:108
> +    return nullptr;

Nit - `return nil;`

[Megan Gardner]

Created attachment 451487 [details]
Patch for landing

[Wenson Hsieh]

The Windows build failures in https://ews-build.webkit.org/#/builders/12/builds/72990 seem legit.

[Megan Gardner]

Created attachment 451490 [details]
Patch

[Megan Gardner]

Created attachment 451540 [details]
Patch for landing

[EWS]

Committed r289541 (247072@main): <https://commits.webkit.org/247072@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 451540 [details].

[Radar WebKit Bug Importer]

<rdar://problem/88758362>