colorFilter as imageFilter for web

[alanwutang11]

Previously, colorFilter on the web sdk was not a subtype of imageFilter. Therefore, when passing in a colorFilter as an imageFilter for any widget that accepts an imageFIlter (BackdropFilter, ImageFiltered, etc.), you would get a compiler error.

The web sdk before:

abstract class ColorFilter { { [...] }
class ImageFilter { [...]} 

The web sdk after:

class ColorFilter implements ImageFilter { { [...] }
class ImageFilter { [...]} 

A refactor of the type hierarchy for colorFIlter was made with the change.

fixes: flutter/flutter#82637

Pre-launch Checklist

• I read the [Contributor Guide] and followed the process outlined there for submitting PRs.
• I read the [Tree Hygiene] wiki page, which explains my responsibilities.
• I read and followed the [Flutter Style Guide] and the [C++, Objective-C, Java style guides].
• I listed at least one issue that this PR fixes in the description above.
• I added new tests to check the change I am making or feature I am adding, or Hixie said the PR is test-exempt. See [testing the engine] for instructions on writing and running engine tests.
• I updated/added relevant documentation (doc comments with ///).
• I signed the [CLA].
• All existing and new tests are passing.

[skia-gold]

Gold has detected about 6 new digest(s) on patchset 1.
View them at https://flutter-engine-gold.skia.org/cl/github/37522

[flutter-dashboard]

Golden file changes have been found for this pull request. Click here to view and triage (e.g. because this is an intentional change).

If you are still iterating on this change and are not ready to resolve the images on the Flutter Gold dashboard, consider marking this PR as a draft pull request above. You will still be able to view image results on the dashboard, commenting will be silenced, and the check will not try to resolve itself until marked ready for review.

Changes reported for pull request #37522 at sha badb3d7

[skia-gold]

Gold has detected about 6 new digest(s) on patchset 2.
View them at https://flutter-engine-gold.skia.org/cl/github/37522

[skia-gold]

Gold has detected about 1 new digest(s) on patchset 3.
View them at https://flutter-engine-gold.skia.org/cl/github/37522

[skia-gold]

Gold has detected about 2 new digest(s) on patchset 4.
View them at https://flutter-engine-gold.skia.org/cl/github/37522

[flutter-dashboard]

Golden file changes are available for triage from new commit, Click here to view.

Changes reported for pull request #37522 at sha 340c791

[eyebrowsoffire]

This seems to be moving things generally in the right direction, but I do still find the type hierarchy and the identity semantics of color filters to be pretty confusing here.

It seems as though you have two things that are sort of masquerading as each other in different situations. The EngineColorFilter, which is basically a plain old data object that doesn't do any real rendering, and then the "renderer color filters" which you can convert them to, which actually do renderer-specific logic. But it looks like that conversion somehow implicitly happens when you round trip it through the Paint object's colorFilter getter/setter. I find that to be really surprising behavior.

One question I have is, do the "renderer color filter" objects and the EngineColorFilter actually have to share a common base class? It feels wrong for them to be passed around interchangeably. It seems like the object using the "renderer color filter" should be aware that it isn't actually an EngineColorFilter and so doesn't have to implement/extend ui.ColorFilter. Perhaps the best thing is to make the "renderer color filter" a separate type of object that is like a ColorFilterProcessor (or something like that) which can be produced from the EngineColorFilter object by the renderer. That way the identity of the EngineColorFilter and the filter processor object are clearly separated and not confused with each other.

I also suggest that instead of having four separate createXXXColorFilter methods on the Renderer class, you just have one that is called createProcessorForColorFilter which takes the color filter and passes back the processor class. And to avoid dynamic return values, you can create a base class for all the color filter processors.

[skia-gold]

Gold has detected about 2 new digest(s) on patchset 6.
View them at https://flutter-engine-gold.skia.org/cl/github/37522

[flutter-dashboard]

Golden file changes are available for triage from new commit, Click here to view.

Changes reported for pull request #37522 at sha cf2e6d1

[alanwutang11]

This seems to be moving things generally in the right direction, but I do still find the type hierarchy and the identity semantics of color filters to be pretty confusing here.

It seems as though you have two things that are sort of masquerading as each other in different situations. The EngineColorFilter, which is basically a plain old data object that doesn't do any real rendering, and then the "renderer color filters" which you can convert them to, which actually do renderer-specific logic. But it looks like that conversion somehow implicitly happens when you round trip it through the Paint object's colorFilter getter/setter. I find that to be really surprising behavior.

One question I have is, do the "renderer color filter" objects and the EngineColorFilter actually have to share a common base class? It feels wrong for them to be passed around interchangeably. It seems like the object using the "renderer color filter" should be aware that it isn't actually an EngineColorFilter and so doesn't have to implement/extend ui.ColorFilter. Perhaps the best thing is to make the "renderer color filter" a separate type of object that is like a ColorFilterProcessor (or something like that) which can be produced from the EngineColorFilter object by the renderer. That way the identity of the EngineColorFilter and the filter processor object are clearly separated and not confused with each other.

I also suggest that instead of having four separate createXXXColorFilter methods on the Renderer class, you just have one that is called createProcessorForColorFilter which takes the color filter and passes back the processor class. And to avoid dynamic return values, you can create a base class for all the color filter processors.

Thanks for the comment and review! With the commit "removed creator and toRendererColorFilter", dynamic return types and methods on the renderer class were removed entirely. The creator field held by the "renderer colorFilters" were also removed as it wasn't necessary for the colorFilter paint getter like I mistakenly thought.

Instead, there are two separate functions, createCkColorFilter (see colorFilter.dart in canvaskit) and createHtmlColorFilter (see shader.dart in html), that handle the conversion from ui.ColorFilter to a "renderer colorFilter". Hopefully these changes make the behavior of a colorFilter and the conversion between ui.ColorFilter and a "renderer colorFilter" a little clearer.

A follow up PR will be necessary to further simplify the type hierarchy of imageFilter and colorFilter. Specifically, the "renderer colorFilters" don't need to implement the "renderer imageFilters". This is going to be another decently sized refactor (especially on the canvaskit side where we have CkManagedSkImageFilterConvertible), so a separate PR might be a better way to chunk the work.

Hopefully for now, these changes are good while fixing the original compiler error. If not, any suggestions for additional changes to clean up the current flow of things are welcome!

[harryterkelsen]

LGTM with some formatting nits

[eyebrowsoffire]

Looks awesome! A huge improvement. A couple small formatting things but this otherwise looks good to merge.

[skia-gold]

Gold has detected about 2 new digest(s) on patchset 8.
View them at https://flutter-engine-gold.skia.org/cl/github/37522