Bug 155444 – Rename FrameData to ImageFrameData and move it to a separate file

• Status: RESOLVED
• Resolution: DUPLICATE
• Component: Images

[Said Abou-Hallawa]

This is work towards CG asynchronous image decoding: https://bugs.webkit.org/show_bug.cgi?id=155322. The plan to move the caching of the ImageFrameData from BitmapImage to ImageSource.

[Said Abou-Hallawa]

Created attachment 273988 [details]
Patch

[Said Abou-Hallawa]

Created attachment 273990 [details]
Patch

[Said Abou-Hallawa]

Created attachment 273992 [details]
Patch

[Said Abou-Hallawa]

Created attachment 273995 [details]
Patch for review

[Brent Fulgham]

Comment on attachment 273995 [details]
Patch for review

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

Could you please rebase this patch so we can confirm it builds and does not regress any tests? For that reason, marking r-.

> Source/WebCore/PlatformEfl.cmake:126
> +    platform/graphics/ImageFrameData.cpp

Since this is in CMakeList.txt, it is not needed in the individual Platform{Efl/Win/WinCairo/Gtk/etc.} files.

There actually seems to be a fair amount of unneeded duplication in these files. I'll try to get that cleaned up soon.

> Source/WebCore/PlatformGTK.cmake:97
> +    platform/graphics/ImageFrameData.cpp

Ditto.

> Source/WebCore/PlatformWinCairo.cmake:42
> +    platform/graphics/ImageFrameData.cpp

Ditto.

> Source/WebCore/platform/graphics/BitmapImage.h:-89
> -

Nice to have this removed!

[Said Abou-Hallawa]

Created attachment 276545 [details]
Patch

[Said Abou-Hallawa]

Created attachment 276549 [details]
Patch

[Said Abou-Hallawa]

Created attachment 276550 [details]
Patch

[Said Abou-Hallawa]

Created attachment 276551 [details]
Patch

[Said Abou-Hallawa]

Created attachment 276621 [details]
Patch

[Said Abou-Hallawa]

Comment on attachment 273995 [details]
Patch for review

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

>> Source/WebCore/PlatformEfl.cmake:126
>> +    platform/graphics/ImageFrameData.cpp
> 
> Since this is in CMakeList.txt, it is not needed in the individual Platform{Efl/Win/WinCairo/Gtk/etc.} files.
> 
> There actually seems to be a fair amount of unneeded duplication in these files. I'll try to get that cleaned up soon.

This file was removed but the platform specific cpp file: ImageFrameDataCairo.cpp was added instead.

>> Source/WebCore/PlatformGTK.cmake:97
>> +    platform/graphics/ImageFrameData.cpp
> 
> Ditto.

This file was removed but the platform specific cpp file: ImageFrameDataCairo.cpp was added instead.

>> Source/WebCore/PlatformWinCairo.cmake:42
>> +    platform/graphics/ImageFrameData.cpp
> 
> Ditto.

This file was removed but the platform specific cpp file: ImageFrameDataCairo.cpp was added instead.

[Said Abou-Hallawa]

Created attachment 280332 [details]
Patch

[Said Abou-Hallawa]

Created attachment 280334 [details]
Patch

[Darin Adler]

Comment on attachment 280334 [details]
Patch

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

Since this is all about refactoring, I was a little more picky with my comments than I would have been otherwise.

I’m going to say review- because I see some things that should be fixed.

> Source/WebCore/platform/graphics/BitmapImage.cpp:72
> +    m_frames.grow(1);
> +    m_frames[0] = ImageFrameData(image);

Really surprised that this patch removes the optimization of doing a WTFMove on the image passed in. That needs to be restored.

This should be:

    m_frames.reserveInitialCapacity(1);
    m_frames.uncheckedAppend(WTFMove(image));

Or alternatively could initialize the vector with a size of 1 and then assign the single element to it. There is no reason to call the grow function. But it’s nice to avoid constructing the empty ImageFrameData just to overwrite it a moment later, which is why I like uncheckedAppend better.

Even better would be adding a constructor to Vector that lets us make one containing a single object with move semantics. Right now we don’t have that constructor, though. A lot that are close, but not quite.

> Source/WebCore/platform/graphics/BitmapImage.h:53
> +template<> struct VectorTraits<WebCore::ImageFrameData> : public SimpleClassVectorTraits {
> +    static const bool canInitializeWithMemset = false; // Not all ImageFrameData members initialize to 0.
> +};

Seems like this belongs in the ImageFrameData header, not the BitmapImage header. It’s a mistake to have this somewhere not right next to the class it's based on.

No need for the "public" here. I also think we should fix this so that all the members initialize to zero.

> Source/WebCore/platform/graphics/ImageFrameData.h:27
> +#ifndef ImageFrameData_h
> +#define ImageFrameData_h

New files should use #pragma once, not the old style guards.

> Source/WebCore/platform/graphics/ImageFrameData.h:29
> +#include "Color.h"

We don’t need to include "Color.h" just to declare functions that return a Color. It can be forward-declared instead.

> Source/WebCore/platform/graphics/ImageFrameData.h:37
> +typedef short SubsamplingLevel;

Please use "using" instead of typedef in new code.

> Source/WebCore/platform/graphics/ImageFrameData.h:41
> +const SubsamplingLevel MinSubsamplingLevel = 0;
> +const SubsamplingLevel MaxSubsamplingLevel = 3;
> +const SubsamplingLevel DefaultSubsamplingLevel = MinSubsamplingLevel;

Should this be an enum instead of an integer? Also, why did we chose the type "short" for a value in the range [0..3]? Seems like it could equally well be int or uint8_t.

> Source/WebCore/platform/graphics/ImageFrameData.h:54
> +enum ImageFrameCaching {
> +    CacheNone,
> +    CacheMetadata,
> +    CacheImage,
> +    CacheMetadataAndImage
> +};
> +
> +enum ImageFrameStatus {
> +    FrameEmpty,
> +    FramePartial,
> +    FrameComplete
> +};

Seems like these should have shorter names, be members of ImageFrameData, and use enum class so the names don’t have to be so repetitive.

> Source/WebCore/platform/graphics/ImageFrameData.h:61
> +namespace NativeImage {
> +IntSize size(const NativeImagePtr&);
> +bool hasAlpha(const NativeImagePtr&);
> +Color singlePixelSolidColor(const NativeImagePtr&);
> +void clear(const NativeImagePtr&);
> +}

I know this is not new, but it seems really strange to group these in a namespace and also to have them here instead of in NativeImagePtr.h. We should turn the NativeImagePtr typedef into a PlatformImage class.

> Source/WebCore/platform/graphics/ImageFrameData.h:63
> +class ImageDecoder;

Forward declarations should go *before* things that a re actually defined in this file.

> Source/WebCore/platform/graphics/ImageFrameData.h:65
> +class ImageFrameData {

Why have "Data" in the name of this class? Why not just ImageFrame?

Is it really significantly better to have this be a class rather than a struct? I guess maybe, but it does mean there are  a lot of getter functions to write.

> Source/WebCore/platform/graphics/ImageFrameData.h:67
> +    ImageFrameData();

Seems quite unfortunate that we have to have this fairly expensive non-inlined default constructor.

> Source/WebCore/platform/graphics/ImageFrameData.h:68
> +    ImageFrameData(const NativeImagePtr&);

Shouldn’t this take NativeImagePtr&& instead? This takes ownership.

> Source/WebCore/platform/graphics/cairo/BitmapImageCairo.cpp:67
> +void clear(const RefPtr<cairo_surface_t>&)
> +{
> +}

Why is it OK to have an empty implementation here? It seems like silently failing to clear would be a problem. Maybe calls to clear should all be inside #if CACHE_SUBIMAGES? I don’t think "clear" expresses the purpose of this function well enough.

[Said Abou-Hallawa]

Created attachment 281796 [details]
Patch

[WebKit Commit Bot]

Attachment 281796 [details] did not pass style-queue:


ERROR: Source/WebCore/platform/graphics/imageFrameData.cpp:27:  Found other header before a header this file implements. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
ERROR: Source/WebCore/platform/graphics/imageFrameData.cpp:28:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 2 in 31 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Said Abou-Hallawa]

Created attachment 282736 [details]
Patch

[Said Abou-Hallawa]

Created attachment 282737 [details]
Patch

[Said Abou-Hallawa]

Created attachment 282738 [details]
Patch

[Said Abou-Hallawa]

Comment on attachment 280334 [details]
Patch

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

>> Source/WebCore/platform/graphics/BitmapImage.cpp:72
>> +    m_frames[0] = ImageFrameData(image);
> 
> Really surprised that this patch removes the optimization of doing a WTFMove on the image passed in. That needs to be restored.
> 
> This should be:
> 
>     m_frames.reserveInitialCapacity(1);
>     m_frames.uncheckedAppend(WTFMove(image));
> 
> Or alternatively could initialize the vector with a size of 1 and then assign the single element to it. There is no reason to call the grow function. But it’s nice to avoid constructing the empty ImageFrameData just to overwrite it a moment later, which is why I like uncheckedAppend better.
> 
> Even better would be adding a constructor to Vector that lets us make one containing a single object with move semantics. Right now we don’t have that constructor, though. A lot that are close, but not quite.

Fixed as suggested.

>> Source/WebCore/platform/graphics/BitmapImage.h:53
>> +};
> 
> Seems like this belongs in the ImageFrameData header, not the BitmapImage header. It’s a mistake to have this somewhere not right next to the class it's based on.
> 
> No need for the "public" here. I also think we should fix this so that all the members initialize to zero.

This initialization was removed because m_hasAlpha can be initialized with false since it is guarded by m_hasMetadata.

>> Source/WebCore/platform/graphics/ImageFrameData.h:27
>> +#define ImageFrameData_h
> 
> New files should use #pragma once, not the old style guards.

Fixed.

>> Source/WebCore/platform/graphics/ImageFrameData.h:29
>> +#include "Color.h"
> 
> We don’t need to include "Color.h" just to declare functions that return a Color. It can be forward-declared instead.

The reference to Color.h was removed from this header file.

>> Source/WebCore/platform/graphics/ImageFrameData.h:37
>> +typedef short SubsamplingLevel;
> 
> Please use "using" instead of typedef in new code.

Fixed.

>> Source/WebCore/platform/graphics/ImageFrameData.h:41
>> +const SubsamplingLevel DefaultSubsamplingLevel = MinSubsamplingLevel;
> 
> Should this be an enum instead of an integer? Also, why did we chose the type "short" for a value in the range [0..3]? Seems like it could equally well be int or uint8_t.

SubsamplingLevel was defined to be uint8_t and the special values were defined in an enum.

>> Source/WebCore/platform/graphics/ImageFrameData.h:54
>> +};
> 
> Seems like these should have shorter names, be members of ImageFrameData, and use enum class so the names don’t have to be so repetitive.

These enum were moved inside ImageFrameData and were defined enum classes.

>> Source/WebCore/platform/graphics/ImageFrameData.h:61
>> +}
> 
> I know this is not new, but it seems really strange to group these in a namespace and also to have them here instead of in NativeImagePtr.h. We should turn the NativeImagePtr typedef into a PlatformImage class.

The PlatformImage was introduced in https://bugs.webkit.org/show_bug.cgi?id=158684.

>> Source/WebCore/platform/graphics/ImageFrameData.h:63
>> +class ImageDecoder;
> 
> Forward declarations should go *before* things that a re actually defined in this file.

Fixed.

>> Source/WebCore/platform/graphics/ImageFrameData.h:65
>> +class ImageFrameData {
> 
> Why have "Data" in the name of this class? Why not just ImageFrame?
> 
> Is it really significantly better to have this be a class rather than a struct? I guess maybe, but it does mean there are  a lot of getter functions to write.

The class ImageFrame is defined in platform/image-decoders/ImageDecoder.h. It is not complied for CG but this class is compiled for all platforms. I have a plan to merge both since for non CG platforms, the ImageFrame is cached twice; one with the ImageDecoder and the second in the BitmapImage. When they are merged this class' name can be changed to ImageFrame.

>> Source/WebCore/platform/graphics/ImageFrameData.h:68
>> +    ImageFrameData(const NativeImagePtr&);
> 
> Shouldn’t this take NativeImagePtr&& instead? This takes ownership.

Done.

>> Source/WebCore/platform/graphics/cairo/BitmapImageCairo.cpp:67
>> +}
> 
> Why is it OK to have an empty implementation here? It seems like silently failing to clear would be a problem. Maybe calls to clear should all be inside #if CACHE_SUBIMAGES? I don’t think "clear" expresses the purpose of this function well enough.

The function name was changed to be clearSubImages() and it is now a member of PlatformImage class.

[Said Abou-Hallawa]

Created attachment 282741 [details]
Patch for review

[Said Abou-Hallawa]

This now is covered by https://bugs.webkit.org/show_bug.cgi?id=159819.

[Said Abou-Hallawa]

*** This bug has been marked as a duplicate of bug 159819 ***

[Brady Eidson]

Comment on attachment 282741 [details]
Patch for review

r-, as this has been pending review for over a year now. It is near-impossible that this patch still applies to trunk and unlikely to still be relevant in its current form.

[Said Abou-Hallawa]

Comment on attachment 282741 [details]
Patch for review

This bug was marked as s duplicate. I was not aware that duplicate bugs can also appear as requesting-review bugs. I think removing the flags is more appropriate in this case.