Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2130753003:
Support viewport units for SVG Length animations (Closed)
|
Created:
4 years, 5 months ago by Shanmuga Pandi Modified:
4 years, 5 months ago CC:
darktears, blink-reviews, blink-reviews-animation_chromium.org, chromium-reviews, Eric Willigers, rjwright, shans Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionSupport viewport units for SVG Length animations
Add support of viewport units such as vm,vw, vmin, vmax for animations on SVG Length.
Chrome already supporting all viewport units for SVGLength.
https://bugs.chromium.org/p/chromium/issues/detail?id=368598
In that case, supporting those units for SVG animation will make sense.
BUG=626297
Committed: https://crrev.com/c51f84657ddb3d5712da3c059883a9686a8c0acb
Cr-Commit-Position: refs/heads/master@{#405038}
Patch Set 1 #
Total comments: 4
Patch Set 2 : moved TCs to existing file #
Messages
Total messages: 29 (12 generated)
Description was changed from ========== Support viewport units for SVG Length animations BUG=626297 ========== to ========== Support viewport units for SVG Length animations Add support of viewport units such as vm,vw, vmin, vmax for animations on SVG Length. BUG=626297 ==========
shanmuga.m@samsung.com changed reviewers: + rob.buis@samsung.com
@rob, PTAL!! Thanks
On 2016/07/07 11:09:34, Shanmuga Pandi wrote: > @rob, > PTAL!! > > Thanks Can you state the part of the spec(s) that specifies this? Thanks.
On 2016/07/07 15:19:36, rwlbuis wrote: > On 2016/07/07 11:09:34, Shanmuga Pandi wrote: > > @rob, > > PTAL!! > > > > Thanks > > Can you state the part of the spec(s) that specifies this? Thanks. We have already supporting all viewport units for SVGLength.. https://bugs.chromium.org/p/chromium/issues/detail?id=368598 In that case, for animation also we need to support viewport units. We have already supporting rem, ch units for animation.
shanmuga.m@samsung.com changed reviewers: + alancutter@chromium.org, fs@opera.com, pdr@chromium.org
++More reviewers!! PTAL!!
This looks reasonable to me, but I'll defer the final say to the animation folks. https://codereview.chromium.org/2130753003/diff/1/third_party/WebKit/LayoutTe... File third_party/WebKit/LayoutTests/animations/svg-attribute-interpolation/svg-length-viewport-units-interpolation.html (right): https://codereview.chromium.org/2130753003/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/animations/svg-attribute-interpolation/svg-length-viewport-units-interpolation.html:13: property: 'width', I believe the common "structure" of this test directory is to group/test per attribute (property), so by that standard this would go in svg-width-... (and possibly in other files as well, the animation people will know better what they prefer)
suzyh@chromium.org changed reviewers: + suzyh@chromium.org
This mostly seems fine to me. Please give a more detailed CL description, including a link to the relevant spec section. (This makes it easier to understand why the change was made, e.g. when browsing code history.) https://codereview.chromium.org/2130753003/diff/1/third_party/WebKit/LayoutTe... File third_party/WebKit/LayoutTests/animations/svg-attribute-interpolation/svg-length-viewport-units-interpolation.html (right): https://codereview.chromium.org/2130753003/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/animations/svg-attribute-interpolation/svg-length-viewport-units-interpolation.html:13: property: 'width', On 2016/07/08 at 11:14:03, fs (ooo to July 18) wrote: > I believe the common "structure" of this test directory is to group/test per attribute (property), so by that standard this would go in svg-width-... (and possibly in other files as well, the animation people will know better what they prefer) Yes, please move these tests into svg-width-interpolation.html. Please also add a couple of tests that use these units where the 'from' and 'to' values use different units, as done in that file and others.
https://codereview.chromium.org/2130753003/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/animation/SVGLengthInterpolationType.cpp (right): https://codereview.chromium.org/2130753003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/animation/SVGLengthInterpolationType.cpp:42: CSSPrimitiveValue::UnitType::ViewportMax Nit: Let's add a trailing comma here to be consistent with the code above.
Description was changed from ========== Support viewport units for SVG Length animations Add support of viewport units such as vm,vw, vmin, vmax for animations on SVG Length. BUG=626297 ========== to ========== Support viewport units for SVG Length animations Add support of viewport units such as vm,vw, vmin, vmax for animations on SVG Length. Chrome already supporting all viewport units for SVGLength. https://bugs.chromium.org/p/chromium/issues/detail?id=368598 In that case, supporting those units for SVG animation will make sense. BUG=626297 ==========
Please take a look. Thanks https://codereview.chromium.org/2130753003/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/animation/SVGLengthInterpolationType.cpp (right): https://codereview.chromium.org/2130753003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/animation/SVGLengthInterpolationType.cpp:42: CSSPrimitiveValue::UnitType::ViewportMax On 2016/07/11 06:28:35, alancutter wrote: > Nit: Let's add a trailing comma here to be consistent with the code above. Done.
lgtm
The CQ bit was checked by shanmuga.m@samsung.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: linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by shanmuga.m@samsung.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: linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by shanmuga.m@samsung.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.
Description was changed from ========== Support viewport units for SVG Length animations Add support of viewport units such as vm,vw, vmin, vmax for animations on SVG Length. Chrome already supporting all viewport units for SVGLength. https://bugs.chromium.org/p/chromium/issues/detail?id=368598 In that case, supporting those units for SVG animation will make sense. BUG=626297 ========== to ========== Support viewport units for SVG Length animations Add support of viewport units such as vm,vw, vmin, vmax for animations on SVG Length. Chrome already supporting all viewport units for SVGLength. https://bugs.chromium.org/p/chromium/issues/detail?id=368598 In that case, supporting those units for SVG animation will make sense. BUG=626297 ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001)
Message was sent while issue was closed.
CQ bit was unchecked.
Message was sent while issue was closed.
Description was changed from ========== Support viewport units for SVG Length animations Add support of viewport units such as vm,vw, vmin, vmax for animations on SVG Length. Chrome already supporting all viewport units for SVGLength. https://bugs.chromium.org/p/chromium/issues/detail?id=368598 In that case, supporting those units for SVG animation will make sense. BUG=626297 ========== to ========== Support viewport units for SVG Length animations Add support of viewport units such as vm,vw, vmin, vmax for animations on SVG Length. Chrome already supporting all viewport units for SVGLength. https://bugs.chromium.org/p/chromium/issues/detail?id=368598 In that case, supporting those units for SVG animation will make sense. BUG=626297 Committed: https://crrev.com/c51f84657ddb3d5712da3c059883a9686a8c0acb Cr-Commit-Position: refs/heads/master@{#405038} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/c51f84657ddb3d5712da3c059883a9686a8c0acb Cr-Commit-Position: refs/heads/master@{#405038} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
