Bug 236386 – Send icons to the WebContent process for rendering of the attachment element

• Component: WebKit Misc.
• Status: RESOLVED FIXED

[Per Arne Vollan]

In order to avoid connecting to the Icon Services daemon in the WebContent process, we should send the icons for the attachment element to the WebContent process from the UI process.

[Per Arne Vollan]

Created attachment 451428 [details]
Patch

[Per Arne Vollan]

(In reply to Per Arne Vollan from comment #1)
> Created attachment 451428 [details]
> Patch

WIP patch.

[Per Arne Vollan]

Created attachment 451438 [details]
Patch

[Per Arne Vollan]

Created attachment 451545 [details]
Patch

[Per Arne Vollan]

Created attachment 451576 [details]
Patch

[Per Arne Vollan]

Created attachment 451701 [details]
Patch

[Geoffrey Garen]

Comment on attachment 451701 [details]
Patch

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

Patch looks good to me; probably want a graphics expert to double-check too.

> Source/WebCore/html/HTMLAttachmentElement.cpp:267
> +void HTMLAttachmentElement::updateIcon(const RefPtr<Image>& icon, const WebCore::FloatSize& size)

icon => iconSize

[Tim Horton]

Comment on attachment 451701 [details]
Patch

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

> Source/WebKit/UIProcess/Cocoa/WebPageProxyCocoa.mm:328
> +    return convertPlatformImageToBitmap(image.get(), WebCore::IntSize(400, 400));

I guess this is your solution to not knowing destination resolution; is the icon any blurrier than it used to be, now that you're using a big representation and scaling it down instead of letting the multi-representation platform image do the right thing?

[Per Arne Vollan]

Created attachment 451758 [details]
Patch

[Per Arne Vollan]

(In reply to Geoffrey Garen from comment #7)
> Comment on attachment 451701 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=451701&action=review
> 
> Patch looks good to me; probably want a graphics expert to double-check too.
> 
> > Source/WebCore/html/HTMLAttachmentElement.cpp:267
> > +void HTMLAttachmentElement::updateIcon(const RefPtr<Image>& icon, const WebCore::FloatSize& size)
> 
> icon => iconSize

Fixed.

Thanks for reviewing!

[Per Arne Vollan]

(In reply to Tim Horton from comment #8)
> Comment on attachment 451701 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=451701&action=review
> 
> > Source/WebKit/UIProcess/Cocoa/WebPageProxyCocoa.mm:328
> > +    return convertPlatformImageToBitmap(image.get(), WebCore::IntSize(400, 400));
> 
> I guess this is your solution to not knowing destination resolution; is the
> icon any blurrier than it used to be, now that you're using a big
> representation and scaling it down instead of letting the
> multi-representation platform image do the right thing?

I have not noticed the icons being blurry when rendered on iOS or macOS.

Thanks for reviewing!

[Per Arne Vollan]

Created attachment 451769 [details]
Patch

[Per Arne Vollan]

Created attachment 451915 [details]
Patch

[Geoffrey Garen]

Comment on attachment 451701 [details]
Patch

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

>>> Source/WebKit/UIProcess/Cocoa/WebPageProxyCocoa.mm:328
>>> +    return convertPlatformImageToBitmap(image.get(), WebCore::IntSize(400, 400));
>> 
>> I guess this is your solution to not knowing destination resolution; is the icon any blurrier than it used to be, now that you're using a big representation and scaling it down instead of letting the multi-representation platform image do the right thing?
> 
> I have not noticed the icons being blurry when rendered on iOS or macOS.
> 
> Thanks for reviewing!

Should we consider trying to send the destination resolution from the WebContent process, or is that something that WebKit just doesn't know?

[Per Arne Vollan]

(In reply to Geoffrey Garen from comment #14)
> Comment on attachment 451701 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=451701&action=review
> 
> >>> Source/WebKit/UIProcess/Cocoa/WebPageProxyCocoa.mm:328
> >>> +    return convertPlatformImageToBitmap(image.get(), WebCore::IntSize(400, 400));
> >> 
> >> I guess this is your solution to not knowing destination resolution; is the icon any blurrier than it used to be, now that you're using a big representation and scaling it down instead of letting the multi-representation platform image do the right thing?
> > 
> > I have not noticed the icons being blurry when rendered on iOS or macOS.
> > 
> > Thanks for reviewing!
> 
> Should we consider trying to send the destination resolution from the
> WebContent process, or is that something that WebKit just doesn't know?

Yes, that is a good point. I think we actually know the destination rect. I will look into this!

Thanks for reviewing!

[Per Arne Vollan]

Created attachment 452024 [details]
Patch

[Per Arne Vollan]

Created attachment 452100 [details]
Patch

[Darin Adler]

Comment on attachment 452100 [details]
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>.

[Per Arne Vollan]

(In reply to Darin Adler from comment #18)
> Comment on attachment 452100 [details]
> 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>.

Thanks for reviewing! I will address the comments in the upcoming patch.

[Per Arne Vollan]

Created attachment 452204 [details]
Patch

[Per Arne Vollan]

Created attachment 452205 [details]
Patch

[Per Arne Vollan]

Created attachment 452207 [details]
Patch

[Per Arne Vollan]

Created attachment 452209 [details]
Patch

[Per Arne Vollan]

Created attachment 452215 [details]
Patch

[Radar WebKit Bug Importer]

<rdar://problem/89038014>

[Per Arne Vollan]

Created attachment 452216 [details]
Patch

[EWS]

Tools/Scripts/svn-apply failed to apply attachment 452216 [details] to trunk.
Please resolve the conflicts and upload a new patch.

[Per Arne Vollan]

Created attachment 452260 [details]
Patch

[EWS]

Committed r289994 (247380@main): <https://commits.webkit.org/247380@main>

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