[webkit-reviews] review denied: [Bug 169771] REGRESSION(213764): Large images should not be decoded asynchronously when they are drawn on a canvas : [Attachment 305438] Patch

[bugzilla-daemon at webkit.org]

Simon Fraser (smfr) <simon.fraser at apple.com> has denied Said Abou-Hallawa
<sabouhallawa at apple.com>'s request for review:
Bug 169771: REGRESSION(213764): Large images should not be decoded
asynchronously when they are drawn on a canvas
https://bugs.webkit.org/show_bug.cgi?id=169771

Attachment 305438: Patch

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




--- Comment #26 from Simon Fraser (smfr) <simon.fraser at apple.com> ---
Comment on attachment 305438
  --> https://bugs.webkit.org/attachment.cgi?id=305438
Patch

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

Looks OK but let's do another rev with the new names.

> Source/WebCore/platform/graphics/BitmapImage.cpp:168
> +    std::optional<IntSize> sizeForDrawing =
enclosingIntRect(context.getCTM().mapRect(destRect)).size();

This does not need to be an optional<> here.

> Source/WebCore/platform/graphics/BitmapImage.h:85
> +    bool frameIsBeingDecodedCompatibleWithOptionsAtIndex(size_t index, const
DecodingOptions& decodingOptions) const { return
m_source.frameIsBeingDecodedCompatibleWithOptionsAtIndex(index,
decodingOptions); }

frameIsBeingDecodedCompatibleWithOptionsAtIndex ->
frameBeingDecodedIsCompatibleWithOptions (I don' think the "at index" is
necessary).

What does this return if no frame is being decoded?

> Source/WebCore/platform/graphics/BitmapImage.h:87
>      bool frameIsCompleteAtIndex(size_t index) const { return
m_source.frameIsCompleteAtIndex(index); }
>      bool frameHasAlphaAtIndex(size_t index) const { return
m_source.frameHasAlphaAtIndex(index); }

I think we could remove the "at index" from these too, later. "Frame" is
implicitly something that has an index.

> Source/WebCore/platform/graphics/BitmapImage.h:89
> +    bool frameHasIntrinsicNativeImageAtIndex(size_t index, SubsamplingLevel
subsamplingLevel) { return m_source.frameHasIntrinsicNativeImageAtIndex(index,
subsamplingLevel); }

hasDecodedFrameWithFullSizeNativeImage()

> Source/WebCore/platform/graphics/BitmapImage.h:90
> +    bool frameHasAsyncNativeImageAtIndex(size_t index, const
std::optional<SubsamplingLevel>& subsamplingLevel, const DecodingOptions&
decodingOptions) { return m_source.frameHasAsyncNativeImageAtIndex(index,
subsamplingLevel, decodingOptions); }

hasDecodedFrameCompatibleWithOptions()

> Source/WebCore/platform/graphics/GradientImage.h:47
> +    void draw(GraphicsContext&, const FloatRect& dstRect, const FloatRect&
srcRect, CompositeOperator, BlendMode, const DecodingOptions&,
ImageOrientationDescription) final;

Maybe just pass DecodingOptions::DecodingMode or DecodingMode

> Source/WebCore/platform/graphics/Image.h:188
> +    virtual void draw(GraphicsContext&, const FloatRect& dstRect, const
FloatRect& srcRect, CompositeOperator, BlendMode, const DecodingOptions&,
ImageOrientationDescription) = 0;

Maybe just pass DecodingOptions::DecodingMode or DecodingMode

> Source/WebCore/platform/graphics/ImageFrame.h:130
> +    bool hasIntrinsicNativeImage(const std::optional<SubsamplingLevel>& = {
}) const;

hasFullSizeNativeImage()

> Source/WebCore/platform/graphics/ImageFrame.h:131
> +    bool hasAsyncNativeImage(const std::optional<SubsamplingLevel>&, const
DecodingOptions&) const;

hasImageCompatibleWithOptions()

> Source/WebCore/platform/graphics/ImageFrame.h:132
> +    bool hasCompleteAsyncNativeImage(const std::optional<SubsamplingLevel>&,
const DecodingOptions&) const;

How is this different from the one above? Should this be
hasImageCompatibleWithOptions() and the previous one be
hasOrIsDecodingImageCompatibleWithOptions()?

> Source/WebCore/platform/graphics/ImageFrameCache.h:96
> +    bool frameHasIntrinsicNativeImageAtIndex(size_t, const
std::optional<SubsamplingLevel>&);

frameHasFullSizeNativeImageAtIndex

> Source/WebCore/platform/graphics/ImageSource.h:96
> -    bool frameIsBeingDecodedAtIndex(size_t index, const
std::optional<IntSize>& sizeForDrawing) { return
m_frameCache->frameIsBeingDecodedAtIndex(index, sizeForDrawing); }
> +    bool frameIsBeingDecodedCompatibleWithOptionsAtIndex(size_t index, const
DecodingOptions& decodingOptions) { return
m_frameCache->frameIsBeingDecodedCompatibleWithOptionsAtIndex(index,
decodingOptions); }
>      bool frameIsCompleteAtIndex(size_t index) { return
m_frameCache->frameIsCompleteAtIndex(index); }
>      bool frameHasAlphaAtIndex(size_t index) { return
m_frameCache->frameHasAlphaAtIndex(index); }
>      bool frameHasImageAtIndex(size_t index) { return
m_frameCache->frameHasImageAtIndex(index); }
> -    bool frameHasValidNativeImageAtIndex(size_t index, const
std::optional<SubsamplingLevel>& subsamplingLevel, const
std::optional<IntSize>& sizeForDrawing) { return
m_frameCache->frameHasValidNativeImageAtIndex(index, subsamplingLevel,
sizeForDrawing); }
> -    bool frameHasDecodedNativeImage(size_t index) { return
m_frameCache->frameHasDecodedNativeImage(index); }
> +    bool frameHasIntrinsicNativeImageAtIndex(size_t index, const
std::optional<SubsamplingLevel>& subsamplingLevel) { return
m_frameCache->frameHasIntrinsicNativeImageAtIndex(index, subsamplingLevel); }
> +    bool frameHasAsyncNativeImageAtIndex(size_t index, const
std::optional<SubsamplingLevel>& subsamplingLevel, const DecodingOptions&
decodingOptions) { return m_frameCache->frameHasAsyncNativeImageAtIndex(index,
subsamplingLevel, decodingOptions); }
>      SubsamplingLevel frameSubsamplingLevelAtIndex(size_t index) { return
m_frameCache->frameSubsamplingLevelAtIndex(index); }

Match the BitmapImage names where possible.

> Source/WebCore/platform/graphics/cg/ImageDecoderCG.cpp:375
> +    if (!decodingOptions.isSynchronous()) {

is that the same as decodingOptions.isAsynchronous()?