| Summary: | updateCSSTransitionsForElementAndProperty should clone RenderStyles | ||||||
|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Dean Jackson <dino> | ||||
| Component: | New Bugs | Assignee: | Dean Jackson <dino> | ||||
| Status: | RESOLVED FIXED | ||||||
| Severity: | Normal | CC: | graouts, koivisto, rniwa, simon.fraser, webkit-bug-importer, zalan | ||||
| Priority: | P2 | Keywords: | InRadar | ||||
| Version: | WebKit Nightly Build | ||||||
| Hardware: | Unspecified | ||||||
| OS: | Unspecified | ||||||
| Attachments: |
|
||||||
|
Description
Dean Jackson
2020-02-27 17:04:55 PST
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> |