Bug 238473

Summary:

HTMLAttachmentElement.getAttachmentIdentifier() should propagate attachment data to the client

Product:

WebKit

Reporter:

Wenson Hsieh <wenson_hsieh>

Component:

HTML Editing

Assignee:

Wenson Hsieh <wenson_hsieh>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

akeerthi, cdumez, changseok, esprehn+autocc, ews-watchlist, gyuyoung.kim, hi, kangil.han, katherine_cheney, megan_gardner, mifenton, thorton, webkit-bug-importer, wenson_hsieh

Priority:

Keywords:

InRadar

Version:

WebKit Nightly Build

Hardware:

Unspecified

OS:

Unspecified

Attachments:

Description	Flags
Patch	none
For EWS	none

Description Wenson Hsieh 2022-03-28 15:48:12 PDT

..in the case where the attachment element is newly created.

rdar://90938796

Comment 1 Wenson Hsieh 2022-03-28 16:16:10 PDT

Created attachment 455966 [details]
Patch

Comment 2 Devin Rousso 2022-03-28 16:33:30 PDT

Comment on attachment 455966 [details]
Patch

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

r=me

> Source/WebCore/editing/Editor.cpp:4233
> +        auto& [contentType, preferredFileName, data] = *attachmentInfo;
> +        client->registerAttachmentIdentifier(identifier, contentType, preferredFileName, WTFMove(data));

NIT: Should these all also be `WTFMove` since that's what's done in the IIFE above?

> Tools/TestWebKitAPI/Tests/WebKitCocoa/WKAttachmentTests.mm:1736
> +    NSString *asyncScript = @"return await new Promise(resolve => {"

NIT: Is the `await` actually needed?

Comment 3 Wenson Hsieh 2022-03-28 16:39:21 PDT

(In reply to Devin Rousso from comment #2)
> Comment on attachment 455966 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=455966&action=review
> 
> r=me
> 
> > Source/WebCore/editing/Editor.cpp:4233
> > +        auto& [contentType, preferredFileName, data] = *attachmentInfo;
> > +        client->registerAttachmentIdentifier(identifier, contentType, preferredFileName, WTFMove(data));
> 
> NIT: Should these all also be `WTFMove` since that's what's done in the IIFE
> above?

I'm not sure. `registerAttachmentIdentifier` takes `const String&` anyways, so I just destructured `attachmentInfo` by reference and passed the results to `registerAttachmentIdentifier`. I guess there's no harm in using WTFMove here.

> 
> > Tools/TestWebKitAPI/Tests/WebKitCocoa/WKAttachmentTests.mm:1736
> > +    NSString *asyncScript = @"return await new Promise(resolve => {"
> 
> NIT: Is the `await` actually needed?

Good point. It looks like `-callAsyncJavaScript:` automatically awaits in the case where the script returns a Promise.

Comment 4 Wenson Hsieh 2022-03-28 17:08:36 PDT

Created attachment 455972 [details]
For EWS

Comment 5 EWS 2022-03-29 08:49:44 PDT

Committed r292032 (248975@main): <https://commits.webkit.org/248975@main>

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