updateCSSTransitionsForElementAndProperty should clone RenderStyles
rdar://59869560
Created attachment 391944 [details] Patch
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 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 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 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?
> 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.
Committed r257640: <https://trac.webkit.org/changeset/257640>