28808 2009-08-28 07:09:19 -0700 [Skia] Fix flaky layout test svg/dynamic-updates/SVGClipPathElement-dom-clipPathUnits-attr.html [DEBUG] 2009-09-01 04:01:49 -0700 1 1 1 Unclassified WebKit Platform 528+ (Nightly build) All All RESOLVED FIXED GoogleBug P2 Normal --- 1 rolandsteiner webkit-unassigned eric oldest_to_newest 143361 0 rolandsteiner 2009-08-28 07:09:19 -0700 Note: DEBUG build It seems the layout test fails in Skia if a render event occurs before the clipPathUnit attribute has been reset to "userSpaceOnUse" from the initally set "objectBoundingBox". This causes the 300px coordinates in the clipping path to be interpreted as 300*100% of the bounding box size, resulting in values that are too large for Skia's fixed point data types. This causes an ASSERT during the value conversion. 143364 1 38733 rolandsteiner 2009-08-28 07:20:02 -0700 Created attachment 38733 patch: check skia safety of path on conversion to local coordinates The patch adds an explicit check for the validity of the path when it is transformed into local coordinates. This was the most suitable place I could find that didn't cause an inordinate amount of refactoring. Also, it seems pertinent and may also catch other cases that are not limited to clipping paths. 143365 2 rolandsteiner 2009-08-28 07:23:23 -0700 (In reply to comment #1) additional remark to the patch: I tested it only under Windows so far, but the issue should be OS-independent. 143610 3 38733 eric 2009-08-31 03:26:28 -0700 Comment on attachment 38733 patch: check skia safety of path on conversion to local coordinates Looks fine. +extern bool isPathSkiaSafe(const SkMatrix& transform, const SkPath& path); should not have names for the arguments since they don't add anything. Not sure why you're moving the inline functions around... I guess so you can export a function? If we're going to export the function we should just add it to a header no? Or to some utility file? This looks OK. Waiting for your response or new patch before landing. 143637 4 rolandsteiner 2009-08-31 05:15:38 -0700 (In reply to comment #3) > (From update of attachment 38733 [details]) > [...] > Not sure why you're moving the inline functions around... I guess so you can > export a function? Yes, I needed to move the relevant functions out of the anonymous namespace, but wanted to retain it for the generically named 'fastMod' and 'square' functions. > If we're going to export the function we should just add it to a header no? Or > to some utility file? Well, since the comment on those functions was that they should go away sooner or later I didn't want to give them additional exposure. (I don't know what would happen to this patch at that point, though.) Also, a short'n'simple 'extern' was sufficient for my purposes so I left it at that. But if you think it'd still be worthwhile I could certainly add them to some header file. 143810 5 38733 eric 2009-09-01 03:01:42 -0700 Comment on attachment 38733 patch: check skia safety of path on conversion to local coordinates The Skia safetybelt is lame, and should die, yes. :) this is OK for now. 143841 6 38733 eric 2009-09-01 04:01:46 -0700 Comment on attachment 38733 patch: check skia safety of path on conversion to local coordinates Clearing flags on attachment: 38733 Committed r47925: <http://trac.webkit.org/changeset/47925> 143842 7 eric 2009-09-01 04:01:49 -0700 All reviewed patches have been landed. Closing bug. 38733 2009-08-28 07:20:02 -0700 2009-09-01 04:01:46 -0700 patch: check skia safety of path on conversion to local coordinates skiaclip.patch text/plain 3756 rolandsteiner SW5kZXg6IFdlYkNvcmUvQ2hhbmdlTG9nCj09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT0KLS0tIFdlYkNvcmUvQ2hhbmdlTG9n CShyZXZpc2lvbiA0Nzg2MSkKKysrIFdlYkNvcmUvQ2hhbmdlTG9nCSh3b3JraW5nIGNvcHkpCkBA IC0xLDMgKzEsMjMgQEAKKzIwMDktMDgtMjggIFJvbGFuZCBTdGVpbmVyICA8cm9sYW5kc3RlaW5l ckBnb29nbGUuY29tPgorCisgICAgICAgIFJldmlld2VkIGJ5IE5PQk9EWSAoT09QUyEpLgorCisg ICAgICAgIEZpeCBidWcgMjg4MDg6ICBbU2tpYV0gRml4IGZsYWt5IGxheW91dCB0ZXN0IHN2Zy9k eW5hbWljLXVwZGF0ZXMvU1ZHQ2xpcFBhdGhFbGVtZW50LWRvbS1jbGlwUGF0aFVuaXRzLWF0dHIu aHRtbCBbREVCVUddCisgICAgICAgIChodHRwczovL2J1Z3Mud2Via2l0Lm9yZy9zaG93X2J1Zy5j Z2k/aWQ9Mjg4MDgpCisKKyAgICAgICAgVGhlIGJ1ZyBmaXggYWRkcyBhbiBleHBsaWNpdCBjaGVj ayBmb3IgdGhlIHZhbGlkaXR5IG9mIHRoZSBwYXRoIHdoZW4gaXQgaXMgCisgICAgICAgIHRyYW5z Zm9ybWVkIGludG8gbG9jYWwgY29vcmRpbmF0ZXMuIAorICAgICAgICBUaGlzIHdhcyB0aGUgbW9z dCBzdWl0YWJsZSBwbGFjZSBJIGNvdWxkIGZpbmQgdGhhdCBkaWRuJ3QgY2F1c2UgYW4gaW5vcmRp bmF0ZSAKKyAgICAgICAgYW1vdW50IG9mIHJlZmFjdG9yaW5nLiBBbHNvLCBpdCBzZWVtcyBwZXJ0 aW5lbnQgYW5kIG1heSBhbHNvIGNhdGNoIG90aGVyIGNhc2VzIAorICAgICAgICB0aGF0IGFyZSBu b3QgbGltaXRlZCB0byBjbGlwcGluZyBwYXRocy4KKworICAgICAgICBURVNUOiBleGlzdGluZyBz dmcvZHluYW1pYy11cGRhdGVzL1NWR0NsaXBQYXRoRWxlbWVudC1kb20tY2xpcFBhdGhVbml0cy1h dHRyLmh0bWwKKworICAgICAgICAqIHBsYXRmb3JtL2dyYXBoaWNzL3NraWEvR3JhcGhpY3NDb250 ZXh0U2tpYS5jcHA6IG1ha2UgaXNQYXRoU2tpYVNhZmUgYWNjZXNzaWJsZQorICAgICAgICAoV2Vi Q29yZTo6KToKKyAgICAgICAgKiBwbGF0Zm9ybS9ncmFwaGljcy9za2lhL1BsYXRmb3JtQ29udGV4 dFNraWEuY3BwOgorICAgICAgICAoUGxhdGZvcm1Db250ZXh0U2tpYTo6Y3VycmVudFBhdGhJbkxv Y2FsQ29vcmRpbmF0ZXMpOiBjaGVjayBTa2lhIHNhZmV0eSBvZiBwYXRoCisKIDIwMDktMDgtMjgg IE1pa2hhaWwgTmFnYW5vdiAgPG1uYWdhbm92QGNocm9taXVtLm9yZz4KIAogICAgICAgICBSZXZp ZXdlZCBieSBUaW1vdGh5IEhhdGNoZXIuCkluZGV4OiBXZWJDb3JlL3BsYXRmb3JtL2dyYXBoaWNz L3NraWEvR3JhcGhpY3NDb250ZXh0U2tpYS5jcHAKPT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PQotLS0gV2ViQ29yZS9wbGF0 Zm9ybS9ncmFwaGljcy9za2lhL0dyYXBoaWNzQ29udGV4dFNraWEuY3BwCShyZXZpc2lvbiA0Nzc0 MCkKKysrIFdlYkNvcmUvcGxhdGZvcm0vZ3JhcGhpY3Mvc2tpYS9HcmFwaGljc0NvbnRleHRTa2lh LmNwcAkod29ya2luZyBjb3B5KQpAQCAtNjAsNiArNjAsMjMgQEAgbmFtZXNwYWNlIFdlYkNvcmUg ewogCiBuYW1lc3BhY2UgewogCitpbmxpbmUgaW50IGZhc3RNb2QoaW50IHZhbHVlLCBpbnQgbWF4 KQoreworICAgIGludCBzaWduID0gU2tFeHRyYWN0U2lnbih2YWx1ZSk7CisgICAgCisgICAgdmFs dWUgPSBTa0FwcGx5U2lnbih2YWx1ZSwgc2lnbik7CisgICAgaWYgKHZhbHVlID49IG1heCkKKyAg ICAgICAgdmFsdWUgJT0gbWF4OworICAgIHJldHVybiBTa0FwcGx5U2lnbih2YWx1ZSwgc2lnbik7 Cit9CisKK2lubGluZSBmbG9hdCBzcXVhcmUoZmxvYXQgbikKK3sKKyAgICByZXR1cm4gbiAqIG47 Cit9CisKK30gIC8vIG5hbWVzcGFjZQorCiAvLyAiU2VhdGJlbHQiIGZ1bmN0aW9ucyAtLS0tLS0t LS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0KIC8vCiAvLyBU aGVzZSBmdW5jdGlvbnMgY2hlY2sgY2VydGFpbiBncmFwaGljcyBwcmltaXRpdmVzIGZvciBiZWlu ZyAic2FmZSIuCkBAIC0xOTUsMjMgKzIxMiw2IEBAIHZvaWQgYWRkQ29ybmVyQXJjKFNrUGF0aCog cGF0aCwgY29uc3QgU2sKICAgICBwYXRoLT5hcmNUbyhyLCBTa0ludFRvU2NhbGFyKHN0YXJ0QW5n bGUpLCBTa0ludFRvU2NhbGFyKDkwKSwgZmFsc2UpOwogfQogCi1pbmxpbmUgaW50IGZhc3RNb2Qo aW50IHZhbHVlLCBpbnQgbWF4KQotewotICAgIGludCBzaWduID0gU2tFeHRyYWN0U2lnbih2YWx1 ZSk7Ci0KLSAgICB2YWx1ZSA9IFNrQXBwbHlTaWduKHZhbHVlLCBzaWduKTsKLSAgICBpZiAodmFs dWUgPj0gbWF4KQotICAgICAgICB2YWx1ZSAlPSBtYXg7Ci0gICAgcmV0dXJuIFNrQXBwbHlTaWdu KHZhbHVlLCBzaWduKTsKLX0KLQotaW5saW5lIGZsb2F0IHNxdWFyZShmbG9hdCBuKQotewotICAg IHJldHVybiBuICogbjsKLX0KLQotfSAgLy8gbmFtZXNwYWNlCi0KIC8vIC0tLS0tLS0tLS0tLS0t LS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0t LS0tLS0tCiAKIC8vIFRoaXMgbWF5IGJlIGNhbGxlZCB3aXRoIGEgTlVMTCBwb2ludGVyIHRvIGNy ZWF0ZSBhIGdyYXBoaWNzIGNvbnRleHQgdGhhdCBoYXMKSW5kZXg6IFdlYkNvcmUvcGxhdGZvcm0v Z3JhcGhpY3Mvc2tpYS9QbGF0Zm9ybUNvbnRleHRTa2lhLmNwcAo9PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09Ci0tLSBXZWJD b3JlL3BsYXRmb3JtL2dyYXBoaWNzL3NraWEvUGxhdGZvcm1Db250ZXh0U2tpYS5jcHAJKHJldmlz aW9uIDQ3NzQwKQorKysgV2ViQ29yZS9wbGF0Zm9ybS9ncmFwaGljcy9za2lhL1BsYXRmb3JtQ29u dGV4dFNraWEuY3BwCSh3b3JraW5nIGNvcHkpCkBAIC00Niw2ICs0NiwxMSBAQAogCiAjaW5jbHVk ZSA8d3RmL01hdGhFeHRyYXMuaD4KIAorbmFtZXNwYWNlIFdlYkNvcmUgCit7CitleHRlcm4gYm9v bCBpc1BhdGhTa2lhU2FmZShjb25zdCBTa01hdHJpeCYgdHJhbnNmb3JtLCBjb25zdCBTa1BhdGgm IHBhdGgpOworfQorCiAvLyBTdGF0ZSAtLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0t LS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLQogCiAvLyBFbmNhcHN1bGF0ZXMg dGhlIGFkZGl0aW9uYWwgcGFpbnRpbmcgc3RhdGUgaW5mb3JtYXRpb24gd2Ugc3RvcmUgZm9yIGVh Y2gKQEAgLTQ4NywxMSArNDkyLDEzIEBAIHZvaWQgUGxhdGZvcm1Db250ZXh0U2tpYTo6YWRkUGF0 aChjb25zdCAKIAogU2tQYXRoIFBsYXRmb3JtQ29udGV4dFNraWE6OmN1cnJlbnRQYXRoSW5Mb2Nh bENvb3JkaW5hdGVzKCkgY29uc3QKIHsKLSAgICBTa1BhdGggbG9jYWxQYXRoID0gbV9wYXRoOwog ICAgIGNvbnN0IFNrTWF0cml4JiBtYXRyaXggPSBtX2NhbnZhcy0+Z2V0VG90YWxNYXRyaXgoKTsK ICAgICBTa01hdHJpeCBpbnZlcnNlTWF0cml4OwogICAgIGlmICghbWF0cml4LmludmVydCgmaW52 ZXJzZU1hdHJpeCkpCiAgICAgICAgIHJldHVybiBTa1BhdGgoKTsKKyAgICBpZiAoIVdlYkNvcmU6 OmlzUGF0aFNraWFTYWZlKGludmVyc2VNYXRyaXgsIG1fcGF0aCkpCisgICAgICAgIHJldHVybiBT a1BhdGgoKTsKKyAgICBTa1BhdGggbG9jYWxQYXRoID0gbV9wYXRoOwogICAgIGxvY2FsUGF0aC50 cmFuc2Zvcm0oaW52ZXJzZU1hdHJpeCk7CiAgICAgcmV0dXJuIGxvY2FsUGF0aDsKIH0K