Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
832873002:
Animation: Remove ASSERT_NOT_REACHED from LengthStyleInterpolation (Closed)
|
Created:
5 years, 11 months ago by evemj (not active) Modified:
5 years, 11 months ago CC:
darktears, blink-reviews, blink-reviews-animation_chromium.org, Eric Willigers, Mike Lawther (Google), rjwright, shans, Steve Block, Timothy Loh Base URL:
https://chromium.googlesource.com/chromium/blink.git@master Target Ref:
refs/heads/master Project:
blink Visibility:
Public. |
DescriptionAnimation: Remove ASSERT_NOT_REACHED from LengthStyleInterpolation
When interpolated, very large floating point numbers could be incorrectly evaluated to 0 causing an assert to be hit. This is an edge case so for now the assert has been removed and 0px is being returned instead. This needs to be fixed once we have multiple interpolators.
BUG=445676, 445684, 446640, 445845, 445469
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=188013
Patch Set 1 #
Total comments: 6
Patch Set 2 : Leave ASSERT_NOT_REACHED in LengthStyleInterpolation.cpp #Patch Set 3 : Add return 0px after ASSERT_NOT_REACHED() #
Total comments: 4
Patch Set 4 : Remove change to interpolableValue and add a TODO #Messages
Total messages: 18 (4 generated)
lgtm
lgtm
https://codereview.chromium.org/832873002/diff/1/Source/core/animation/Length... File Source/core/animation/LengthStyleInterpolation.cpp (left): https://codereview.chromium.org/832873002/diff/1/Source/core/animation/Length... Source/core/animation/LengthStyleInterpolation.cpp:98: ASSERT_NOT_REACHED(); Shouldn't we leave the ASSERT_NOT_REACHED here? https://codereview.chromium.org/832873002/diff/1/Source/core/animation/Length... Source/core/animation/LengthStyleInterpolation.cpp:98: ASSERT_NOT_REACHED(); Shouldn't we leave the ASSERT_NOT_REACHED here?
Patchset #2 (id:20001) has been deleted
https://codereview.chromium.org/832873002/diff/1/Source/core/animation/Length... File Source/core/animation/LengthStyleInterpolation.cpp (left): https://codereview.chromium.org/832873002/diff/1/Source/core/animation/Length... Source/core/animation/LengthStyleInterpolation.cpp:98: ASSERT_NOT_REACHED(); On 2015/01/04 23:54:16, dstockwell wrote: > Shouldn't we leave the ASSERT_NOT_REACHED here? Done. https://codereview.chromium.org/832873002/diff/1/Source/core/animation/Length... Source/core/animation/LengthStyleInterpolation.cpp:98: ASSERT_NOT_REACHED(); On 2015/01/04 23:54:16, dstockwell wrote: > Shouldn't we leave the ASSERT_NOT_REACHED here? Done.
https://codereview.chromium.org/832873002/diff/1/Source/core/animation/Length... File Source/core/animation/LengthStyleInterpolation.cpp (left): https://codereview.chromium.org/832873002/diff/1/Source/core/animation/Length... Source/core/animation/LengthStyleInterpolation.cpp:98: ASSERT_NOT_REACHED(); On 2015/01/05 02:28:47, evem1 wrote: > On 2015/01/04 23:54:16, dstockwell wrote: > > Shouldn't we leave the ASSERT_NOT_REACHED here? > > Done. Doug may have intended the following:- ASSERT_NOT_REACHED(); return CSSPrimitiveValue::create(0, CSSPrimitiveValue::CSS_PX); so release builds fail safely.
On 2015/01/05 at 02:32:22, ericwilligers wrote: > https://codereview.chromium.org/832873002/diff/1/Source/core/animation/Length... > File Source/core/animation/LengthStyleInterpolation.cpp (left): > > https://codereview.chromium.org/832873002/diff/1/Source/core/animation/Length... > Source/core/animation/LengthStyleInterpolation.cpp:98: ASSERT_NOT_REACHED(); > On 2015/01/05 02:28:47, evem1 wrote: > > On 2015/01/04 23:54:16, dstockwell wrote: > > > Shouldn't we leave the ASSERT_NOT_REACHED here? > > > > Done. > > Doug may have intended the following:- > > ASSERT_NOT_REACHED(); > return CSSPrimitiveValue::create(0, CSSPrimitiveValue::CSS_PX); > > so release builds fail safely. Yes.
https://codereview.chromium.org/832873002/diff/1/Source/core/animation/Length... File Source/core/animation/LengthStyleInterpolation.cpp (left): https://codereview.chromium.org/832873002/diff/1/Source/core/animation/Length... Source/core/animation/LengthStyleInterpolation.cpp:98: ASSERT_NOT_REACHED(); On 2015/01/05 02:32:22, Eric Willigers wrote: > On 2015/01/05 02:28:47, evem1 wrote: > > On 2015/01/04 23:54:16, dstockwell wrote: > > > Shouldn't we leave the ASSERT_NOT_REACHED here? > > > > Done. > > Doug may have intended the following:- > > ASSERT_NOT_REACHED(); > return CSSPrimitiveValue::create(0, CSSPrimitiveValue::CSS_PX); > > so release builds fail safely. Done.
lgtm
shans@chromium.org changed reviewers: + shans@chromium.org
https://codereview.chromium.org/832873002/diff/60001/Source/core/animation/In... File Source/core/animation/InterpolableValue.cpp (right): https://codereview.chromium.org/832873002/diff/60001/Source/core/animation/In... Source/core/animation/InterpolableValue.cpp:22: resultNumber.m_value = m_value + (toNumber.m_value - m_value) * progress; Don't do this - you'll cause interpolation from large negative values to large positive values to produce incorrect results. https://codereview.chromium.org/832873002/diff/60001/Source/core/animation/Le... File Source/core/animation/LengthStyleInterpolation.cpp (left): https://codereview.chromium.org/832873002/diff/60001/Source/core/animation/Le... Source/core/animation/LengthStyleInterpolation.cpp:98: ASSERT_NOT_REACHED(); Instead remove this assert and add a TODO to fix the issue properly once we have multiple interpolators.
https://codereview.chromium.org/832873002/diff/60001/Source/core/animation/In... File Source/core/animation/InterpolableValue.cpp (right): https://codereview.chromium.org/832873002/diff/60001/Source/core/animation/In... Source/core/animation/InterpolableValue.cpp:22: resultNumber.m_value = m_value + (toNumber.m_value - m_value) * progress; On 2015/01/07 23:07:49, shans wrote: > Don't do this - you'll cause interpolation from large negative values to large > positive values to produce incorrect results. Done. https://codereview.chromium.org/832873002/diff/60001/Source/core/animation/Le... File Source/core/animation/LengthStyleInterpolation.cpp (left): https://codereview.chromium.org/832873002/diff/60001/Source/core/animation/Le... Source/core/animation/LengthStyleInterpolation.cpp:98: ASSERT_NOT_REACHED(); On 2015/01/07 23:07:49, shans wrote: > Instead remove this assert and add a TODO to fix the issue properly once we have > multiple interpolators. Done.
lgtm
The CQ bit was checked by evem@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/832873002/80001
Message was sent while issue was closed.
Committed patchset #4 (id:80001) as https://src.chromium.org/viewvc/blink?view=rev&revision=188013 |
