Bug 238295 – DOM GPUP: paintSystemPreviewBadge (AR QuickLook element badge)

• Component: New Bugs
• Status: RESOLVED FIXED

[Antoine Quint]

DOM GPUP: paintSystemPreviewBadge (AR QuickLook element badge)

[Antoine Quint]

rdar://89196787

[Antoine Quint]

Created attachment 455570 [details]
Patch

[Antoine Quint]

rdar://83580608

[Antoine Quint]

Created attachment 455572 [details]
Patch

[Said Abou-Hallawa]

Comment on attachment 455572 [details]
Patch

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

> Source/WebCore/Modules/system-preview/ARKitBadgeSystemImage.h:41
> +class WEBCORE_EXPORT ARKitBadgeSystemImage final : public SystemImage {

I am not sure why ARKitBadgeSystemImage should be a super calls of SystemImage or even a superclass of Image. I think all we need to do is to route the drawing of the Image as an ARKitBadgeImage through GraphicsContext. Just follow the pattern of drawNativeImage() or drawPattern():

// RenderThemeIOS.mm
void RenderThemeIOS::paintSystemPreviewBadge(Image& image, const PaintInfo& paintInfo, const FloatRect& rect)
{
    if (auto nativeImage = image.nativeImage())
        paintInfo.context().drawARKitBadgeImage(*nativeImage, rect);
}

// GraphicsContext.h
class GraphicsContext {
public:
    virtual void drawARKitBadgeImage(NativeImage&, const FloatRect&) const { }
}

// GraphicsContextCG.h
class GraphicsContextCG : public GraphicsContext {
private:
    void drawARKitBadgeImage(NativeImage&, const FloatRect& destRect) const final;
}

// GraphicsContextCG.cpp
void GraphicsContextCG::drawARKitBadgeImage(NativeImage& nativeImage, const FloatRect& destRect)
{
     drawARKitBadgeNativeImage(nativeImage, destRect);
}

// ARKitBadgeSystemImage.mm
void drawARKitBadgeNativeImage(NativeImage& nativeImage, const FloatRect& destRect)
{
    // the new code that you added in this file
}

// Alternatively you can implement GraphicsContextCG::drawARKitBadgeImage() in the new file ARKitBadgeSystemImage.mm.

// DisplayListReccorder.h
class Recorder : public GraphicsContext {
protected:
    virtual void recordDrawARKitBadgeImage(RenderingResourceIdentifier imageIdentifier, const FloatRect& destRect) = 0;

private:
    WEBCORE_EXPORT void drawARKitBadgeNativeImage(NativeImage&, const FloatRect& destRect) final;
}

// DisplayListReccorder.cpp
void Recorder:: drawARKitBadgeNativeImage(NativeImage& nativeImage, const FloatRect& destRect)
{
    appendStateChangeItemIfNecessary();
    recordResourceUse(image);
    recordDrawARKitBadgeImage(nativeImage.renderingResourceIdentifier(), destRect);
}

// DisplayListReccorderImp.h
class RecorderImpl : public Recorder {
private:
    void recordDrawARKitBadgeImage(RenderingResourceIdentifier imageIdentifier, const FloatRect& destRect) final;
}

// DisplayListReccorderImp.cpp
void RecorderImpl:: recordDrawARKitBadgeImage(RenderingResourceIdentifier imageIdentifier, const FloatRect& destRect)
{
    append<DrawARKitBadgeImage>(imageIdentifier, destRect);
}

// RemoteDisplayListRecorderProxy.h
class RemoteDisplayListRecorderProxy : public WebCore::DisplayList::Recorder {
private:
    void recordDrawARKitBadgeImage(WebCore::RenderingResourceIdentifier imageIdentifier, const WebCore::FloatRect& destRec) final;
};

// RemoteDisplayListRecorderProxy.cpp
void RemoteDisplayListRecorderProxy:: recordDrawARKitBadgeImage(RenderingResourceIdentifier imageIdentifier, const FloatRect& destRect)
{
    send(Messages::RemoteDisplayListRecorder::DrawARKitBadgeImage(imageIdentifier, destRect));
}

// RemoteDisplayListRecorder.messages.in
messages -> RemoteDisplayListRecorder NotRefCounted Stream {
    DrawARKitBadgeImage(WebCore::RenderingResourceIdentifier imageIdentifier, WebCore::FloatRect destRect)
}

// RemoteDisplayListRecorder.cpp

void RemoteDisplayListRecorder::drawARKitBadgeImage(RenderingResourceIdentifier imageIdentifier, const FloatRect& destRect)
{
    drawNativeImageWithQualifiedIdentifier({ imageIdentifier, m_webProcessIdentifier }, destRect);
}

void RemoteDisplayListRecorder:: drawARKitBadgeImage WithQualifiedIdentifier(QualifiedRenderingResourceIdentifier imageIdentifier, const FloatRect& destRect)
{
    RefPtr image = resourceCache().cachedNativeImage(imageIdentifier);
    if (!image) {
        ASSERT_NOT_REACHED();
        return;
    }

    handleItem(DisplayList::DrawARKitBadgeImage(imageIdentifier.object(), destRect), *image);
}

// DisplayListItems.h
class DrawARKitBadgeImage {
public:
    static constexpr ItemType itemType = ItemType::DrawARKitBadgeImage;
    static constexpr bool isInlineItem = true;
    static constexpr bool isDrawingItem = true;

    DrawARKitBadgeImage(RenderingResourceIdentifier imageIdentifier, const FloatRect& destRect)
        : m_imageIdentifier(imageIdentifier)
        , m_destinationRect(destRect)
    {
    }

    RenderingResourceIdentifier imageIdentifier() const { return m_imageIdentifier; }
    const FloatRect& destinationRect() const { return m_destinationRect; }

    NO_RETURN_DUE_TO_ASSERT void apply(GraphicsContext&) const;
    WEBCORE_EXPORT void apply(GraphicsContext&, NativeImage&) const;

    std::optional<FloatRect> globalBounds() const { return std::nullopt; }
    std::optional<FloatRect> localBounds(const GraphicsContext&) const { return m_destinationRect; }

private:
    RenderingResourceIdentifier m_imageIdentifier;
    FloatRect m_destinationRect;
};

NO_RETURN_DUE_TO_ASSERT void DrawARKitBadgeImage::apply(GraphicsContext&) const
{
    ASSERT_NOT_REACHED();
}

void DrawARKitBadgeImage::apply(GraphicsContext& context, NativeImage& image) const
{
    context.drawARKitBadgeImage(image, m_destinationRect);
}

[Tim Horton]

(In reply to Said Abou-Hallawa from comment #5)
> Comment on attachment 455572 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=455572&action=review
> 
> > Source/WebCore/Modules/system-preview/ARKitBadgeSystemImage.h:41
> > +class WEBCORE_EXPORT ARKitBadgeSystemImage final : public SystemImage {
> 
> I am not sure why ARKitBadgeSystemImage should be a super calls of
> SystemImage or even a superclass of Image. I think all we need to do is to
> route the drawing of the Image as an ARKitBadgeImage through
> GraphicsContext. Just follow the pattern of drawNativeImage() or
> drawPattern():

Said, the whole point of SystemImage is to *avoid* having to add things to GraphicsContext (which is in the deep "platform" layer in the WebKit lasagna and shouldn't know about high-level things like this) that know about "system preview" or Apple Pay or etc.; see the conversation in the bug that introduced it.

[Said Abou-Hallawa]

Comment on attachment 455572 [details]
Patch

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

>>> Source/WebCore/Modules/system-preview/ARKitBadgeSystemImage.h:41
>>> +class WEBCORE_EXPORT ARKitBadgeSystemImage final : public SystemImage {
>> 
>> I am not sure why ARKitBadgeSystemImage should be a super calls of SystemImage or even a superclass of Image. I think all we need to do is to route the drawing of the Image as an ARKitBadgeImage through GraphicsContext. Just follow the pattern of drawNativeImage() or drawPattern():
>> 
>> // RenderThemeIOS.mm
>> void RenderThemeIOS::paintSystemPreviewBadge(Image& image, const PaintInfo& paintInfo, const FloatRect& rect)
>> {
>>     if (auto nativeImage = image.nativeImage())
>>         paintInfo.context().drawARKitBadgeImage(*nativeImage, rect);
>> }
>> 
>> // GraphicsContext.h
>> class GraphicsContext {
>> public:
>>     virtual void drawARKitBadgeImage(NativeImage&, const FloatRect&) const { }
>> }
>> 
>> // GraphicsContextCG.h
>> class GraphicsContextCG : public GraphicsContext {
>> private:
>>     void drawARKitBadgeImage(NativeImage&, const FloatRect& destRect) const final;
>> }
>> 
>> // GraphicsContextCG.cpp
>> void GraphicsContextCG::drawARKitBadgeImage(NativeImage& nativeImage, const FloatRect& destRect)
>> {
>>      drawARKitBadgeNativeImage(nativeImage, destRect);
>> }
>> 
>> // ARKitBadgeSystemImage.mm
>> void drawARKitBadgeNativeImage(NativeImage& nativeImage, const FloatRect& destRect)
>> {
>>     // the new code that you added in this file
>> }
>> 
>> // Alternatively you can implement GraphicsContextCG::drawARKitBadgeImage() in the new file ARKitBadgeSystemImage.mm.
>> 
>> // DisplayListReccorder.h
>> class Recorder : public GraphicsContext {
>> protected:
>>     virtual void recordDrawARKitBadgeImage(RenderingResourceIdentifier imageIdentifier, const FloatRect& destRect) = 0;
>> 
>> private:
>>     WEBCORE_EXPORT void drawARKitBadgeNativeImage(NativeImage&, const FloatRect& destRect) final;
>> }
>> 
>> // DisplayListReccorder.cpp
>> void Recorder:: drawARKitBadgeNativeImage(NativeImage& nativeImage, const FloatRect& destRect)
>> {
>>     appendStateChangeItemIfNecessary();
>>     recordResourceUse(image);
>>     recordDrawARKitBadgeImage(nativeImage.renderingResourceIdentifier(), destRect);
>> }
>> 
>> // DisplayListReccorderImp.h
>> class RecorderImpl : public Recorder {
>> private:
>>     void recordDrawARKitBadgeImage(RenderingResourceIdentifier imageIdentifier, const FloatRect& destRect) final;
>> }
>> 
>> // DisplayListReccorderImp.cpp
>> void RecorderImpl:: recordDrawARKitBadgeImage(RenderingResourceIdentifier imageIdentifier, const FloatRect& destRect)
>> {
>>     append<DrawARKitBadgeImage>(imageIdentifier, destRect);
>> }
>> 
>> // RemoteDisplayListRecorderProxy.h
>> class RemoteDisplayListRecorderProxy : public WebCore::DisplayList::Recorder {
>> private:
>>     void recordDrawARKitBadgeImage(WebCore::RenderingResourceIdentifier imageIdentifier, const WebCore::FloatRect& destRec) final;
>> };
>> 
>> // RemoteDisplayListRecorderProxy.cpp
>> void RemoteDisplayListRecorderProxy:: recordDrawARKitBadgeImage(RenderingResourceIdentifier imageIdentifier, const FloatRect& destRect)
>> {
>>     send(Messages::RemoteDisplayListRecorder::DrawARKitBadgeImage(imageIdentifier, destRect));
>> }
>> 
>> // RemoteDisplayListRecorder.messages.in
>> messages -> RemoteDisplayListRecorder NotRefCounted Stream {
>>     DrawARKitBadgeImage(WebCore::RenderingResourceIdentifier imageIdentifier, WebCore::FloatRect destRect)
>> }
>> 
>> // RemoteDisplayListRecorder.cpp
>> 
>> void RemoteDisplayListRecorder::drawARKitBadgeImage(RenderingResourceIdentifier imageIdentifier, const FloatRect& destRect)
>> {
>>     drawNativeImageWithQualifiedIdentifier({ imageIdentifier, m_webProcessIdentifier }, destRect);
>> }
>> 
>> void RemoteDisplayListRecorder:: drawARKitBadgeImage WithQualifiedIdentifier(QualifiedRenderingResourceIdentifier imageIdentifier, const FloatRect& destRect)
>> {
>>     RefPtr image = resourceCache().cachedNativeImage(imageIdentifier);
>>     if (!image) {
>>         ASSERT_NOT_REACHED();
>>         return;
>>     }
>> 
>>     handleItem(DisplayList::DrawARKitBadgeImage(imageIdentifier.object(), destRect), *image);
>> }
>> 
>> // DisplayListItems.h
>> class DrawARKitBadgeImage {
>> public:
>>     static constexpr ItemType itemType = ItemType::DrawARKitBadgeImage;
>>     static constexpr bool isInlineItem = true;
>>     static constexpr bool isDrawingItem = true;
>> 
>>     DrawARKitBadgeImage(RenderingResourceIdentifier imageIdentifier, const FloatRect& destRect)
>>         : m_imageIdentifier(imageIdentifier)
>>         , m_destinationRect(destRect)
>>     {
>>     }
>> 
>>     RenderingResourceIdentifier imageIdentifier() const { return m_imageIdentifier; }
>>     const FloatRect& destinationRect() const { return m_destinationRect; }
>> 
>>     NO_RETURN_DUE_TO_ASSERT void apply(GraphicsContext&) const;
>>     WEBCORE_EXPORT void apply(GraphicsContext&, NativeImage&) const;
>> 
>>     std::optional<FloatRect> globalBounds() const { return std::nullopt; }
>>     std::optional<FloatRect> localBounds(const GraphicsContext&) const { return m_destinationRect; }
>> 
>> private:
>>     RenderingResourceIdentifier m_imageIdentifier;
>>     FloatRect m_destinationRect;
>> };
>> 
>> NO_RETURN_DUE_TO_ASSERT void DrawARKitBadgeImage::apply(GraphicsContext&) const
>> {
>>     ASSERT_NOT_REACHED();
>> }
>> 
>> void DrawARKitBadgeImage::apply(GraphicsContext& context, NativeImage& image) const
>> {
>>     context.drawARKitBadgeImage(image, m_destinationRect);
>> }
> 
> Said, the whole point of SystemImage is to *avoid* having to add things to GraphicsContext (which is in the deep "platform" layer in the WebKit lasagna and shouldn't know about high-level things like this) that know about "system preview" or Apple Pay or etc.; see the conversation in the bug that introduced it.

GraphicsContext already has different ways/modes to display the Image, the NativeImage and the ImageBuffer:

Image: (this can be BitmapImage, SVGImage, PDFDocumentImage, ...etc)
drawImage(Image&,...)
drawTiledImage(Image&, ...)
drawPattern(Image&,...)

NativeImage:
drawNativeImage(NativeImage&,...)
drawPattern(NativeImage&, ...)

ImageBuffer:
drawImageBuffer(ImageBuffer&,...)
drawConsumingImageBuffer(RefPtr<ImageBuffer>,...)
drawFilteredImageBuffer(ImageBuffer*,...)

We recently added the SystemImage which seems reasonable since all it takes is a SystemImageType. But ARKitBadgeSystemImage is just a wrapper around an Image. My suggestion above is to add a new way/mode to draw the Image or the NativeImage.

[Antoine Quint]

Created attachment 455621 [details]
Patch for landing

[Antoine Quint]

Created attachment 455625 [details]
[fast-cq] Patch for landing

[EWS]

Committed r291817 (248844@main): <https://commits.webkit.org/248844@main>

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

[Brent Fulgham]

*** Bug 232877 has been marked as a duplicate of this bug. ***