New issue
Advanced search Search tips

Issue 791086 link

Starred by 1 user
Status: Started
Owner:
smcgruer@chromium.org
Cc:
flackr@chromium.org
jbroman@chromium.org
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Task



Sign in to add a comment

Code cleanup: remove blink::IsNull, blink::NullValue

Project Member Reported by smcgruer@chromium.org, Dec 1 2017 
Currently the animations codebase uses std::numeric_limits<double>::quiet_NaN() to represent a 'null' or missing value for various doubles. It also defines top-level blink::IsNull and blink::NullValue functions to compare against or generate quiet_NaNs.

WTF::Optional is a thing, and makes the 'nullability' of members clear. We should use that thing, and get rid of blink::IsNull and blink::NullValue.

Comment 1 by ericwilligers@chromium.org, Dec 3 2017

Labels: Hotlist-CodeHealth
Project Member

Comment 2 by bugdroid1@chromium.org, Dec 6 2017 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/243148b03397982b3fd3a0baf30f3e2a5794d2ca

commit 243148b03397982b3fd3a0baf30f3e2a5794d2ca
Author: Stephen McGruer <smcgruer@chromium.org>
Date: Wed Dec 06 06:01:48 2017

Web Animations: use WTF::Optional for Keyframe::offset_

Previously nullable values in the animations code were tracked by
storing nulls as quiet_NaN() and using std::isnan as a null-detector.
It is much more explicit to store such values using WTF::Optional, which
forces code to consider whether or not the offset exists.

Bug: 791086
Change-Id: I4db0457b593d6be61dedb5c13a9daf8b671206b0
Reviewed-on: https://chromium-review.googlesource.com/804116
Reviewed-by: Eric Willigers <ericwilligers@chromium.org>
Reviewed-by: Andrey Kosyakov <caseq@chromium.org>
Commit-Queue: Stephen McGruer <smcgruer@chromium.org>
Cr-Commit-Position: refs/heads/master@{#522021}
[modify] https://crrev.com/243148b03397982b3fd3a0baf30f3e2a5794d2ca/third_party/WebKit/LayoutTests/external/wpt/web-animations/interfaces/Animatable/animate-expected.txt
[modify] https://crrev.com/243148b03397982b3fd3a0baf30f3e2a5794d2ca/third_party/WebKit/LayoutTests/external/wpt/web-animations/interfaces/KeyframeEffect/constructor-expected.txt
[modify] https://crrev.com/243148b03397982b3fd3a0baf30f3e2a5794d2ca/third_party/WebKit/Source/core/animation/CompositorAnimationsTest.cpp
[modify] https://crrev.com/243148b03397982b3fd3a0baf30f3e2a5794d2ca/third_party/WebKit/Source/core/animation/EffectInputTest.cpp
[modify] https://crrev.com/243148b03397982b3fd3a0baf30f3e2a5794d2ca/third_party/WebKit/Source/core/animation/Keyframe.cpp
[modify] https://crrev.com/243148b03397982b3fd3a0baf30f3e2a5794d2ca/third_party/WebKit/Source/core/animation/Keyframe.h
[modify] https://crrev.com/243148b03397982b3fd3a0baf30f3e2a5794d2ca/third_party/WebKit/Source/core/animation/KeyframeEffectModel.cpp
[modify] https://crrev.com/243148b03397982b3fd3a0baf30f3e2a5794d2ca/third_party/WebKit/Source/core/animation/KeyframeEffectTest.cpp
[modify] https://crrev.com/243148b03397982b3fd3a0baf30f3e2a5794d2ca/third_party/WebKit/Source/core/animation/TransitionKeyframe.cpp
[modify] https://crrev.com/243148b03397982b3fd3a0baf30f3e2a5794d2ca/third_party/WebKit/Source/core/animation/css/CSSAnimations.cpp
[modify] https://crrev.com/243148b03397982b3fd3a0baf30f3e2a5794d2ca/third_party/WebKit/Source/core/inspector/InspectorAnimationAgent.cpp
Project Member

Comment 3 by bugdroid1@chromium.org, Feb 6 2018 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/024cfdf6c02001a13e1dcb77eb102f4b19b8fdf7

commit 024cfdf6c02001a13e1dcb77eb102f4b19b8fdf7
Author: Stephen McGruer <smcgruer@chromium.org>
Date: Tue Feb 06 21:48:27 2018

Inline CompareOffsets and disallow null offsets

As pointed out by jbroman@ during a review, null offsets will violate
the requirements of std::sort. Luckily CompareOffsets is only ever used
where offsets will be non-null, so enforce that in the code. Also since
it is used exactly once, just inline it.

Bug: 791086
Change-Id: I9df7f1201fdc3024a727c62bfd7e917791de12b6
Reviewed-on: https://chromium-review.googlesource.com/905100
Commit-Queue: Stephen McGruer <smcgruer@chromium.org>
Reviewed-by: Jeremy Roman <jbroman@chromium.org>
Cr-Commit-Position: refs/heads/master@{#534804}
[modify] https://crrev.com/024cfdf6c02001a13e1dcb77eb102f4b19b8fdf7/third_party/WebKit/Source/core/animation/Keyframe.cpp
[modify] https://crrev.com/024cfdf6c02001a13e1dcb77eb102f4b19b8fdf7/third_party/WebKit/Source/core/animation/Keyframe.h
[modify] https://crrev.com/024cfdf6c02001a13e1dcb77eb102f4b19b8fdf7/third_party/WebKit/Source/core/animation/css/CSSAnimations.cpp
Project Member

Comment 4 by bugdroid1@chromium.org, Mar 1 2018 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/a026d7608daab7c107a4a3a474ab9ca78c0e9e20

commit a026d7608daab7c107a4a3a474ab9ca78c0e9e20
Author: Stephen McGruer <smcgruer@chromium.org>
Date: Thu Mar 01 18:16:16 2018

Web Animations: use WTF::Optional for Progress()

Previously nullable values in the animations code were tracked by
storing nulls as quiet_NaN() and using std::isnan as a null-detector.
It is much more explicit to store such values using WTF::Optional, which
forces code to consider whether or not the offset exists.

Bug: 791086
Change-Id: Iad5e53456c40859bb7e3322c0cb6152b01f376e2
Reviewed-on: https://chromium-review.googlesource.com/939748
Commit-Queue: Stephen McGruer <smcgruer@chromium.org>
Reviewed-by: Robert Flack <flackr@chromium.org>
Cr-Commit-Position: refs/heads/master@{#540212}
[modify] https://crrev.com/a026d7608daab7c107a4a3a474ab9ca78c0e9e20/third_party/WebKit/Source/core/animation/AnimationEffectReadOnly.cpp
[modify] https://crrev.com/a026d7608daab7c107a4a3a474ab9ca78c0e9e20/third_party/WebKit/Source/core/animation/AnimationEffectReadOnly.h
[modify] https://crrev.com/a026d7608daab7c107a4a3a474ab9ca78c0e9e20/third_party/WebKit/Source/core/animation/AnimationEffectReadOnlyTest.cpp
[modify] https://crrev.com/a026d7608daab7c107a4a3a474ab9ca78c0e9e20/third_party/WebKit/Source/core/animation/InertEffect.cpp
[modify] https://crrev.com/a026d7608daab7c107a4a3a474ab9ca78c0e9e20/third_party/WebKit/Source/core/animation/KeyframeEffectModel.cpp
[modify] https://crrev.com/a026d7608daab7c107a4a3a474ab9ca78c0e9e20/third_party/WebKit/Source/core/animation/KeyframeEffectReadOnly.cpp
[modify] https://crrev.com/a026d7608daab7c107a4a3a474ab9ca78c0e9e20/third_party/WebKit/Source/core/animation/TimingCalculations.h
[modify] https://crrev.com/a026d7608daab7c107a4a3a474ab9ca78c0e9e20/third_party/WebKit/Source/core/animation/TimingCalculationsTest.cpp
[modify] https://crrev.com/a026d7608daab7c107a4a3a474ab9ca78c0e9e20/third_party/WebKit/Source/core/animation/css/CSSAnimations.cpp
Project Member

Comment 6 by bugdroid1@chromium.org, Mar 7 2018 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/71d23d568f857be5095346c47bfc8fd3aea3448e

commit 71d23d568f857be5095346c47bfc8fd3aea3448e
Author: Stephen McGruer <smcgruer@chromium.org>
Date: Wed Mar 07 20:54:53 2018

Revert "Web Animations: use WTF::Optional for Animation::start_time_"

This reverts commit c838bf406fc8f97bd8c26a713f80e7290c1f39d3.

Reason for revert: virtual/threaded/animations/hit-testing/composited-with-hit-testing.html fails in Debug, flaky in Release

Builders failed on: 
- WebKit Linux Trusty (dbg): 
 https://build.chromium.org/p/chromium.webkit/builders/WebKit%20Linux%20Trusty%20%28dbg%29

- Win7 Tests (dbg)(1):   https://build.chromium.org/p/chromium.win/builders/Win7%20Tests%20%28dbg%29%281%29

Flakiness Dashboard:
https://test-results.appspot.com/dashboards/flakiness_dashboard.html#testType=webkit_layout_tests&tests=virtual%2Fthreaded%2Fanimations%2Fhit-testing%2Fcomposited-with-hit-testing.html

Bug:  819591 

Original change's description:
> Web Animations: use WTF::Optional for Animation::start_time_
> 
> Bug: 791086
> Change-Id: I916508fc5cf0c16543decd3f8197d034f2a4e6d8
> Reviewed-on: https://chromium-review.googlesource.com/941963
> Commit-Queue: Stephen McGruer <smcgruer@chromium.org>
> Reviewed-by: Robert Flack <flackr@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#541223}

TBR=flackr@chromium.org,smcgruer@chromium.org

Change-Id: I2094317fbf67d28bb5c32124a7d3fa62c7323a3b
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug: 791086
Reviewed-on: https://chromium-review.googlesource.com/953363
Reviewed-by: Stephen McGruer <smcgruer@chromium.org>
Commit-Queue: Stephen McGruer <smcgruer@chromium.org>
Cr-Commit-Position: refs/heads/master@{#541572}
[modify] https://crrev.com/71d23d568f857be5095346c47bfc8fd3aea3448e/third_party/WebKit/Source/core/animation/Animation.cpp
[modify] https://crrev.com/71d23d568f857be5095346c47bfc8fd3aea3448e/third_party/WebKit/Source/core/animation/Animation.h
[modify] https://crrev.com/71d23d568f857be5095346c47bfc8fd3aea3448e/third_party/WebKit/Source/core/animation/AnimationTest.cpp
[modify] https://crrev.com/71d23d568f857be5095346c47bfc8fd3aea3448e/third_party/WebKit/Source/core/animation/PendingAnimations.cpp
[modify] https://crrev.com/71d23d568f857be5095346c47bfc8fd3aea3448e/third_party/WebKit/Source/core/animation/css/CSSAnimations.cpp
[modify] https://crrev.com/71d23d568f857be5095346c47bfc8fd3aea3448e/third_party/WebKit/Source/core/inspector/InspectorAnimationAgent.cpp
Project Member

Comment 7 by bugdroid1@chromium.org, Mar 21 2018 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/59f06466fb092c256db7fbe4c392079ddf13e96a

commit 59f06466fb092c256db7fbe4c392079ddf13e96a
Author: Stephen McGruer <smcgruer@chromium.org>
Date: Wed Mar 21 13:25:28 2018

Reland "Web Animations: use WTF::Optional for Animation::start_time_"

This is a reland of c838bf406fc8f97bd8c26a713f80e7290c1f39d3

The original CL contained two subtle bugs from a comparison of
compositor_state_->start_time to start_time_. Previously if both were
'null' (represented by NaN), the comparison would still say they were
inequal because NaN != NaN. The original CL 'fixed' those comparisons,
but it turns out the behavior was intentional. This version of the CL
restores the intentional behavior, adds a test for it, and documents it.

Bug: 791086,  819591 
Change-Id: I6587d028e66c6f6a9213b60f8d5f51b1dac3f899
Reviewed-on: https://chromium-review.googlesource.com/956280
Reviewed-by: Robert Flack <flackr@chromium.org>
Commit-Queue: Stephen McGruer <smcgruer@chromium.org>
Cr-Commit-Position: refs/heads/master@{#544679}
[modify] https://crrev.com/59f06466fb092c256db7fbe4c392079ddf13e96a/third_party/WebKit/Source/core/animation/Animation.cpp
[modify] https://crrev.com/59f06466fb092c256db7fbe4c392079ddf13e96a/third_party/WebKit/Source/core/animation/Animation.h
[modify] https://crrev.com/59f06466fb092c256db7fbe4c392079ddf13e96a/third_party/WebKit/Source/core/animation/AnimationTest.cpp
[modify] https://crrev.com/59f06466fb092c256db7fbe4c392079ddf13e96a/third_party/WebKit/Source/core/animation/PendingAnimations.cpp
[modify] https://crrev.com/59f06466fb092c256db7fbe4c392079ddf13e96a/third_party/WebKit/Source/core/animation/css/CSSAnimations.cpp
[modify] https://crrev.com/59f06466fb092c256db7fbe4c392079ddf13e96a/third_party/WebKit/Source/core/inspector/InspectorAnimationAgent.cpp
Project Member

Comment 8 by bugdroid1@chromium.org, Mar 27 2018 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/5c6b234848895eaec120ba7046df89dc454ff485

commit 5c6b234848895eaec120ba7046df89dc454ff485
Author: Stephen McGruer <smcgruer@chromium.org>
Date: Tue Mar 27 17:10:11 2018

Web Animations: use WTF::Optional for StartAnimationOnCompositor's start_time

Previously nullable values in the animations code were tracked by
storing nulls as quiet_NaN() and using std::isnan as a null-detector.
It is much more explicit to store such values using WTF::Optional, which
forces code to consider whether or not the offset exists.

Bug: 791086
Change-Id: Ice80a1d3d686b0ff1f0bc2a1f3eac2eff51a8e38
Reviewed-on: https://chromium-review.googlesource.com/981076
Reviewed-by: Robert Flack <flackr@chromium.org>
Commit-Queue: Stephen McGruer <smcgruer@chromium.org>
Cr-Commit-Position: refs/heads/master@{#546145}
[modify] https://crrev.com/5c6b234848895eaec120ba7046df89dc454ff485/third_party/WebKit/Source/core/animation/Animation.cpp
[modify] https://crrev.com/5c6b234848895eaec120ba7046df89dc454ff485/third_party/WebKit/Source/core/animation/CompositorAnimations.cpp
[modify] https://crrev.com/5c6b234848895eaec120ba7046df89dc454ff485/third_party/WebKit/Source/core/animation/CompositorAnimations.h
[modify] https://crrev.com/5c6b234848895eaec120ba7046df89dc454ff485/third_party/WebKit/Source/core/animation/CompositorAnimationsTest.cpp
[modify] https://crrev.com/5c6b234848895eaec120ba7046df89dc454ff485/third_party/WebKit/Source/core/animation/KeyframeEffect.cpp
[modify] https://crrev.com/5c6b234848895eaec120ba7046df89dc454ff485/third_party/WebKit/Source/core/animation/KeyframeEffect.h
[modify] https://crrev.com/5c6b234848895eaec120ba7046df89dc454ff485/third_party/WebKit/Source/modules/animationworklet/WorkletAnimation.cpp
Project Member

Comment 9 by bugdroid1@chromium.org, Mar 27 2018 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/190a7873d0a9ba7a780c68a4fcd5f433abf5f9a9

commit 190a7873d0a9ba7a780c68a4fcd5f433abf5f9a9
Author: Stephen McGruer <smcgruer@chromium.org>
Date: Tue Mar 27 21:10:57 2018

Web Animations: Use WTF::Optional for hold_time_

Previously nullable values in the animations code were tracked by
storing nulls as quiet_NaN() and using std::isnan as a null-detector.
It is much more explicit to store such values using WTF::Optional, which
forces code to consider whether or not the offset exists.

This CL also combines held_ and hold_time_ into a single variable, since
held_ was used to indicate whether hold_time_ should (or could) be read
already - which is built into WTF::Optional.

Bug: 791086
Change-Id: I223cd07b5f6b9c15be172491512a4ab319138503
Reviewed-on: https://chromium-review.googlesource.com/979774
Commit-Queue: Stephen McGruer <smcgruer@chromium.org>
Reviewed-by: Robert Flack <flackr@chromium.org>
Cr-Commit-Position: refs/heads/master@{#546261}
[modify] https://crrev.com/190a7873d0a9ba7a780c68a4fcd5f433abf5f9a9/third_party/WebKit/Source/core/animation/Animation.cpp
[modify] https://crrev.com/190a7873d0a9ba7a780c68a4fcd5f433abf5f9a9/third_party/WebKit/Source/core/animation/Animation.h

Comment 10 by smcgruer@chromium.org, Apr 26 2018

Labels: -Type-Bug Type-Task
Project Member

Comment 11 by bugdroid1@chromium.org, Jul 13 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/01426da43a73cf03a4f932108ee44702c29a19ba

commit 01426da43a73cf03a4f932108ee44702c29a19ba
Author: Stephen McGruer <smcgruer@chromium.org>
Date: Fri Jul 13 13:56:28 2018

Remove unnecessary use of NullValue/IsNull in css_animations.cc

We can just directly use the base::Optional value here.

Bug: 791086
Change-Id: I3be79503ea2af4e68cbd751c1391ba651739fa34
Reviewed-on: https://chromium-review.googlesource.com/1135118
Reviewed-by: Robert Flack <flackr@chromium.org>
Commit-Queue: Stephen McGruer <smcgruer@chromium.org>
Cr-Commit-Position: refs/heads/master@{#574903}
[modify] https://crrev.com/01426da43a73cf03a4f932108ee44702c29a19ba/third_party/blink/renderer/core/animation/css/css_animations.cc
►

Sign in to add a comment