Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
96283002:
Web Animations API: Start implementation of Element.animate(). (Closed)
|
Created:
7 years ago by rjwright Modified:
7 years ago CC:
blink-reviews, alancutter (OOO until 2018), Mike Lawther (Google), dglazkov+blink, Timothy Loh, apavlov+blink_chromium.org, Inactive, darktears, arv+blink, dino_apple.com, watchdog-blink-watchlist_google.com, Eric Willigers Base URL:
https://chromium.googlesource.com/chromium/blink.git@master Visibility:
Public. |
DescriptionWeb Animations API: Start implementation of Element.animate().
This is the first in a series of patches to implement the Web Animations API (see http://dev.w3.org/fxtf/web-animations/).
Please note that this work is being completed behind a runtime flag.
While the Web Animations model is now used to support CSS Animations and Transitions, there are some important differences between that use and the API:
* CSS Animations and Transitions operate on computed style, and are inherently resolution-dependent mechanisms. Web Animations is style resolution independent.
* CSS Animations and Transitions accept the use of -webkit prefixed properties. Following the principle of deprecating prefixing for new APIs, the Web Animations JS API does not. Note that this *may* require us to gate release of this feature on unprefixing of -webkit-transform.
As an initial patch, this code takes some shortcuts that will eventually be replaced. Most notably, we temporarily make use of StyleResolver for the purpose of parsing CSS Strings into usable values. This will be switched out in the future in favor of an approach that doesn't resolve styles at the same time.
BUG=327564
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=163761
Patch Set 1 #
Total comments: 56
Patch Set 2 : #
Total comments: 19
Patch Set 3 : Remove explicit calls to StyleResolverState into StyleResolver. #Patch Set 4 : Convert camelCase property names into CSSPropertyIDs in the CSSParser #
Total comments: 4
Patch Set 5 : Small changes to StyleResolver::setKeyframePropertyValues. #Patch Set 6 : Add layout test for animate() #
Total comments: 29
Patch Set 7 : Move property name parsing. Return KAE from setKeyframePropertyValues. Improve test. #
Total comments: 2
Patch Set 8 : Added unit test (WIP) #
Total comments: 28
Patch Set 9 : Move web-animations-api layout tests into LayoutTest. #
Total comments: 20
Patch Set 10 : Added test for camelCaseCSSPropertyNameToID. Improved layout and unit tests. #Patch Set 11 : Change first param type of StyleResolver::createKeyframeAnimationEffect to Element& #Patch Set 12 : Added test to check that animate() does not leak into stable. #
Total comments: 2
Patch Set 13 : Change description of LayoutTests/webexposed/element-animate.html. #Patch Set 14 : #Patch Set 15 : Fix broken unit test #
Total comments: 1
Patch Set 16 : Cleaned up unit test and made startAnimation private. #Patch Set 17 : Fix nits #Messages
Total messages: 53 (0 generated)
I just skimmed quickly, here are some high level comments: https://codereview.chromium.org/96283002/diff/1/Source/core/animation/Animate... File Source/core/animation/AnimatedElement.h (right): https://codereview.chromium.org/96283002/diff/1/Source/core/animation/Animate... Source/core/animation/AnimatedElement.h:31: #ifndef AnimatedElement_h Upload with --no-find-copies https://codereview.chromium.org/96283002/diff/1/Source/core/animation/Animate... File Source/core/animation/AnimatedElement.idl (right): https://codereview.chromium.org/96283002/diff/1/Source/core/animation/Animate... Source/core/animation/AnimatedElement.idl:30: partial interface Element { This should probably be named ElementAnimation, most other partial interfaces I could find are named that way. eg. WindowNotifications, DocumentFullscreen, WindowMediaSource https://codereview.chromium.org/96283002/diff/1/Source/core/animation/Animate... Source/core/animation/AnimatedElement.idl:31: void animate(sequence<Dictionary> keyframes); Shouldn't this be runtime enabled? [RuntimeEnabled=WebAnimationsAPI]
Exciting stuff! We should't land this without some tests. Also did you look into a test to avoid 'leaking' this prematurely? > The flag is webAnimationsAPI. Nit. That's the RuntimeEnabledFeature. I don't think the chrome flag is hooked up yet, right? That will be --enable-web-animations-api https://codereview.chromium.org/96283002/diff/1/Source/core/animation/Animate... File Source/core/animation/AnimatedElement.cpp (right): https://codereview.chromium.org/96283002/diff/1/Source/core/animation/Animate... Source/core/animation/AnimatedElement.cpp:32: NO blank line here https://codereview.chromium.org/96283002/diff/1/Source/core/animation/Animate... Source/core/animation/AnimatedElement.cpp:52: static bool isUpper(UChar ch) { return isASCIIUpper(ch); } Why do you need this wrapper function? https://codereview.chromium.org/96283002/diff/1/Source/core/animation/Animate... Source/core/animation/AnimatedElement.cpp:54: static CSSPropertyID v8PropertyToCSSPropertyID(String propertyName) I'm surprised we don't have this logic elsewhere for the CSSOM but I imagine you already checked this? Why 'V8' property? This doesn't seem to be specific to V8. Maybe camelCaseCSSPropertyNameToID()? We should add a unit test for this function. https://codereview.chromium.org/96283002/diff/1/Source/core/animation/Animate... Source/core/animation/AnimatedElement.cpp:60: if (end == kNotFound) { Is this simpler ... size_t end; while (end = propertyName.find(isUpper, position) != kNotFound) { builder.append(propertyName.substring(position, end) + "-" + toASCIILower(propertyName[end])); position = end + 1; } builder.append(propertyName.substring(position)); https://codereview.chromium.org/96283002/diff/1/Source/core/animation/Animate... Source/core/animation/AnimatedElement.cpp:72: void AnimatedElement::animate(Element* element, Vector<Dictionary> dictionaryKeyframesVector) s/dictionaryKeyframesVector/keyframeDictionaryVector to match the spec terminology? https://codereview.chromium.org/96283002/diff/1/Source/core/animation/Animate... Source/core/animation/AnimatedElement.cpp:83: if (!keyframeProperties.size()) This is incorrect. The spec requires that animation.effect.getFrames() should return the original list of keyframes, as specified. This will be relevant once we hook up element.animate() to return a Player/TimedItem. If there's a particular reason for not handling empty keyframes now, add a FIXME and perhaps a RELEASE_ASSERT_WITH_MESSAGE(). https://codereview.chromium.org/96283002/diff/1/Source/core/animation/Animate... Source/core/animation/AnimatedElement.cpp:86: keyframes.append(Keyframe::create()); Probably best to build up the keyframe before appending it to keyframes, to save repeated calls to keyframes.last(). https://codereview.chromium.org/96283002/diff/1/Source/core/animation/Animate... Source/core/animation/AnimatedElement.cpp:92: keyframes.last()->setOffset(std::numeric_limits<double>::quiet_NaN()); No need for this, it's the default offset for a keyframe. In any case, you should use nullValue(). https://codereview.chromium.org/96283002/diff/1/Source/core/animation/Animate... Source/core/animation/AnimatedElement.cpp:99: keyframes.last()->setComposite(AnimationEffect::CompositeReplace); Again, keyframes use replace composition by default. https://codereview.chromium.org/96283002/diff/1/Source/core/animation/Animate... Source/core/animation/AnimatedElement.cpp:107: if (!id || !CSSAnimations::isAnimatableProperty(id)) Isn't there a CSSPropertyInvalid we can use here? https://codereview.chromium.org/96283002/diff/1/Source/core/animation/Animate... Source/core/animation/AnimatedElement.cpp:108: continue; Again, we shouldn't be skipping invalid keyframes as this stage, so that animation.effect.getFrames() returns the original set of keyframes. This logic should go in KeyframeAnimationEffect::normalizedKeyframes() (which matches the algorithm described in the spec). https://codereview.chromium.org/96283002/diff/1/Source/core/animation/Animate... Source/core/animation/AnimatedElement.cpp:110: continue; Same as above https://codereview.chromium.org/96283002/diff/1/Source/core/animation/Animate... Source/core/animation/AnimatedElement.cpp:118: if (!keyframes.last()->properties().size() && isnan(keyframes.last()->offset())) Same comment about getFrames(). Also, use isEmpty() rather than !size() https://codereview.chromium.org/96283002/diff/1/Source/core/animation/Animate... Source/core/animation/AnimatedElement.cpp:128: timing.iterationCount = 1; Superfluous. The default iterationCount is 1. https://codereview.chromium.org/96283002/diff/1/Source/core/animation/Animate... Source/core/animation/AnimatedElement.cpp:132: DocumentTimeline* timeline = element->document().timeline(); ASSERT(timeline) https://codereview.chromium.org/96283002/diff/1/Source/core/animation/Animate... Source/core/animation/AnimatedElement.cpp:134: // only one keyframe) this crashes. This shouldn't be the case. KeyframeAnimationEffect should handle synthetic keyframes for any distribution of properties in keyframes. If you can send me a repro case (maybe an addition to KeyframeAnimationEffectTest), I'll take a look. https://codereview.chromium.org/96283002/diff/1/Source/core/animation/Animate... Source/core/animation/AnimatedElement.cpp:139: // FIXME: If there are more than two keyframes and any of those keyframes doesn't have a specified It's not clear which of these you plan to fix before landing vs after landing this patch. Uploading a work in progress is fine, but it's good to make this clear. FIXME is used for TODOs you plan to fix after landing, so maybe use FIXME_BEFORE_LANDING otherwise. https://codereview.chromium.org/96283002/diff/1/Source/core/animation/Animate... Source/core/animation/AnimatedElement.cpp:141: // Handling for this case not yet implemented. This will be handled by KeyframeAnimationEffect. See the FIXME in KeyframeAnimationEffect::normalizedKeyframes(). The reason I have't fixed this yet is that the spec behaviour is still being debated. We should probably just RELEASE_ASSERT_WITH_MESSAGE() if _any_ keyframes are missing offsets for now. https://codereview.chromium.org/96283002/diff/1/Source/core/animation/Animate... File Source/core/animation/AnimatedElement.h (right): https://codereview.chromium.org/96283002/diff/1/Source/core/animation/Animate... Source/core/animation/AnimatedElement.h:35: #include "core/css/CSSParser.h" Is this needed? https://codereview.chromium.org/96283002/diff/1/Source/core/animation/Animate... Source/core/animation/AnimatedElement.h:42: static void animate(Element*, Vector<Dictionary>); Should name the second param, as it's not obvious from the type https://codereview.chromium.org/96283002/diff/1/Source/core/animation/Animate... Source/core/animation/AnimatedElement.h:47: #endif // AnimatedElement_h https://codereview.chromium.org/96283002/diff/1/Source/core/animation/Animate... File Source/core/animation/AnimatedElement.idl (right): https://codereview.chromium.org/96283002/diff/1/Source/core/animation/Animate... Source/core/animation/AnimatedElement.idl:29: */ Blank line https://codereview.chromium.org/96283002/diff/1/Source/core/animation/Animate... Source/core/animation/AnimatedElement.idl:32: }; upload with --no-find-copies
https://codereview.chromium.org/96283002/diff/1/Source/core/animation/Animate... File Source/core/animation/AnimatedElement.cpp (right): https://codereview.chromium.org/96283002/diff/1/Source/core/animation/Animate... Source/core/animation/AnimatedElement.cpp:54: static CSSPropertyID v8PropertyToCSSPropertyID(String propertyName) On 2013/11/29 07:31:19, Steve Block wrote: > I'm surprised we don't have this logic elsewhere for the CSSOM but I imagine you > already checked this? > > Why 'V8' property? This doesn't seem to be specific to V8. Maybe > camelCaseCSSPropertyNameToID()? > > We should add a unit test for this function. FWIW, this logic (and a little bit more :)) is found in V8CSSStyleDeclarationCustom.cpp, static CSSPropertyInfo* cssPropertyInfo(v8::Handle<v8::String> v8PropertyName). Factoring out the required chunk (to return CSSPropertyInfo* given a normal String) is fairly straightforward.
On 2013/11/29 07:31:19, Steve Block wrote: > > The flag is webAnimationsAPI. > Nit. That's the RuntimeEnabledFeature. I don't think the chrome flag is hooked > up yet, right? That will be --enable-web-animations-api The runtime feature is experimental so we shouldn't need a command line flag for this.
> The runtime feature is experimental so we shouldn't need a > command line flag for this. Sure, assuming we don't want to test it independently of other experimental web platform features. https://codereview.chromium.org/96283002/diff/1/Source/core/animation/Animate... File Source/core/animation/AnimatedElement.cpp (right): https://codereview.chromium.org/96283002/diff/1/Source/core/animation/Animate... Source/core/animation/AnimatedElement.cpp:108: continue; I guess we need to decide whether to change the API of KAE to describe properties by strings (thereby allowing it to handle invalid properties) so that we can expose KAE directly to JS, or to add a wrapper class around KAE that keeps track of the 'original' keyframe list and handles the conversion between string and CSSPropertyID. We should discuss this.
https://codereview.chromium.org/96283002/diff/1/Source/core/animation/Animate... File Source/core/animation/AnimatedElement.cpp (right): https://codereview.chromium.org/96283002/diff/1/Source/core/animation/Animate... Source/core/animation/AnimatedElement.cpp:95: dictionaryKeyframesVector[i].get("compositeOperation", compositeOperationString); This should be 'composite'. 'CompositeOperation' is the name of the type. http://dev.w3.org/fxtf/web-animations/#the-keyframe-dictionary https://codereview.chromium.org/96283002/diff/1/Source/core/animation/Animate... Source/core/animation/AnimatedElement.cpp:134: // only one keyframe) this crashes. I think you're just seeing the fact that add composition isn't yet supported. See https://codereview.chromium.org/98243002 Let me know if you see anything else. http://dev.w3.org/fxtf/web-animations/#the-unaccumulated-animation-value-of-a...
https://codereview.chromium.org/96283002/diff/1/Source/core/animation/Animate... File Source/core/animation/AnimatedElement.cpp (right): https://codereview.chromium.org/96283002/diff/1/Source/core/animation/Animate... Source/core/animation/AnimatedElement.cpp:32: On 2013/11/29 07:31:19, Steve Block wrote: > NO blank line here Done. https://codereview.chromium.org/96283002/diff/1/Source/core/animation/Animate... Source/core/animation/AnimatedElement.cpp:52: static bool isUpper(UChar ch) { return isASCIIUpper(ch); } On 2013/11/29 07:31:19, Steve Block wrote: > Why do you need this wrapper function? We don't. Done. https://codereview.chromium.org/96283002/diff/1/Source/core/animation/Animate... Source/core/animation/AnimatedElement.cpp:54: static CSSPropertyID v8PropertyToCSSPropertyID(String propertyName) On 2013/11/29 07:31:19, Steve Block wrote: > I'm surprised we don't have this logic elsewhere for the CSSOM but I imagine you > already checked this? > > Why 'V8' property? This doesn't seem to be specific to V8. Maybe > camelCaseCSSPropertyNameToID()? > > We should add a unit test for this function. Name change done. The logic exists elsewhere but not in a useful form. https://codereview.chromium.org/96283002/diff/1/Source/core/animation/Animate... Source/core/animation/AnimatedElement.cpp:54: static CSSPropertyID v8PropertyToCSSPropertyID(String propertyName) On 2013/11/29 09:19:02, apavlov wrote: > On 2013/11/29 07:31:19, Steve Block wrote: > > I'm surprised we don't have this logic elsewhere for the CSSOM but I imagine > you > > already checked this? > > > > Why 'V8' property? This doesn't seem to be specific to V8. Maybe > > camelCaseCSSPropertyNameToID()? > > > > We should add a unit test for this function. > > FWIW, this logic (and a little bit more :)) is found in > V8CSSStyleDeclarationCustom.cpp, > static CSSPropertyInfo* cssPropertyInfo(v8::Handle<v8::String> v8PropertyName). > > Factoring out the required chunk (to return CSSPropertyInfo* given a normal > String) is fairly straightforward. Yes that's where I copied it from. There was no obvious way to expose it to Web Animations so I copied it instead. If you have any suggestions for how to expose it, or a more central place to re-implement it, then I am all ears. https://codereview.chromium.org/96283002/diff/1/Source/core/animation/Animate... Source/core/animation/AnimatedElement.cpp:60: if (end == kNotFound) { On 2013/11/29 07:31:19, Steve Block wrote: > Is this simpler ... > > size_t end; > while (end = propertyName.find(isUpper, position) != kNotFound) { > builder.append(propertyName.substring(position, end) + "-" + > toASCIILower(propertyName[end])); > position = end + 1; > } > builder.append(propertyName.substring(position)); Done. https://codereview.chromium.org/96283002/diff/1/Source/core/animation/Animate... Source/core/animation/AnimatedElement.cpp:72: void AnimatedElement::animate(Element* element, Vector<Dictionary> dictionaryKeyframesVector) On 2013/11/29 07:31:19, Steve Block wrote: > s/dictionaryKeyframesVector/keyframeDictionaryVector to match the spec > terminology? Done. https://codereview.chromium.org/96283002/diff/1/Source/core/animation/Animate... Source/core/animation/AnimatedElement.cpp:83: if (!keyframeProperties.size()) On 2013/11/29 07:31:19, Steve Block wrote: > This is incorrect. The spec requires that animation.effect.getFrames() should > return the original list of keyframes, as specified. This will be relevant once > we hook up element.animate() to return a Player/TimedItem. If there's a > particular reason for not handling empty keyframes now, add a FIXME and perhaps > a RELEASE_ASSERT_WITH_MESSAGE(). Done. https://codereview.chromium.org/96283002/diff/1/Source/core/animation/Animate... Source/core/animation/AnimatedElement.cpp:86: keyframes.append(Keyframe::create()); On 2013/11/29 07:31:19, Steve Block wrote: > Probably best to build up the keyframe before appending it to keyframes, to save > repeated calls to keyframes.last(). Done. https://codereview.chromium.org/96283002/diff/1/Source/core/animation/Animate... Source/core/animation/AnimatedElement.cpp:92: keyframes.last()->setOffset(std::numeric_limits<double>::quiet_NaN()); On 2013/11/29 07:31:19, Steve Block wrote: > No need for this, it's the default offset for a keyframe. In any case, you > should use nullValue(). Done. nullValue()? https://codereview.chromium.org/96283002/diff/1/Source/core/animation/Animate... Source/core/animation/AnimatedElement.cpp:95: dictionaryKeyframesVector[i].get("compositeOperation", compositeOperationString); On 2013/12/02 04:45:45, Steve Block wrote: > This should be 'composite'. 'CompositeOperation' is the name of the type. > > http://dev.w3.org/fxtf/web-animations/#the-keyframe-dictionary Done. https://codereview.chromium.org/96283002/diff/1/Source/core/animation/Animate... Source/core/animation/AnimatedElement.cpp:99: keyframes.last()->setComposite(AnimationEffect::CompositeReplace); On 2013/11/29 07:31:19, Steve Block wrote: > Again, keyframes use replace composition by default. Done. https://codereview.chromium.org/96283002/diff/1/Source/core/animation/Animate... Source/core/animation/AnimatedElement.cpp:107: if (!id || !CSSAnimations::isAnimatableProperty(id)) On 2013/11/29 07:31:19, Steve Block wrote: > Isn't there a CSSPropertyInvalid we can use here? If the property from the dictionary isn't a valid css property then id will be CSSPropertyInvalid. Trying to apply a value to CSSPropertyInvalid and use it to make an AnimatableValue hits an assert somewhere. https://codereview.chromium.org/96283002/diff/1/Source/core/animation/Animate... Source/core/animation/AnimatedElement.cpp:108: continue; On 2013/12/02 01:59:43, Steve Block wrote: > I guess we need to decide whether to change the API of KAE to describe > properties by strings (thereby allowing it to handle invalid properties) so that > we can expose KAE directly to JS, or to add a wrapper class around KAE that > keeps track of the 'original' keyframe list and handles the conversion between > string and CSSPropertyID. We should discuss this. We should discuss this. There's a bunch of different options and I don't like any of them :P https://codereview.chromium.org/96283002/diff/1/Source/core/animation/Animate... Source/core/animation/AnimatedElement.cpp:110: continue; On 2013/11/29 07:31:19, Steve Block wrote: > Same as above I can't change this to anything more meaningful until we decide what to do with invalid inputs. https://codereview.chromium.org/96283002/diff/1/Source/core/animation/Animate... Source/core/animation/AnimatedElement.cpp:118: if (!keyframes.last()->properties().size() && isnan(keyframes.last()->offset())) On 2013/11/29 07:31:19, Steve Block wrote: > Same comment about getFrames(). Also, use isEmpty() rather than !size() Done. https://codereview.chromium.org/96283002/diff/1/Source/core/animation/Animate... Source/core/animation/AnimatedElement.cpp:128: timing.iterationCount = 1; On 2013/11/29 07:31:19, Steve Block wrote: > Superfluous. The default iterationCount is 1. Done. https://codereview.chromium.org/96283002/diff/1/Source/core/animation/Animate... Source/core/animation/AnimatedElement.cpp:132: DocumentTimeline* timeline = element->document().timeline(); On 2013/11/29 07:31:19, Steve Block wrote: > ASSERT(timeline) Done. https://codereview.chromium.org/96283002/diff/1/Source/core/animation/Animate... Source/core/animation/AnimatedElement.cpp:134: // only one keyframe) this crashes. On 2013/12/02 04:45:45, Steve Block wrote: > I think you're just seeing the fact that add composition isn't yet supported. > See https://codereview.chromium.org/98243002 Let me know if you see anything > else. > > http://dev.w3.org/fxtf/web-animations/#the-unaccumulated-animation-value-of-a... Yep that's it. In that case I'll just pass those cases through. https://codereview.chromium.org/96283002/diff/1/Source/core/animation/Animate... Source/core/animation/AnimatedElement.cpp:139: // FIXME: If there are more than two keyframes and any of those keyframes doesn't have a specified On 2013/11/29 07:31:19, Steve Block wrote: > It's not clear which of these you plan to fix before landing vs after landing > this patch. Uploading a work in progress is fine, but it's good to make this > clear. FIXME is used for TODOs you plan to fix after landing, so maybe use > FIXME_BEFORE_LANDING otherwise. Done. https://codereview.chromium.org/96283002/diff/1/Source/core/animation/Animate... Source/core/animation/AnimatedElement.cpp:141: // Handling for this case not yet implemented. On 2013/11/29 07:31:19, Steve Block wrote: > This will be handled by KeyframeAnimationEffect. See the FIXME in > KeyframeAnimationEffect::normalizedKeyframes(). The reason I have't fixed this > yet is that the spec behaviour is still being debated. We should probably just > RELEASE_ASSERT_WITH_MESSAGE() if _any_ keyframes are missing offsets for now. Done. https://codereview.chromium.org/96283002/diff/1/Source/core/animation/Animate... File Source/core/animation/AnimatedElement.h (right): https://codereview.chromium.org/96283002/diff/1/Source/core/animation/Animate... Source/core/animation/AnimatedElement.h:35: #include "core/css/CSSParser.h" On 2013/11/29 07:31:19, Steve Block wrote: > Is this needed? Done. https://codereview.chromium.org/96283002/diff/1/Source/core/animation/Animate... Source/core/animation/AnimatedElement.h:42: static void animate(Element*, Vector<Dictionary>); On 2013/11/29 07:31:19, Steve Block wrote: > Should name the second param, as it's not obvious from the type Done. https://codereview.chromium.org/96283002/diff/1/Source/core/animation/Animate... Source/core/animation/AnimatedElement.h:47: #endif On 2013/11/29 07:31:19, Steve Block wrote: > // AnimatedElement_h Done. https://codereview.chromium.org/96283002/diff/1/Source/core/animation/Animate... File Source/core/animation/AnimatedElement.idl (right): https://codereview.chromium.org/96283002/diff/1/Source/core/animation/Animate... Source/core/animation/AnimatedElement.idl:29: */ On 2013/11/29 07:31:19, Steve Block wrote: > Blank line Done. https://codereview.chromium.org/96283002/diff/1/Source/core/animation/Animate... Source/core/animation/AnimatedElement.idl:30: partial interface Element { On 2013/11/29 06:32:49, dstockwell wrote: > This should probably be named ElementAnimation, most other partial interfaces I > could find are named that way. > > eg. WindowNotifications, DocumentFullscreen, WindowMediaSource Done. https://codereview.chromium.org/96283002/diff/1/Source/core/animation/Animate... Source/core/animation/AnimatedElement.idl:31: void animate(sequence<Dictionary> keyframes); On 2013/11/29 06:32:49, dstockwell wrote: > Shouldn't this be runtime enabled? > > [RuntimeEnabled=WebAnimationsAPI] Done.
https://codereview.chromium.org/96283002/diff/40001/Source/core/animation/Ele... File Source/core/animation/ElementAnimation.cpp (right): https://codereview.chromium.org/96283002/diff/40001/Source/core/animation/Ele... Source/core/animation/ElementAnimation.cpp:102: RefPtr<RenderStyle> style = RenderStyle::clone(element->styleForRenderer().get()); Calling styleForRenderer() repeatedly for each property is crazy expensive, you're match every rule over and over outside style recalc so we can't even style share. styleForRenderer() should not even be public, you probably want ->computedStyle(). https://codereview.chromium.org/96283002/diff/40001/Source/core/animation/Ele... Source/core/animation/ElementAnimation.cpp:103: StyleResolverState state(element->document(), element); Constructing states is not cheap, one per property is going to be quite slow. https://codereview.chromium.org/96283002/diff/40001/Source/core/animation/Ele... Source/core/animation/ElementAnimation.cpp:106: keyframe->setPropertyValue(id, CSSAnimatableValueFactory::create(id, *style.get()).get()); This does not look like the right way to do this. StyleResolverState is a private API, you shouldn't be using it over here. You probably want to create a MutableStylePropertySet and fill it with values, that will parse it for you too. Then you should add an API to StyleResolver that takes a PropertySet and an element and spits out what you want.
https://codereview.chromium.org/96283002/diff/40001/Source/core/animation/Ele... File Source/core/animation/ElementAnimation.cpp (right): https://codereview.chromium.org/96283002/diff/40001/Source/core/animation/Ele... Source/core/animation/ElementAnimation.cpp:52: static CSSPropertyID camelCasePropertyToCSSPropertyID(String propertyName) No need for static now this is in an anonymous namespace https://codereview.chromium.org/96283002/diff/40001/Source/core/animation/Ele... Source/core/animation/ElementAnimation.cpp:62: return cssPropertyID(builder.toString()); We should check RuntimeCSSEnabled::isCSSPropertyEnabled(), like in cssPropertyInfo(). I think apavlov is right that we should re-use that method, especially as it provides caching. Maybe move it to a static method on CSSParser and change the input type to StringImpl? https://codereview.chromium.org/96283002/diff/40001/Source/core/animation/Ele... Source/core/animation/ElementAnimation.cpp:76: keyframeDictionaryVector[i].getOwnPropertyNames(keyframeProperties); Move this down to line 91 https://codereview.chromium.org/96283002/diff/40001/Source/core/animation/Ele... Source/core/animation/ElementAnimation.cpp:97: if (!id || !CSSAnimations::isAnimatableProperty(id)) (id != CSSPropertyInvalid || ...) for clarity https://codereview.chromium.org/96283002/diff/40001/Source/core/animation/Ele... Source/core/animation/ElementAnimation.cpp:98: continue; There should be a RELEASE_ASSERT or at least a FIXME about the fact that we shouldn't drop any properties at this stage, so that we can hook up getFrames() at a later date. Also, we should do this before we get the property value https://codereview.chromium.org/96283002/diff/40001/Source/core/animation/Ele... Source/core/animation/ElementAnimation.cpp:100: continue; Same thing about a RELEASE_ASSERT or FIXME https://codereview.chromium.org/96283002/diff/40001/Source/core/css/resolver/... File Source/core/css/resolver/StyleResolver.cpp (right): https://codereview.chromium.org/96283002/diff/40001/Source/core/css/resolver/... Source/core/css/resolver/StyleResolver.cpp:1086: RELEASE_ASSERT_WITH_MESSAGE(!iter->value->dependsOnUnderlyingValue(), "Web Animations not yet implemented: An interface for compositing onto the underlying value."); I don't think we want to do this. This is used by CSS - we don't want to assert in release builds in the wild.
Did you look into a test to avoid 'leaking' this prematurely?
Not yet. Will do tests next though. On Fri, Dec 6, 2013 at 3:36 PM, <steveblock@chromium.org> wrote: > Did you look into a test to avoid 'leaking' this prematurely? > > > https://codereview.chromium.org/96283002/ > To unsubscribe from this group and stop receiving emails from it, send an email to blink-reviews+unsubscribe@chromium.org.
I've done what you asked the best I could, Elliott. PTAL. Steve, I will come back to your comments. I just wanted to get the changes to StyleResolver up ASAP. https://codereview.chromium.org/96283002/diff/40001/Source/core/animation/Ele... File Source/core/animation/ElementAnimation.cpp (right): https://codereview.chromium.org/96283002/diff/40001/Source/core/animation/Ele... Source/core/animation/ElementAnimation.cpp:52: static CSSPropertyID camelCasePropertyToCSSPropertyID(String propertyName) On 2013/12/06 03:27:27, Steve Block wrote: > No need for static now this is in an anonymous namespace Done. https://codereview.chromium.org/96283002/diff/40001/Source/core/animation/Ele... Source/core/animation/ElementAnimation.cpp:76: keyframeDictionaryVector[i].getOwnPropertyNames(keyframeProperties); On 2013/12/06 03:27:27, Steve Block wrote: > Move this down to line 91 Done. https://codereview.chromium.org/96283002/diff/40001/Source/core/animation/Ele... Source/core/animation/ElementAnimation.cpp:97: if (!id || !CSSAnimations::isAnimatableProperty(id)) On 2013/12/06 03:27:27, Steve Block wrote: > (id != CSSPropertyInvalid || ...) for clarity Done. https://codereview.chromium.org/96283002/diff/40001/Source/core/animation/Ele... Source/core/animation/ElementAnimation.cpp:98: continue; On 2013/12/06 03:27:27, Steve Block wrote: > There should be a RELEASE_ASSERT or at least a FIXME about the fact that we > shouldn't drop any properties at this stage, so that we can hook up getFrames() > at a later date. > > Also, we should do this before we get the property value Done. https://codereview.chromium.org/96283002/diff/40001/Source/core/animation/Ele... Source/core/animation/ElementAnimation.cpp:102: RefPtr<RenderStyle> style = RenderStyle::clone(element->styleForRenderer().get()); On 2013/12/04 06:14:13, esprehn wrote: > Calling styleForRenderer() repeatedly for each property is crazy expensive, > you're match every rule over and over outside style recalc so we can't even > style share. styleForRenderer() should not even be public, you probably want > ->computedStyle(). Done. https://codereview.chromium.org/96283002/diff/40001/Source/core/animation/Ele... Source/core/animation/ElementAnimation.cpp:103: StyleResolverState state(element->document(), element); On 2013/12/04 06:14:13, esprehn wrote: > Constructing states is not cheap, one per property is going to be quite slow. Done. https://codereview.chromium.org/96283002/diff/40001/Source/core/animation/Ele... Source/core/animation/ElementAnimation.cpp:106: keyframe->setPropertyValue(id, CSSAnimatableValueFactory::create(id, *style.get()).get()); On 2013/12/04 06:14:13, esprehn wrote: > This does not look like the right way to do this. StyleResolverState is a > private API, you shouldn't be using it over here. > > You probably want to create a MutableStylePropertySet and fill it with values, > that will parse it for you too. Then you should add an API to StyleResolver that > takes a PropertySet and an element and spits out what you want. Done. https://codereview.chromium.org/96283002/diff/40001/Source/core/css/resolver/... File Source/core/css/resolver/StyleResolver.cpp (right): https://codereview.chromium.org/96283002/diff/40001/Source/core/css/resolver/... Source/core/css/resolver/StyleResolver.cpp:1086: RELEASE_ASSERT_WITH_MESSAGE(!iter->value->dependsOnUnderlyingValue(), "Web Animations not yet implemented: An interface for compositing onto the underlying value."); On 2013/12/06 03:27:27, Steve Block wrote: > I don't think we want to do this. This is used by CSS - we don't want to assert > in release builds in the wild. Done.
Ok Steve I think that's all your stuff except testing, which I'll do next. PTAL. Cheers :) https://codereview.chromium.org/96283002/diff/40001/Source/core/animation/Ele... File Source/core/animation/ElementAnimation.cpp (right): https://codereview.chromium.org/96283002/diff/40001/Source/core/animation/Ele... Source/core/animation/ElementAnimation.cpp:62: return cssPropertyID(builder.toString()); On 2013/12/06 03:27:27, Steve Block wrote: > We should check RuntimeCSSEnabled::isCSSPropertyEnabled(), like in > cssPropertyInfo(). I think apavlov is right that we should re-use that method, > especially as it provides caching. > > Maybe move it to a static method on CSSParser and change the input type to > StringImpl? Done.
https://codereview.chromium.org/96283002/diff/70001/Source/core/css/resolver/... File Source/core/css/resolver/StyleResolver.cpp (right): https://codereview.chromium.org/96283002/diff/70001/Source/core/css/resolver/... Source/core/css/resolver/StyleResolver.cpp:862: void StyleResolver::setKeyframePropertyValues(KeyframeAnimationEffect::KeyframeVector& keyframes, Vector<RefPtr<MutableStylePropertySet> >& propertySetVector, Element* element) Can propertySetVector and element be const? Also, out params like keyframes are usually last. https://codereview.chromium.org/96283002/diff/70001/Source/core/css/resolver/... Source/core/css/resolver/StyleResolver.cpp:868: RefPtr<RenderStyle> style = RenderStyle::clone(element->computedStyle()); As discussed, I don't think this is required - we only need the style properties that are explicitly set on the keyframe.
https://codereview.chromium.org/96283002/diff/70001/Source/core/css/resolver/... File Source/core/css/resolver/StyleResolver.cpp (right): https://codereview.chromium.org/96283002/diff/70001/Source/core/css/resolver/... Source/core/css/resolver/StyleResolver.cpp:862: void StyleResolver::setKeyframePropertyValues(KeyframeAnimationEffect::KeyframeVector& keyframes, Vector<RefPtr<MutableStylePropertySet> >& propertySetVector, Element* element) On 2013/12/09 03:11:15, Steve Block wrote: > Can propertySetVector and element be const? Also, out params like keyframes are > usually last. Done. propertySetVector yes, element no. https://codereview.chromium.org/96283002/diff/70001/Source/core/css/resolver/... Source/core/css/resolver/StyleResolver.cpp:868: RefPtr<RenderStyle> style = RenderStyle::clone(element->computedStyle()); On 2013/12/09 03:11:15, Steve Block wrote: > As discussed, I don't think this is required - we only need the style properties > that are explicitly set on the keyframe. Done.
Added a layout test. It took me all day to work out how to do it and it's probably terrible. Help appreciated.
On 2013/12/09 12:01:28, rjwright wrote: > Added a layout test. It took me all day to work out how to do it and it's > probably terrible. Help appreciated. There's a problem with the expectations. In some places they show up like normal (PASS or FAIL with descriptions), but in other places they show up as a totally different format. Do I just copy the good version everywhere where the weird version shows up, or what?
https://codereview.chromium.org/96283002/diff/130001/Source/core/animation/El... File Source/core/animation/ElementAnimation.cpp (right): https://codereview.chromium.org/96283002/diff/130001/Source/core/animation/El... Source/core/animation/ElementAnimation.cpp:60: RELEASE_ASSERT_WITH_MESSAGE(false, "Web Animations not yet implemented: Handling for keyframes without specified offset."); Can you hit this with JS today if you turn the flag on? You should never be able to crash a shipping build on an ASSERT even for features that are not done yet. https://codereview.chromium.org/96283002/diff/130001/Source/core/animation/El... Source/core/animation/ElementAnimation.cpp:85: continue; This continue doesn't matter, you're at the end of the loop. Also you shouldn't need to call the parser directly, MutableStylePropertySet::setProperty handles parsing internally, just use setProperty(id, value) https://codereview.chromium.org/96283002/diff/130001/Source/core/animation/El... Source/core/animation/ElementAnimation.cpp:90: RefPtr<KeyframeAnimationEffect> effect = KeyframeAnimationEffect::create(keyframes); This should be the return value of the styleResolver method. RefPtr<KeyframeAnimationEffect> ... = StyleResolver::createKeyframes(...); https://codereview.chromium.org/96283002/diff/130001/Source/core/css/CSSParse... File Source/core/css/CSSParser-in.cpp (right): https://codereview.chromium.org/96283002/diff/130001/Source/core/css/CSSParse... Source/core/css/CSSParser-in.cpp:10235: builder.append(propertyName->substring(position, end) + "-" + toASCIILower((*propertyName)[end])); You shouldn't add this, instead refactor the V8CSSStyleDeclarationCustom::cssPropertyInfo into a shared place that takes a const String& and use it. https://codereview.chromium.org/96283002/diff/130001/Source/core/css/CSSParse... Source/core/css/CSSParser-in.cpp:10242: map.add(propertyName, id); This is a memory leak, it'll just grow forever with all unknown properties that go through this function. It seems we already have such a leak in cssPropertyInfo (blah), so lets not add another. https://codereview.chromium.org/96283002/diff/130001/Source/core/css/resolver... File Source/core/css/resolver/StyleResolver.h (right): https://codereview.chromium.org/96283002/diff/130001/Source/core/css/resolver... Source/core/css/resolver/StyleResolver.h:127: static void setKeyframePropertyValues(Element*, const Vector<RefPtr<MutableStylePropertySet> >&, KeyframeAnimationEffect::KeyframeVector&); Should take an Element&, and return a KeyframeAnimationEffect.
https://codereview.chromium.org/96283002/diff/130001/Source/core/animation/El... File Source/core/animation/ElementAnimation.cpp (right): https://codereview.chromium.org/96283002/diff/130001/Source/core/animation/El... Source/core/animation/ElementAnimation.cpp:60: RELEASE_ASSERT_WITH_MESSAGE(false, "Web Animations not yet implemented: Handling for keyframes without specified offset."); On 2013/12/09 18:51:25, esprehn wrote: > Can you hit this with JS today if you turn the flag on? You should never be able > to crash a shipping build on an ASSERT even for features that are not done yet. This feature is behind the experimental web platform features flag. What do you propose is the appropriate mechanism for bailing cleanly when partially implemented (and off-by-default) features wish to expose JavaScript APIs? https://codereview.chromium.org/96283002/diff/130001/Source/core/css/CSSParse... File Source/core/css/CSSParser-in.cpp (right): https://codereview.chromium.org/96283002/diff/130001/Source/core/css/CSSParse... Source/core/css/CSSParser-in.cpp:10242: map.add(propertyName, id); On 2013/12/09 18:51:25, esprehn wrote: > This is a memory leak, it'll just grow forever with all unknown properties that > go through this function. It seems we already have such a leak in > cssPropertyInfo (blah), so lets not add another. Invalid property names are explicitly filtered out on the line directly above this. This map will never contain more than the 400-odd known property names.
https://codereview.chromium.org/96283002/diff/130001/Source/core/animation/El... File Source/core/animation/ElementAnimation.cpp (right): https://codereview.chromium.org/96283002/diff/130001/Source/core/animation/El... Source/core/animation/ElementAnimation.cpp:60: RELEASE_ASSERT_WITH_MESSAGE(false, "Web Animations not yet implemented: Handling for keyframes without specified offset."); Yes, you can. Is this policy documented somewhere? When implementing the Web Animations model we used this approach to make sure that not-yet-implemented features didn't get forgotten about, and it worked rather well. I guess the alternative is a debug assertion and early-out. https://codereview.chromium.org/96283002/diff/130001/Source/core/css/CSSParse... File Source/core/css/CSSParser-in.cpp (right): https://codereview.chromium.org/96283002/diff/130001/Source/core/css/CSSParse... Source/core/css/CSSParser-in.cpp:10235: builder.append(propertyName->substring(position, end) + "-" + toASCIILower((*propertyName)[end])); I recommended this too, but after discussion with Renee, I'm not sure it's worth it. cssPropertyInfo() checks 'webkit' and 'css' prefixed variants, and allows the dashed version of names, all of which we don't need. It also builds a CSSPropertyInfo struct, which contains information that we don't need and which is built up by character-by-character searching. To share the function we'd have to introduce a bunch of 'if' statements, and wouldn't be able to make use of 'find()', as we do here.
Adding ojan and eseidel, landing stuff behind a flag with a reachable from JS RELEASE_ASSERT(false) doesn't look good at all. I'm not sure why we allowed this in the past. https://codereview.chromium.org/96283002/diff/130001/Source/core/animation/El... File Source/core/animation/ElementAnimation.cpp (right): https://codereview.chromium.org/96283002/diff/130001/Source/core/animation/El... Source/core/animation/ElementAnimation.cpp:60: RELEASE_ASSERT_WITH_MESSAGE(false, "Web Animations not yet implemented: Handling for keyframes without specified offset."); On 2013/12/09 23:32:20, Steve Block wrote: > Yes, you can. > > Is this policy documented somewhere? When implementing the Web Animations model > we used this approach to make sure that not-yet-implemented features didn't get > forgotten about, and it worked rather well. I guess the alternative is a debug > assertion and early-out. We should never ship something that we know will crash the renderer and is available from script. This could mean that a popular page with a demo starts crashing thousands of people on Canary and floods go/crash. https://codereview.chromium.org/96283002/diff/130001/Source/core/css/CSSParse... File Source/core/css/CSSParser-in.cpp (right): https://codereview.chromium.org/96283002/diff/130001/Source/core/css/CSSParse... Source/core/css/CSSParser-in.cpp:10235: builder.append(propertyName->substring(position, end) + "-" + toASCIILower((*propertyName)[end])); On 2013/12/09 23:32:20, Steve Block wrote: > I recommended this too, but after discussion with Renee, I'm not sure it's worth > it. cssPropertyInfo() checks 'webkit' and 'css' prefixed variants, and allows > the dashed version of names, all of which we don't need. It also builds a > CSSPropertyInfo struct, which contains information that we don't need and which > is built up by character-by-character searching. To share the function we'd have > to introduce a bunch of 'if' statements, and wouldn't be able to make use of > 'find()', as we do here. Why is it safe to ignore the webkit prefix? It doesn't make sense to have two tables with the exact same values in them, and two functions that do nearly the same thing like this. https://codereview.chromium.org/96283002/diff/130001/Source/core/css/CSSParse... Source/core/css/CSSParser-in.cpp:10242: map.add(propertyName, id); On 2013/12/09 23:25:57, shans wrote: > On 2013/12/09 18:51:25, esprehn wrote: > > This is a memory leak, it'll just grow forever with all unknown properties > that > > go through this function. It seems we already have such a leak in > > cssPropertyInfo (blah), so lets not add another. > > Invalid property names are explicitly filtered out on the line directly above > this. This map will never contain more than the 400-odd known property names. Awesome.
https://codereview.chromium.org/96283002/diff/130001/Source/core/animation/El... File Source/core/animation/ElementAnimation.cpp (right): https://codereview.chromium.org/96283002/diff/130001/Source/core/animation/El... Source/core/animation/ElementAnimation.cpp:60: RELEASE_ASSERT_WITH_MESSAGE(false, "Web Animations not yet implemented: Handling for keyframes without specified offset."); On 2013/12/09 23:36:16, esprehn wrote: > On 2013/12/09 23:32:20, Steve Block wrote: > > Yes, you can. > > > > Is this policy documented somewhere? When implementing the Web Animations > model > > we used this approach to make sure that not-yet-implemented features didn't > get > > forgotten about, and it worked rather well. I guess the alternative is a debug > > assertion and early-out. > > We should never ship something that we know will crash the renderer and is > available from script. This could mean that a popular page with a demo starts > crashing thousands of people on Canary and floods go/crash. It can only crash for people who have the flag enabled. Again, though, what is your proposed alternative? The specific case here is that we wish to pursue an incremental implementation approach - in the short term there is going to be stuff that just isn't implemented. How do we gracefully deal with JavaScript requests that tickle unimplemented features?
On 2013/12/09 23:41:26, shans wrote: > ... > Again, though, what is your proposed alternative? The specific case here is that > we wish to pursue an incremental implementation approach - in the short term > there is going to be stuff that just isn't implemented. How do we gracefully > deal with JavaScript requests that tickle unimplemented features? You can console.log, you can just return early and fail to animate anything at all, you can even animate the wrong thing, you could put up a photo of yourself or Renée. Crashing the renderer, which might contain N+ more tabs on a RELEASE_ASSERT is not okay though. Failing a RELEASE_ASSERT means something horrible has happened and to continue the security and integrity of all tabs in the process would be compromised.
> There's a problem with the expectations. In some places > they show up like normal (PASS or FAIL with descriptions), > but in other places they show up as a totally different > format. Not sure what you mean by this. Grab me if you're having problems. Also, I think the test directory should be something more descriptive than animations/api/ - animations/web-animations-api/ ? https://codereview.chromium.org/96283002/diff/130001/LayoutTests/animations/a... File LayoutTests/animations/api/element-animate-list-of-keyframes.html (right): https://codereview.chromium.org/96283002/diff/130001/LayoutTests/animations/a... LayoutTests/animations/api/element-animate-list-of-keyframes.html:15: <div id='e1' class='anim'></div> You should write a valid html page, with html and body tags. https://codereview.chromium.org/96283002/diff/130001/LayoutTests/animations/a... LayoutTests/animations/api/element-animate-list-of-keyframes.html:28: var e1Style = window.getComputedStyle(e1); No need to explicitly use 'window'. https://codereview.chromium.org/96283002/diff/130001/LayoutTests/animations/a... LayoutTests/animations/api/element-animate-list-of-keyframes.html:43: }, 1010); Using setTimeout() like this will be flaky. You should be able to use the DRT pause API, right? Also, testing after the animation has completed isn't really testing the animation. You should test at at least one time while things are changing. Finally, 1010 looks like a magic constant. You should probably add a comment that for now, duration is hardcoded to 1s. For this reason, I'd be happy adding this test at a later date. The only test I think we need now is one that checks that this isn't exposed in the wild, if that's possible. https://codereview.chromium.org/96283002/diff/130001/LayoutTests/animations/a... LayoutTests/animations/api/element-animate-list-of-keyframes.html:78: }, 'Calling animate() with a pre-constructed keyframes list should start an animation.'); I don't think this test adds anything. You're effectively testing use of a variable, which is a JS language feature and is not specific to Element.animate(). https://codereview.chromium.org/96283002/diff/130001/LayoutTests/animations/a... LayoutTests/animations/api/element-animate-list-of-keyframes.html:91: assert_equals(e5Style.backgroundColor, 'black'); You should also check that e5style.foo is undefined.
https://codereview.chromium.org/96283002/diff/130001/Source/core/css/CSSParse... File Source/core/css/CSSParser-in.cpp (right): https://codereview.chromium.org/96283002/diff/130001/Source/core/css/CSSParse... Source/core/css/CSSParser-in.cpp:10235: builder.append(propertyName->substring(position, end) + "-" + toASCIILower((*propertyName)[end])); On 2013/12/09 23:36:16, esprehn wrote: > On 2013/12/09 23:32:20, Steve Block wrote: > > I recommended this too, but after discussion with Renee, I'm not sure it's > worth > > it. cssPropertyInfo() checks 'webkit' and 'css' prefixed variants, and allows > > the dashed version of names, all of which we don't need. It also builds a > > CSSPropertyInfo struct, which contains information that we don't need and > which > > is built up by character-by-character searching. To share the function we'd > have > > to introduce a bunch of 'if' statements, and wouldn't be able to make use of > > 'find()', as we do here. > > Why is it safe to ignore the webkit prefix? It doesn't make sense to have two > tables with the exact same values in them, and two functions that do nearly the > same thing like this. We don't intend the Web Animations API to support the legacy prefixing approach to experimental features. Prefixed properties will not be animatable using this feature. The two tables don't have the exact same values in them. Consider the following keys that will populate the old table but that are not admitted into the new table: "webkitLength" "margin-left" "cssHeight" Furthermore, the new table maps strings to CSSPropertyID values, not CSSPropertyInfo structs.
On 2013/12/09 23:44:12, esprehn wrote: > On 2013/12/09 23:41:26, shans wrote: > > ... > > Again, though, what is your proposed alternative? The specific case here is > that > > we wish to pursue an incremental implementation approach - in the short term > > there is going to be stuff that just isn't implemented. How do we gracefully > > deal with JavaScript requests that tickle unimplemented features? > > You can console.log, you can just return early and fail to animate anything at > all, you can even animate the wrong thing, you could put up a photo of yourself > or Renée. Crashing the renderer, which might contain N+ more tabs on a > RELEASE_ASSERT is not okay though. > > Failing a RELEASE_ASSERT means something horrible has happened and to continue > the security and integrity of all tabs in the process would be compromised. +1 to early return + FIXME. Also, if you want to you can NOTIMPLEMENTED, which will log to stderr anytime it's hit: https://code.google.com/p/chromium/codesearch#search/&q=NOTIMPLEMENTED&sq=pac.... Not sure if it's exposed to Blink code, but I wouldn't be opposed to adding it.
On 2013/12/09 23:48:40, shans wrote: > https://codereview.chromium.org/96283002/diff/130001/Source/core/css/CSSParse... > File Source/core/css/CSSParser-in.cpp (right): > > https://codereview.chromium.org/96283002/diff/130001/Source/core/css/CSSParse... > Source/core/css/CSSParser-in.cpp:10235: > builder.append(propertyName->substring(position, end) + "-" + > toASCIILower((*propertyName)[end])); > On 2013/12/09 23:36:16, esprehn wrote: > > On 2013/12/09 23:32:20, Steve Block wrote: > > > I recommended this too, but after discussion with Renee, I'm not sure it's > > worth > > > it. cssPropertyInfo() checks 'webkit' and 'css' prefixed variants, and > allows > > > the dashed version of names, all of which we don't need. It also builds a > > > CSSPropertyInfo struct, which contains information that we don't need and > > which > > > is built up by character-by-character searching. To share the function we'd > > have > > > to introduce a bunch of 'if' statements, and wouldn't be able to make use of > > > 'find()', as we do here. > > > > Why is it safe to ignore the webkit prefix? It doesn't make sense to have two > > tables with the exact same values in them, and two functions that do nearly > the > > same thing like this. > > We don't intend the Web Animations API to support the legacy prefixing approach > to experimental features. Prefixed properties will not be animatable using this > feature. Is anyone working on unprefixing -webkit-transform, et al? That's one of the the most common things people animate.
On 2013/12/09 23:58:49, ojan wrote: > On 2013/12/09 23:48:40, shans wrote: > > > https://codereview.chromium.org/96283002/diff/130001/Source/core/css/CSSParse... > > File Source/core/css/CSSParser-in.cpp (right): > > > > > https://codereview.chromium.org/96283002/diff/130001/Source/core/css/CSSParse... > > Source/core/css/CSSParser-in.cpp:10235: > > builder.append(propertyName->substring(position, end) + "-" + > > toASCIILower((*propertyName)[end])); > > On 2013/12/09 23:36:16, esprehn wrote: > > > On 2013/12/09 23:32:20, Steve Block wrote: > > > > I recommended this too, but after discussion with Renee, I'm not sure it's > > > worth > > > > it. cssPropertyInfo() checks 'webkit' and 'css' prefixed variants, and > > allows > > > > the dashed version of names, all of which we don't need. It also builds a > > > > CSSPropertyInfo struct, which contains information that we don't need and > > > which > > > > is built up by character-by-character searching. To share the function > we'd > > > have > > > > to introduce a bunch of 'if' statements, and wouldn't be able to make use > of > > > > 'find()', as we do here. > > > > > > Why is it safe to ignore the webkit prefix? It doesn't make sense to have > two > > > tables with the exact same values in them, and two functions that do nearly > > the > > > same thing like this. > > > > We don't intend the Web Animations API to support the legacy prefixing > approach > > to experimental features. Prefixed properties will not be animatable using > this > > feature. > > Is anyone working on unprefixing -webkit-transform, et al? That's one of the the > most common things people animate. Yeah, dstockwell is since this blocks unprefixing animations. Fundamentally though, this API isn't going to be released to stable for a long time - I'd be .. disappointed .. if we hadn't unprefixed transforms first.
I've moved camelCaseCSSPropertyNameToID back into ElementAnimation.cpp, and changed it to reject property names with dashes. Removed RELEASE_ASSERT for keyframes with no specified offset (unimplemented feature) in favor of return. Changed return type of setKeyframePropertyValues to KeyframeAnimationEffect. https://codereview.chromium.org/96283002/diff/130001/LayoutTests/animations/a... File LayoutTests/animations/api/element-animate-list-of-keyframes.html (right): https://codereview.chromium.org/96283002/diff/130001/LayoutTests/animations/a... LayoutTests/animations/api/element-animate-list-of-keyframes.html:15: <div id='e1' class='anim'></div> On 2013/12/09 23:48:04, Steve Block wrote: > You should write a valid html page, with html and body tags. Done. https://codereview.chromium.org/96283002/diff/130001/LayoutTests/animations/a... LayoutTests/animations/api/element-animate-list-of-keyframes.html:28: var e1Style = window.getComputedStyle(e1); On 2013/12/09 23:48:04, Steve Block wrote: > No need to explicitly use 'window'. Done. https://codereview.chromium.org/96283002/diff/130001/LayoutTests/animations/a... LayoutTests/animations/api/element-animate-list-of-keyframes.html:43: }, 1010); On 2013/12/09 23:48:04, Steve Block wrote: > Using setTimeout() like this will be flaky. You should be able to use the DRT > pause API, right? > > Also, testing after the animation has completed isn't really testing the > animation. You should test at at least one time while things are changing. > > Finally, 1010 looks like a magic constant. You should probably add a comment > that for now, duration is hardcoded to 1s. For this reason, I'd be happy adding > this test at a later date. The only test I think we need now is one that checks > that this isn't exposed in the wild, if that's possible. Done. https://codereview.chromium.org/96283002/diff/130001/LayoutTests/animations/a... LayoutTests/animations/api/element-animate-list-of-keyframes.html:78: }, 'Calling animate() with a pre-constructed keyframes list should start an animation.'); On 2013/12/09 23:48:04, Steve Block wrote: > I don't think this test adds anything. You're effectively testing use of a > variable, which is a JS language feature and is not specific to > Element.animate(). Done. Yeah, I mostly added it for my own curiosity and didn't think to remove it. Done :) https://codereview.chromium.org/96283002/diff/130001/LayoutTests/animations/a... LayoutTests/animations/api/element-animate-list-of-keyframes.html:91: assert_equals(e5Style.backgroundColor, 'black'); On 2013/12/09 23:48:04, Steve Block wrote: > You should also check that e5style.foo is undefined. Done. https://codereview.chromium.org/96283002/diff/130001/Source/core/animation/El... File Source/core/animation/ElementAnimation.cpp (right): https://codereview.chromium.org/96283002/diff/130001/Source/core/animation/El... Source/core/animation/ElementAnimation.cpp:60: RELEASE_ASSERT_WITH_MESSAGE(false, "Web Animations not yet implemented: Handling for keyframes without specified offset."); On 2013/12/09 18:51:25, esprehn wrote: > Can you hit this with JS today if you turn the flag on? You should never be able > to crash a shipping build on an ASSERT even for features that are not done yet. Done. https://codereview.chromium.org/96283002/diff/130001/Source/core/animation/El... Source/core/animation/ElementAnimation.cpp:85: continue; On 2013/12/09 18:51:25, esprehn wrote: > This continue doesn't matter, you're at the end of the loop. Also you shouldn't > need to call the parser directly, MutableStylePropertySet::setProperty handles > parsing internally, just use setProperty(id, value) Done. https://codereview.chromium.org/96283002/diff/130001/Source/core/animation/El... Source/core/animation/ElementAnimation.cpp:90: RefPtr<KeyframeAnimationEffect> effect = KeyframeAnimationEffect::create(keyframes); On 2013/12/09 18:51:25, esprehn wrote: > This should be the return value of the styleResolver method. > > RefPtr<KeyframeAnimationEffect> ... = StyleResolver::createKeyframes(...); Done. https://codereview.chromium.org/96283002/diff/130001/Source/core/css/resolver... File Source/core/css/resolver/StyleResolver.h (right): https://codereview.chromium.org/96283002/diff/130001/Source/core/css/resolver... Source/core/css/resolver/StyleResolver.h:127: static void setKeyframePropertyValues(Element*, const Vector<RefPtr<MutableStylePropertySet> >&, KeyframeAnimationEffect::KeyframeVector&); On 2013/12/09 18:51:25, esprehn wrote: > Should take an Element&, and return a KeyframeAnimationEffect. Done.
https://codereview.chromium.org/96283002/diff/140001/Source/core/css/resolver... File Source/core/css/resolver/StyleResolver.cpp (right): https://codereview.chromium.org/96283002/diff/140001/Source/core/css/resolver... Source/core/css/resolver/StyleResolver.cpp:862: PassRefPtr<KeyframeAnimationEffect> StyleResolver::setKeyframePropertyValues(Element* element, const Vector<RefPtr<MutableStylePropertySet> >& propertySetVector, KeyframeAnimationEffect::KeyframeVector& keyframes) createKeyframeAnimationEffect(). This is not a setter.
Add unit test for animate(). WIP but thought you might want to have a look, shans.
https://codereview.chromium.org/96283002/diff/160001/LayoutTests/virtual/lega... File LayoutTests/virtual/legacy-animations-engine/animations/web-animations-api/element-animate-list-of-keyframes-expected.txt (right): https://codereview.chromium.org/96283002/diff/160001/LayoutTests/virtual/lega... LayoutTests/virtual/legacy-animations-engine/animations/web-animations-api/element-animate-list-of-keyframes-expected.txt:3: at Object.<anonymous> (file:///usr/local/google/home/rjwright/chromium/src/third_party/WebKit/LayoutTests/animations/api/element-animate-list-of-keyframes.html:35:8) Obviously we can't check in expected results like this that include a local path, and there's not much utility in expected results that represent a complete failure. We could mark all of virtual/legacy-animations-engine/animations/web-animations-api as skipped, or move web-animations-api to be a top-level directory in LayoutTests so that it's not run as part of the virtual suite. I suggest the latter, as the animations directory is really for CSS animations, and the Web Animations API is distinct from this. https://codereview.chromium.org/96283002/diff/160001/Source/core/animation/El... File Source/core/animation/ElementAnimationTest.cpp (right): https://codereview.chromium.org/96283002/diff/160001/Source/core/animation/El... Source/core/animation/ElementAnimationTest.cpp:88: Vector<Dictionary> jsData; jsKeyframes? jsKeyframesDictionary? https://codereview.chromium.org/96283002/diff/160001/Source/core/animation/El... Source/core/animation/ElementAnimationTest.cpp:102: EXPECT_EQ("100px", value1); These aren't testing anything to do with Web Animations, just V8 stuff, so they're prerequisities of the test and should probably be ASSERT_EQ/TRUE https://codereview.chromium.org/96283002/diff/160001/Source/core/animation/El... Source/core/animation/ElementAnimationTest.cpp:123: const AnimatableValue* k1Width = keyframes[0]->propertyValue(CSSPropertyWidth); Don't abbreviate - keyframe1Width https://codereview.chromium.org/96283002/diff/160001/Source/core/animation/El... Source/core/animation/ElementAnimationTest.cpp:125: ASSERT(k1Width && k2Width); Probably best to assert each separately https://codereview.chromium.org/96283002/diff/160001/Source/core/animation/El... Source/core/animation/ElementAnimationTest.cpp:129: EXPECT_TRUE(k2Width->isLength()); These 2 seem superfluous given the assertion above https://codereview.chromium.org/96283002/diff/160001/Source/core/animation/El... Source/core/animation/ElementAnimationTest.cpp:132: EXPECT_EQ("0px", static_cast<const AnimatableLength*>(k2Width)->toCSSValue()->cssText()); Use toAnimatableLength() (it also asserts isLength())
lgtm pending esprehn's review
https://codereview.chromium.org/96283002/diff/160001/LayoutTests/animations/w... File LayoutTests/animations/web-animations-api/element-animate-list-of-keyframes.html (right): https://codereview.chromium.org/96283002/diff/160001/LayoutTests/animations/w... LayoutTests/animations/web-animations-api/element-animate-list-of-keyframes.html:15: <html> This seems like an odd place for the <html> element, since this has the html5 doctype you don't need it anyway. https://codereview.chromium.org/96283002/diff/160001/Source/core/animation/El... File Source/core/animation/ElementAnimation.cpp (right): https://codereview.chromium.org/96283002/diff/160001/Source/core/animation/El... Source/core/animation/ElementAnimation.cpp:52: if (propertyName->find('-') != kNotFound) Check this before the lookup? https://codereview.chromium.org/96283002/diff/160001/Source/core/animation/El... Source/core/animation/ElementAnimation.cpp:126: RefPtr<KeyframeAnimationEffect> effect = StyleResolver::createKeyframeAnimationEffect(element, propertySetVector, keyframes); There should probably be a fixme here since resolving the property values at this point is just a stopgap. https://codereview.chromium.org/96283002/diff/160001/Source/core/animation/El... File Source/core/animation/ElementAnimationTest.cpp (right): https://codereview.chromium.org/96283002/diff/160001/Source/core/animation/El... Source/core/animation/ElementAnimationTest.cpp:66: element = Element::create(nullQName() , document.get()); Alternatively, just use the regular binding: document->createElement("foo", ASSERT_NO_EXCEPTION) https://codereview.chromium.org/96283002/diff/160001/Source/core/animation/El... Source/core/animation/ElementAnimationTest.cpp:73: document.release(); is this needed? https://codereview.chromium.org/96283002/diff/160001/Source/core/animation/El... Source/core/animation/ElementAnimationTest.cpp:129: EXPECT_TRUE(k2Width->isLength()); On 2013/12/10 23:26:45, Steve Block wrote: > These 2 seem superfluous given the assertion above The assertion should be removed. https://codereview.chromium.org/96283002/diff/160001/Source/core/animation/El... Source/core/animation/ElementAnimationTest.cpp:131: EXPECT_EQ("100px", static_cast<const AnimatableLength*>(k1Width)->toCSSValue()->cssText()); toAnimatableLength
You need to update style before doing any kind of style resolution, otherwise we can't correctly resolve relative units, named values, variables, etc. https://codereview.chromium.org/96283002/diff/180001/Source/core/animation/El... File Source/core/animation/ElementAnimation.cpp (right): https://codereview.chromium.org/96283002/diff/180001/Source/core/animation/El... Source/core/animation/ElementAnimation.cpp:64: // Doesn't handle prefixed properties. We won't be able to ship this until either transform is finished and unprefixed or you support prefixes. https://codereview.chromium.org/96283002/diff/180001/Source/core/animation/El... Source/core/animation/ElementAnimation.cpp:77: ASSERT(RuntimeEnabledFeatures::webAnimationsAPIEnabled()); You need an early return for when calling this on elements that are not in active documents. if (!document.isActive()) return; https://codereview.chromium.org/96283002/diff/180001/Source/core/animation/El... Source/core/animation/ElementAnimation.cpp:79: KeyframeAnimationEffect::KeyframeVector keyframes; You need to call updateLayoutIgnorePendingStylesheets() or updateStyleIfNeeded(), otherwise all relative values below are computed wrong since you didn't update style so 2em has nothing to resolve against. Does animations support animating from 20% to 50% on width? That would require doing a layout here. https://codereview.chromium.org/96283002/diff/180001/Source/core/animation/El... Source/core/animation/ElementAnimation.cpp:102: if (compositeString == String("add")) Don't wrap in String, you want the char* overload. You're doing a malloc here for no reason. == "add" https://codereview.chromium.org/96283002/diff/180001/Source/core/animation/El... Source/core/animation/ElementAnimation.cpp:116: // KeyframeAnimationEffect, store input keyframes and implement getFrames. I don't think this is how CSS generally works, we drop unknown properties. Your API should probably work the same. https://codereview.chromium.org/96283002/diff/180001/Source/core/animation/El... Source/core/animation/ElementAnimation.cpp:126: RefPtr<KeyframeAnimationEffect> effect = StyleResolver::createKeyframeAnimationEffect(element, propertySetVector, keyframes); document.ensureStyleResolver().createKeyframeAnimationEffect(...) https://codereview.chromium.org/96283002/diff/180001/Source/core/animation/El... Source/core/animation/ElementAnimation.cpp:135: ASSERT(timeline); Timeline should return a reference, but we can fix that in a future patch. https://codereview.chromium.org/96283002/diff/180001/Source/core/animation/El... File Source/core/animation/ElementAnimation.h (right): https://codereview.chromium.org/96283002/diff/180001/Source/core/animation/El... Source/core/animation/ElementAnimation.h:34: #include "bindings/v8/Dictionary.h" You're missing Vector.h here. https://codereview.chromium.org/96283002/diff/180001/Source/core/animation/El... Source/core/animation/ElementAnimation.h:41: nit: extra nl https://codereview.chromium.org/96283002/diff/180001/Source/core/css/resolver... File Source/core/css/resolver/StyleResolver.cpp (right): https://codereview.chromium.org/96283002/diff/180001/Source/core/css/resolver... Source/core/css/resolver/StyleResolver.cpp:862: PassRefPtr<KeyframeAnimationEffect> StyleResolver::createKeyframeAnimationEffect(Element* element, const Vector<RefPtr<MutableStylePropertySet> >& propertySetVector, KeyframeAnimationEffect::KeyframeVector& keyframes) Element& https://codereview.chromium.org/96283002/diff/180001/Source/core/css/resolver... Source/core/css/resolver/StyleResolver.cpp:867: StyleResolverState state(element->document(), element); This method should probably not be static, you want (document(), element)
Steve and Doug I've done your comments. Steve: I haven't forgotten the leakage test. https://codereview.chromium.org/96283002/diff/140001/Source/core/css/resolver... File Source/core/css/resolver/StyleResolver.cpp (right): https://codereview.chromium.org/96283002/diff/140001/Source/core/css/resolver... Source/core/css/resolver/StyleResolver.cpp:862: PassRefPtr<KeyframeAnimationEffect> StyleResolver::setKeyframePropertyValues(Element* element, const Vector<RefPtr<MutableStylePropertySet> >& propertySetVector, KeyframeAnimationEffect::KeyframeVector& keyframes) On 2013/12/10 03:49:00, esprehn wrote: > createKeyframeAnimationEffect(). > > This is not a setter. Done. https://codereview.chromium.org/96283002/diff/160001/LayoutTests/animations/w... File LayoutTests/animations/web-animations-api/element-animate-list-of-keyframes.html (right): https://codereview.chromium.org/96283002/diff/160001/LayoutTests/animations/w... LayoutTests/animations/web-animations-api/element-animate-list-of-keyframes.html:15: <html> On 2013/12/11 00:04:28, dstockwell wrote: > This seems like an odd place for the <html> element, since this has the html5 > doctype you don't need it anyway. Yeah I have no idea how to structure a web code TBH. Some people seem to do style then html then script, some people mince in all in together. I do not get it. https://codereview.chromium.org/96283002/diff/160001/LayoutTests/virtual/lega... File LayoutTests/virtual/legacy-animations-engine/animations/web-animations-api/element-animate-list-of-keyframes-expected.txt (right): https://codereview.chromium.org/96283002/diff/160001/LayoutTests/virtual/lega... LayoutTests/virtual/legacy-animations-engine/animations/web-animations-api/element-animate-list-of-keyframes-expected.txt:3: at Object.<anonymous> (file:///usr/local/google/home/rjwright/chromium/src/third_party/WebKit/LayoutTests/animations/api/element-animate-list-of-keyframes.html:35:8) On 2013/12/10 23:26:45, Steve Block wrote: > Obviously we can't check in expected results like this that include a local > path, and there's not much utility in expected results that represent a complete > failure. We could mark all of > virtual/legacy-animations-engine/animations/web-animations-api as skipped, or > move web-animations-api to be a top-level directory in LayoutTests so that it's > not run as part of the virtual suite. I suggest the latter, as the animations > directory is really for CSS animations, and the Web Animations API is distinct > from this. Done. https://codereview.chromium.org/96283002/diff/160001/Source/core/animation/El... File Source/core/animation/ElementAnimation.cpp (right): https://codereview.chromium.org/96283002/diff/160001/Source/core/animation/El... Source/core/animation/ElementAnimation.cpp:52: if (propertyName->find('-') != kNotFound) On 2013/12/11 00:04:28, dstockwell wrote: > Check this before the lookup? Done. https://codereview.chromium.org/96283002/diff/160001/Source/core/animation/El... Source/core/animation/ElementAnimation.cpp:126: RefPtr<KeyframeAnimationEffect> effect = StyleResolver::createKeyframeAnimationEffect(element, propertySetVector, keyframes); On 2013/12/11 00:04:28, dstockwell wrote: > There should probably be a fixme here since resolving the property values at > this point is just a stopgap. Huh? What are we going to change it to in future? https://codereview.chromium.org/96283002/diff/160001/Source/core/animation/El... File Source/core/animation/ElementAnimationTest.cpp (right): https://codereview.chromium.org/96283002/diff/160001/Source/core/animation/El... Source/core/animation/ElementAnimationTest.cpp:66: element = Element::create(nullQName() , document.get()); On 2013/12/11 00:04:28, dstockwell wrote: > Alternatively, just use the regular binding: > document->createElement("foo", ASSERT_NO_EXCEPTION) Done. https://codereview.chromium.org/96283002/diff/160001/Source/core/animation/El... Source/core/animation/ElementAnimationTest.cpp:73: document.release(); On 2013/12/11 00:04:28, dstockwell wrote: > is this needed? I don't know. Do you? https://codereview.chromium.org/96283002/diff/160001/Source/core/animation/El... Source/core/animation/ElementAnimationTest.cpp:88: Vector<Dictionary> jsData; On 2013/12/10 23:26:45, Steve Block wrote: > jsKeyframes? jsKeyframesDictionary? Done. https://codereview.chromium.org/96283002/diff/160001/Source/core/animation/El... Source/core/animation/ElementAnimationTest.cpp:102: EXPECT_EQ("100px", value1); On 2013/12/10 23:26:45, Steve Block wrote: > These aren't testing anything to do with Web Animations, just V8 stuff, so > they're prerequisities of the test and should probably be ASSERT_EQ/TRUE Done. https://codereview.chromium.org/96283002/diff/160001/Source/core/animation/El... Source/core/animation/ElementAnimationTest.cpp:123: const AnimatableValue* k1Width = keyframes[0]->propertyValue(CSSPropertyWidth); On 2013/12/10 23:26:45, Steve Block wrote: > Don't abbreviate - keyframe1Width Done. https://codereview.chromium.org/96283002/diff/160001/Source/core/animation/El... Source/core/animation/ElementAnimationTest.cpp:125: ASSERT(k1Width && k2Width); On 2013/12/10 23:26:45, Steve Block wrote: > Probably best to assert each separately Done. https://codereview.chromium.org/96283002/diff/160001/Source/core/animation/El... Source/core/animation/ElementAnimationTest.cpp:129: EXPECT_TRUE(k2Width->isLength()); On 2013/12/11 00:04:28, dstockwell wrote: > On 2013/12/10 23:26:45, Steve Block wrote: > > These 2 seem superfluous given the assertion above > > The assertion should be removed. Done. https://codereview.chromium.org/96283002/diff/160001/Source/core/animation/El... Source/core/animation/ElementAnimationTest.cpp:129: EXPECT_TRUE(k2Width->isLength()); On 2013/12/10 23:26:45, Steve Block wrote: > These 2 seem superfluous given the assertion above Done. https://codereview.chromium.org/96283002/diff/160001/Source/core/animation/El... Source/core/animation/ElementAnimationTest.cpp:131: EXPECT_EQ("100px", static_cast<const AnimatableLength*>(k1Width)->toCSSValue()->cssText()); On 2013/12/11 00:04:28, dstockwell wrote: > toAnimatableLength Done. https://codereview.chromium.org/96283002/diff/160001/Source/core/animation/El... Source/core/animation/ElementAnimationTest.cpp:132: EXPECT_EQ("0px", static_cast<const AnimatableLength*>(k2Width)->toCSSValue()->cssText()); On 2013/12/10 23:26:45, Steve Block wrote: > Use toAnimatableLength() (it also asserts isLength()) Done.
NB: New issue description. https://codereview.chromium.org/96283002/diff/180001/Source/core/animation/El... File Source/core/animation/ElementAnimation.cpp (right): https://codereview.chromium.org/96283002/diff/180001/Source/core/animation/El... Source/core/animation/ElementAnimation.cpp:64: // Doesn't handle prefixed properties. On 2013/12/11 00:26:11, esprehn wrote: > We won't be able to ship this until either transform is finished and unprefixed > or you support prefixes. See issue description. https://codereview.chromium.org/96283002/diff/180001/Source/core/animation/El... Source/core/animation/ElementAnimation.cpp:77: ASSERT(RuntimeEnabledFeatures::webAnimationsAPIEnabled()); On 2013/12/11 00:26:11, esprehn wrote: > You need an early return for when calling this on elements that are not in > active documents. > > if (!document.isActive()) > return; Why? https://codereview.chromium.org/96283002/diff/180001/Source/core/animation/El... Source/core/animation/ElementAnimation.cpp:79: KeyframeAnimationEffect::KeyframeVector keyframes; On 2013/12/11 00:26:11, esprehn wrote: > You need to call updateLayoutIgnorePendingStylesheets() or > updateStyleIfNeeded(), otherwise all relative values below are computed wrong > since you didn't update style so 2em has nothing to resolve against. > > Does animations support animating from 20% to 50% on width? That would require > doing a layout here. See issue description. https://codereview.chromium.org/96283002/diff/180001/Source/core/animation/El... Source/core/animation/ElementAnimation.cpp:102: if (compositeString == String("add")) On 2013/12/11 00:26:11, esprehn wrote: > Don't wrap in String, you want the char* overload. You're doing a malloc here > for no reason. > > == "add" Done. https://codereview.chromium.org/96283002/diff/180001/Source/core/animation/El... Source/core/animation/ElementAnimation.cpp:116: // KeyframeAnimationEffect, store input keyframes and implement getFrames. On 2013/12/11 00:26:11, esprehn wrote: > I don't think this is how CSS generally works, we drop unknown properties. Your > API should probably work the same. This is required by the specification and is unrelated to CSS. It's a feature of the JS API. https://codereview.chromium.org/96283002/diff/180001/Source/core/animation/El... Source/core/animation/ElementAnimation.cpp:126: RefPtr<KeyframeAnimationEffect> effect = StyleResolver::createKeyframeAnimationEffect(element, propertySetVector, keyframes); On 2013/12/11 00:26:11, esprehn wrote: > document.ensureStyleResolver().createKeyframeAnimationEffect(...) As discussed in the issue description, this is a temporary measure until resolution-independent animation primitives have been introduced. Why do you think this needs to be non-static? https://codereview.chromium.org/96283002/diff/180001/Source/core/animation/El... File Source/core/animation/ElementAnimation.h (right): https://codereview.chromium.org/96283002/diff/180001/Source/core/animation/El... Source/core/animation/ElementAnimation.h:34: #include "bindings/v8/Dictionary.h" On 2013/12/11 00:26:11, esprehn wrote: > You're missing Vector.h here. Dictionary.h includes Vector.h. What is our policy on includes? Should you include everything you use or only what is actually required to compile? https://codereview.chromium.org/96283002/diff/180001/Source/core/animation/El... Source/core/animation/ElementAnimation.h:41: On 2013/12/11 00:26:11, esprehn wrote: > nit: extra nl Done. https://codereview.chromium.org/96283002/diff/180001/Source/core/css/resolver... File Source/core/css/resolver/StyleResolver.cpp (right): https://codereview.chromium.org/96283002/diff/180001/Source/core/css/resolver... Source/core/css/resolver/StyleResolver.cpp:862: PassRefPtr<KeyframeAnimationEffect> StyleResolver::createKeyframeAnimationEffect(Element* element, const Vector<RefPtr<MutableStylePropertySet> >& propertySetVector, KeyframeAnimationEffect::KeyframeVector& keyframes) On 2013/12/11 00:26:11, esprehn wrote: > Element& Done.
On 2013/12/06 04:36:10, Steve Block wrote: > Did you look into a test to avoid 'leaking' this prematurely? Done. The test fails when Element.animate() is exposed, so there is an expected failure in LayoutTests/webexposed, but expected pass in LayoutTests/virtual/stable/webexposed. Leakage will generate an unexpected failure.
>> Did you look into a test to avoid 'leaking' this >> prematurely? > Done. Thanks! https://codereview.chromium.org/96283002/diff/230001/LayoutTests/webexposed/e... File LayoutTests/webexposed/element-animate.html (right): https://codereview.chromium.org/96283002/diff/230001/LayoutTests/webexposed/e... LayoutTests/webexposed/element-animate.html:6: // and pass in LayoutTests/virtual/stable/webexposed (where flags are off). This comment is probably superfluous and likely to get stale. If you want to include this text, it should probably be part of the description.
https://codereview.chromium.org/96283002/diff/230001/LayoutTests/webexposed/e... File LayoutTests/webexposed/element-animate.html (right): https://codereview.chromium.org/96283002/diff/230001/LayoutTests/webexposed/e... LayoutTests/webexposed/element-animate.html:6: // and pass in LayoutTests/virtual/stable/webexposed (where flags are off). On 2013/12/11 05:24:08, Steve Block wrote: > This comment is probably superfluous and likely to get stale. If you want to > include this text, it should probably be part of the description. Done.
Can you explain what this means: "This will be switched out in the future in favor of an approach that doesn't resolve styles at the same time. ". How can you animate from 5em to 10em without resolving style? When this is all done, where will the style resolve happen?
On 2013/12/11 07:44:26, esprehn wrote: > Can you explain what this means: "This will be switched out in the future in > favor of an approach that doesn't resolve styles at the same time. ". > > How can you animate from 5em to 10em without resolving style? When this is all > done, where will the style resolve happen? This will likely happen the same time it normally does, during style recalc (when we interpolate and apply to the render style).
On 2013/12/11 08:20:09, dstockwell wrote: > On 2013/12/11 07:44:26, esprehn wrote: > > Can you explain what this means: "This will be switched out in the future in > > favor of an approach that doesn't resolve styles at the same time. ". > > > > How can you animate from 5em to 10em without resolving style? When this is all > > done, where will the style resolve happen? > > This will likely happen the same time it normally does, during style recalc > (when we interpolate and apply to the render style). Okay, please fix my other comments and reupload and this is probably fine to land.
lgtm As best I can tell, the only thing still outstanding is the question around active documents. For that and anything else that comes up I think Renee can land and iterate at this point.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/rjwright@chromium.org/96283002/70002
On 2013/12/11 09:00:33, dstockwell wrote: > lgtm > > As best I can tell, the only thing still outstanding is the question around > active documents. For that and anything else that comes up I think Renee can > land and iterate at this point. No, I mentioned that she needs to call updateStyleIfNeeded() before resolving style. The only reason this patch appears to work is because the test calls getComputedStyle before trying to use the animate API. You cannot even get your parentStyle(), which is what is happening inside the StyleResolver method, without doing that first because your parent may not be attached yet or may have a dirty style. In the test, had she not called getComputedStyle none of the elements would even be attached and so resolving style against them should fail (and ideally ASSERT but that doesn't quite work yet). I also asked that the method become a real method of StyleResolver, not something that was static, but that's not critical. She also needs to check element->inActiveDocument() (my bad, I said document.isActive()) at the top too. You cannot resolve style (and therefore cannot run animations) against an element not in an active document. There's no way to resolve relative units, and calling into the resolver when your parent has no style and you have no RenderView is an error and will/should crash. This was all in my previous review, please don't check the cq.
btw trivial fix for the update style part is probably: if (!element->inActiveDocument()) return; element->document().updateStyleIfNeeded(); if (!element->renderer()) return; The renderer() bit is to deal with display: none.
On 2013/12/11 10:04:24, esprehn wrote: > btw trivial fix for the update style part is probably: > > if (!element->inActiveDocument()) > return; > element->document().updateStyleIfNeeded(); > if (!element->renderer()) > return; > > The renderer() bit is to deal with display: none. Done
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/rjwright@chromium.org/96283002/260001
lgtm, one nit https://codereview.chromium.org/96283002/diff/270001/Source/core/animation/El... File Source/core/animation/ElementAnimation.cpp (right): https://codereview.chromium.org/96283002/diff/270001/Source/core/animation/El... Source/core/animation/ElementAnimation.cpp:77: } nit: braces are not needed for one line if statemenet
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/rjwright@chromium.org/96283002/280014
Message was sent while issue was closed.
Change committed as 163761 |
