Bug 208356 – updateCSSTransitionsForElementAndProperty should clone RenderStyles

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Dean Jackson]

updateCSSTransitionsForElementAndProperty should clone RenderStyles

[Dean Jackson]

rdar://59869560

[Dean Jackson]

Created attachment 391944 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 391944 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=391944&action=review

> Source/WebCore/animation/AnimationTimeline.cpp:462
> +    auto beforeChangeStyle = [&]() -> RenderStyle {

Why not return Ref<RenderStyle>?

[Ryosuke Niwa]

Comment on attachment 391944 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=391944&action=review

>> Source/WebCore/animation/AnimationTimeline.cpp:462
>> +    auto beforeChangeStyle = [&]() -> RenderStyle {
> 
> Why not return Ref<RenderStyle>?

RenderStyle isn't a Ref counted object, right?

> Source/WebCore/animation/AnimationTimeline.cpp:472
> +                    return animatedStyle;

Another approach is to have an unique_ptr declared outside of this function to which this code stores RenderStyle.

> Source/WebCore/animation/AnimationTimeline.cpp:482
> -                return *unanimatedStyle;
> +                return RenderStyle::clone(*unanimatedStyle);

That way, we don't have to make a clone here.

[Antti Koivisto]

Comment on attachment 391944 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=391944&action=review

>>> Source/WebCore/animation/AnimationTimeline.cpp:462
>>> +    auto beforeChangeStyle = [&]() -> RenderStyle {
>> 
>> Why not return Ref<RenderStyle>?
> 
> RenderStyle isn't a Ref counted object, right?

Probably don't need to specify the return type explicitly, that is [&] { should be sufficient.

>> Source/WebCore/animation/AnimationTimeline.cpp:482
>> +                return RenderStyle::clone(*unanimatedStyle);
> 
> That way, we don't have to make a clone here.

RenderStyle is an internally refcounted copy-on-write type. Cloning it is ok.

[Dean Jackson]

Comment on attachment 391944 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=391944&action=review

>>>> Source/WebCore/animation/AnimationTimeline.cpp:462
>>>> +    auto beforeChangeStyle = [&]() -> RenderStyle {
>>> 
>>> Why not return Ref<RenderStyle>?
>> 
>> RenderStyle isn't a Ref counted object, right?
> 
> Probably don't need to specify the return type explicitly, that is [&] { should be sufficient.

Nice! (and yes, can't make a Ref<>)

>> Source/WebCore/animation/AnimationTimeline.cpp:472
>> +                    return animatedStyle;
> 
> Another approach is to have an unique_ptr declared outside of this function to which this code stores RenderStyle.

I considered that.

>>> Source/WebCore/animation/AnimationTimeline.cpp:482
>>> +                return RenderStyle::clone(*unanimatedStyle);
>> 
>> That way, we don't have to make a clone here.
> 
> RenderStyle is an internally refcounted copy-on-write type. Cloning it is ok.

My unnamed "expert" gave me the same advice. It would be nice if you could just return foo directly without calling clone, but I guess that would mean the copy constructor would have to not be deleted?

[Antti Koivisto]

> My unnamed "expert" gave me the same advice. It would be nice if you could
> just return foo directly without calling clone, but I guess that would mean
> the copy constructor would have to not be deleted?

It is movable so if you have a local instance just returning it will work.

I haven't made it copyable so far since it has a bunch of refcounted subobjects and copies are not entirely trivial. But it should probably be, for ease of use.

[Dean Jackson]

Committed r257640: <https://trac.webkit.org/changeset/257640>