Bug 188933 - [Attachment Support] Dropping and pasting images should insert inline image elements with _WKAttachments
Summary: [Attachment Support] Dropping and pasting images should insert inline image e...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit Misc. (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Wenson Hsieh
URL:
Keywords: InRadar
Depends on:
Blocks: 188903
  Show dependency treegraph
 
Reported: 2018-08-24 15:21 PDT by Wenson Hsieh
Modified: 2018-08-26 19:54 PDT (History)
9 users (show)

See Also:


Attachments
First pass (94.02 KB, patch)
2018-08-24 16:42 PDT, Wenson Hsieh
ews: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ews106 for mac-sierra-wk2 (3.30 MB, application/zip)
2018-08-24 17:43 PDT, Build Bot
no flags Details
Patch (95.43 KB, patch)
2018-08-25 16:15 PDT, Wenson Hsieh
darin: review+
Details | Formatted Diff | Diff
Patch for landing (95.87 KB, patch)
2018-08-26 18:59 PDT, Wenson Hsieh
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Wenson Hsieh 2018-08-24 15:21:54 PDT
Dropping and pasting images should insert inline image elements with _WKAttachments.
Comment 1 Radar WebKit Bug Importer 2018-08-24 15:22:15 PDT
<rdar://problem/43699724>
Comment 2 Wenson Hsieh 2018-08-24 16:42:30 PDT
Created attachment 348055 [details]
First pass
Comment 3 Build Bot 2018-08-24 17:43:49 PDT Comment hidden (obsolete)
Comment 4 Build Bot 2018-08-24 17:43:51 PDT Comment hidden (obsolete)
Comment 5 Wenson Hsieh 2018-08-25 16:15:10 PDT
Created attachment 348086 [details]
Patch
Comment 6 Darin Adler 2018-08-26 15:08:02 PDT
Comment on attachment 348086 [details]
Patch

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

> Source/WebCore/editing/markup.cpp:418
> +            // These attributes are only intended for File deserialization, and are removed from the generated attachment
> +            // element after we've deserialized and set its backing File.

The comment here says "are removed", but doesn’t make it clear where the code that removes them is. Related, a comment like this that makes promises for code elsewhere ("are removed") is dangerous unless the code that does the removing also has a comment pointing back here.

> Source/WebCore/editing/markup.cpp:937
> +    // We use a fake body element here to trick the HTML parser to using the InBody insertion mode.

"trick into" is the normal idiom, rather than "trick to" that we are using here

> Source/WebCore/page/DragController.cpp:1383
> +        attachment = downcast<HTMLAttachmentElement>(&element);

Compiler probably gets this right without help, but it’s theoretically more efficient to do the "&" after casting, rather than converting to a pointer first.

    attachment = &downcast<HTMLAttachmentElement>(element);

> Source/WebCore/page/DragController.cpp:1385
> +    if (is<HTMLImageElement>(element))

Maybe use else if here for clarity?

> Source/WebKit/UIProcess/API/Cocoa/APIAttachmentCocoa.mm:48
> +    return UTTypeIsDeclared((CFStringRef)type) || UTTypeIsDynamic((CFStringRef)type);

Please use __bridge in these typecasts.

> Source/WebKit/UIProcess/API/Cocoa/APIAttachmentCocoa.mm:58
> +    auto mimeType = adoptCF(UTTypeCopyPreferredTagWithClass((CFStringRef)contentType, kUTTagClassMIMEType));
> +    return (NSString *)mimeType.autorelease();

There is no reason to use autorelease here. We can turn a CFString into a WTF::String without involving NSString. Should be:

    return adoptCF(UTTypeCopyPreferredTagWithClass((__bridge CFStringRef)contentType, kUTTagClassMIMEType)).get();

> Source/WebKit/UIProcess/API/Cocoa/APIAttachmentCocoa.mm:68
> +    auto utiType = adoptCF(UTTypeCreatePreferredIdentifierForTag(kUTTagClassMIMEType, (CFStringRef)contentType, nil));
> +    return (NSString *)utiType.autorelease();

There is no reason to use autorelease here. We can turn a CFString into a WTF::String without involving NSString. Also should use a __bridge cast and should not use "nil" for a non-Objective-C-object pointer. Should be:

    return adoptCF(UTTypeCreatePreferredIdentifierForTag(kUTTagClassMIMEType, (__bridge CFStringRef)contentType, nullptr));
Comment 7 Wenson Hsieh 2018-08-26 15:49:42 PDT
Comment on attachment 348086 [details]
Patch

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

>> Source/WebCore/editing/markup.cpp:418
>> +            // element after we've deserialized and set its backing File.
> 
> The comment here says "are removed", but doesn’t make it clear where the code that removes them is. Related, a comment like this that makes promises for code elsewhere ("are removed") is dangerous unless the code that does the removing also has a comment pointing back here.

Got it — added to this comment describing where we expect to remove these attributes, and also added a comment where we remove these attributes.

>> Source/WebCore/editing/markup.cpp:937
>> +    // We use a fake body element here to trick the HTML parser to using the InBody insertion mode.
> 
> "trick into" is the normal idiom, rather than "trick to" that we are using here

Fixed!

>> Source/WebCore/page/DragController.cpp:1383
>> +        attachment = downcast<HTMLAttachmentElement>(&element);
> 
> Compiler probably gets this right without help, but it’s theoretically more efficient to do the "&" after casting, rather than converting to a pointer first.
> 
>     attachment = &downcast<HTMLAttachmentElement>(element);

Fixed!

>> Source/WebCore/page/DragController.cpp:1385
>> +    if (is<HTMLImageElement>(element))
> 
> Maybe use else if here for clarity?

Ok!

>> Source/WebKit/UIProcess/API/Cocoa/APIAttachmentCocoa.mm:48
>> +    return UTTypeIsDeclared((CFStringRef)type) || UTTypeIsDynamic((CFStringRef)type);
> 
> Please use __bridge in these typecasts.

Fixed!

>> Source/WebKit/UIProcess/API/Cocoa/APIAttachmentCocoa.mm:58
>> +    return (NSString *)mimeType.autorelease();
> 
> There is no reason to use autorelease here. We can turn a CFString into a WTF::String without involving NSString. Should be:
> 
>     return adoptCF(UTTypeCopyPreferredTagWithClass((__bridge CFStringRef)contentType, kUTTagClassMIMEType)).get();

Good point — fixed!

>> Source/WebKit/UIProcess/API/Cocoa/APIAttachmentCocoa.mm:68
>> +    return (NSString *)utiType.autorelease();
> 
> There is no reason to use autorelease here. We can turn a CFString into a WTF::String without involving NSString. Also should use a __bridge cast and should not use "nil" for a non-Objective-C-object pointer. Should be:
> 
>     return adoptCF(UTTypeCreatePreferredIdentifierForTag(kUTTagClassMIMEType, (__bridge CFStringRef)contentType, nullptr));

Fixed! (with a .get() at the end)
Comment 8 Wenson Hsieh 2018-08-26 18:59:48 PDT
Created attachment 348109 [details]
Patch for landing
Comment 9 WebKit Commit Bot 2018-08-26 19:37:30 PDT
Comment on attachment 348109 [details]
Patch for landing

Clearing flags on attachment: 348109

Committed r235343: <https://trac.webkit.org/changeset/235343>