[webkit-reviews] review granted: [Bug 236386] Send icons to the WebContent process for rendering of the attachment element : [Attachment 452100] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Per Arne Vollan <pvollan at apple.com>'s
request for review:
Bug 236386: Send icons to the WebContent process for rendering of the
attachment element
https://bugs.webkit.org/show_bug.cgi?id=236386

Attachment 452100: Patch

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




--- Comment #18 from Darin Adler <darin at apple.com> ---
Comment on attachment 452100
  --> https://bugs.webkit.org/attachment.cgi?id=452100
Patch

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

> Source/WebCore/html/HTMLAttachmentElement.cpp:283
> +    return;

Don’t need this at the end of a function.

> Source/WebCore/html/HTMLAttachmentElement.cpp:289
> +FloatSize HTMLAttachmentElement::iconSize() const
> +{
> +    return m_iconSize;
> +}

Consider putting this in the header.

> Source/WebCore/page/EmptyAttachmentElementClient.h:40
> +    virtual void requestAttachmentIcon(const String&, const FloatSize&) { }

Should be marked "final", not "virtual".

> Source/WebCore/rendering/RenderThemeIOS.h:71
> +    WEBCORE_EXPORT static RetainPtr<UIImage> iconForAttachment(const String&
fileName, const String& attachmentType, const String& title, FloatSize&);

I suggest returning a structure containing both the UIImage and FloatSize
rather than using an out argument for that.

> Source/WebCore/rendering/RenderThemeIOS.mm:1805
> +    auto name = fileName;
> +    if (name.isEmpty())
> +	   name = title;
> +
> +    [documentInteractionController setName:name];

I would have written:

    [documentInteractionController setName:fileName.isEmpty() ? title :
fileName];

Rather than the 4 line version.

> Source/WebCore/rendering/RenderThemeMac.mm:2567
> +RetainPtr<NSImage> RenderThemeMac::iconForAttachment(const String& fileName,
const String& attachmentType, const String& title)

Seems a shame that we need all those different nsImage() lines rather than a
function returning an Icon and another that returns the NSImage *.

> Source/WebCore/rendering/RenderThemeMac.mm:2574
> +ALLOW_DEPRECATED_DECLARATIONS_BEGIN
> +	       if (auto icon = Icon::createIconForUTI(kUTTypeFolder))
> +		   return icon->nsImage();
> +ALLOW_DEPRECATED_DECLARATIONS_END

Just moved code, but I have some ideas for refinement. Would have been nice to
wrap this around just a single line:

ALLOW_DEPRECATED_DECLARATIONS_BEGIN
    auto type = kUTTypeFolder;
ALLOW_DEPRECATED_DECLARATIONS_END

> Source/WebCore/rendering/RenderThemeMac.mm:2576
> +	       String UTI;

I would have just named this "type".

> Source/WebCore/rendering/RenderThemeMac.mm:2592
> +    NSString *fileExtension = [static_cast<NSString *>(title)
pathExtension];

I recommend using a local variable instead of a static_cast, and auto instead
of writing out the type.

    NSString *cocoaTitle = title;
    if (auto fileExtension = cocoaTitle.pathExtension; fileExtension.length) {

> Source/WebCore/rendering/RenderThemeMac.mm:2601
> +    return nullptr;

Should be nil instead of nullptr.

> Source/WebCore/rendering/RenderThemeMac.mm:2641
> +    String attachmentType = attachment.attachmentElement().attachmentType();

This looks unused. Why compute it if we aren’t using it?

> Source/WebCore/rendering/RenderThemeMac.mm:2649
> +    if (context.paintingDisabled())
> +	   return;

Do we want to request the icon if painting is disabled?

> Source/WebCore/rendering/RenderThemeMac.mm:2651
> +    auto nsImage = icon->nsImage();

Can just call this "image", no need to say "nsImage".

> Source/WebKit/UIProcess/WebPageProxy.cpp:10207
> +    auto icon = iconForAttachment(fileName, contentType, title, size);
> +    if (icon)

if (auto icon = ...)

> Source/WebKit/WebProcess/WebCoreSupport/WebAttachmentElementClient.h:39
> +    WebAttachmentElementClient(WebPage&);

This should be marked explicit.

> Source/WebKit/WebProcess/WebCoreSupport/WebAttachmentElementClient.h:40
> +    virtual ~WebAttachmentElementClient() { }

This is not needed or helpful.

> Source/WebKit/WebProcess/WebCoreSupport/WebAttachmentElementClient.h:42
> +    virtual void requestAttachmentIcon(const String& identifier, const
WebCore::FloatSize&);

This should be marked "final", not "virtual". Also could probably be private.

> Source/WebKit/WebProcess/WebPage/WebPage.cpp:7330
> +	   if (RefPtr<ShareableBitmap> icon = !iconHandle.isNull() ?
ShareableBitmap::create(iconHandle) : nullptr)

This can be RefPtr or auto, I think, doesn’t need to be
RefPtr<ShareableBitmap>.