Imagefilter wrapper object

[flar]

This PR creates a Dart wrapper object around the NativeFieldWrapper object which allows us to compare ImageFilter objects in the framework (and also print their values in a human readable form).

The main value for comparing their values is in the various objects which hold an ImageFilter (mainly BackdropFilter, but potentially an ImageFilterWidget in the future). Such objects will avoid marking themselves as requiring an update in the various levels of tree building if they can determine that the filter hasn't changed - but currently filters are only comparable via their identity rather than their values.

Tests are also added to check on the various construction, comparison and assignment operations (in particular Paint.imageFilter = ImageFilter...() where the image is not stored in its framework format).

There is a potential breaking change in that the external-facing ImageFilter used to implement the NativeFieldWrapper2 interface and now that interface is only implemented by an internally maintained engine-accessible value (see the new ImageFilter._nativeFilter field and the new _ImageFilter hidden class that it stores).

[flar]

Some comments on the implementation...

This is based on the implementation of ColorFilter which immediately precedes ImageFilter in painting.dart. As I was following that template, a few issues struck me, but I wanted to remain true to following the template. In particular:

• ColorFilter and ImageFilter are 2 of the 3 objects held in the _objects array of Paint objects. They could probably stand to be built on a common (private) helper base class, but I didn't want to embrace that change in this PR. If we can convert the 3rd object to this style (Shader I believe?) then I think we should invest in the helper base class at that point. Until then, there is duplicate code for both the Paint.colorFilter and Paint.imageFilter properties and classes.
• There are Dart style irregularities in ColorFilter that I kept. One of the main ones was the use of curly braces around single-statement if's. I left the code in the style of ColorFilter since it was the closest code in the type hierarchy (and in the source file), but perhaps both should be revised for style at some point.
• ColorFilter has const constructors, but the ImageFilter that I was replacing did not. I wasn't sure if it would be a compatible change to add "const" to the ImageFilter constructors so I left them non-const.
• In my last commit (efd0285) I removed a comment that I had copied from ColorFilter about keeping the constants in sync with native code. It didn't make sense to me and I have since investigated - it appears that ColorFilter copied that comment from some other code where we do need to keep the constants in sync with native since the constants are passed in the data array of the Paint object. But, the type of ColorFilter and ImageFilter do not get passed in that array so the comment was not applicable. I removed it from ImageFilter, but did not remove it from ColorFilter as I am not working on that code in this PR.

[flar]

I consolidated my commits into a single commit to avoid a nasty merge with the ongoing changes to the web_ui version of the ImageFilter implementation. I will submit an issue for them to implement the necessary ==/hashcode/toString methods and enable the tests when this PR is landed.

[liyuqian]

The non-web change LGTM. @yjbanov : what should we do about the web_ui test failure? Adding some dummy ==/hashcode/toString implementation so this PR can merge with passing tests?

[yjbanov]

The non-web change LGTM. @yjbanov : what should we do about the web_ui test failure? Adding some dummy ==/hashcode/toString implementation so this PR can merge with passing tests?

Why add a dummy implementation and not a real one?

[liyuqian]

@yjbanov : Of course real ones are better. I got an impression that the real ones are currently blocked by something else (#13711 (comment)):

I will submit an issue for them to implement the necessary ==/hashcode/toString methods

@flar : can you please specify who are "them", and what need to be implemented by "them"? My previous experience is that I just copied the real implementation to web_ui. Are there any issue of doing that for this case?

[flar]

I've implemented ==/hashcode/toString in lib/ui for ImageFilter.
A presubmit test requires the equivalent class in lib/web_ui to also implement those methods. I can't just leave them missing and avoid the tests.
the lib/web_ui version of ImageFilter has a concrete implementation of ImageFilter.blur so I can implement ==/hashcode/toString in the helper classes for that factory.
the lib/web_ui version of ImageFilter doesn't have an implementation of ImageFilter.matrix - it is hardcoded to throw an exception instead. I have no place to put the appropriate implementation of those methods in that case. I can flesh the implementaiton of ImageFilter.matrix out further, but I don't know what the direction is for those classes on web so it isn't clear how to proceed there. Unfortunately, I also can't punt because it is a hard build requirement that the API contracts of lib/ui must match the classes in lib/web_ui.

I have some potential solutions, but right now I cannot even run a web test on my local engine and I have other things on my plate that are in the way of figuring that out.

[flar]

One question arises as to why the API comparison test is complaining that the web_ui versions of ImageFilter don't implement ==/hashcode/toString since those are implemented by default on all objects. What it really means is that lib/ui overrides them, but lib/web_ui does not. Is that really a concern?

[flar]

If I modify api_conform_test.dart to exclude ==/hashCode/toString from requiring matching methods then that check passes. The tests I wrote for those methods are marked TestOn('!chrome') as per other tests that shouldn't run on web, so they won't cause problems until web_ui decides to implement those methods.

The question is, should api_conform_test.dart exclude these standard methods?
@yjbanov ?

[flar]

Hmmm. We learned something new about Dart today. I can simply add abstract declarations of ==/hashCode/toString to the web_ui version of ImageFilter and the compiler accepts them without question and you can even call them and they just defer to the base implementations. This means I can get the code to pass the tests with 3 lines and then web_ui can come back and worry about their implementations on their own schedule.

[flar]

For completeness I implemented the 3 methods for web_ui:ImageFilter.blur...

[flar]

I would like feedback from @yjbanov about what I did with the web_ui classes before I push this.

[yjbanov]

/cc @jonahwilliams - the author of the API check. Was checking for Object methods in the API checker intentional? Maybe we should relax it?

[jonahwilliams]

We should relax it for toString,hashCode,and operator == but I thought I already did that...

[flar]

I've pushed most of @yjbanov 's suggestions, but left a couple of questions on the table from his previous review...

[jonahwilliams]

The api_conform check is relaxed in #13907 (comment)