This repository was archived by the owner on Feb 25, 2025. It is now read-only.
Refactor ColorFilter to have a native wrapper #9668
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Hixie
reviewed
Jul 3, 2019
lib/ui/painting.dart
Outdated
| @@ -2600,6 +2588,48 @@ class ColorFilter { | |||
| } | |||
| } | |||
|
|
|||
| /// A [ColorFilter] that is backed by a native SkColorFilter. | |||
| // We could not make [ColorFilter] be this, because it would no longer be const | |||
| // constructible and would complicate the == and hashCode implementations. | |||
There was a problem hiding this comment.
I would phrase this comment differently. The current comment is a commit message. Consider something like: "This is a private class, rather than being the implementation of the public ColorFilter, because we want ColorFilter to be const constructible and we want it to be efficiently comparable, so that widgets can check for ColorFilter equality to avoid repainting."
liyuqian
reviewed
Jul 8, 2019
| sk_sp<SkColorFilter> color_filter = ExtractColorFilter(uint_data, values); | ||
| if (color_filter) { | ||
| paint_.setColorFilter(color_filter); | ||
| if (uint_data[kInvertColorIndex]) { |
There was a problem hiding this comment.
This simplification looks great! It would be nice to have a unit test (maybe in a newly created paint_unittests.cc) to cover all the cases.
| } | ||
|
|
||
| return null; | ||
| return _objects[_kColorFilterIndex].creator; | ||
| } | ||
|
|
||
| set colorFilter(ColorFilter value) { |
There was a problem hiding this comment.
I like the overall approach! My only concern is the test coverage. We could have a C++ unit test to cover paint.cc.
It might be a little more challenging to add a unit test for the Dart code. I wonder if we can use the fixture similar to the shell_test.dart that used in https://github.com/flutter/engine/blob/master/shell/common/shell_unittests.cc#L156. It allows the C++ unit test to specify a Dart function as the entry point, and a C++ native callback. I've used it to set a onReportTimings callback on the Dart side, and check its results on the C++ side (https://github.com/flutter/engine/blob/master/shell/common/shell_unittests.cc#L315).
I think it would be nice if we can create the ColorFilter on the Dart side, and check the SkColorFilter object is properly set on the C++ side. CC @chinmaygarde who might have more suggestions how to do this kind of Dart fixture test.
There was a problem hiding this comment.
I've added dart tests that will serve to test the C++ side, and that cover the same code. LMK if you think we need more.
engine-flutter-autoroll
added a commit
to engine-flutter-autoroll/flutter
that referenced
this pull request
Jul 8, 2019
engine-flutter-autoroll
added a commit
to engine-flutter-autoroll/flutter
that referenced
this pull request
Jul 9, 2019
This was referenced Jul 9, 2019
engine-flutter-autoroll
added a commit
to engine-flutter-autoroll/flutter
that referenced
this pull request
Jul 9, 2019
engine-flutter-autoroll
added a commit
to engine-flutter-autoroll/flutter
that referenced
this pull request
Jul 9, 2019
This was referenced Jul 9, 2019
engine-flutter-autoroll
added a commit
to engine-flutter-autoroll/flutter
that referenced
this pull request
Jul 9, 2019
engine-flutter-autoroll
added a commit
to engine-flutter-autoroll/flutter
that referenced
this pull request
Jul 9, 2019
engine-flutter-autoroll
added a commit
to engine-flutter-autoroll/flutter
that referenced
this pull request
Jul 9, 2019
This was referenced Jul 9, 2019
engine-flutter-autoroll
added a commit
to engine-flutter-autoroll/flutter
that referenced
this pull request
Jul 10, 2019
engine-flutter-autoroll
added a commit
to engine-flutter-autoroll/flutter
that referenced
this pull request
Jul 10, 2019
This was referenced Jul 10, 2019
engine-flutter-autoroll
added a commit
to engine-flutter-autoroll/flutter
that referenced
this pull request
Jul 10, 2019
engine-flutter-autoroll
added a commit
to engine-flutter-autoroll/flutter
that referenced
this pull request
Jul 10, 2019
engine-flutter-autoroll
added a commit
to engine-flutter-autoroll/flutter
that referenced
this pull request
Jul 10, 2019
engine-flutter-autoroll
added a commit
to engine-flutter-autoroll/flutter
that referenced
this pull request
Jul 10, 2019
engine-flutter-autoroll
added a commit
to engine-flutter-autoroll/flutter
that referenced
this pull request
Jul 10, 2019
engine-flutter-autoroll
added a commit
to engine-flutter-autoroll/flutter
that referenced
this pull request
Jul 10, 2019
engine-flutter-autoroll
added a commit
to engine-flutter-autoroll/flutter
that referenced
this pull request
Jul 10, 2019
engine-flutter-autoroll
added a commit
to engine-flutter-autoroll/flutter
that referenced
this pull request
Jul 11, 2019
engine-flutter-autoroll
added a commit
to engine-flutter-autoroll/flutter
that referenced
this pull request
Jul 11, 2019
engine-flutter-autoroll
added a commit
to engine-flutter-autoroll/flutter
that referenced
this pull request
Jul 11, 2019
engine-flutter-autoroll
added a commit
to engine-flutter-autoroll/flutter
that referenced
this pull request
Jul 11, 2019
engine-flutter-autoroll
added a commit
to flutter/flutter
that referenced
this pull request
Jul 11, 2019
flutter/engine@75387db...49445ce git log 75387db..49445ce --no-merges --oneline 49445ce FLEViewController/Engine API changes (flutter/engine#9750) 2a79462 Add Fuchsia build CI presubmit steps (flutter/engine#9736) 67cebdb Roll fuchsia/sdk/core/linux-amd64 from KGmm_RIJoXS19zTm2crjM3RYpmp5Y03-fLUeVdylbTYC to ehWVT9QJbC-vFMM6SkkQM9HJ9oITFCws7FC9JnrFq2gC (flutter/engine#9765) 089c740 Roll fuchsia/sdk/core/mac-amd64 from lCQWEeR_Ert7t_qAbMRycwrRyZC-dIprYPyPJzwPmg4C to EYnRdXFT9l-d8Qkz4zeTRXnqfV3KQzpQhoPs1r0-740C (flutter/engine#9759) b22410e Include SkParagraph headers only when the enable-skshaper flag is on (flutter/engine#9758) 2cd650d Minimal integration with the Skia text shaper module (flutter/engine#9556) f775f5e Re-enable the Wuffs GIF decoder (flutter/engine#9466) aca0482 Make all shell unit tests use the OpenGL rasterizer. (flutter/engine#9746) bc57291 Make FLEViewController's view an internal detail (flutter/engine#9741) 9776043 Synchronize main thread and gpu thread for first render frame (flutter/engine#9506) f600ae8 Use libc++ variant of string view and remove the FML variant. (flutter/engine#9737) 564f53f Revert "Improve caching limits for Skia (#9503)" (flutter/engine#9740) b453d3c libtxt: fix reference counting of SkFontStyleSets held by font asset providers (flutter/engine#9561) fa7627d Fix backspace crash on Chinese devices (flutter/engine#9734) 56885f7 Let pushColorFilter accept all types of ColorFilters (flutter/engine#9641) 6dccb21 Roll src/third_party/skia 96fdfe0fe88e..af4e7b6cf616 (1 commits) (flutter/engine#9735) 8511d9b Roll fuchsia/sdk/core/mac-amd64 from byM-kyxL4bemlTYNqhKUfJfZoIUrCSzS6XzsFr4n9-MC to lCQWEeR_Ert7t_qAbMRycwrRyZC-dIprYPyPJzwPmg4C (flutter/engine#9742) b3bf0a1 Roll fuchsia/sdk/core/linux-amd64 from I2Qe1zxgckzIzMBTztvzeWYsDgcb9Fw-idSI16oIlx8C to KGmm_RIJoXS19zTm2crjM3RYpmp5Y03-fLUeVdylbTYC (flutter/engine#9743) 7e56823 Fix windows test by not attempting to open a directory as a file. (flutter/engine#9745) 6cf0d13 Roll src/third_party/skia a3ffaabcc4f2..96fdfe0fe88e (5 commits) (flutter/engine#9731) 49a00ae Fix Fuchsia build. (flutter/engine#9730) b3bb39b Roll src/third_party/dart 06c3d7ad3a...09fc76bc51 (flutter/engine#9728) 2284210 Make the license script compatible with recently changed Dart I/O stream APIs (flutter/engine#9725) ad582b5 Rework image & texture management to use concurrent message queues. (flutter/engine#9486) 1dcd5f5 Roll src/third_party/skia 6b82cf638682..a3ffaabcc4f2 (24 commits) (flutter/engine#9726) 129979c Revert "Roll src/third_party/dart 06c3d7ad3a..7acecda2cc (12 commits)" (flutter/engine#9724) 8020d7e Roll src/third_party/skia 56065d9b875f..6b82cf638682 (3 commits) (flutter/engine#9718) e24bd78 Roll src/third_party/dart 06c3d7ad3a..7acecda2cc (12 commits) 3d2668c Reland isolate group changes 802bd15 iOS platform view opacity (flutter/engine#9667) 3b6265b Roll src/third_party/dart b5aeaa6796..06c3d7ad3a (44 commits) 887e052 Refactor ColorFilter to have a native wrapper (flutter/engine#9668) The AutoRoll server is located here: https://autoroll.skia.org/r/flutter-engine-flutter-autoroll Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+/master/autoroll/README.md If the roll is causing failures, please contact the current sheriff ([email protected]), and stop the roller if necessary.
johnsonmh
pushed a commit
to johnsonmh/flutter
that referenced
this pull request
Jul 30, 2019
flutter/engine@75387db...49445ce git log 75387db..49445ce --no-merges --oneline 49445ce FLEViewController/Engine API changes (flutter/engine#9750) 2a79462 Add Fuchsia build CI presubmit steps (flutter/engine#9736) 67cebdb Roll fuchsia/sdk/core/linux-amd64 from KGmm_RIJoXS19zTm2crjM3RYpmp5Y03-fLUeVdylbTYC to ehWVT9QJbC-vFMM6SkkQM9HJ9oITFCws7FC9JnrFq2gC (flutter/engine#9765) 089c740 Roll fuchsia/sdk/core/mac-amd64 from lCQWEeR_Ert7t_qAbMRycwrRyZC-dIprYPyPJzwPmg4C to EYnRdXFT9l-d8Qkz4zeTRXnqfV3KQzpQhoPs1r0-740C (flutter/engine#9759) b22410e Include SkParagraph headers only when the enable-skshaper flag is on (flutter/engine#9758) 2cd650d Minimal integration with the Skia text shaper module (flutter/engine#9556) f775f5e Re-enable the Wuffs GIF decoder (flutter/engine#9466) aca0482 Make all shell unit tests use the OpenGL rasterizer. (flutter/engine#9746) bc57291 Make FLEViewController&flutter#39;s view an internal detail (flutter/engine#9741) 9776043 Synchronize main thread and gpu thread for first render frame (flutter/engine#9506) f600ae8 Use libc&flutter#43;&flutter#43; variant of string view and remove the FML variant. (flutter/engine#9737) 564f53f Revert &flutter#34;Improve caching limits for Skia (flutter#9503)&flutter#34; (flutter/engine#9740) b453d3c libtxt: fix reference counting of SkFontStyleSets held by font asset providers (flutter/engine#9561) fa7627d Fix backspace crash on Chinese devices (flutter/engine#9734) 56885f7 Let pushColorFilter accept all types of ColorFilters (flutter/engine#9641) 6dccb21 Roll src/third_party/skia 96fdfe0fe88e..af4e7b6cf616 (1 commits) (flutter/engine#9735) 8511d9b Roll fuchsia/sdk/core/mac-amd64 from byM-kyxL4bemlTYNqhKUfJfZoIUrCSzS6XzsFr4n9-MC to lCQWEeR_Ert7t_qAbMRycwrRyZC-dIprYPyPJzwPmg4C (flutter/engine#9742) b3bf0a1 Roll fuchsia/sdk/core/linux-amd64 from I2Qe1zxgckzIzMBTztvzeWYsDgcb9Fw-idSI16oIlx8C to KGmm_RIJoXS19zTm2crjM3RYpmp5Y03-fLUeVdylbTYC (flutter/engine#9743) 7e56823 Fix windows test by not attempting to open a directory as a file. (flutter/engine#9745) 6cf0d13 Roll src/third_party/skia a3ffaabcc4f2..96fdfe0fe88e (5 commits) (flutter/engine#9731) 49a00ae Fix Fuchsia build. (flutter/engine#9730) b3bb39b Roll src/third_party/dart 06c3d7ad3a...09fc76bc51 (flutter/engine#9728) 2284210 Make the license script compatible with recently changed Dart I/O stream APIs (flutter/engine#9725) ad582b5 Rework image & texture management to use concurrent message queues. (flutter/engine#9486) 1dcd5f5 Roll src/third_party/skia 6b82cf638682..a3ffaabcc4f2 (24 commits) (flutter/engine#9726) 129979c Revert &flutter#34;Roll src/third_party/dart 06c3d7ad3a..7acecda2cc (12 commits)&flutter#34; (flutter/engine#9724) 8020d7e Roll src/third_party/skia 56065d9b875f..6b82cf638682 (3 commits) (flutter/engine#9718) e24bd78 Roll src/third_party/dart 06c3d7ad3a..7acecda2cc (12 commits) 3d2668c Reland isolate group changes 802bd15 iOS platform view opacity (flutter/engine#9667) 3b6265b Roll src/third_party/dart b5aeaa6796..06c3d7ad3a (44 commits) 887e052 Refactor ColorFilter to have a native wrapper (flutter/engine#9668) The AutoRoll server is located here: https://autoroll.skia.org/r/flutter-engine-flutter-autoroll Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+/master/autoroll/README.md If the roll is causing failures, please contact the current sheriff ([email protected]), and stop the roller if necessary.
xxrlzzz
reviewed
Feb 20, 2021
|
|
||
| namespace flutter { | ||
|
|
||
| // A handle to an SkCodec object. |
There was a problem hiding this comment.
This comment is right? I don't see any SkCodec usage in this class
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This was split from #9641 - this should make that PR's approach cleaner for sharing logic between Paint and SceneBuilder.
Unfortunately, Paint is a bit of a weird object that wants to store native and non-native fields efficiently. To work around this, I'm stuffing the existing
ColorFilterinto_ColorFilterso that we can still do comparisons/access the right data when we want to implementget colorFilterandset colorFilteron Paint. We also don't want to go right to a NativeWrapperFieldClass2 for ColorFilter, because it would no longer be const constructible and would make implementing==andhashCodemore complicated (and potentially degrade framework performance, that wants to treat this like a value class).I've modeled the
init*on the Shader class.This should be landed before #9641, which can then be rebased on it. I haven't added any new tests, because no new behavior is introduced - this is intended to be a completely compatible refactoring.