Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2831273002:
Split active animation interpolation storage between standard and custom properties (Closed)
|
Created:
3 years, 8 months ago by alancutter (OOO until 2018) Modified:
3 years, 6 months ago Reviewers:
suzyh_UTC10 (ex-contributor) CC:
darktears, apavlov+blink_chromium.org, blink-reviews, blink-reviews-animation_chromium.org, blink-reviews-css, blink-reviews-style_chromium.org, chromium-reviews, dglazkov+blink, Eric Willigers, rjwright, rwlbuis, shans Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionSplit active animation interpolation storage between standard and custom properties
This change refactors the storage of active animation interpolations
during a style resolve update by whether it targets a custom property
or a standard property. This makes it consistent with active transition
interpolations.
This change is necessary to process custom property animations separately
from standard properties in future patches.
This patch introduces no changes in behaviour.
BUG=671904
Review-Url: https://codereview.chromium.org/2831273002
Cr-Commit-Position: refs/heads/master@{#476179}
Committed: https://chromium.googlesource.com/chromium/src/+/5ef4817127ece8d7ead5fe0ae91b97228b7af578
Patch Set 1 #Patch Set 2 : rebased #
Total comments: 9
Patch Set 3 : Review Review changes #
Depends on Patchset: Dependent Patchsets: Messages
Total messages: 23 (16 generated)
Description was changed from ========== git squash commit for +splitAnimationInterpolationStorage. 560586383e8f8c30a8e90217c9940082ce1207ec +splitAnimationInterpolationStorage bfc41bc8ce5fbf34c105dcf78aced1a17b4e808f dogs 51eecf359bc58a825736464708bce2ca1fe1919c dogs dca8e68457e730cd99bec543141f69bc60c1b905 iamadog 9de179e70498ac7e13d6ef649725a490615fe321 dogsarethebestaaa BUG= ========== to ========== git squash commit for +splitAnimationInterpolationStorage. 560586383e8f8c30a8e90217c9940082ce1207ec +splitAnimationInterpolationStorage bfc41bc8ce5fbf34c105dcf78aced1a17b4e808f dogs 51eecf359bc58a825736464708bce2ca1fe1919c dogs dca8e68457e730cd99bec543141f69bc60c1b905 iamadog 9de179e70498ac7e13d6ef649725a490615fe321 dogsarethebestaaa BUG=671904 ==========
Description was changed from ========== git squash commit for +splitAnimationInterpolationStorage. 560586383e8f8c30a8e90217c9940082ce1207ec +splitAnimationInterpolationStorage bfc41bc8ce5fbf34c105dcf78aced1a17b4e808f dogs 51eecf359bc58a825736464708bce2ca1fe1919c dogs dca8e68457e730cd99bec543141f69bc60c1b905 iamadog 9de179e70498ac7e13d6ef649725a490615fe321 dogsarethebestaaa BUG=671904 ========== to ========== Split active animation interpolation storage between standard and custom properties This change refactors the storage of active animation interpolations during a style resolve update by whether it targets a custom property or a standard property. This change is necessary to process custom property animations separately from standard properties. This patch introduces no changes in behaviour. BUG=671904 ==========
alancutter@chromium.org changed reviewers: + suzyh@chromium.org
Description was changed from ========== Split active animation interpolation storage between standard and custom properties This change refactors the storage of active animation interpolations during a style resolve update by whether it targets a custom property or a standard property. This change is necessary to process custom property animations separately from standard properties. This patch introduces no changes in behaviour. BUG=671904 ========== to ========== Split active animation interpolation storage between standard and custom properties This change refactors the storage of active animation interpolations during a style resolve update by whether it targets a custom property or a standard property. This change is necessary to process custom property animations separately from standard properties in future patches. This patch introduces no changes in behaviour. BUG=671904 ==========
Description was changed from ========== Split active animation interpolation storage between standard and custom properties This change refactors the storage of active animation interpolations during a style resolve update by whether it targets a custom property or a standard property. This change is necessary to process custom property animations separately from standard properties in future patches. This patch introduces no changes in behaviour. BUG=671904 ========== to ========== Split active animation interpolation storage between standard and custom properties This change refactors the storage of active animation interpolations during a style resolve update by whether it targets a custom property or a standard property. This makes it consistent with active transition interpolations. This change is necessary to process custom property animations separately from standard properties in future patches. This patch introduces no changes in behaviour. BUG=671904 ==========
https://codereview.chromium.org/2831273002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/animation/css/CSSAnimations.cpp (right): https://codereview.chromium.org/2831273002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/animation/css/CSSAnimations.cpp:952: static bool IsCustomPropertyHandle(const PropertyHandle& property) { If this is not inside the CSSAnimations class, why is it static? https://codereview.chromium.org/2831273002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/animation/css/CSSAnimations.cpp:976: const auto& adoptActiveInterpolations = I find this difficult to read. What do you see as the major disadvantages of defining it as a separate helper function above? https://codereview.chromium.org/2831273002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/animation/css/CSSAnimations.cpp:981: EffectStack::ActiveInterpolations( Would this be simplified if EffectStack knew about custom properties vs standard properties, or is that a distinction we wish to keep out of EffectStack? https://codereview.chromium.org/2831273002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/css/resolver/StyleResolver.cpp (right): https://codereview.chromium.org/2831273002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/resolver/StyleResolver.cpp:1174: const ActiveInterpolationsMap& active_interpolations_map_for_animations = Add "standard" to variable name to match the transitions case?
Thanks for the quick review. https://codereview.chromium.org/2831273002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/animation/css/CSSAnimations.cpp (right): https://codereview.chromium.org/2831273002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/animation/css/CSSAnimations.cpp:952: static bool IsCustomPropertyHandle(const PropertyHandle& property) { On 2017/05/26 at 01:56:00, suzyh_UTC10 wrote: > If this is not inside the CSSAnimations class, why is it static? If you don't make it static then it becomes available for other compilation units to link to. This function is only meant to be available to this file. https://codereview.chromium.org/2831273002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/animation/css/CSSAnimations.cpp:976: const auto& adoptActiveInterpolations = On 2017/05/26 at 01:56:00, suzyh_UTC10 wrote: > I find this difficult to read. What do you see as the major disadvantages of defining it as a separate helper function above? I don't see any major advantages to it, just minor ones. Changed. https://codereview.chromium.org/2831273002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/animation/css/CSSAnimations.cpp:981: EffectStack::ActiveInterpolations( On 2017/05/26 at 01:56:00, suzyh_UTC10 wrote: > Would this be simplified if EffectStack knew about custom properties vs standard properties, or is that a distinction we wish to keep out of EffectStack? It's a distiction that would be out of place in EffectStack (it deals with SVG animations as well) and I don't think it would make this code much easier to read, they still need to be stored in separate places on the update object. https://codereview.chromium.org/2831273002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/css/resolver/StyleResolver.cpp (right): https://codereview.chromium.org/2831273002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/resolver/StyleResolver.cpp:1174: const ActiveInterpolationsMap& active_interpolations_map_for_animations = On 2017/05/26 at 01:56:00, suzyh_UTC10 wrote: > Add "standard" to variable name to match the transitions case? Good call, done.
lgtm with optional nit https://codereview.chromium.org/2831273002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/animation/css/CSSAnimations.cpp (right): https://codereview.chromium.org/2831273002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/animation/css/CSSAnimations.cpp:952: static bool IsCustomPropertyHandle(const PropertyHandle& property) { On 2017/05/26 at 03:09:14, alancutter wrote: > On 2017/05/26 at 01:56:00, suzyh_UTC10 wrote: > > If this is not inside the CSSAnimations class, why is it static? > > If you don't make it static then it becomes available for other compilation units to link to. This function is only meant to be available to this file. I thought without having it in the header then that wouldn't be an issue. Anyway, I think an anonymous namespace is the recommended approach in the style guide: https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md... If you make this change, remember to do the same with the other static functions in the file for consistency.
On 2017/05/26 at 04:02:09, suzyh wrote: > lgtm with optional nit > > https://codereview.chromium.org/2831273002/diff/20001/third_party/WebKit/Sour... > File third_party/WebKit/Source/core/animation/css/CSSAnimations.cpp (right): > > https://codereview.chromium.org/2831273002/diff/20001/third_party/WebKit/Sour... > third_party/WebKit/Source/core/animation/css/CSSAnimations.cpp:952: static bool IsCustomPropertyHandle(const PropertyHandle& property) { > On 2017/05/26 at 03:09:14, alancutter wrote: > > On 2017/05/26 at 01:56:00, suzyh_UTC10 wrote: > > > If this is not inside the CSSAnimations class, why is it static? > > > > If you don't make it static then it becomes available for other compilation units to link to. This function is only meant to be available to this file. > > I thought without having it in the header then that wouldn't be an issue. Unfortunately no, header files have no special meaning to the compiler. > Anyway, I think an anonymous namespace is the recommended approach in the style guide: https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md... > > If you make this change, remember to do the same with the other static functions in the file for consistency. Will do in a separate patch.
The CQ bit was checked by alancutter@chromium.org 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.
The CQ bit was checked by alancutter@chromium.org 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.
The CQ bit was checked by alancutter@chromium.org
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 40001, "attempt_start_ts": 1496275954324570,
"parent_rev": "1053faaf3d0458556000acde907116a5d869c815", "commit_rev":
"5ef4817127ece8d7ead5fe0ae91b97228b7af578"}
Message was sent while issue was closed.
Description was changed from ========== Split active animation interpolation storage between standard and custom properties This change refactors the storage of active animation interpolations during a style resolve update by whether it targets a custom property or a standard property. This makes it consistent with active transition interpolations. This change is necessary to process custom property animations separately from standard properties in future patches. This patch introduces no changes in behaviour. BUG=671904 ========== to ========== Split active animation interpolation storage between standard and custom properties This change refactors the storage of active animation interpolations during a style resolve update by whether it targets a custom property or a standard property. This makes it consistent with active transition interpolations. This change is necessary to process custom property animations separately from standard properties in future patches. This patch introduces no changes in behaviour. BUG=671904 Review-Url: https://codereview.chromium.org/2831273002 Cr-Commit-Position: refs/heads/master@{#476179} Committed: https://chromium.googlesource.com/chromium/src/+/5ef4817127ece8d7ead5fe0ae91b... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://chromium.googlesource.com/chromium/src/+/5ef4817127ece8d7ead5fe0ae91b... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
