Bug 157249 – Deduplicated initializer lists of BitmapImage constructors.

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

[Konstantin Tokarev]

BitmapImage has 3 constructors, which initialize the same variables. Because of this duplication it's easy to leave some field uninitialized in some code path. Actually, there are already which are already left initialized in 1 or 2 of constructors, e.g. m_desiredFrameStartTime, m_decodedPropertiesSize, m_progressiveLoadChunkTime, m_progressiveLoadChunkCount, m_hasUniformFrameSize.

Use constructor delegation and inline filed initializers to avoid this problem.

[Konstantin Tokarev]

Created attachment 277869 [details]
Patch

[Darin Adler]

Comment on attachment 277869 [details]
Patch

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

> Source/WebCore/platform/graphics/BitmapImage.cpp:48
> +BitmapImage::BitmapImage(ImageObserver* observer, bool fromNativeImage)

This approach of taking a boolean is a little bit inelegant, violates our coding style rule of not using literal "true" for a mysterious meaning that requires reading the code to find out what it means, and also adds a few branches to the constructors, but I think it’s OK.

> Source/WebCore/platform/graphics/BitmapImage.h:302
> +    int m_repetitionCount { cAnimationNone }; // How many total animation loops we should do.

Not sure why we removed the comment explaining the meaning of cAnimationNone. Might be better to move it somewhere rather than removing it entirely.

[Konstantin Tokarev]

Created attachment 278074 [details]
Patch

[Konstantin Tokarev]

(In reply to comment #2)
> Comment on attachment 277869 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=277869&action=review
> 
> > Source/WebCore/platform/graphics/BitmapImage.cpp:48
> > +BitmapImage::BitmapImage(ImageObserver* observer, bool fromNativeImage)
> 
> This approach of taking a boolean is a little bit inelegant, violates our
> coding style rule of not using literal "true" for a mysterious meaning that
> requires reading the code to find out what it means, and also adds a few
> branches to the constructors, but I think it’s OK.

I've got rid of branch in constructor by using template + std::true_type/false_type. Now it should compile to the same code as before, though I understand that it's inelegant.

> 
> > Source/WebCore/platform/graphics/BitmapImage.h:302
> > +    int m_repetitionCount { cAnimationNone }; // How many total animation loops we should do.
> 
> Not sure why we removed the comment explaining the meaning of
> cAnimationNone. Might be better to move it somewhere rather than removing it
> entirely.

I thought it's not so important after we have initializer. Returned it back now.

[Darin Adler]

Comment on attachment 278074 [details]
Patch

OK

[WebKit Commit Bot]

Comment on attachment 278074 [details]
Patch

Clearing flags on attachment: 278074

Committed r200417: <http://trac.webkit.org/changeset/200417>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Michael Catanzaro]

(In reply to comment #4)
> I've got rid of branch in constructor by using template +
> std::true_type/false_type. Now it should compile to the same code as before,
> though I understand that it's inelegant.

Inelegant indeed... I think we are being too clever here. std::true_type and std::false_type are obtuse and still violate the spirit of the rule against using boolean parameters. I'm also skeptical that there will actually be any significant performance benefit to using templates instead. (Remember that increased code size is likely to hurt performance.) I think this code would be better with boolean parameters, and best with an simple two-value enum class.

[Konstantin Tokarev]

Template here just generates two separate initializer lists for "true" and "false" cases, so no additional bloat.

[Konstantin Tokarev]

If you like, I can change it back to non-inline function + branch with enum argument, or keep template and replace true_type and false_type with tag classes like

BitmapImage(observer, FromNative())

[Said Abou-Hallawa]

Another cleaner way is the following:

1. Convert all the bit-field boolean members in the BitImage class to be non bit-field so you can initialize them in the class header like that:
   << bool m_isSolidColor : 1;
   >> bool m_isSolidColor { false };

2. Move the platform-dependent constructor BitmapImage::BitmapImage() from BitmapImageCairo.cpp and BitmapImageCG.cpp to BitmapImage.cpp and have only one constructor written like that

   BitmapImage(NativeImagePtr&& image, ImageObserver* observer)
        : Image(observer)
        , m_frameCount(1)
        , m_animationFinished(true)
        ...

3. Add two new static methods in FrameData which return the size and the hasAlpha of a NativeImagePtr. And call these functions from the constructor like that:

    BitmapImage::BitmapImage(NativeImagePtr&& image, ImageObserver* observer)
        : mage(observer)
        , m_frameCount(1)
        , m_animationFinished(true)
        ...
    {
        m_size = FrameData::size(image);
        m_decodedSize = size.area() * 4;

        m_frames.grow(1);
        m_frames[0].m_haveMetadata = true;
        m_frames[0].m_hasAlpha = FrameData::hasAlpha(image;
        m_frames[0].m_image = WTFMove(image);

        checkForSolidColor();
    }

5. Have different implementations for these function in BitmapImageCairo.cpp and BitmapImageCG.cpp.

e.g. in BitmapImageCG.cpp:
    FrameData::hasAlpha(NativeImagePtr&)
    {
       return true;
    }

    FrameData::size(NativeImagePtr& image)
    {
       return IntSize(CGImageGetWidth(image.get()), CGImageGetHeight(image.get()))
    }