Prototype an implementation of animation-trigger, as described by https://lists.w3.org/Archives/Public/www-style/2014Sep/0135.html
<rdar://problem/20162335>
Created attachment 248632 [details] WIP
Attachment 248632 [details] did not pass style-queue: ERROR: Source/WebCore/css/CSSToStyleMap.cpp:529: One line control clauses should not use braces. [whitespace/braces] [4] ERROR: Source/WebCore/platform/animation/Animation.cpp:138: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Source/WebCore/platform/animation/Animation.cpp:151: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Source/WebCore/css/CSSComputedStyleDeclaration.cpp:1229: A case label should not be indented, but line up with its switch statement. [whitespace/indent] [4] ERROR: Source/WebCore/platform/animation/Animation.h:31: Alphabetical sorting problem. [build/include_order] [4] ERROR: Source/WebCore/platform/animation/Animation.h:189: Please declare enum bitfields as unsigned integral types. [runtime/enum_bitfields] [5] ERROR: Source/WebCore/ChangeLog:8: You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible. [changelog/nonewtests] [5] Total errors found: 7 in 29 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 248714 [details] Patch
Attachment 248714 [details] did not pass style-queue: ERROR: Source/WebCore/platform/animation/Animation.h:202: Please declare enum bitfields as unsigned integral types. [runtime/enum_bitfields] [5] ERROR: Source/WebCore/css/CSSComputedStyleDeclaration.cpp:1236: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Source/WebCore/css/CSSComputedStyleDeclaration.cpp:1237: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Total errors found: 3 in 21 files If any of these errors are false positives, please file a bug against check-webkit-style.
(In reply to comment #5) > Attachment 248714 [details] did not pass style-queue: > > > ERROR: Source/WebCore/platform/animation/Animation.h:202: Please declare > enum bitfields as unsigned integral types. [runtime/enum_bitfields] [5] > ERROR: Source/WebCore/css/CSSComputedStyleDeclaration.cpp:1236: Weird > number of spaces at line-start. Are you using a 4-space indent? > [whitespace/indent] [3] > ERROR: Source/WebCore/css/CSSComputedStyleDeclaration.cpp:1237: Weird > number of spaces at line-start. Are you using a 4-space indent? > [whitespace/indent] [3] > Total errors found: 3 in 21 files > I don't like either of those warnings. I'm going to file a bug for the first one at least.
Created attachment 248730 [details] Patch
Attachment 248730 [details] did not pass style-queue: ERROR: Source/WebCore/platform/animation/Animation.h:202: Please declare enum bitfields as unsigned integral types. [runtime/enum_bitfields] [5] ERROR: Source/WebCore/css/CSSComputedStyleDeclaration.cpp:1236: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Source/WebCore/css/CSSComputedStyleDeclaration.cpp:1237: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Total errors found: 3 in 21 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 248730 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=248730&action=review > Source/WebCore/css/CSSAnimationTriggerScrollValue.cpp:37 > + if (m_hasEndValue) > + return "container-scroll(" + m_startValue->cssText() + ", " + m_endValue->cssText() + ')'; > + return "container-scroll(" + m_startValue->cssText() + ')'; This should probably use StringBuilder. > Source/WebCore/css/CSSAnimationTriggerScrollValue.cpp:42 > + return m_startValue->equals(*other.m_startValue.get()) && m_endValue->equals(*other.m_endValue.get()) && m_hasEndValue == other.m_hasEndValue; Check m_hasEndValue == other.m_hasEndValue before comparing m_endValues? > Source/WebCore/css/CSSAnimationTriggerScrollValue.h:44 > + CSSValue* startValue() const { return m_startValue.get(); } > + CSSValue* endValue() const { return m_endValue.get(); } Return const CSSValue* > Source/WebCore/css/CSSAnimationTriggerScrollValue.h:62 > + RefPtr<CSSValue> m_endValue; > + bool m_hasEndValue; Do you need to store m_hasEndValue rather than just null-check m_endValue? > Source/WebCore/css/CSSComputedStyleDeclaration.cpp:1230 > +static Ref<CSSValue> createAnimationTriggerValue(const AnimationTrigger* trigger, const RenderStyle* style) Pass AnimationTrigger by reference. > Source/WebCore/css/CSSParser.cpp:4768 > + if (!validateUnit(firstArgumentWithCalculation, FLength)) Has to be a length? No percentages? > Source/WebCore/css/CSSToStyleMap.h:71 > + void mapAnimationTrigger(Animation&, CSSValue&); These CSSValue& should all be const :| > Source/WebCore/platform/animation/Animation.h:141 > + const PassRefPtr<AnimationTrigger> trigger() const { return m_trigger; } This should return a raw const*. You're not passing off ownership. > Source/WebCore/platform/animation/Animation.h:155 > + void setTrigger(PassRefPtr<AnimationTrigger> f) { m_trigger = f; m_triggerSet = true; } f? Do you want to do m_triggerSet = true if the trigger is null? > Source/WebCore/platform/animation/AnimationTrigger.h:42 > + enum AnimationTriggerType { enum class. > Source/WebCore/platform/animation/AnimationTrigger.h:101 > + if (other.isScrollAnimationTrigger()) { Early return would be better. > Source/WebCore/platform/animation/AnimationTrigger.h:105 > + const ScrollAnimationTrigger* otherTrigger = static_cast<const ScrollAnimationTrigger*>(&other); > + return m_startValue == otherTrigger->m_startValue > + && m_endValue == otherTrigger->m_endValue > + && m_hasEndValue == otherTrigger->m_hasEndValue; Doesn't ScrollAnimationTrigger have an operator== you can use?
Comment on attachment 248730 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=248730&action=review >> Source/WebCore/css/CSSAnimationTriggerScrollValue.cpp:37 >> + return "container-scroll(" + m_startValue->cssText() + ')'; > > This should probably use StringBuilder. Done. Now: StringBuilder result; result.append("container-scroll("); result.append(m_startValue->cssText()); if (m_hasEndValue) { result.append(", "); result.append(m_endValue->cssText()); } result.append(')'); return result.toString(); >> Source/WebCore/css/CSSAnimationTriggerScrollValue.cpp:42 >> + return m_startValue->equals(*other.m_startValue.get()) && m_endValue->equals(*other.m_endValue.get()) && m_hasEndValue == other.m_hasEndValue; > > Check m_hasEndValue == other.m_hasEndValue before comparing m_endValues? Done. >> Source/WebCore/css/CSSAnimationTriggerScrollValue.h:44 >> + CSSValue* endValue() const { return m_endValue.get(); } > > Return const CSSValue* Done >> Source/WebCore/css/CSSAnimationTriggerScrollValue.h:62 >> + bool m_hasEndValue; > > Do you need to store m_hasEndValue rather than just null-check m_endValue? Removed. >> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:1230 >> +static Ref<CSSValue> createAnimationTriggerValue(const AnimationTrigger* trigger, const RenderStyle* style) > > Pass AnimationTrigger by reference. I don't think there is any need to do this, and the rest of CSSComputedStyleDeclaration passes around the const pointers here. >> Source/WebCore/css/CSSParser.cpp:4768 >> + if (!validateUnit(firstArgumentWithCalculation, FLength)) > > Has to be a length? No percentages? For now it is a length. I'll add percentages soon (and you'll definitely want them). >> Source/WebCore/css/CSSToStyleMap.h:71 >> + void mapAnimationTrigger(Animation&, CSSValue&); > > These CSSValue& should all be const :| Separate patch. >> Source/WebCore/platform/animation/Animation.h:141 >> + const PassRefPtr<AnimationTrigger> trigger() const { return m_trigger; } > > This should return a raw const*. You're not passing off ownership. We can't do this due to the way StyleBuilder works. It expects to get and set RefPtrs. >> Source/WebCore/platform/animation/Animation.h:155 >> + void setTrigger(PassRefPtr<AnimationTrigger> f) { m_trigger = f; m_triggerSet = true; } > > f? Do you want to do m_triggerSet = true if the trigger is null? Yeah - we want to know if it is ever been changed from the initial value. >> Source/WebCore/platform/animation/AnimationTrigger.h:42 >> + enum AnimationTriggerType { > > enum class. Done >> Source/WebCore/platform/animation/AnimationTrigger.h:105 >> + && m_hasEndValue == otherTrigger->m_hasEndValue; > > Doesn't ScrollAnimationTrigger have an operator== you can use? This is ScrollAnimationTrigger::operator== :)
Committed r181572: <http://trac.webkit.org/changeset/181572>
Looks like this broke a test: https://webkit-test-results.appspot.com/dashboards/flakiness_dashboard.html#showAllRuns=true&tests=transitions%2Ftransitions-parsing.html
Also this one: https://webkit-test-results.appspot.com/dashboards/flakiness_dashboard.html#showAllRuns=true&tests=animations%2Fanimation-shorthand.html
And now this one: https://build.webkit.org/results/Apple%20Yosemite%20Release%20WK2%20(Tests)/r181581%20(3643)/animations/fill-unset-properties-pretty-diff.html Every time, the parsed list values are in wrong order and on different tests. It looks like this patch caused weird side effects.
Re-opened since this is blocked by bug 142755
Landed again in r181602