Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2357633003:
Move buildFilterOperations to FilterEffectBuilder (Closed)
|
Created:
4 years, 3 months ago by fs Modified:
4 years, 2 months ago CC:
ajuma+watch_chromium.org, darktears, blink-reviews, blink-reviews-animation_chromium.org, blink-reviews-paint_chromium.org, blink-reviews-platform-graphics_chromium.org, Rik, chromium-reviews, danakj+watch_chromium.org, dshwang, drott+blinkwatch_chromium.org, krit, Eric Willigers, f(malita), jbroman, Justin Novosad, pdr+graphicswatchlist_chromium.org, rjwright, rwlbuis, Stephen Chennney, shans, slimming-paint-reviews_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionMove buildFilterOperations to FilterEffectBuilder
This puts all FilterOperations conversions (to FilterEffect and
CompositorFilterOperations) in one spot - FilterEffectBuilder.
This allows folding the functionality of resolveReferenceFilters into
FilterEffectBuilder, and hence get rid of the explicit extra step to
update the "cached" Filter chain in ReferenceFilterOperation.
This is one step on the way to turning FilterOperations into a core
style type, to allow for more straight-forward interaction with other
parts of the style system.
BUG=439970
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
Committed: https://crrev.com/42791da1d3cce84f4d46dc07b6587395ef20d99b
Cr-Commit-Position: refs/heads/master@{#420844}
Patch Set 1 #Patch Set 2 : Element -> Node #
Total comments: 11
Patch Set 3 : Fold toCompositorFilterOperations #Messages
Total messages: 38 (18 generated)
Description was changed from ========== Move buildFilterOperations to FilterEffectBuilder This puts all FilterOperations conversions (to FilterEffect and CompositorFilterOperations) in one spot - FilterEffectBuilder. This allows folding the functionality of resolveReferenceFilters into FilterEffectBuilder, and hence get rid of the explicit extra step to update the "cached" Filter chain in ReferenceFilterOperation. BUG=439970 ========== to ========== Move buildFilterOperations to FilterEffectBuilder This puts all FilterOperations conversions (to FilterEffect and CompositorFilterOperations) in one spot - FilterEffectBuilder. This allows folding the functionality of resolveReferenceFilters into FilterEffectBuilder, and hence get rid of the explicit extra step to update the "cached" Filter chain in ReferenceFilterOperation. BUG=439970 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ==========
The CQ bit was checked by fs@opera.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)
The CQ bit was checked by fs@opera.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
fs@opera.com changed reviewers: + chrishtr@chromium.org, loyso@chromium.org, senorblanco@chromium.org
https://codereview.chromium.org/2357633003/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/animation/AnimationTranslationUtilTest.cpp (left): https://codereview.chromium.org/2357633003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/animation/AnimationTranslationUtilTest.cpp:74: TEST(AnimationTranslationUtilTest, filtersWork) I removed this because it required DEPS to include cc/, which we may not want for core/paint/ (I guess it would be possible to special-case a specific test file though.) Could also just convert the setup into something that doesn't rely on toCompositorFilterOperations, and put it into a CompositorFilterOperationsTest.
LGTM, but please wait for loyso@ to comment on the AnimationTranslationUtil test changes. https://codereview.chromium.org/2357633003/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/paint/PaintLayer.cpp (right): https://codereview.chromium.org/2357633003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/paint/PaintLayer.cpp:2692: FilterOperations PaintLayer::addReflectionToFilterOperations(const ComputedStyle& style) const I'm wondering if it would make sense to make the builder responsible for this step. As in, we would construct the builder with just the style, and it would extract the filters and box-reflect (if any) internally. It would avoid a bit of duplicated code below. OTOH, we'd probably have to have a "buildFilterOperations()" and "buildBackdropFilterOperations()" on the builder. WDYT?
https://codereview.chromium.org/2357633003/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/paint/PaintLayer.cpp (right): https://codereview.chromium.org/2357633003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/paint/PaintLayer.cpp:2692: FilterOperations PaintLayer::addReflectionToFilterOperations(const ComputedStyle& style) const On 2016/09/21 at 14:21:24, Stephen White wrote: > I'm wondering if it would make sense to make the builder responsible for this step. As in, we would construct the builder with just the style, and it would extract the filters and box-reflect (if any) internally. It would avoid a bit of duplicated code below. OTOH, we'd probably have to have a "buildFilterOperations()" and "buildBackdropFilterOperations()" on the builder. WDYT? Yes, I've been pondering this a little bit... the downside to putting in the builder is that it requires even more state to be tracked. Handling it outside, as a separate step, has other issues (might need to instantiate a Filter if 'filter' is 'none' and things like that.) I guess I could experiment a bit with it and see what it will look like.
https://codereview.chromium.org/2357633003/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/animation/AnimationTranslationUtilTest.cpp (left): https://codereview.chromium.org/2357633003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/animation/AnimationTranslationUtilTest.cpp:74: TEST(AnimationTranslationUtilTest, filtersWork) On 2016/09/21 at 13:23:26, fs wrote: > I removed this because it required DEPS to include cc/, which we may not want for core/paint/ (I guess it would be possible to special-case a specific test file though.) Could also just convert the setup into something that doesn't rely on toCompositorFilterOperations, and put it into a CompositorFilterOperationsTest. Can't you leave the test in this file though? It's in platform/
https://codereview.chromium.org/2357633003/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/animation/AnimationTranslationUtilTest.cpp (left): https://codereview.chromium.org/2357633003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/animation/AnimationTranslationUtilTest.cpp:74: TEST(AnimationTranslationUtilTest, filtersWork) On 2016/09/21 at 17:56:50, chrishtr wrote: > On 2016/09/21 at 13:23:26, fs wrote: > > I removed this because it required DEPS to include cc/, which we may not want for core/paint/ (I guess it would be possible to special-case a specific test file though.) Could also just convert the setup into something that doesn't rely on toCompositorFilterOperations, and put it into a CompositorFilterOperationsTest. > > Can't you leave the test in this file though? It's in platform/ Then we'd get a platform/ -> core/ dependency - which seems like the worst of the various options, at least cursorily.
https://codereview.chromium.org/2357633003/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/animation/AnimationTranslationUtilTest.cpp (left): https://codereview.chromium.org/2357633003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/animation/AnimationTranslationUtilTest.cpp:74: TEST(AnimationTranslationUtilTest, filtersWork) On 2016/09/21 at 18:22:17, fs wrote: > On 2016/09/21 at 17:56:50, chrishtr wrote: > > On 2016/09/21 at 13:23:26, fs wrote: > > > I removed this because it required DEPS to include cc/, which we may not want for core/paint/ (I guess it would be possible to special-case a specific test file though.) Could also just convert the setup into something that doesn't rely on toCompositorFilterOperations, and put it into a CompositorFilterOperationsTest. > > > > Can't you leave the test in this file though? It's in platform/ > > Then we'd get a platform/ -> core/ dependency - which seems like the worst of the various options, at least cursorily. I see. Alternatively you can use the non-cc FilterOperation enum right? It seems they are required to be synced?
https://codereview.chromium.org/2357633003/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/animation/AnimationTranslationUtilTest.cpp (left): https://codereview.chromium.org/2357633003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/animation/AnimationTranslationUtilTest.cpp:74: TEST(AnimationTranslationUtilTest, filtersWork) On 2016/09/21 at 19:04:40, chrishtr wrote: > On 2016/09/21 at 18:22:17, fs wrote: > > On 2016/09/21 at 17:56:50, chrishtr wrote: > > > On 2016/09/21 at 13:23:26, fs wrote: > > > > I removed this because it required DEPS to include cc/, which we may not want for core/paint/ (I guess it would be possible to special-case a specific test file though.) Could also just convert the setup into something that doesn't rely on toCompositorFilterOperations, and put it into a CompositorFilterOperationsTest. > > > > > > Can't you leave the test in this file though? It's in platform/ > > > > Then we'd get a platform/ -> core/ dependency - which seems like the worst of the various options, at least cursorily. > > I see. Alternatively you can use the non-cc FilterOperation enum right? It seems they are required > to be synced? That doesn't eliminate the dep because the item (w/ type()) is still a cc::FilterOperation.
https://codereview.chromium.org/2357633003/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/animation/AnimationTranslationUtilTest.cpp (left): https://codereview.chromium.org/2357633003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/animation/AnimationTranslationUtilTest.cpp:74: TEST(AnimationTranslationUtilTest, filtersWork) On 2016/09/21 at 20:29:49, fs wrote: > On 2016/09/21 at 19:04:40, chrishtr wrote: > > On 2016/09/21 at 18:22:17, fs wrote: > > > On 2016/09/21 at 17:56:50, chrishtr wrote: > > > > On 2016/09/21 at 13:23:26, fs wrote: > > > > > I removed this because it required DEPS to include cc/, which we may not want for core/paint/ (I guess it would be possible to special-case a specific test file though.) Could also just convert the setup into something that doesn't rely on toCompositorFilterOperations, and put it into a CompositorFilterOperationsTest. > > > > > > > > Can't you leave the test in this file though? It's in platform/ > > > > > > Then we'd get a platform/ -> core/ dependency - which seems like the worst of the various options, at least cursorily. > > > > I see. Alternatively you can use the non-cc FilterOperation enum right? It seems they are required > > to be synced? > > That doesn't eliminate the dep because the item (w/ type()) is still a cc::FilterOperation. I see. This is a pretty weird situation, but ok...
https://codereview.chromium.org/2357633003/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/animation/AnimationTranslationUtilTest.cpp (left): https://codereview.chromium.org/2357633003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/animation/AnimationTranslationUtilTest.cpp:74: TEST(AnimationTranslationUtilTest, filtersWork) On 2016/09/21 at 20:35:45, chrishtr wrote: > On 2016/09/21 at 20:29:49, fs wrote: > > On 2016/09/21 at 19:04:40, chrishtr wrote: > > > On 2016/09/21 at 18:22:17, fs wrote: > > > > On 2016/09/21 at 17:56:50, chrishtr wrote: > > > > > On 2016/09/21 at 13:23:26, fs wrote: > > > > > > I removed this because it required DEPS to include cc/, which we may not want for core/paint/ (I guess it would be possible to special-case a specific test file though.) Could also just convert the setup into something that doesn't rely on toCompositorFilterOperations, and put it into a CompositorFilterOperationsTest. > > > > > > > > > > Can't you leave the test in this file though? It's in platform/ > > > > > > > > Then we'd get a platform/ -> core/ dependency - which seems like the worst of the various options, at least cursorily. > > > > > > I see. Alternatively you can use the non-cc FilterOperation enum right? It seems they are required > > > to be synced? > > > > That doesn't eliminate the dep because the item (w/ type()) is still a cc::FilterOperation. > > I see. This is a pretty weird situation, but ok... Indeed it is...
SkiaImageFilterBuilder::buildFilterOperations / toCompositorFilterOperations was a very simple function which converts blink/platform representation of data to CC representation in 1:1 manner. Such a conversion must stay at platform layer together with that unit test. What do you thing on the following approaches: 1) Pass a "Cache context" abstract object to platform's buildFilterOperations function so it may ask a virtual function for some cached data from the core level. OR 2) Split FilterEffectBuilder into to entities so one of them lives at platform layer and you organize their interaction via inheritance or aggregation. https://codereview.chromium.org/2357633003/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/animation/CompositorAnimations.cpp (right): https://codereview.chromium.org/2357633003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/animation/CompositorAnimations.cpp:419: CompositorFilterOperations toCompositorFilterOperations(const FilterOperations& inOperations) There is no need to establish this function - there is just one call site in this file. https://codereview.chromium.org/2357633003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/animation/CompositorAnimations.cpp:421: FilterEffectBuilder builder(nullptr, FloatRect(), 1); Can we have a dedicated ctor with no args for this case? Now we have all those forwarding C++11 ctors.
On 2016/09/22 at 04:55:51, loyso wrote: > SkiaImageFilterBuilder::buildFilterOperations / toCompositorFilterOperations was a very simple function which converts blink/platform representation of data to CC representation in 1:1 manner. > > Such a conversion must stay at platform layer together with that unit test. The conversion to a CC representation will still stay in platform since that's what CompositorFilterOperations encapsulates. What I ultimately want to get is a layering where (blink::)FilterOperations is a core (style) type that produces one (or both) of the platform representations (CompositorFilterOperations / chain of FilterEffects). The primary benefactor to that would be the "reference" filter, but for instance the drop-shadow effect would benefit from this as well. So while both of the below suggestions might be workable at this point in time, they would represent steps backwards in a wider perspective than just this CL. > What do you thing on the following approaches: > 1) Pass a "Cache context" abstract object to platform's buildFilterOperations function so it may ask a virtual function for some cached data from the core level. > OR > 2) Split FilterEffectBuilder into to entities so one of them lives at platform layer and you organize their interaction via inheritance or aggregation. > > https://codereview.chromium.org/2357633003/diff/20001/third_party/WebKit/Sour... > File third_party/WebKit/Source/core/animation/CompositorAnimations.cpp (right): > > https://codereview.chromium.org/2357633003/diff/20001/third_party/WebKit/Sour... > third_party/WebKit/Source/core/animation/CompositorAnimations.cpp:419: CompositorFilterOperations toCompositorFilterOperations(const FilterOperations& inOperations) > There is no need to establish this function - there is just one call site in this file. Folded. > https://codereview.chromium.org/2357633003/diff/20001/third_party/WebKit/Sour... > third_party/WebKit/Source/core/animation/CompositorAnimations.cpp:421: FilterEffectBuilder builder(nullptr, FloatRect(), 1); > Can we have a dedicated ctor with no args for this case? Now we have all those forwarding C++11 ctors. This is an "outlier", so I don't see why we would give it preferential treatment. It seems better to spell out the restrictions imposed by/at this call-site rather hide them.
On 2016/09/22 09:01:41, fs wrote: > On 2016/09/22 at 04:55:51, loyso wrote: > So while both of the below suggestions might be workable at this point in time, > they would represent steps backwards in a wider perspective than just this CL. Ok then. Unfortunately, I'm not aware of your future design. Thanks for the folding! LGTM.
On 2016/09/23 at 00:08:49, loyso wrote: > On 2016/09/22 09:01:41, fs wrote: > > On 2016/09/22 at 04:55:51, loyso wrote: > > So while both of the below suggestions might be workable at this point in time, > > they would represent steps backwards in a wider perspective than just this CL. > Ok then. Unfortunately, I'm not aware of your future design. Yes, reading minds can be hard sometimes =P; I should've added something to the description about it. Will try come up with something before landing.
Description was changed from ========== Move buildFilterOperations to FilterEffectBuilder This puts all FilterOperations conversions (to FilterEffect and CompositorFilterOperations) in one spot - FilterEffectBuilder. This allows folding the functionality of resolveReferenceFilters into FilterEffectBuilder, and hence get rid of the explicit extra step to update the "cached" Filter chain in ReferenceFilterOperation. BUG=439970 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ========== to ========== Move buildFilterOperations to FilterEffectBuilder This puts all FilterOperations conversions (to FilterEffect and CompositorFilterOperations) in one spot - FilterEffectBuilder. This allows folding the functionality of resolveReferenceFilters into FilterEffectBuilder, and hence get rid of the explicit extra step to update the "cached" Filter chain in ReferenceFilterOperation. This is one step on the way to turning FilterOperations into a core style type, to allow for more straight-forward interaction with other parts of the style system. BUG=439970 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ==========
The CQ bit was checked by fs@opera.com
The patchset sent to the CQ was uploaded after l-g-t-m from senorblanco@chromium.org Link to the patchset: https://codereview.chromium.org/2357633003/#ps40001 (title: "Fold toCompositorFilterOperations")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was checked by fs@opera.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
The CQ bit was checked by fs@opera.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== Move buildFilterOperations to FilterEffectBuilder This puts all FilterOperations conversions (to FilterEffect and CompositorFilterOperations) in one spot - FilterEffectBuilder. This allows folding the functionality of resolveReferenceFilters into FilterEffectBuilder, and hence get rid of the explicit extra step to update the "cached" Filter chain in ReferenceFilterOperation. This is one step on the way to turning FilterOperations into a core style type, to allow for more straight-forward interaction with other parts of the style system. BUG=439970 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ========== to ========== Move buildFilterOperations to FilterEffectBuilder This puts all FilterOperations conversions (to FilterEffect and CompositorFilterOperations) in one spot - FilterEffectBuilder. This allows folding the functionality of resolveReferenceFilters into FilterEffectBuilder, and hence get rid of the explicit extra step to update the "cached" Filter chain in ReferenceFilterOperation. This is one step on the way to turning FilterOperations into a core style type, to allow for more straight-forward interaction with other parts of the style system. BUG=439970 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 Committed: https://crrev.com/42791da1d3cce84f4d46dc07b6587395ef20d99b Cr-Commit-Position: refs/heads/master@{#420844} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/42791da1d3cce84f4d46dc07b6587395ef20d99b Cr-Commit-Position: refs/heads/master@{#420844} |
