208356 2020-02-27 17:04:55 -0800 updateCSSTransitionsForElementAndProperty should clone RenderStyles 2020-02-28 09:34:45 -0800 1 1 1 Unclassified WebKit New Bugs WebKit Nightly Build Unspecified Unspecified RESOLVED FIXED InRadar P2 Normal --- 1 dino dino graouts koivisto rniwa simon.fraser webkit-bug-importer zalan oldest_to_newest 1623781 0 dino 2020-02-27 17:04:55 -0800 updateCSSTransitionsForElementAndProperty should clone RenderStyles 1623782 1 dino 2020-02-27 17:05:39 -0800 rdar://59869560 1623783 2 391944 dino 2020-02-27 17:07:27 -0800 Created attachment 391944 Patch 1623798 3 391944 simon.fraser 2020-02-27 17:34:15 -0800 Comment on attachment 391944 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>? 1623875 4 391944 rniwa 2020-02-27 21:28:36 -0800 Comment on attachment 391944 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. 1623907 5 391944 koivisto 2020-02-28 01:10:16 -0800 Comment on attachment 391944 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. 1623921 6 391944 dino 2020-02-28 02:52:20 -0800 Comment on attachment 391944 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? 1623932 7 koivisto 2020-02-28 04:31:06 -0800 > 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. 1623977 8 dino 2020-02-28 09:34:45 -0800 Committed r257640: <https://trac.webkit.org/changeset/257640> 391944 2020-02-27 17:07:27 -0800 2020-02-28 01:10:16 -0800 Patch bug-208356-20200228120724.patch text/plain 4306 dino U3VidmVyc2lvbiBSZXZpc2lvbjogMjU3NTk1CmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViQ29yZS9D aGFuZ2VMb2cgYi9Tb3VyY2UvV2ViQ29yZS9DaGFuZ2VMb2cKaW5kZXggOWVmNDg2MzQzZDRhNjIw N2E3ODNmZGJkMTk5YzVkNzFmOTIwYTI4My4uMGU2OTc0ODAyMDgzYTQxZTk2MDdiNWY1NWQzYzUx NWQ0ZDRjNWNmOCAxMDA2NDQKLS0tIGEvU291cmNlL1dlYkNvcmUvQ2hhbmdlTG9nCisrKyBiL1Nv dXJjZS9XZWJDb3JlL0NoYW5nZUxvZwpAQCAtMSwzICsxLDE3IEBACisyMDIwLTAyLTI3ICBEZWFu IEphY2tzb24gIDxkaW5vQGFwcGxlLmNvbT4KKworICAgICAgICB1cGRhdGVDU1NUcmFuc2l0aW9u c0ZvckVsZW1lbnRBbmRQcm9wZXJ0eSBzaG91bGQgY2xvbmUgUmVuZGVyU3R5bGVzCisgICAgICAg IGh0dHBzOi8vYnVncy53ZWJraXQub3JnL3Nob3dfYnVnLmNnaT9pZD0yMDgzNTYKKworICAgICAg ICBSZXZpZXdlZCBieSBOT0JPRFkgKE9PUFMhKS4KKworICAgICAgICBNYWtlIG93bmVyc2hpcCBv ZiB0aGUgbG9jYWwgdmFyaWFibGUgY2xlYXIgYnkgY2xvbmluZyB0aGUgUmVuZGVyU3R5bGVzCisg ICAgICAgIHVzZWQgaW4gdXBkYXRlQ1NTVHJhbnNpdGlvbnNGb3JFbGVtZW50QW5kUHJvcGVydHkg cmF0aGVyIHRoYW4gcmVmZXJlbmNpbmcKKyAgICAgICAgZGlmZmVyZW50IHZlcnNpb25zLgorCisg ICAgICAgICogYW5pbWF0aW9uL0FuaW1hdGlvblRpbWVsaW5lLmNwcDoKKyAgICAgICAgKFdlYkNv cmU6OkFuaW1hdGlvblRpbWVsaW5lOjp1cGRhdGVDU1NUcmFuc2l0aW9uc0ZvckVsZW1lbnRBbmRQ cm9wZXJ0eSk6CisKIDIwMjAtMDItMjcgIERhbmllbCBCYXRlcyAgPGRhYmF0ZXNAYXBwbGUuY29t PgogCiAgICAgICAgIENoYW5nZSBIaXRUZXN0UmVxdWVzdFR5cGUgdG8gYW4gT3B0aW9uU2V0CmRp ZmYgLS1naXQgYS9Tb3VyY2UvV2ViQ29yZS9hbmltYXRpb24vQW5pbWF0aW9uVGltZWxpbmUuY3Bw IGIvU291cmNlL1dlYkNvcmUvYW5pbWF0aW9uL0FuaW1hdGlvblRpbWVsaW5lLmNwcAppbmRleCA1 MjZhNWY3ZmU2NTUyZjcyODU5Mjk2MzdhNDkzMTJlNjU3MWZhM2E2Li42ZjQ3ZDBiYTRhM2NjZDY5 YjhkNTE2ZDg0NDMxOWEyZjU0NzYyN2FjIDEwMDY0NAotLS0gYS9Tb3VyY2UvV2ViQ29yZS9hbmlt YXRpb24vQW5pbWF0aW9uVGltZWxpbmUuY3BwCisrKyBiL1NvdXJjZS9XZWJDb3JlL2FuaW1hdGlv bi9BbmltYXRpb25UaW1lbGluZS5jcHAKQEAgLTQ1OSw3ICs0NTksNyBAQCB2b2lkIEFuaW1hdGlv blRpbWVsaW5lOjp1cGRhdGVDU1NUcmFuc2l0aW9uc0ZvckVsZW1lbnRBbmRQcm9wZXJ0eShFbGVt ZW50JiBlbGVtZQogICAgIC8vIERlZmluZSB0aGUgYmVmb3JlLWNoYW5nZSBzdHlsZSBhcyB0aGUg Y29tcHV0ZWQgdmFsdWVzIG9mIGFsbCBwcm9wZXJ0aWVzIG9uIHRoZSBlbGVtZW50IGFzIG9mIHRo ZSBwcmV2aW91cyBzdHlsZSBjaGFuZ2UgZXZlbnQsIGV4Y2VwdCB3aXRoCiAgICAgLy8gYW55IHN0 eWxlcyBkZXJpdmVkIGZyb20gZGVjbGFyYXRpdmUgYW5pbWF0aW9ucyBzdWNoIGFzIENTUyBUcmFu c2l0aW9ucywgQ1NTIEFuaW1hdGlvbnMsIGFuZCBTTUlMIEFuaW1hdGlvbnMgdXBkYXRlZCB0byB0 aGUgY3VycmVudCB0aW1lLgogICAgIGJvb2wgaGFzUnVubmluZ1RyYW5zaXRpb24gPSBydW5uaW5n VHJhbnNpdGlvbnNCeVByb3BlcnR5LmNvbnRhaW5zKHByb3BlcnR5KTsKLSAgICBhdXRvJiBiZWZv cmVDaGFuZ2VTdHlsZSA9IFsmXSgpIC0+IGNvbnN0IFJlbmRlclN0eWxlJiB7CisgICAgYXV0byBi ZWZvcmVDaGFuZ2VTdHlsZSA9IFsmXSgpIC0+IFJlbmRlclN0eWxlIHsKICAgICAgICAgaWYgKGhh c1J1bm5pbmdUcmFuc2l0aW9uICYmIENTU1Byb3BlcnR5QW5pbWF0aW9uOjphbmltYXRpb25PZlBy b3BlcnR5SXNBY2NlbGVyYXRlZChwcm9wZXJ0eSkpIHsKICAgICAgICAgICAgIC8vIEluIGNhc2Ug d2UgaGF2ZSBhbiBhY2NlbGVyYXRlZCB0cmFuc2l0aW9uIHJ1bm5pbmcgZm9yIHRoaXMgZWxlbWVu dCwgd2UgbmVlZCB0byBnZXQgaXRzIGNvbXB1dGVkIHN0eWxlIGFzIHRoZSBiZWZvcmUtY2hhbmdl IHN0eWxlCiAgICAgICAgICAgICAvLyBzaW5jZSBvdGhlcndpc2UgdGhlIGFuaW1hdGVkIHZhbHVl IGZvciB0aGF0IHByb3BlcnR5IHdvbid0IGJlIHZpc2libGUuCkBAIC00NjcsOSArNDY3LDkgQEAg dm9pZCBBbmltYXRpb25UaW1lbGluZTo6dXBkYXRlQ1NTVHJhbnNpdGlvbnNGb3JFbGVtZW50QW5k UHJvcGVydHkoRWxlbWVudCYgZWxlbWUKICAgICAgICAgICAgIGlmIChpczxLZXlmcmFtZUVmZmVj dD4ocnVubmluZ1RyYW5zaXRpb24tPmVmZmVjdCgpKSkgewogICAgICAgICAgICAgICAgIGF1dG8m IGtleWZyYW1lRWZmZWN0ID0gKmRvd25jYXN0PEtleWZyYW1lRWZmZWN0PihydW5uaW5nVHJhbnNp dGlvbi0+ZWZmZWN0KCkpOwogICAgICAgICAgICAgICAgIGlmIChrZXlmcmFtZUVmZmVjdC5pc1J1 bm5pbmdBY2NlbGVyYXRlZCgpKSB7Ci0gICAgICAgICAgICAgICAgICAgIGF1dG8gYW5pbWF0ZWRT dHlsZSA9IFJlbmRlclN0eWxlOjpjbG9uZVB0cihjdXJyZW50U3R5bGUpOwotICAgICAgICAgICAg ICAgICAgICBydW5uaW5nVHJhbnNpdGlvbi0+cmVzb2x2ZSgqYW5pbWF0ZWRTdHlsZSk7Ci0gICAg ICAgICAgICAgICAgICAgIHJldHVybiAqYW5pbWF0ZWRTdHlsZTsKKyAgICAgICAgICAgICAgICAg ICAgYXV0byBhbmltYXRlZFN0eWxlID0gUmVuZGVyU3R5bGU6OmNsb25lKGN1cnJlbnRTdHlsZSk7 CisgICAgICAgICAgICAgICAgICAgIHJ1bm5pbmdUcmFuc2l0aW9uLT5yZXNvbHZlKGFuaW1hdGVk U3R5bGUpOworICAgICAgICAgICAgICAgICAgICByZXR1cm4gYW5pbWF0ZWRTdHlsZTsKICAgICAg ICAgICAgICAgICB9CiAgICAgICAgICAgICB9CiAgICAgICAgIH0KQEAgLTQ3OSwxNyArNDc5LDE3 IEBAIHZvaWQgQW5pbWF0aW9uVGltZWxpbmU6OnVwZGF0ZUNTU1RyYW5zaXRpb25zRm9yRWxlbWVu dEFuZFByb3BlcnR5KEVsZW1lbnQmIGVsZW1lCiAgICAgICAgICAgICAvLyBzdGFydCB2YWx1ZSBv ZiB0aGUgdHJhbnNpdGlvbiBzaG91ZCBiZSB0byBtYWtlIHN1cmUgdGhhdCB3ZSBkb24ndCBhY2Nv dW50IGZvciBhbmltYXRlZCB2YWx1ZXMgdGhhdCB3b3VsZCBoYXZlIGJlZW4gYmxlbmRlZCBvbnRv CiAgICAgICAgICAgICAvLyB0aGUgc3R5bGUgYXBwbGllZCBkdXJpbmcgdGhlIGxhc3Qgc3R5bGUg cmVzb2x1dGlvbi4KICAgICAgICAgICAgIGlmIChhdXRvKiB1bmFuaW1hdGVkU3R5bGUgPSBrZXlm cmFtZUVmZmVjdC0+dW5hbmltYXRlZFN0eWxlKCkpCi0gICAgICAgICAgICAgICAgcmV0dXJuICp1 bmFuaW1hdGVkU3R5bGU7CisgICAgICAgICAgICAgICAgcmV0dXJuIFJlbmRlclN0eWxlOjpjbG9u ZSgqdW5hbmltYXRlZFN0eWxlKTsKIAogICAgICAgICAgICAgLy8gSWYgd2UgaGF2ZSBhIGtleWZy YW1lIGVmZmVjdCB0YXJnZXRpbmcgdGhpcyBwcm9wZXJ0eSwgYnV0IGl0IGRvZXNuJ3QgeWV0IGhh dmUgYW4gdW5hbmltYXRlZCBzdHlsZSwgdGhpcyBpcyBiZWNhdXNlIGl0IGhhcyBub3QKICAgICAg ICAgICAgIC8vIGhhZCBhIGNoYW5jZSB0byBhcHBseSBpdHNlbGYgd2l0aCBhIG5vbi1udWxsIHBy b2dyZXNzLiBJbiB0aGlzIGNhc2UsIHRoZSBiZWZvcmUtY2hhbmdlIGFuZCBhZnRlci1jaGFuZ2Ug c3R5bGVzIHNob3VsZCBiZSB0aGUKICAgICAgICAgICAgIC8vIHNhbWUgaW4gb3JkZXIgdG8gcHJl dmVudCBhIHRyYW5zaXRpb24gZnJvbSBiZWluZyB0cmlnZ2VyZWQgYXMgdGhlIHVuYW5pbWF0ZWQg c3R5bGUgZm9yIHRoaXMga2V5ZnJhbWUgZWZmZWN0IHdpbGwgbW9zdCBsaWtlbHkKICAgICAgICAg ICAgIC8vIGJlIHRoaXMgYWZ0ZXItY2hhbmdlIHN0eWxlLCBvciBhbnkgZnV0dXJlIHN0eWxlIGNo YW5nZSB0aGF0IG1heSBoYXBwZW4gYmVmb3JlIHRoZSBrZXlmcmFtZSBlZmZlY3Qgc3RhcnRzIGJs ZW5kaW5nIGFuaW1hdGVkIHZhbHVlcy4KLSAgICAgICAgICAgIHJldHVybiBhZnRlckNoYW5nZVN0 eWxlOworICAgICAgICAgICAgcmV0dXJuIFJlbmRlclN0eWxlOjpjbG9uZShhZnRlckNoYW5nZVN0 eWxlKTsKICAgICAgICAgfQogCiAgICAgICAgIC8vIEluIGFueSBvdGhlciBzY2VuYXJpbywgdGhl IGJlZm9yZS1jaGFuZ2Ugc3R5bGUgc2hvdWxkIGJlIHRoZSBwcmV2aW91c2x5IHJlc29sdmVkIHN0 eWxlIGZvciB0aGlzIGVsZW1lbnQuCi0gICAgICAgIHJldHVybiBjdXJyZW50U3R5bGU7CisgICAg ICAgIHJldHVybiBSZW5kZXJTdHlsZTo6Y2xvbmUoY3VycmVudFN0eWxlKTsKICAgICB9KCk7CiAK ICAgICBpZiAoIWhhc1J1bm5pbmdUcmFuc2l0aW9uCg==