Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(951)

Issue 2697743002: Reduce strictness of length animation DCHECK (Closed)

Created:
3 years, 10 months ago by alancutter (OOO until 2018)
Modified:
3 years, 10 months ago
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.

Description

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/+/0918ae11092a7ee1b278cc1a55259e7d886f156d

Patch Set 1 #

Total comments: 4

Patch Set 2 : reviewChanges #

Total comments: 2

Patch Set 3 : _dcheckLengthApply #

Unified diffs Side-by-side diffs Delta from patch set Stats (+15 lines, -7 lines) Patch
A third_party/WebKit/LayoutTests/animations/length-zero-percent-crash.html View 1 1 chunk +9 lines, -0 lines 0 comments Download
M third_party/WebKit/Source/core/animation/CSSLengthInterpolationType.cpp View 1 2 1 chunk +6 lines, -7 lines 0 comments Download

Messages

Total messages: 29 (15 generated)
alancutter (OOO until 2018)
3 years, 10 months ago (2017-02-14 02:11:36 UTC) #2
suzyh_UTC10 (ex-contributor)
https://codereview.chromium.org/2697743002/diff/1/third_party/WebKit/LayoutTests/animations/length-zero-percent-crash.html File third_party/WebKit/LayoutTests/animations/length-zero-percent-crash.html (right): https://codereview.chromium.org/2697743002/diff/1/third_party/WebKit/LayoutTests/animations/length-zero-percent-crash.html#newcode8 third_party/WebKit/LayoutTests/animations/length-zero-percent-crash.html:8: }, 'Don\'t crash when animating length properties to 0%'); ...
3 years, 10 months ago (2017-02-14 04:38:51 UTC) #4
alancutter (OOO until 2018)
https://codereview.chromium.org/2697743002/diff/1/third_party/WebKit/LayoutTests/animations/length-zero-percent-crash.html File third_party/WebKit/LayoutTests/animations/length-zero-percent-crash.html (right): https://codereview.chromium.org/2697743002/diff/1/third_party/WebKit/LayoutTests/animations/length-zero-percent-crash.html#newcode8 third_party/WebKit/LayoutTests/animations/length-zero-percent-crash.html:8: }, 'Don\'t crash when animating length properties to 0%'); ...
3 years, 10 months ago (2017-02-14 04:46:12 UTC) #5
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2697743002/20001
3 years, 10 months ago (2017-02-14 04:46:43 UTC) #7
commit-bot: I haz the power
No L-G-T-M from a valid reviewer yet. CQ run can only be started by full ...
3 years, 10 months ago (2017-02-14 04:46:45 UTC) #9
suzyh_UTC10 (ex-contributor)
https://codereview.chromium.org/2697743002/diff/20001/third_party/WebKit/Source/core/animation/CSSLengthInterpolationType.cpp File third_party/WebKit/Source/core/animation/CSSLengthInterpolationType.cpp (right): https://codereview.chromium.org/2697743002/diff/20001/third_party/WebKit/Source/core/animation/CSSLengthInterpolationType.cpp#newcode168 third_party/WebKit/Source/core/animation/CSSLengthInterpolationType.cpp:168: DCHECK_EQ(before.type, after.type()); Can we add more tests around this? ...
3 years, 10 months ago (2017-02-14 05:40:49 UTC) #10
alancutter (OOO until 2018)
https://codereview.chromium.org/2697743002/diff/20001/third_party/WebKit/Source/core/animation/CSSLengthInterpolationType.cpp File third_party/WebKit/Source/core/animation/CSSLengthInterpolationType.cpp (right): https://codereview.chromium.org/2697743002/diff/20001/third_party/WebKit/Source/core/animation/CSSLengthInterpolationType.cpp#newcode168 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: > ...
3 years, 10 months ago (2017-02-15 01:52:26 UTC) #12
suzyh_UTC10 (ex-contributor)
lgtm
3 years, 10 months ago (2017-02-15 22:42:44 UTC) #16
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2697743002/40001
3 years, 10 months ago (2017-02-16 01:05:44 UTC) #18
commit-bot: I haz the power
Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
3 years, 10 months ago (2017-02-16 03:09:35 UTC) #20
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2697743002/40001
3 years, 10 months ago (2017-02-16 03:12:41 UTC) #22
commit-bot: I haz the power
Try jobs failed on following builders: cast_shell_linux on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
3 years, 10 months ago (2017-02-16 04:58:34 UTC) #24
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2697743002/40001
3 years, 10 months ago (2017-02-17 00:07:41 UTC) #26
commit-bot: I haz the power
3 years, 10 months ago (2017-02-17 02:49:14 UTC) #29
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/chromium/src/+/0918ae11092a7ee1b278cc1a5525...

Powered by Google App Engine
This is Rietveld 408576698