Bug 238295

Created attachment 455570 [details] Patch

Created attachment 455572 [details] Patch

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); }

(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.

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.

Created attachment 455621 [details] Patch for landing

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

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].

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