Bug 208356 - updateCSSTransitionsForElementAndProperty should clone RenderStyles
Summary: updateCSSTransitionsForElementAndProperty should clone RenderStyles
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Dean Jackson
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2020-02-27 17:04 PST by Dean Jackson
Modified: 2020-02-28 09:34 PST (History)
6 users (show)

See Also:


Attachments
Patch (4.21 KB, patch)
2020-02-27 17:07 PST, Dean Jackson
koivisto: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Dean Jackson 2020-02-27 17:04:55 PST
updateCSSTransitionsForElementAndProperty should clone RenderStyles
Comment 1 Dean Jackson 2020-02-27 17:05:39 PST
rdar://59869560
Comment 2 Dean Jackson 2020-02-27 17:07:27 PST
Created attachment 391944 [details]
Patch
Comment 3 Simon Fraser (smfr) 2020-02-27 17:34:15 PST
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>?
Comment 4 Ryosuke Niwa 2020-02-27 21:28:36 PST
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.
Comment 5 Antti Koivisto 2020-02-28 01:10:16 PST
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.
Comment 6 Dean Jackson 2020-02-28 02:52:20 PST
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?
Comment 7 Antti Koivisto 2020-02-28 04:31:06 PST
> 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.
Comment 8 Dean Jackson 2020-02-28 09:34:45 PST
Committed r257640: <https://trac.webkit.org/changeset/257640>