Bug 155444 - Rename FrameData to ImageFrameData and move it to a separate file
Summary: Rename FrameData to ImageFrameData and move it to a separate file
Status: RESOLVED DUPLICATE of bug 159819
Alias: None
Product: WebKit
Classification: Unclassified
Component: Images (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Said Abou-Hallawa
URL:
Keywords:
Depends on: 155422 158684
Blocks: 155322 155456 159679
  Show dependency treegraph
 
Reported: 2016-03-14 10:36 PDT by Said Abou-Hallawa
Modified: 2017-08-21 12:16 PDT (History)
8 users (show)

See Also:


Attachments
Patch (120.44 KB, patch)
2016-03-14 10:38 PDT, Said Abou-Hallawa
no flags Details | Formatted Diff | Diff
Patch (122.69 KB, patch)
2016-03-14 10:46 PDT, Said Abou-Hallawa
no flags Details | Formatted Diff | Diff
Patch (117.50 KB, patch)
2016-03-14 11:14 PDT, Said Abou-Hallawa
no flags Details | Formatted Diff | Diff
Patch for review (38.09 KB, patch)
2016-03-14 11:49 PDT, Said Abou-Hallawa
no flags Details | Formatted Diff | Diff
Patch (51.82 KB, patch)
2016-04-15 22:19 PDT, Said Abou-Hallawa
no flags Details | Formatted Diff | Diff
Patch (52.35 KB, patch)
2016-04-15 23:04 PDT, Said Abou-Hallawa
no flags Details | Formatted Diff | Diff
Patch (52.34 KB, patch)
2016-04-15 23:16 PDT, Said Abou-Hallawa
no flags Details | Formatted Diff | Diff
Patch (52.33 KB, patch)
2016-04-15 23:25 PDT, Said Abou-Hallawa
no flags Details | Formatted Diff | Diff
Patch (55.99 KB, patch)
2016-04-17 20:27 PDT, Said Abou-Hallawa
no flags Details | Formatted Diff | Diff
Patch (36.94 KB, patch)
2016-06-02 08:51 PDT, Said Abou-Hallawa
no flags Details | Formatted Diff | Diff
Patch (37.35 KB, patch)
2016-06-02 09:04 PDT, Said Abou-Hallawa
no flags Details | Formatted Diff | Diff
Patch (95.04 KB, patch)
2016-06-21 17:28 PDT, Said Abou-Hallawa
no flags Details | Formatted Diff | Diff
Patch (95.78 KB, patch)
2016-07-04 16:48 PDT, Said Abou-Hallawa
no flags Details | Formatted Diff | Diff
Patch (95.79 KB, patch)
2016-07-04 17:12 PDT, Said Abou-Hallawa
no flags Details | Formatted Diff | Diff
Patch (95.85 KB, patch)
2016-07-04 17:43 PDT, Said Abou-Hallawa
no flags Details | Formatted Diff | Diff
Patch for review (31.93 KB, patch)
2016-07-04 20:20 PDT, Said Abou-Hallawa
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Said Abou-Hallawa 2016-03-14 10:36:17 PDT
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.
Comment 1 Said Abou-Hallawa 2016-03-14 10:38:04 PDT
Created attachment 273988 [details]
Patch
Comment 2 Said Abou-Hallawa 2016-03-14 10:46:47 PDT
Created attachment 273990 [details]
Patch
Comment 3 Said Abou-Hallawa 2016-03-14 11:14:18 PDT
Created attachment 273992 [details]
Patch
Comment 4 Said Abou-Hallawa 2016-03-14 11:49:02 PDT
Created attachment 273995 [details]
Patch for review
Comment 5 Brent Fulgham 2016-04-13 22:29:13 PDT
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!
Comment 6 Said Abou-Hallawa 2016-04-15 22:19:48 PDT
Created attachment 276545 [details]
Patch
Comment 7 Said Abou-Hallawa 2016-04-15 23:04:41 PDT
Created attachment 276549 [details]
Patch
Comment 8 Said Abou-Hallawa 2016-04-15 23:16:14 PDT
Created attachment 276550 [details]
Patch
Comment 9 Said Abou-Hallawa 2016-04-15 23:25:21 PDT
Created attachment 276551 [details]
Patch
Comment 10 Said Abou-Hallawa 2016-04-17 20:27:41 PDT
Created attachment 276621 [details]
Patch
Comment 11 Said Abou-Hallawa 2016-04-18 10:28:35 PDT
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.
Comment 12 Said Abou-Hallawa 2016-06-02 08:51:51 PDT
Created attachment 280332 [details]
Patch
Comment 13 Said Abou-Hallawa 2016-06-02 09:04:26 PDT
Created attachment 280334 [details]
Patch
Comment 14 Darin Adler 2016-06-02 09:40:30 PDT
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.
Comment 15 Said Abou-Hallawa 2016-06-21 17:28:24 PDT
Created attachment 281796 [details]
Patch
Comment 16 WebKit Commit Bot 2016-06-21 17:35:49 PDT
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.
Comment 17 Said Abou-Hallawa 2016-07-04 16:48:07 PDT
Created attachment 282736 [details]
Patch
Comment 18 Said Abou-Hallawa 2016-07-04 17:12:04 PDT
Created attachment 282737 [details]
Patch
Comment 19 Said Abou-Hallawa 2016-07-04 17:43:53 PDT
Created attachment 282738 [details]
Patch
Comment 20 Said Abou-Hallawa 2016-07-04 20:17:31 PDT
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.
Comment 21 Said Abou-Hallawa 2016-07-04 20:20:45 PDT
Created attachment 282741 [details]
Patch for review
Comment 22 Said Abou-Hallawa 2016-09-15 14:27:38 PDT
This now is covered by https://bugs.webkit.org/show_bug.cgi?id=159819.
Comment 23 Said Abou-Hallawa 2016-09-15 14:29:19 PDT

*** This bug has been marked as a duplicate of bug 159819 ***
Comment 24 Brady Eidson 2017-08-19 16:02:28 PDT
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.
Comment 25 Said Abou-Hallawa 2017-08-21 12:16:16 PDT
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.