Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
68173013:
Web Animations CSS: More cleaning of new CompositorAnimation code. (Closed)
|
Created:
7 years, 1 month ago by mithro-old Modified:
7 years, 1 month ago CC:
blink-reviews, shans, rjwright, alancutter (OOO until 2018), Mike Lawther (Google), dstockwell, Timothy Loh, darktears, Steve Block, dino_apple.com, Eric Willigers Base URL:
https://chromium.googlesource.com/chromium/blink.git@master Visibility:
Public. |
DescriptionWeb Animations CSS: More cleaning of new CompositorAnimation code.
BUG=
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=162001
Patch Set 1 #
Total comments: 30
Patch Set 2 : Review updates. #
Total comments: 2
Patch Set 3 : Review fixes. #
Messages
Total messages: 11 (0 generated)
https://codereview.chromium.org/68173013/diff/1/Source/core/animation/Composi... File Source/core/animation/CompositorAnimations.cpp (right): https://codereview.chromium.org/68173013/diff/1/Source/core/animation/Composi... Source/core/animation/CompositorAnimations.cpp:323: out.scaledDuration = timing.iterationDuration / timing.playbackRate; I think we should just bail out if playbackRate is not 1. We can consider how best to implement this when we need it. https://codereview.chromium.org/68173013/diff/1/Source/core/animation/Composi... Source/core/animation/CompositorAnimations.cpp:329: int pastIterations = std::floor(std::abs(scaledStartDelay) / out.scaledDuration); It's not really clear to me what's going on here. I'd assumed after our earlier discussion that it was possible to just map a negative start delay to the time offset.
https://codereview.chromium.org/68173013/diff/1/Source/core/animation/Composi... File Source/core/animation/CompositorAnimations.cpp (right): https://codereview.chromium.org/68173013/diff/1/Source/core/animation/Composi... Source/core/animation/CompositorAnimations.cpp:323: out.scaledDuration = timing.iterationDuration / timing.playbackRate; On 2013/11/13 11:54:04, dstockwell wrote: > I think we should just bail out if playbackRate is not 1. We can consider how > best to implement this when we need it. Trivial to implement, but can remove it you want. https://codereview.chromium.org/68173013/diff/1/Source/core/animation/Composi... Source/core/animation/CompositorAnimations.cpp:329: int pastIterations = std::floor(std::abs(scaledStartDelay) / out.scaledDuration); On 2013/11/13 11:54:04, dstockwell wrote: > It's not really clear to me what's going on here. I'd assumed after our earlier > discussion that it was possible to just map a negative start delay to the time > offset. When start delay greater than iteration duration means you need to trim off the iterations which have already happened in the past. Then if you are alternating, you need to fix the reverse value.
https://codereview.chromium.org/68173013/diff/1/Source/core/animation/Composi... File Source/core/animation/CompositorAnimations.cpp (right): https://codereview.chromium.org/68173013/diff/1/Source/core/animation/Composi... Source/core/animation/CompositorAnimations.cpp:279: KeyframeAnimationEffect::KeyframeVector timingFrames; Yes, I think we should convert this function to take 'const KeyframeAnimationEffect::KeyframeVector*' and pass null here https://codereview.chromium.org/68173013/diff/1/Source/core/animation/Composi... Source/core/animation/CompositorAnimations.cpp:294: // FIXME: Shouldn't this be asserted in timing.assertValid? Good question. It should always be true right now, as CSS always produces a timing function. As for the general model case, I'm pretty sure it will remain true then too. So please fix this as part of this patch. We can always revisit later if required. https://codereview.chromium.org/68173013/diff/1/Source/core/animation/Composi... Source/core/animation/CompositorAnimations.cpp:304: if (timing.iterationStart != 0.0) if (timing.iterationStart) Ugly I know, but that's the style guide https://codereview.chromium.org/68173013/diff/1/Source/core/animation/Composi... Source/core/animation/CompositorAnimations.cpp:314: if (timing.playbackRate <= 0) Why doesn't this one get a FIXME? https://codereview.chromium.org/68173013/diff/1/Source/core/animation/Composi... Source/core/animation/CompositorAnimations.cpp:321: || timing.direction == Timing::PlaybackDirectionAlternateReverse); Move these down to line 334 where they're used, to avoid doing this if possible. https://codereview.chromium.org/68173013/diff/1/Source/core/animation/Composi... Source/core/animation/CompositorAnimations.cpp:338: ASSERT(out.adjustedIterationCount > 0); What if timing.iterationCount is zero? For example ... timing.iterationCount = 0 timing.iterationDuration = 1 timing.startDelay = 0 timing.playbackRate = 1 -> out.scaledDuration = 1 -> scaledStartDelay = 0 -> pastIterations = 0 -> out.adjustedIterationCount = 0 We'll hit this assert. I imagine we shouldn't composite if the iteration count is zero? https://codereview.chromium.org/68173013/diff/1/Source/core/animation/Composi... Source/core/animation/CompositorAnimations.cpp:461: ASSERT(timingValid); I think this will need to be an ASSERT_UNUSED for release builds.
https://codereview.chromium.org/68173013/diff/1/Source/core/animation/Composi... File Source/core/animation/CompositorAnimations.cpp (right): https://codereview.chromium.org/68173013/diff/1/Source/core/animation/Composi... Source/core/animation/CompositorAnimations.cpp:323: out.scaledDuration = timing.iterationDuration / timing.playbackRate; On 2013/11/13 12:01:43, mithro wrote: > On 2013/11/13 11:54:04, dstockwell wrote: > > I think we should just bail out if playbackRate is not 1. We can consider how > > best to implement this when we need it. > > Trivial to implement, but can remove it you want. Remove it. https://codereview.chromium.org/68173013/diff/1/Source/core/animation/Composi... Source/core/animation/CompositorAnimations.cpp:329: int pastIterations = std::floor(std::abs(scaledStartDelay) / out.scaledDuration); On 2013/11/13 12:01:43, mithro wrote: > On 2013/11/13 11:54:04, dstockwell wrote: > > It's not really clear to me what's going on here. I'd assumed after our > earlier > > discussion that it was possible to just map a negative start delay to the time > > offset. > > When start delay greater than iteration duration means you need to trim off the > iterations which have already happened in the past. > > Then if you are alternating, you need to fix the reverse value. OK. Then I think something like skippedIterations is better than pastIterations. https://codereview.chromium.org/68173013/diff/1/Source/core/animation/Composi... Source/core/animation/CompositorAnimations.cpp:338: ASSERT(out.adjustedIterationCount > 0); On 2013/11/13 23:17:01, Steve Block wrote: > What if timing.iterationCount is zero? > > For example ... > timing.iterationCount = 0 > timing.iterationDuration = 1 > timing.startDelay = 0 > timing.playbackRate = 1 > > -> out.scaledDuration = 1 > -> scaledStartDelay = 0 > -> pastIterations = 0 > -> out.adjustedIterationCount = 0 > > We'll hit this assert. I imagine we shouldn't composite if the iteration count > is zero? Right, bail out if iteration count is 0. https://codereview.chromium.org/68173013/diff/1/Source/core/animation/Composi... Source/core/animation/CompositorAnimations.cpp:465: timingFunction = CompositorAnimationsTimingFunctionReverser::reverse(timingFunction.get()); Hmm, why do we need to reverse the timing function? Couldn't we just flip the value of reverse?
https://codereview.chromium.org/68173013/diff/1/Source/core/animation/Composi... File Source/core/animation/CompositorAnimations.cpp (right): https://codereview.chromium.org/68173013/diff/1/Source/core/animation/Composi... Source/core/animation/CompositorAnimations.cpp:465: timingFunction = CompositorAnimationsTimingFunctionReverser::reverse(timingFunction.get()); On 2013/11/13 23:29:31, dstockwell wrote: > Hmm, why do we need to reverse the timing function? Couldn't we just flip the > value of reverse? Probably because reverse isn't passed through. Something seems odd given that the interface supports alternate.
https://codereview.chromium.org/68173013/diff/1/Source/core/animation/Composi... File Source/core/animation/CompositorAnimations.cpp (right): https://codereview.chromium.org/68173013/diff/1/Source/core/animation/Composi... Source/core/animation/CompositorAnimations.cpp:465: timingFunction = CompositorAnimationsTimingFunctionReverser::reverse(timingFunction.get()); That's my understanding too. The compositor interface needs help, but for now we should just wire it up to get things working. (This is why I was loath to add reversing logic to TimingFunction, as it's just a temporary band-aid)
https://codereview.chromium.org/68173013/diff/1/Source/core/animation/Composi... File Source/core/animation/CompositorAnimations.cpp (right): https://codereview.chromium.org/68173013/diff/1/Source/core/animation/Composi... Source/core/animation/CompositorAnimations.cpp:279: KeyframeAnimationEffect::KeyframeVector timingFrames; On 2013/11/13 23:17:01, Steve Block wrote: > Yes, I think we should convert this function to take 'const > KeyframeAnimationEffect::KeyframeVector*' and pass null here Done. https://codereview.chromium.org/68173013/diff/1/Source/core/animation/Composi... Source/core/animation/CompositorAnimations.cpp:294: // FIXME: Shouldn't this be asserted in timing.assertValid? On 2013/11/13 23:17:01, Steve Block wrote: > Good question. It should always be true right now, as CSS always produces a > timing function. As for the general model case, I'm pretty sure it will remain > true then too. So please fix this as part of this patch. We can always revisit > later if required. Done. https://codereview.chromium.org/68173013/diff/1/Source/core/animation/Composi... Source/core/animation/CompositorAnimations.cpp:304: if (timing.iterationStart != 0.0) On 2013/11/13 23:17:01, Steve Block wrote: > if (timing.iterationStart) > > Ugly I know, but that's the style guide Done. Normally the style checker picks this up, wonder why it didn't catch this one. https://codereview.chromium.org/68173013/diff/1/Source/core/animation/Composi... Source/core/animation/CompositorAnimations.cpp:314: if (timing.playbackRate <= 0) On 2013/11/13 23:17:01, Steve Block wrote: > Why doesn't this one get a FIXME? Does now! Changed to == 1.0 as Doug requested. https://codereview.chromium.org/68173013/diff/1/Source/core/animation/Composi... Source/core/animation/CompositorAnimations.cpp:321: || timing.direction == Timing::PlaybackDirectionAlternateReverse); On 2013/11/13 23:17:01, Steve Block wrote: > Move these down to line 334 where they're used, to avoid doing this if possible. Done. https://codereview.chromium.org/68173013/diff/1/Source/core/animation/Composi... Source/core/animation/CompositorAnimations.cpp:323: out.scaledDuration = timing.iterationDuration / timing.playbackRate; On 2013/11/13 23:29:31, dstockwell wrote: > On 2013/11/13 12:01:43, mithro wrote: > > On 2013/11/13 11:54:04, dstockwell wrote: > > > I think we should just bail out if playbackRate is not 1. We can consider > how > > > best to implement this when we need it. > > > > Trivial to implement, but can remove it you want. > > Remove it. Done. https://codereview.chromium.org/68173013/diff/1/Source/core/animation/Composi... Source/core/animation/CompositorAnimations.cpp:329: int pastIterations = std::floor(std::abs(scaledStartDelay) / out.scaledDuration); On 2013/11/13 23:29:31, dstockwell wrote: > On 2013/11/13 12:01:43, mithro wrote: > > On 2013/11/13 11:54:04, dstockwell wrote: > > > It's not really clear to me what's going on here. I'd assumed after our > > earlier > > > discussion that it was possible to just map a negative start delay to the > time > > > offset. > > > > When start delay greater than iteration duration means you need to trim off > the > > iterations which have already happened in the past. > > > > Then if you are alternating, you need to fix the reverse value. > > OK. Then I think something like skippedIterations is better than pastIterations. Done. https://codereview.chromium.org/68173013/diff/1/Source/core/animation/Composi... Source/core/animation/CompositorAnimations.cpp:338: ASSERT(out.adjustedIterationCount > 0); On 2013/11/13 23:17:01, Steve Block wrote: > What if timing.iterationCount is zero? > > For example ... > timing.iterationCount = 0 > timing.iterationDuration = 1 > timing.startDelay = 0 > timing.playbackRate = 1 > > -> out.scaledDuration = 1 > -> scaledStartDelay = 0 > -> skippedIterations = 0 > -> out.adjustedIterationCount = 0 > > We'll hit this assert. I imagine we shouldn't composite if the iteration count > is zero? Actually we'd bail at line 331, in the zero case skippedIterations == timing.iterationCount. if (skippedIterations >= timing.iterationCount) Also added a check for iterationCount == 0 at the finite/integer area because catching it on line 331 is a bit obscure. https://codereview.chromium.org/68173013/diff/1/Source/core/animation/Composi... Source/core/animation/CompositorAnimations.cpp:461: ASSERT(timingValid); On 2013/11/13 23:17:01, Steve Block wrote: > I think this will need to be an ASSERT_UNUSED for release builds. Done. https://codereview.chromium.org/68173013/diff/1/Source/core/animation/Composi... Source/core/animation/CompositorAnimations.cpp:465: timingFunction = CompositorAnimationsTimingFunctionReverser::reverse(timingFunction.get()); On 2013/11/13 23:56:23, Steve Block wrote: > That's my understanding too. The compositor interface needs help, but for now we > should just wire it up to get things working. (This is why I was loath to add > reversing logic to TimingFunction, as it's just a temporary band-aid) Correct. If we could just pass reverse, life would be much more peachy :). https://codereview.chromium.org/68173013/diff/1/Source/core/animation/Composi... Source/core/animation/CompositorAnimations.cpp:465: timingFunction = CompositorAnimationsTimingFunctionReverser::reverse(timingFunction.get()); On 2013/11/13 23:41:05, dstockwell wrote: > On 2013/11/13 23:29:31, dstockwell wrote: > > Hmm, why do we need to reverse the timing function? Couldn't we just flip the > > value of reverse? > > Probably because reverse isn't passed through. Something seems odd given that > the interface supports alternate. Correct. https://codereview.chromium.org/68173013/diff/1/Source/core/animation/Composi... Source/core/animation/CompositorAnimations.cpp:465: timingFunction = CompositorAnimationsTimingFunctionReverser::reverse(timingFunction.get()); On 2013/11/13 23:29:31, dstockwell wrote: > Hmm, why do we need to reverse the timing function? Couldn't we just flip the > value of reverse? Because it's not passed to the compositor. To simulate reverse, we reverse the timing functions and flip the locations of the keyframes (see the tests if you want to look into it further).
lgtm https://codereview.chromium.org/68173013/diff/1/Source/core/animation/Composi... File Source/core/animation/CompositorAnimations.cpp (right): https://codereview.chromium.org/68173013/diff/1/Source/core/animation/Composi... Source/core/animation/CompositorAnimations.cpp:338: ASSERT(out.adjustedIterationCount > 0); > Also added a check for iterationCount == 0 at the finite/integer area because > catching it on line 331 is a bit obscure. Sounds good. This will be required once we allow positive start delay, as I could use a positive start delay to avoid the check on line 331 catching this. https://codereview.chromium.org/68173013/diff/100001/Source/core/animation/Ti... File Source/core/animation/Timing.h (right): https://codereview.chromium.org/68173013/diff/100001/Source/core/animation/Ti... Source/core/animation/Timing.h:76: ASSERT(timingFunction); It's possible that some tests don't set the timingFunction. You should run them to check.
lgtm https://codereview.chromium.org/68173013/diff/100001/Source/core/animation/Co... File Source/core/animation/CompositorAnimations.cpp (right): https://codereview.chromium.org/68173013/diff/100001/Source/core/animation/Co... Source/core/animation/CompositorAnimations.cpp:304: // FIXME: Compositor only supports finite, non-zero, integer iteration nit, it would support 0 but there's no point.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mithro@mithis.com/68173013/240001
Message was sent while issue was closed.
Change committed as 162001 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
