[webkit-reviews] review granted: [Bug 226917] AcceleratedImageBuffer not instantiated but objects are punned to the type : [Attachment 431192] Patch

[bugzilla-daemon at webkit.org]

Said Abou-Hallawa <sabouhallawa at apple.com> has granted Kimmo Kinnunen
<kkinnunen at apple.com>'s request for review:
Bug 226917: AcceleratedImageBuffer not instantiated but objects are punned to
the type
https://bugs.webkit.org/show_bug.cgi?id=226917

Attachment 431192: Patch

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




--- Comment #6 from Said Abou-Hallawa <sabouhallawa at apple.com> ---
Comment on attachment 431192
  --> https://bugs.webkit.org/attachment.cgi?id=431192
Patch

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

> Source/WebCore/platform/graphics/PlatformImageBuffer.h:45
> +namespace Detail {

I am not sure I agree with this change especially there are similar definitions
in PlatformImageBufferShareableBackend.h. Can we move this change to a operate
patch since it is unrelated to what you are trying to fix?

> Source/WebCore/platform/graphics/PlatformImageBuffer.h:82
> +    IOSurfaceImageBuffer(const WebCore::ImageBufferBackend::Parameters&
parameters, std::unique_ptr<ImageBufferIOSurfaceBackend>&& backend)
> +	   : Base(parameters, WTFMove(backend))
> +    {
> +    }

This constructor can be replaced by:

    using Base::Base;

> Source/WebCore/platform/graphics/cg/ImageBufferCGBitmapBackend.h:40
> +    WEBCORE_EXPORT static IntSize calculateSafeBackendSize(const
Parameters&);
> +    WEBCORE_EXPORT static size_t calculateMemoryCost(const Parameters&);

Why do we need to export these functions? I guess this is because the new api
test calls ConcreteImageBuffer::create()?

>
Source/WebCore/platform/graphics/coreimage/FilterEffectRendererCoreImage.mm:280
> +    auto outputImageBuffer = IOSurfaceImageBuffer::create(clampedSize,
lastEffect.filter().filterScale(), lastEffect.resultColorSpace(),
PixelFormat::BGRA8);

What is the point of using this local variable and then moving it to the member
m_outputImageBuffer?

Is this because m_outputImageBuffer defined to be RefPtr<ImageBuffer> and you
want to avoid casting?
     RefPtr<ImageBuffer> m_outputImageBuffer;

I think it is better to change it to be RefPtr<AcceleratedImageBuffer> or even
better RefPtr<IOSurfaceImageBuffer>.