[webkit-reviews] review granted: [Bug 188933] [Attachment Support] Dropping and pasting images should insert inline image elements with _WKAttachments : [Attachment 348086] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Wenson Hsieh
<wenson_hsieh at apple.com>'s request for review:
Bug 188933: [Attachment Support] Dropping and pasting images should insert
inline image elements with _WKAttachments
https://bugs.webkit.org/show_bug.cgi?id=188933

Attachment 348086: Patch

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




--- Comment #6 from Darin Adler <darin at apple.com> ---
Comment on attachment 348086
  --> https://bugs.webkit.org/attachment.cgi?id=348086
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));