127337 2014-01-21 02:37:58 -0800 Fix SVG animations which set rx or ry attributes 2014-01-22 06:11:21 -0800 1 1 1 Unclassified WebKit SVG 528+ (Nightly build) Unspecified Unspecified RESOLVED FIXED BlinkMergeCandidate P2 Normal --- 1 pmolnar.u-szeged webkit-unassigned commit-queue d-r esprehn+autocc fmalita glenn gyuyoung.kim kondapallykalyan pdr schenney zimmermann oldest_to_newest 970211 0 pmolnar.u-szeged 2014-01-21 02:37:58 -0800 Fix SVG animations which set rx or ry attributes Merged from Blink: https://src.chromium.org/viewvc/blink?revision=152376&view=revision 970213 1 221730 pmolnar.u-szeged 2014-01-21 02:39:25 -0800 Created attachment 221730 patch 970222 2 221730 commit-queue 2014-01-21 05:24:46 -0800 Comment on attachment 221730 patch Clearing flags on attachment: 221730 Committed r162438: <http://trac.webkit.org/changeset/162438> 970223 3 commit-queue 2014-01-21 05:24:48 -0800 All reviewed patches have been landed. Closing bug. 970530 4 221730 darin 2014-01-21 18:41:59 -0800 Comment on attachment 221730 patch View in context: https://bugs.webkit.org/attachment.cgi?id=221730&action=review > Source/WebCore/rendering/svg/SVGPathData.cpp:119 > + bool hasRx = rect->rx().value(lengthContext) > 0; > + bool hasRy = rect->ry().value(lengthContext) > 0; Wasteful to repeat these expressions once to evaluate the has booleans and a second time if either is true. Should refactor so we don’t do it twice. Also, is > 0 really the correct test? Is that from the spec, or just copying what the Blink folks did? Did the Blink folks give a rationale for that particular test? 970538 5 pdr 2014-01-21 19:06:04 -0800 (In reply to comment #4) > (From update of attachment 221730 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=221730&action=review > > > Source/WebCore/rendering/svg/SVGPathData.cpp:119 > > + bool hasRx = rect->rx().value(lengthContext) > 0; > > + bool hasRy = rect->ry().value(lengthContext) > 0; > > Wasteful to repeat these expressions once to evaluate the has booleans and a second time if either is true. Should refactor so we don’t do it twice. Excellent catch Darin. We should definitely refactor that into a helper function. > > Also, is > 0 really the correct test? Is that from the spec, or just copying what the Blink folks did? Did the Blink folks give a rationale for that particular test? The Blink folks did a terrible job of describing this change and I apologize for that :( The spec is clear about negative rx/ry values in https://svgwg.org/svg2-draft/single-page.html#shapes-RectElement, and the new behavior matches Firefox (IE does not support SMIL). This change is really about making the negative rx/ry logic work for animations as well. The hasAttribute check is not appropriate for attributes that can be set by an animation element. 970678 6 pmolnar.u-szeged 2014-01-22 06:11:21 -0800 (In reply to comment #5) > (In reply to comment #4) > > (From update of attachment 221730 [details] [details]) > > View in context: https://bugs.webkit.org/attachment.cgi?id=221730&action=review > > > > > Source/WebCore/rendering/svg/SVGPathData.cpp:119 > > > + bool hasRx = rect->rx().value(lengthContext) > 0; > > > + bool hasRy = rect->ry().value(lengthContext) > 0; > > > > Wasteful to repeat these expressions once to evaluate the has booleans and a second time if either is true. Should refactor so we don’t do it twice. > > Excellent catch Darin. We should definitely refactor that into a helper function. > > > > > Also, is > 0 really the correct test? Is that from the spec, or just copying what the Blink folks did? Did the Blink folks give a rationale for that particular test? > > The Blink folks did a terrible job of describing this change and I apologize for that :( > The spec is clear about negative rx/ry values in https://svgwg.org/svg2-draft/single-page.html#shapes-RectElement, and the new behavior matches Firefox (IE does not support SMIL). This change is really about making the negative rx/ry logic work for animations as well. The hasAttribute check is not appropriate for attributes that can be set by an animation element. I created a new bug for the follow-up: https://bugs.webkit.org/show_bug.cgi?id=127423 The double calculation issue is resolved there. 221730 2014-01-21 02:39:25 -0800 2014-01-21 18:41:59 -0800 patch svg-rxry.patch text/plain 4777 pmolnar.u-szeged ZGlmZiAtLWdpdCBhL0xheW91dFRlc3RzL0NoYW5nZUxvZyBiL0xheW91dFRlc3RzL0NoYW5nZUxv ZwppbmRleCBjZmZmM2IyLi43ZTI4ZWQ0IDEwMDY0NAotLS0gYS9MYXlvdXRUZXN0cy9DaGFuZ2VM b2cKKysrIGIvTGF5b3V0VGVzdHMvQ2hhbmdlTG9nCkBAIC0xLDMgKzEsMTUgQEAKKzIwMTQtMDEt MjEgIFBldGVyIE1vbG5hciAgPHBtb2xuYXIudS1zemVnZWRAcGFydG5lci5zYW1zdW5nLmNvbT4K KworICAgICAgICBGaXggU1ZHIGFuaW1hdGlvbnMgd2hpY2ggc2V0IHJ4IG9yIHJ5IGF0dHJpYnV0 ZXMKKyAgICAgICAgaHR0cHM6Ly9idWdzLndlYmtpdC5vcmcvc2hvd19idWcuY2dpP2lkPTEyNzMz NworCisgICAgICAgIFJldmlld2VkIGJ5IE5PQk9EWSAoT09QUyEpLgorCisgICAgICAgIE1lcmdl ZCBmcm9tIEJsaW5rOiBodHRwczovL3NyYy5jaHJvbWl1bS5vcmcvdmlld3ZjL2JsaW5rP3Jldmlz aW9uPTE1MjM3NiZ2aWV3PXJldmlzaW9uCisKKyAgICAgICAgKiBzdmcvc3Ryb2tlL2FuaW1hdGVk LW5vbi1zY2FsaW5nLXJ4cnktZXhwZWN0ZWQuaHRtbDogQWRkZWQuCisgICAgICAgICogc3ZnL3N0 cm9rZS9hbmltYXRlZC1ub24tc2NhbGluZy1yeHJ5Lmh0bWw6IEFkZGVkLgorCiAyMDE0LTAxLTIx ICBLcnp5c3p0b2YgQ3plY2ggIDxrLmN6ZWNoQHNhbXN1bmcuY29tPgogCiAgICAgICAgIFtBVEtd IEV4cG9zZSBhcmlhLWZsb3d0byB0aHJvdWdoIEFUS19SRUxBVElPTl9GTE9XU19UTwpkaWZmIC0t Z2l0IGEvTGF5b3V0VGVzdHMvc3ZnL3N0cm9rZS9hbmltYXRlZC1ub24tc2NhbGluZy1yeHJ5LWV4 cGVjdGVkLmh0bWwgYi9MYXlvdXRUZXN0cy9zdmcvc3Ryb2tlL2FuaW1hdGVkLW5vbi1zY2FsaW5n LXJ4cnktZXhwZWN0ZWQuaHRtbApuZXcgZmlsZSBtb2RlIDEwMDY0NAppbmRleCAwMDAwMDAwLi5k N2M3Y2RmCi0tLSAvZGV2L251bGwKKysrIGIvTGF5b3V0VGVzdHMvc3ZnL3N0cm9rZS9hbmltYXRl ZC1ub24tc2NhbGluZy1yeHJ5LWV4cGVjdGVkLmh0bWwKQEAgLTAsMCArMSw5IEBACis8IURPQ1RZ UEUgaHRtbD4KKzxodG1sPgorPGJvZHk+CisgIFRlc3QgZm9yIGNyYnVnLmNvbS8yMzUyNTY6IHRo aXMgdGVzdCBwYXNzZXMgaWYgdGhlIHJlY3QgaXMgcm91bmRlZDxicj4KKyAgPHN2ZyB3aWR0aD0i MzAwIiBoZWlnaHQ9IjMwMCI+CisgICAgPHJlY3QgeD0iMjAiIHk9IjIwIiB3aWR0aD0iMjAwIiBo ZWlnaHQ9IjIwMCIgZmlsbD0iZ3JlZW4iIHJ4PSI1MCIgcnk9IjIwIj48L3JlY3Q+CisgIDwvc3Zn PgorPC9ib2R5PgorPC9odG1sPgpkaWZmIC0tZ2l0IGEvTGF5b3V0VGVzdHMvc3ZnL3N0cm9rZS9h bmltYXRlZC1ub24tc2NhbGluZy1yeHJ5Lmh0bWwgYi9MYXlvdXRUZXN0cy9zdmcvc3Ryb2tlL2Fu aW1hdGVkLW5vbi1zY2FsaW5nLXJ4cnkuaHRtbApuZXcgZmlsZSBtb2RlIDEwMDY0NAppbmRleCAw MDAwMDAwLi4xMjZkZTIzCi0tLSAvZGV2L251bGwKKysrIGIvTGF5b3V0VGVzdHMvc3ZnL3N0cm9r ZS9hbmltYXRlZC1ub24tc2NhbGluZy1yeHJ5Lmh0bWwKQEAgLTAsMCArMSwxMiBAQAorPCFET0NU WVBFIGh0bWw+Cis8aHRtbD4KKzxib2R5PgorICBUZXN0IGZvciBjcmJ1Zy5jb20vMjM1MjU2OiB0 aGlzIHRlc3QgcGFzc2VzIGlmIHRoZSByZWN0IGlzIHJvdW5kZWQ8YnI+CisgIDxzdmcgd2lkdGg9 IjMwMCIgaGVpZ2h0PSIzMDAiPgorICAgIDxyZWN0IHg9IjIwIiB5PSIyMCIgd2lkdGg9IjIwMCIg aGVpZ2h0PSIyMDAiIGZpbGw9ImdyZWVuIj4KKyAgICAgIDxzZXQgYXR0cmlidXRlbmFtZT0icngi IHRvPSI1MCIgYmVnaW49IjBzIiBmaWxsPSJmcmVlemUiLz4KKyAgICAgIDxzZXQgYXR0cmlidXRl bmFtZT0icnkiIHRvPSIyMCIgYmVnaW49IjBzIiBmaWxsPSJmcmVlemUiLz4KKyAgICA8L3JlY3Q+ CisgIDwvc3ZnPgorPC9ib2R5PgorPC9odG1sPgpkaWZmIC0tZ2l0IGEvU291cmNlL1dlYkNvcmUv Q2hhbmdlTG9nIGIvU291cmNlL1dlYkNvcmUvQ2hhbmdlTG9nCmluZGV4IDA5MmY1OTEuLmUzOTBj YTBhIDEwMDY0NAotLS0gYS9Tb3VyY2UvV2ViQ29yZS9DaGFuZ2VMb2cKKysrIGIvU291cmNlL1dl YkNvcmUvQ2hhbmdlTG9nCkBAIC0xLDMgKzEsMTkgQEAKKzIwMTQtMDEtMjEgIFBldGVyIE1vbG5h ciAgPHBtb2xuYXIudS1zemVnZWRAcGFydG5lci5zYW1zdW5nLmNvbT4KKworICAgICAgICBGaXgg U1ZHIGFuaW1hdGlvbnMgd2hpY2ggc2V0IHJ4IG9yIHJ5IGF0dHJpYnV0ZXMKKyAgICAgICAgaHR0 cHM6Ly9idWdzLndlYmtpdC5vcmcvc2hvd19idWcuY2dpP2lkPTEyNzMzNworCisgICAgICAgIFJl dmlld2VkIGJ5IE5PQk9EWSAoT09QUyEpLgorCisgICAgICAgIFRlc3Q6IHN2Zy9zdHJva2UvYW5p bWF0ZWQtbm9uLXNjYWxpbmctcnhyeS5odG1sCisKKyAgICAgICAgTWVyZ2VkIGZyb20gQmxpbms6 IGh0dHBzOi8vc3JjLmNocm9taXVtLm9yZy92aWV3dmMvYmxpbms/cmV2aXNpb249MTUyMzc2JnZp ZXc9cmV2aXNpb24KKworICAgICAgICAqIHJlbmRlcmluZy9zdmcvUmVuZGVyU1ZHUmVjdC5jcHA6 CisgICAgICAgIChXZWJDb3JlOjpSZW5kZXJTVkdSZWN0Ojp1cGRhdGVTaGFwZUZyb21FbGVtZW50 KToKKyAgICAgICAgKiByZW5kZXJpbmcvc3ZnL1NWR1BhdGhEYXRhLmNwcDoKKyAgICAgICAgKFdl YkNvcmU6OnVwZGF0ZVBhdGhGcm9tUmVjdEVsZW1lbnQpOgorCiAyMDE0LTAxLTIxICBLcnp5c3p0 b2YgQ3plY2ggIDxrLmN6ZWNoQHNhbXN1bmcuY29tPgogCiAgICAgICAgIFtBVEtdIEV4cG9zZSBh cmlhLWZsb3d0byB0aHJvdWdoIEFUS19SRUxBVElPTl9GTE9XU19UTwpkaWZmIC0tZ2l0IGEvU291 cmNlL1dlYkNvcmUvcmVuZGVyaW5nL3N2Zy9SZW5kZXJTVkdSZWN0LmNwcCBiL1NvdXJjZS9XZWJD b3JlL3JlbmRlcmluZy9zdmcvUmVuZGVyU1ZHUmVjdC5jcHAKaW5kZXggZjcwNzkzNy4uYmZkYTYy ZiAxMDA2NDQKLS0tIGEvU291cmNlL1dlYkNvcmUvcmVuZGVyaW5nL3N2Zy9SZW5kZXJTVkdSZWN0 LmNwcAorKysgYi9Tb3VyY2UvV2ViQ29yZS9yZW5kZXJpbmcvc3ZnL1JlbmRlclNWR1JlY3QuY3Bw CkBAIC01NywxNSArNTcsMTUgQEAgdm9pZCBSZW5kZXJTVkdSZWN0Ojp1cGRhdGVTaGFwZUZyb21F bGVtZW50KCkKICAgICBtX2lubmVyU3Ryb2tlUmVjdCA9IEZsb2F0UmVjdCgpOwogICAgIG1fb3V0 ZXJTdHJva2VSZWN0ID0gRmxvYXRSZWN0KCk7CiAKKyAgICBTVkdMZW5ndGhDb250ZXh0IGxlbmd0 aENvbnRleHQoJnJlY3RFbGVtZW50KCkpOwogICAgIC8vIEZhbGxiYWNrIHRvIFJlbmRlclNWR1No YXBlIGlmIHJlY3QgaGFzIHJvdW5kZWQgY29ybmVycyBvciBhIG5vbi1zY2FsaW5nIHN0cm9rZS4K LSAgICBpZiAocmVjdEVsZW1lbnQoKS5oYXNBdHRyaWJ1dGUoU1ZHTmFtZXM6OnJ4QXR0cikgfHwg cmVjdEVsZW1lbnQoKS5oYXNBdHRyaWJ1dGUoU1ZHTmFtZXM6OnJ5QXR0cikgfHwgaGFzTm9uU2Nh bGluZ1N0cm9rZSgpKSB7CisgICAgaWYgKHJlY3RFbGVtZW50KCkucngoKS52YWx1ZShsZW5ndGhD b250ZXh0KSA+IDAgfHwgcmVjdEVsZW1lbnQoKS5yeSgpLnZhbHVlKGxlbmd0aENvbnRleHQpID4g MCB8fCBoYXNOb25TY2FsaW5nU3Ryb2tlKCkpIHsKICAgICAgICAgUmVuZGVyU1ZHU2hhcGU6OnVw ZGF0ZVNoYXBlRnJvbUVsZW1lbnQoKTsKICAgICAgICAgbV91c2VQYXRoRmFsbGJhY2sgPSB0cnVl OwogICAgICAgICByZXR1cm47Ci0gICAgfSBlbHNlCi0gICAgICAgIG1fdXNlUGF0aEZhbGxiYWNr ID0gZmFsc2U7CisgICAgfQogCi0gICAgU1ZHTGVuZ3RoQ29udGV4dCBsZW5ndGhDb250ZXh0KCZy ZWN0RWxlbWVudCgpKTsKKyAgICBtX3VzZVBhdGhGYWxsYmFjayA9IGZhbHNlOwogICAgIEZsb2F0 U2l6ZSBib3VuZGluZ0JveFNpemUocmVjdEVsZW1lbnQoKS53aWR0aCgpLnZhbHVlKGxlbmd0aENv bnRleHQpLCByZWN0RWxlbWVudCgpLmhlaWdodCgpLnZhbHVlKGxlbmd0aENvbnRleHQpKTsKICAg ICBpZiAoYm91bmRpbmdCb3hTaXplLmlzRW1wdHkoKSkKICAgICAgICAgcmV0dXJuOwpkaWZmIC0t Z2l0IGEvU291cmNlL1dlYkNvcmUvcmVuZGVyaW5nL3N2Zy9TVkdQYXRoRGF0YS5jcHAgYi9Tb3Vy Y2UvV2ViQ29yZS9yZW5kZXJpbmcvc3ZnL1NWR1BhdGhEYXRhLmNwcAppbmRleCAxNmNjNjQzLi5m ZjI1MTcyIDEwMDY0NAotLS0gYS9Tb3VyY2UvV2ViQ29yZS9yZW5kZXJpbmcvc3ZnL1NWR1BhdGhE YXRhLmNwcAorKysgYi9Tb3VyY2UvV2ViQ29yZS9yZW5kZXJpbmcvc3ZnL1NWR1BhdGhEYXRhLmNw cApAQCAtMTE1LDggKzExNSw4IEBAIHN0YXRpYyB2b2lkIHVwZGF0ZVBhdGhGcm9tUmVjdEVsZW1l bnQoU1ZHRWxlbWVudCogZWxlbWVudCwgUGF0aCYgcGF0aCkKICAgICAgICAgcmV0dXJuOwogICAg IGZsb2F0IHggPSByZWN0LT54KCkudmFsdWUobGVuZ3RoQ29udGV4dCk7CiAgICAgZmxvYXQgeSA9 IHJlY3QtPnkoKS52YWx1ZShsZW5ndGhDb250ZXh0KTsKLSAgICBib29sIGhhc1J4ID0gcmVjdC0+ aGFzQXR0cmlidXRlKFNWR05hbWVzOjpyeEF0dHIpOwotICAgIGJvb2wgaGFzUnkgPSByZWN0LT5o YXNBdHRyaWJ1dGUoU1ZHTmFtZXM6OnJ5QXR0cik7CisgICAgYm9vbCBoYXNSeCA9IHJlY3QtPnJ4 KCkudmFsdWUobGVuZ3RoQ29udGV4dCkgPiAwOworICAgIGJvb2wgaGFzUnkgPSByZWN0LT5yeSgp LnZhbHVlKGxlbmd0aENvbnRleHQpID4gMDsKICAgICBpZiAoaGFzUnggfHwgaGFzUnkpIHsKICAg ICAgICAgZmxvYXQgcnggPSByZWN0LT5yeCgpLnZhbHVlKGxlbmd0aENvbnRleHQpOwogICAgICAg ICBmbG9hdCByeSA9IHJlY3QtPnJ5KCkudmFsdWUobGVuZ3RoQ29udGV4dCk7Cg==