Bug 158684 – Move the BitmapImage platform dependent code to a new file named NativeImage[CG|Cairo].cpp

• Status: RESOLVED
• Resolution: FIXED
• Component: Images

[Said Abou-Hallawa]

This addresses one of the review comments of https://bugs.webkit.org/show_bug.cgi?id=155444.

[Said Abou-Hallawa]

Created attachment 281150 [details]
Patch

[Said Abou-Hallawa]

Created attachment 281155 [details]
Patch

[Said Abou-Hallawa]

Created attachment 281156 [details]
Patch

[Said Abou-Hallawa]

Created attachment 281157 [details]
Patch

[Said Abou-Hallawa]

Created attachment 281158 [details]
Patch

[Said Abou-Hallawa]

Created attachment 281160 [details]
Patch

[Said Abou-Hallawa]

Created attachment 288280 [details]
Patch

[Said Abou-Hallawa]

Created attachment 288283 [details]
Patch

[Said Abou-Hallawa]

Created attachment 288291 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 288291 [details]
Patch

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

> Source/WebCore/platform/graphics/BitmapImage.cpp:402
> +NativeImagePtr BitmapImage::nativeImageOfSize(const IntSize& size)
> +{
> +    size_t count = frameCount();
> +    
> +    for (size_t i = 0; i < count; ++i) {
> +        auto image = frameImageAtIndex(i);
> +        if (image && sizeOfImage(image) == size)
> +            return image;
> +    }
> +    
> +    // Fallback to the first frame image if we can't find the right size
> +    return frameImageAtIndex(0);
> +}

This was only used for CG for icons. I don't think you should generalize it for all platforms.

> Source/WebCore/platform/graphics/BitmapImage.cpp:420
> +Vector<NativeImagePtr> BitmapImage::framesNativeImages()
> +{
> +    Vector<NativeImagePtr> images;
> +    size_t count = frameCount();
> +    
> +    for (size_t i = 0; i < count; ++i) {
> +        if (auto image = frameImageAtIndex(i))
> +            images.append(image);
> +    }
> +    
> +    return images;
> +}

Ditto.

> Source/WebCore/platform/graphics/BitmapImage.cpp:475
> +    if (!haveFrameImageAtIndex(0) && m_source.frameSizeAtIndex(0, 0) != IntSize(1, 1)) {

Isn't that second 0 a subsampling level?

> Source/WebCore/platform/graphics/BitmapImage.cpp:481
> +    if (!ensureFrameIsCached(0))

What does this 0 mean? Would be nicer if the function name had "at index" in it.

> Source/WebCore/platform/graphics/BitmapImage.cpp:552
> +    if (!m_cachedImage) {

Blank line above this please.

> Source/WebCore/platform/graphics/BitmapImage.cpp:594
> +    m_frameTimer = std::make_unique<Timer>(*this, &BitmapImage::advanceAnimation);

This is odd. Normally we store Timers by value.

> Source/WebCore/platform/graphics/BitmapImage.h:62
> -        , m_hasAlpha(true)
> +        , m_hasAlpha(false)

Why? m_hasAlpha(false) is the "safe" value (if we get it wrong, we risk drawing garbage pixels).

> Source/WebCore/platform/graphics/BitmapImage.h:72
>      // Returns whether there was cached image data to clear.

Comment is no longer accurate.

> Source/WebCore/platform/graphics/BitmapImage.h:80
> +        m_subsamplingLevel = 0;

Don't we have enums for this?

> Source/WebCore/platform/graphics/NativeImage.h:59
> +IntSize sizeOfImage(const NativeImagePtr&);
> +bool hasAlphaOfImage(const NativeImagePtr&);
> +Color singlePixelSolidColorOfImage(const NativeImagePtr&);

Why the "OfImage"? Old names seem fine.

> Source/WebCore/platform/graphics/NativeImage.h:64
> +void drawImage(const NativeImagePtr&, GraphicsContext&, const FloatRect&, const FloatRect&, const IntSize&, CompositeOperator, BlendMode, const ImageOrientation&);
> +void clearImageSubImages(const NativeImagePtr&);

Maybe drawNativeImage, clearNativeImageSubImages

> Source/WebCore/platform/graphics/cg/NativeImageCG.cpp:52
> +    // FIXME: Answer correctly the question: does the CGImageRef have alpha channnel?

ImageDiff implements this as:
    CGImageAlphaInfo info = CGImageGetAlphaInfo(image);    
    return (info >= kCGImageAlphaPremultipliedLast) && (info <= kCGImageAlphaFirst);

[Said Abou-Hallawa]

Created attachment 288336 [details]
Patch

[Said Abou-Hallawa]

Created attachment 288361 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 288361 [details]
Patch

Clearing flags on attachment: 288361

Committed r205682: <http://trac.webkit.org/changeset/205682>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Said Abou-Hallawa]

Comment on attachment 288291 [details]
Patch

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

>> Source/WebCore/platform/graphics/BitmapImage.cpp:402
>> +}
> 
> This was only used for CG for icons. I don't think you should generalize it for all platforms.

Done. nativeImageOfSize() and framesNativeImages() are now declared and implemented within if USE(CG) ... #endif.

>> Source/WebCore/platform/graphics/BitmapImage.cpp:475
>> +    if (!haveFrameImageAtIndex(0) && m_source.frameSizeAtIndex(0, 0) != IntSize(1, 1)) {
> 
> Isn't that second 0 a subsampling level?

Done. the SubsamplingLevel enum is defined in ImageSource.h

>> Source/WebCore/platform/graphics/BitmapImage.cpp:481
>> +    if (!ensureFrameIsCached(0))
> 
> What does this 0 mean? Would be nicer if the function name had "at index" in it.

Done. This function is now named ensureFrameAtIndexIsCached().

>> Source/WebCore/platform/graphics/BitmapImage.cpp:552
>> +    if (!m_cachedImage) {
> 
> Blank line above this please.

Done. A new line is added.

>> Source/WebCore/platform/graphics/BitmapImage.cpp:594
>> +    m_frameTimer = std::make_unique<Timer>(*this, &BitmapImage::advanceAnimation);
> 
> This is odd. Normally we store Timers by value.

This is how it was implemented. I will clean it up in a follow-up patch.

>> Source/WebCore/platform/graphics/BitmapImage.h:62
>> +        , m_hasAlpha(false)
> 
> Why? m_hasAlpha(false) is the "safe" value (if we get it wrong, we risk drawing garbage pixels).

m_hasAlpha can be initialized with false because it is accessed only in BitmapImage::frameHasAlphaAtIndex() and it is guarded by m_haveMetadata. Both m_haveMetadata and m_hasAlpha are set in BitmapImage constructor and in BitmapImage::cacheFrame().

>> Source/WebCore/platform/graphics/BitmapImage.h:72
>>      // Returns whether there was cached image data to clear.
> 
> Comment is no longer accurate.

The comment was removed.

>> Source/WebCore/platform/graphics/BitmapImage.h:80
>> +        m_subsamplingLevel = 0;
> 
> Don't we have enums for this?

Fixed.

>> Source/WebCore/platform/graphics/NativeImage.h:59
>> +Color singlePixelSolidColorOfImage(const NativeImagePtr&);
> 
> Why the "OfImage"? Old names seem fine.

Fixed. Names were changed to:
nativeImageSize()
nativeImageHasAlpha()
nativeImageSinglePixelSolidColor()

>> Source/WebCore/platform/graphics/NativeImage.h:64
>> +void clearImageSubImages(const NativeImagePtr&);
> 
> Maybe drawNativeImage, clearNativeImageSubImages

Fixed. Names were changed as suggested.

>> Source/WebCore/platform/graphics/cg/NativeImageCG.cpp:52
>> +    // FIXME: Answer correctly the question: does the CGImageRef have alpha channnel?
> 
> ImageDiff implements this as:
>     CGImageAlphaInfo info = CGImageGetAlphaInfo(image);    
>     return (info >= kCGImageAlphaPremultipliedLast) && (info <= kCGImageAlphaFirst);

Forgot to add in the <http://trac.webkit.org/changeset/205682>. I will add it in a follow up patch.