|
|
Created:
3 years, 10 months ago by alancutter (OOO until 2018) Modified:
3 years, 10 months ago Reviewers:
suzyh_UTC10 (ex-contributor) CC:
darktears, blink-reviews, blink-reviews-animation_chromium.org, chromium-reviews, Eric Willigers, rjwright, shans Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionReduce strictness of length animation DCHECK
This patch reduces the strictness of the check in length animations that
the applied value matches what the StyleBuilder would apply for the
equivalent CSSValue. In the case of 0% values there are negligible
differences in internal representation.
With this patch the code no longer asserts on Length type, instead it
checks a more general isSpecified() property that distinguishes
keyword lengths from pixel and percentage lengths.
BUG=691182
Review-Url: https://codereview.chromium.org/2697743002
Cr-Commit-Position: refs/heads/master@{#451198}
Committed: https://chromium.googlesource.com/chromium/src/+/0918ae11092a7ee1b278cc1a55259e7d886f156d
Patch Set 1 #
Total comments: 4
Patch Set 2 : reviewChanges #
Total comments: 2
Patch Set 3 : _dcheckLengthApply #
Messages
Total messages: 29 (15 generated)
alancutter@chromium.org changed reviewers: + suzyh@chromium.org
Description was changed from ========== Reduce strictness of length animation DCHECK This patch reduces the strictness of the check in length animations that the applied value matches what the StyleBuilder would apply for the equivalent CSSValue. In the case of 0% values there are negligible differences in internal representation. This patch no longer asserts on Length type, instead it checks a more general isSpecified() property that distinguishes keyword lengths from pixel and percentage lengths. BUG=691182 ========== to ========== Reduce strictness of length animation DCHECK This patch reduces the strictness of the check in length animations that the applied value matches what the StyleBuilder would apply for the equivalent CSSValue. In the case of 0% values there are negligible differences in internal representation. With this patch the code no longer asserts on Length type, instead it checks a more general isSpecified() property that distinguishes keyword lengths from pixel and percentage lengths. BUG=691182 ==========
https://codereview.chromium.org/2697743002/diff/1/third_party/WebKit/LayoutTe... File third_party/WebKit/LayoutTests/animations/length-zero-percent-crash.html (right): https://codereview.chromium.org/2697743002/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/animations/length-zero-percent-crash.html:8: }, 'Don\'t crash when animating length properties to 0%'); Nit: I prefer "Don't ..." to 'Don\'t ...', unless there's a style guide rule I've forgotten. https://codereview.chromium.org/2697743002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/animation/CSSLengthInterpolationType.cpp (right): https://codereview.chromium.org/2697743002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/animation/CSSLengthInterpolationType.cpp:161: DCHECK_EQ(before.isSpecified(), after.isSpecified()); I'm not very familiar with LengthType. I think this makes sense for the case where both before and after are specified, but I'm curious whether it also makes sense if before.isSpecified() and after.isSpecified() are both false. Should the type equality check still apply in that case?
https://codereview.chromium.org/2697743002/diff/1/third_party/WebKit/LayoutTe... File third_party/WebKit/LayoutTests/animations/length-zero-percent-crash.html (right): https://codereview.chromium.org/2697743002/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/animations/length-zero-percent-crash.html:8: }, 'Don\'t crash when animating length properties to 0%'); On 2017/02/14 at 04:38:51, suzyh wrote: > Nit: I prefer "Don't ..." to 'Don\'t ...', unless there's a style guide rule I've forgotten. Done. https://codereview.chromium.org/2697743002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/animation/CSSLengthInterpolationType.cpp (right): https://codereview.chromium.org/2697743002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/animation/CSSLengthInterpolationType.cpp:161: DCHECK_EQ(before.isSpecified(), after.isSpecified()); On 2017/02/14 at 04:38:51, suzyh wrote: > I'm not very familiar with LengthType. I think this makes sense for the case where both before and after are specified, but I'm curious whether it also makes sense if before.isSpecified() and after.isSpecified() are both false. Should the type equality check still apply in that case? Good point, added type check back for that case.
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...
The CQ bit was unchecked by commit-bot@chromium.org
No L-G-T-M from a valid reviewer yet. CQ run can only be started by full committers or once the patch has received an L-G-T-M from a full committer. Even if an L-G-T-M may have been provided, it was from a non-committer, _not_ a full super star committer. Committers are members of the group "project-chromium-committers". Note that this has nothing to do with OWNERS files.
https://codereview.chromium.org/2697743002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/animation/CSSLengthInterpolationType.cpp (right): https://codereview.chromium.org/2697743002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/animation/CSSLengthInterpolationType.cpp:168: DCHECK_EQ(before.type, after.type()); Can we add more tests around this? I'm concerned this code is not getting enough test coverage.
The CQ bit was checked by alancutter@chromium.org to run a CQ dry run
https://codereview.chromium.org/2697743002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/animation/CSSLengthInterpolationType.cpp (right): https://codereview.chromium.org/2697743002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/animation/CSSLengthInterpolationType.cpp:168: DCHECK_EQ(before.type, after.type()); On 2017/02/14 at 05:40:49, suzyh wrote: > Can we add more tests around this? I'm concerned this code is not getting enough test coverage. Turns out the else case should be unreachable.
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.
lgtm
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...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
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...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: cast_shell_linux on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
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": 1487289961580450, "parent_rev": "ecd5c343a257212e661afd2b7980e756c55fd6a0", "commit_rev": "0918ae11092a7ee1b278cc1a55259e7d886f156d"}
Message was sent while issue was closed.
Description was changed from ========== Reduce strictness of length animation DCHECK This patch reduces the strictness of the check in length animations that the applied value matches what the StyleBuilder would apply for the equivalent CSSValue. In the case of 0% values there are negligible differences in internal representation. With this patch the code no longer asserts on Length type, instead it checks a more general isSpecified() property that distinguishes keyword lengths from pixel and percentage lengths. BUG=691182 ========== to ========== Reduce strictness of length animation DCHECK This patch reduces the strictness of the check in length animations that the applied value matches what the StyleBuilder would apply for the equivalent CSSValue. In the case of 0% values there are negligible differences in internal representation. With this patch the code no longer asserts on Length type, instead it checks a more general isSpecified() property that distinguishes keyword lengths from pixel and percentage lengths. BUG=691182 Review-Url: https://codereview.chromium.org/2697743002 Cr-Commit-Position: refs/heads/master@{#451198} Committed: https://chromium.googlesource.com/chromium/src/+/0918ae11092a7ee1b278cc1a5525... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://chromium.googlesource.com/chromium/src/+/0918ae11092a7ee1b278cc1a5525... |