[Webkit-unassigned] [Bug 184418] ImageFrame type used by non-Cocoa image decoder should not be the same as that used by ImageSource

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=184418

--- Comment #15 from Antti Koivisto <koivisto at iki.fi> ---
> When ImageSource asks the non CG decoder for image metadata or the
> NativeImage, the decoder checks first its cache. If the metadata or the
> NativeImage is not available, it calculates it only once and cache it in the
> ImageFrame. Then it sends it to the ImageSource which caches it again in the
> other ImageFrame. All the members which are surrounded by #if !USE(CG) and
> #endif are shared and used by the decoder and the ImageSource. This means
> all these data members are shared and have the same meaning:

There are no functional changes in this patch. ImageFrames for ImageSources don't use m_backingSource and other #if !USE(CG) members at all. Those are only used by the ScalableImageDecoder case. 

The patch makes ImageFrame an implementation detail of ImageSource and adds a new type ScalableImageDecoderFrame that is an implementation detail of ScalableImageDecoder. I think this is better than shared type because

1) They don't share that much data and what is shared is mostly trivial (size etc).  Most of the functions are also only needed for one case.

2) Most important things (like the actual image data) are stored differently. This 
requires ifs in code to handle. Having unused fields in both cases is confusing.

3) They define enum classes that are used only in one client (DisposalMethod and Caching)

4) ImageSource and ScalableImageDecoder are on different abstraction levels. One is generic WebCore/platform type used by all platforms while another is part of platform specific decoder implementation (cleaning up #ifs is direct benefit).

5) They could share a common base class or a common struct containing shared fields. Considering how simple both types are I don't think that would be super helpful either.

Also it is not likely that any further additions to ImageFrame would actually be relevant for the Decoder use case, piling on to the unneeded-field problem.

> What are you trying to achieve? What difficulties does this merge prevent
> you from doing?

I'm adding global tracking for image frames so we can manage memory usage for image animations on global basis rather than per-image. Without cleanups this will end up piling on more complexity and technical debt. 

> If you explain your goal and how the double caching can be solved in the
> future this will help me understand the purpose of this patch. I am just
> afraid that I or somebody else comes in the future may say the same thing
> about it: "I think the refactoring to split the ImageFrame was misguided."

Note that this patch only affects small part of your refactoring. Most of it was really useful.

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.webkit.org/pipermail/webkit-unassigned/attachments/20180409/1b095131/attachment-0002.html>