Consider : style.transitionTimingFunction = "ease"; computedStyle.transitionTimingFunction will return cubic-bezier(0.25, 0.1, 0.25, 1) as Opera or FF for example. It is valid for ease-out, ease-in, ease-in-out, ease where the return value will be similar (e.g. cubic-bezier(x1, y1 , x2 , y2)). For some reason linear does not return cubic-bezier(0, 0, 1, 1) but "linear" making it inconsistent with other possible values and also inconsistent with other UA.
Created attachment 180318 [details] Patch
Created attachment 180321 [details] Patch
[16:17] <smfr> darktears: at some point css-wg talked about returning the "shorthand" functions [16:17] <smfr> darktears: does the spec say anything about this? [16:17] <darktears> smfr: not a word [16:17] <darktears> smfr: the serialization is not really documented, at least I made them consistent [16:18] <darktears> /s/made/make [16:18] <smfr> TabAtkins: any opinion on ^^ [16:20] <TabAtkins> smfr: Serialization is undefined, obviously. I'm fine with always returning a cubic-bezier().
I disagree here. I think we should return 'linear' when we know it is a linear timing function. The fact that we implement it under the hood as a cubic bezier doesn't need to be exposed. We originally returned cubic-bezier in all cases, but once we added the steps timing function it meant that developers had to do more than just look for a regular expression if they were going to interpret computed timing functions. So I figured it was better to be clear (and closer to what the developer specified) and return 'linear' where possible. I probably intended to do the same to the 'ease' forms, but they are a little trickier. You could also imagine a future where we have more timing function forms - like "bounce" or trig functions.
(In reply to comment #4) > I disagree here. I think we should return 'linear' when we know it is a linear timing function. The fact that we implement it under the hood as a cubic bezier doesn't need to be exposed. > > We originally returned cubic-bezier in all cases, but once we added the steps timing function it meant that developers had to do more than just look for a regular expression if they were going to interpret computed timing functions. So I figured it was better to be clear (and closer to what the developer specified) and return 'linear' where possible. I probably intended to do the same to the 'ease' forms, but they are a little trickier. > > You could also imagine a future where we have more timing function forms - like "bounce" or trig functions. If we do that, then we should go ahead and do it for all of them. We can either guess based on float equality, or just plumb a flag through so that the parser can tell you when it read a keyword.
(In reply to comment #5) > If we do that, then we should go ahead and do it for all of them. We can either guess based on float equality, or just plumb a flag through so that the parser can tell you when it read a keyword. Yes, I agree. Plumbing it through is a bit annoying, but otherwise we might return linear if someone specified cubic-bezier(0, 0, 1, 1)
(In reply to comment #6) > (In reply to comment #5) > > > If we do that, then we should go ahead and do it for all of them. We can either guess based on float equality, or just plumb a flag through so that the parser can tell you when it read a keyword. > > Yes, I agree. Plumbing it through is a bit annoying, but otherwise we might return linear if someone specified cubic-bezier(0, 0, 1, 1) Ok I'll give a shot and rename the bug later if needed.
Created attachment 180890 [details] Patch
Comment on attachment 180890 [details] Patch Attachment 180890 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/15550954
Comment on attachment 180890 [details] Patch Attachment 180890 [details] did not pass qt-wk2-ews (qt): Output: http://queues.webkit.org/results/15549880
Comment on attachment 180890 [details] Patch Attachment 180890 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/15546978
Created attachment 180891 [details] Patch
Comment on attachment 180891 [details] Patch Attachment 180891 [details] did not pass qt-wk2-ews (qt): Output: http://queues.webkit.org/results/15551883
Comment on attachment 180891 [details] Patch Attachment 180891 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/15559789
Comment on attachment 180891 [details] Patch Attachment 180891 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/15557836 New failing tests: svg/css/getComputedStyle-basic.xhtml fast/sub-pixel/sub-pixel-iframe-copy-on-scroll.html fast/sub-pixel/sub-pixel-accumulates-to-layers.html fast/sub-pixel/transformed-iframe-copy-on-scroll.html
Created attachment 180895 [details] Attempt to fix WK2 builds with CoordinatedGraphics
Created attachment 180898 [details] Another attempt to fix WK2 builds with CoordinatedGraphics
Comment on attachment 180898 [details] Another attempt to fix WK2 builds with CoordinatedGraphics View in context: https://bugs.webkit.org/attachment.cgi?id=180898&action=review LGTM > Source/WebCore/css/CSSComputedStyleDeclaration.cpp:1108 > + } > + if (tf->isStepsTimingFunction()) { > + const StepsTimingFunction* stf = static_cast<const StepsTimingFunction*>(tf); > + return CSSStepsTimingFunctionValue::create(stf->numberOfSteps(), stf->stepAtStart()); > + } > + return CSSLinearTimingFunctionValue::create(); I would suggest a newline around this if > Source/WebCore/css/CSSComputedStyleDeclaration.cpp:1122 > const TimingFunction* tf = animList->animation(i)->timingFunction().get(); > - if (tf->isCubicBezierTimingFunction()) { > - const CubicBezierTimingFunction* ctf = static_cast<const CubicBezierTimingFunction*>(tf); > - list->append(CSSCubicBezierTimingFunctionValue::create(ctf->x1(), ctf->y1(), ctf->x2(), ctf->y2())); > - } else if (tf->isStepsTimingFunction()) { > - const StepsTimingFunction* stf = static_cast<const StepsTimingFunction*>(tf); > - list->append(CSSStepsTimingFunctionValue::create(stf->numberOfSteps(), stf->stepAtStart())); > - } else { > - list->append(CSSLinearTimingFunctionValue::create()); > - } > + list->append(createAnimationValue(tf)); > } > } else { > // Note that initialAnimationTimingFunction() is used for both transitions and animations > RefPtr<TimingFunction> tf = Animation::initialAnimationTimingFunction(); > - if (tf->isCubicBezierTimingFunction()) { > - const CubicBezierTimingFunction* ctf = static_cast<const CubicBezierTimingFunction*>(tf.get()); > - list->append(CSSCubicBezierTimingFunctionValue::create(ctf->x1(), ctf->y1(), ctf->x2(), ctf->y2())); > - } else if (tf->isStepsTimingFunction()) { > - const StepsTimingFunction* stf = static_cast<const StepsTimingFunction*>(tf.get()); > - list->append(CSSStepsTimingFunctionValue::create(stf->numberOfSteps(), stf->stepAtStart())); > - } else { > - list->append(CSSLinearTimingFunctionValue::create()); > - } > + list->append(createAnimationValue(tf.get())); why not use tf.get() both places. like change const TimingFunction* tf = animList->animation(i)->timingFunction().get(); to not include the .get() part? > Source/WebCore/platform/animation/TimingFunction.h:83 > + enum CubicBezierTimingFunctionType { > + Ease, EaseIn, EaseOut, EaseInOut, Custom > + }; I believe we should have those on separate lines > Source/WebCore/platform/animation/TimingFunction.h:88 > + > + static PassRefPtr<CubicBezierTimingFunction> create(CubicBezierTimingFunctionType type, double x1, double y1, double x2, double y2) > { > - return adoptRef(new CubicBezierTimingFunction(x1, y1, x2, y2)); > + return adoptRef(new CubicBezierTimingFunction(type, x1, y1, x2, y2)); > } In a way I don't like you call it type, when I can reuse a type with different values (which would then be considered an error). What about calling it ...FunctionDescription?
Created attachment 181027 [details] Patch
Comment on attachment 181027 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=181027&action=review > Source/WebCore/css/CSSComputedStyleDeclaration.cpp:1079 > +static PassRefPtr<CSSValue> createAnimationValue(const TimingFunction* tf) > +{ > + if (tf->isCubicBezierTimingFunction()) { You should expand 'tf' to timingFunction. > Source/WebCore/css/CSSComputedStyleDeclaration.cpp:1080 > + const CubicBezierTimingFunction* ctf = static_cast<const CubicBezierTimingFunction*>(tf); Avoid abbreviations like ctf. > Source/WebCore/css/CSSComputedStyleDeclaration.cpp:1082 > + CSSValueID id = CSSValueInvalid; Don't use 'id' (it's a reserved word in Objective-C). > Source/WebCore/css/CSSToStyleMap.cpp:460 > - animation->setTimingFunction(CubicBezierTimingFunction::create(0.42, 0.0, 1.0, 1.0)); > + animation->setTimingFunction(CubicBezierTimingFunction::create(CubicBezierTimingFunction::EaseIn, 0.42, 0.0, 1.0, 1.0)); Seems redundant to pass in both the enum, and the values that it corresponds to. Would be better to have an overloaded CubicBezierTimingFunction::create() and ctor that takes the enum only. > Source/WebCore/platform/animation/TimingFunction.h:105 > + return m_x1 == ctf->m_x1 && m_y1 == ctf->m_y1 && m_x2 == ctf->m_x2 && m_y2 == ctf->m_y2 && m_cubicBezierTimingFunctionDescription == ctf->m_cubicBezierTimingFunctionDescription; Can't you test the type first; you only need to test the values for custom functions.
Created attachment 181170 [details] Patch
Comment on attachment 181170 [details] Patch Attachment 181170 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/15635832 New failing tests: inspector-protocol/debugger-terminate-dedicated-worker-while-paused.html
Comment on attachment 181170 [details] Patch Putting cq? again as the test failing is completely unrelated to my change.
Comment on attachment 181170 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=181170&action=review > Source/WebCore/platform/animation/TimingFunction.h:81 > + enum CubicBezierTimingFunctionType { CubicBezierTimingFunctionType is a bit long-winded, esp. since this is in class scope. I think this would be better as TimingFunctionPreset or something, 'Type' seems a bit too general. > Source/WebCore/platform/animation/TimingFunction.h:122 > + return (m_cubicBezierTimingFunctionType != Custom && m_cubicBezierTimingFunctionType == ctf->m_cubicBezierTimingFunctionType) || (m_x1 == ctf->m_x1 && m_y1 == ctf->m_y1 && m_x2 == ctf->m_x2 && m_y2 == ctf->m_y2); I think this would be clearer as two separate if tests.
Created attachment 181197 [details] Patch for landing
Comment on attachment 181197 [details] Patch for landing Clearing flags on attachment: 181197 Committed r138728: <http://trac.webkit.org/changeset/138728>
All reviewed patches have been landed. Closing bug.
We have many new crashes after this patch on debug bots: http://build.webkit.org/results/EFL%20Linux%2064-bit%20Debug%20WK2/r138728%20(7648)/results.html
(In reply to comment #28) > We have many new crashes after this patch on debug bots: > > http://build.webkit.org/results/EFL%20Linux%2064-bit%20Debug%20WK2/r138728%20(7648)/results.html I filed Bug 106083 to track the regression and uploaded a bugfix patch there.