27187 2009-07-11 21:06:03 -0700 Match Gecko behaviour for canvas path mutation APIs on an empty path 2009-07-14 11:20:27 -0700 1 1 1 Unclassified WebKit DOM 528+ (Nightly build) PC OS X 10.5 RESOLVED FIXED http://www.blahbleh.com/clock.php P2 Normal --- 1 oliver oliver deanm dimich oldest_to_newest 131158 0 oliver 2009-07-11 21:06:03 -0700 Gecko treats lineTo, etc as planting a new point in the canvas path if the current path is empty. Failure to do this results in a significant performance penalty (due to CGs incessant logging) and site breakages like the one linked in this bug. 131159 1 32623 oliver 2009-07-11 21:38:10 -0700 Created attachment 32623 fixeration! 131160 2 32623 simon.fraser 2009-07-11 22:15:05 -0700 Comment on attachment 32623 fixeration! > + Simple API change, check for the mpty path and add appropriate point if necessary. Typo here. r=me 131161 3 oliver 2009-07-11 22:18:09 -0700 Committed r45759 131512 4 deanm 2009-07-14 02:56:56 -0700 (In reply to comment #3) > Committed r45759 This broke a bunch of <canvas> stuff. Part of the bug is that for quadraticCurveTo and bezierCurveTo you move to the control point, which I don't think makes a lot of sense, since the control point does not lie on the path. I am digging into the other things that broke now. Thanks 131514 5 deanm 2009-07-14 03:08:54 -0700 (In reply to comment #4) > (In reply to comment #3) > > Committed r45759 > > This broke a bunch of <canvas> stuff. > > Part of the bug is that for quadraticCurveTo and bezierCurveTo you move to the > control point, which I don't think makes a lot of sense, since the control > point does not lie on the path. > > I am digging into the other things that broke now. > > Thanks Ok, so here is the problem, it might be Skia specific: From Skia: bool SkPath::isEmpty() const { SkDEBUGCODE(this->validate();) int count = fVerbs.count(); return count == 0 || (count == 1 && fVerbs[0] == kMove_Verb); } And the documentation specifically states that isEmpty returns whether there is any lines or curves, not whether there is a move to point. There are no comments for WebCore::Path::isEmpty(), so I don't know what the expected behavior is. I can understand going both ways with isEmpty(), and in fact it seems like there should be two separate methods, one which asks if there are any points in the path, and one which asks if there are any lines or curves in the path. An additional option, if it is decided that isEmpty should mean there are no points at all in the path, would be to change the Skia backend to reflect this. However, that will probably break some other things, and additionally after a brief skim I cannot see how to get that information out of Skia. This is something I could add to Skia. In the mean time, it would be nice not to have our <canvas> tag completely broken :( Thanks -- dean 131516 6 deanm 2009-07-14 03:19:21 -0700 After digging a bit more, cairo does this with cairo_has_current_point. I think this is a more accurate name, hasCurrentPoint(), and it is not necessarily the same as the path being empty. Does it make sense to add hasCurrentPoint() to WebCore::Path? (In reply to comment #5) > (In reply to comment #4) > > (In reply to comment #3) > > > Committed r45759 > > > > This broke a bunch of <canvas> stuff. > > > > Part of the bug is that for quadraticCurveTo and bezierCurveTo you move to the > > control point, which I don't think makes a lot of sense, since the control > > point does not lie on the path. > > > > I am digging into the other things that broke now. > > > > Thanks > > Ok, so here is the problem, it might be Skia specific: > > From Skia: > > bool SkPath::isEmpty() const { > SkDEBUGCODE(this->validate();) > > int count = fVerbs.count(); > return count == 0 || (count == 1 && fVerbs[0] == kMove_Verb); > } > > And the documentation specifically states that isEmpty returns whether there is > any lines or curves, not whether there is a move to point. > > There are no comments for WebCore::Path::isEmpty(), so I don't know what the > expected behavior is. I can understand going both ways with isEmpty(), and in > fact it seems like there should be two separate methods, one which asks if > there are any points in the path, and one which asks if there are any lines or > curves in the path. > > An additional option, if it is decided that isEmpty should mean there are no > points at all in the path, would be to change the Skia backend to reflect this. > However, that will probably break some other things, and additionally after a > brief skim I cannot see how to get that information out of Skia. This is > something I could add to Skia. > > In the mean time, it would be nice not to have our <canvas> tag completely > broken :( > > Thanks > -- dean 131592 7 oliver 2009-07-14 09:38:50 -0700 (In reply to comment #6) > After digging a bit more, cairo does this with cairo_has_current_point. I > think this is a more accurate name, hasCurrentPoint(), and it is not > necessarily the same as the path being empty. > > Does it make sense to add hasCurrentPoint() to WebCore::Path? Seems reasonable -- but we'll want a different bug to track that 131601 8 dimich 2009-07-14 10:17:30 -0700 Created separate bug to track the proposed hasCurrentPoint() addition: https://bugs.webkit.org/show_bug.cgi?id=27266 131614 9 deanm 2009-07-14 11:00:41 -0700 (In reply to comment #8) > Created separate bug to track the proposed hasCurrentPoint() addition: > https://bugs.webkit.org/show_bug.cgi?id=27266 On more thinking I don't actually think this patch is correct: From http://dev.w3.org/html5/spec/Overview.html: The lineTo(x, y) method must do nothing if the context has no subpaths. Otherwise, it must connect the last point in the subpath to the given point (x, y) using a straight line, and must then add the given point (x, y) to the subpath. This means if I do lineTo(10, 10); lineTo(20, 20); I should get no lines, since I never started the path with a moveTo(). With this patch you get the second line. 131618 10 oliver 2009-07-14 11:20:27 -0700 Yes, unfortunately firefox violates the spec in a way that people seem to depend on. I've already emailed whatwg about this. It's safe to move to the firefox behaviour as it's a relaxation of existing API. 32623 2009-07-11 21:38:10 -0700 2009-07-11 22:15:05 -0700 fixeration! canvas-empty-path.patch text/plain 5836 oliver ZGlmZiAtLWdpdCBhL0xheW91dFRlc3RzL0NoYW5nZUxvZyBiL0xheW91dFRlc3RzL0NoYW5nZUxv ZwppbmRleCBiNmRjOGRjLi5kMWU0OTU0IDEwMDY0NAotLS0gYS9MYXlvdXRUZXN0cy9DaGFuZ2VM b2cKKysrIGIvTGF5b3V0VGVzdHMvQ2hhbmdlTG9nCkBAIC0xLDMgKzEsMTcgQEAKKzIwMDktMDct MTEgIE9saXZlciBIdW50ICA8b2xpdmVyQGFwcGxlLmNvbT4KKworICAgICAgICBSZXZpZXdlZCBi eSBOT0JPRFkgKE9PUFMhKS4KKworICAgICAgICBCdWcgMjcxODcgLSBNYXRjaCBHZWNrbyBiZWhh dmlvdXIgZm9yIGNhbnZhcyBwYXRoIG11dGF0aW9uIEFQSXMgb24gYW4gZW1wdHkgcGF0aAorICAg ICAgICA8aHR0cHM6Ly9idWdzLndlYmtpdC5vcmcvc2hvd19idWcuY2dpP2lkPTI3MTg3PgorCisg ICAgICAgIEFkZCB0ZXN0cyBmb3IgdGhlIGJlaGF2aW91ciBvZiBlZmZlY3RlZCBBUElzLgorCisg ICAgICAgICogZmFzdC9jYW52YXMvY2FudmFzLW1vZGlmeS1lbXB0eVBhdGgtZXhwZWN0ZWQudHh0 OiBBZGRlZC4KKyAgICAgICAgKiBmYXN0L2NhbnZhcy9jYW52YXMtbW9kaWZ5LWVtcHR5UGF0aC5o dG1sOiBBZGRlZC4KKyAgICAgICAgKiBmYXN0L2NhbnZhcy9yZXNvdXJjZXMvY2FudmFzLW1vZGlm eS1lbXB0eVBhdGguanM6IEFkZGVkLgorICAgICAgICAoZ2V0Q29sb3IpOgorCiAyMDA5LTA3LTEx ICBTaW1vbiBGcmFzZXIgIDxzaW1vbi5mcmFzZXJAYXBwbGUuY29tPgogCiAgICAgICAgIEVuYWJs ZSBzdXBwb3J0IGZvciBhY2NlbGVyYXRlZCBjb21wb3NpdGluZyBhbmQgM2QgdHJhbnNmb3JtcyBv biBMZW9wYXJkLgpkaWZmIC0tZ2l0IGEvTGF5b3V0VGVzdHMvZmFzdC9jYW52YXMvY2FudmFzLW1v ZGlmeS1lbXB0eVBhdGgtZXhwZWN0ZWQudHh0IGIvTGF5b3V0VGVzdHMvZmFzdC9jYW52YXMvY2Fu dmFzLW1vZGlmeS1lbXB0eVBhdGgtZXhwZWN0ZWQudHh0Cm5ldyBmaWxlIG1vZGUgMTAwNjQ0Cmlu ZGV4IDAwMDAwMDAuLjFkMGRlYWMKLS0tIC9kZXYvbnVsbAorKysgYi9MYXlvdXRUZXN0cy9mYXN0 L2NhbnZhcy9jYW52YXMtbW9kaWZ5LWVtcHR5UGF0aC1leHBlY3RlZC50eHQKQEAgLTAsMCArMSwx NyBAQAorVGhpcyB0ZXN0cyBiZWhhdmlvdXIgb2YgcGF0aCBtb2RpZmljYXRpb24gQVBJcyBvbiBh biBlbXB0eSBwYXRoLgorCitPbiBzdWNjZXNzLCB5b3Ugd2lsbCBzZWUgYSBzZXJpZXMgb2YgIlBB U1MiIG1lc3NhZ2VzLCBmb2xsb3dlZCBieSAiVEVTVCBDT01QTEVURSIuCisKKworVGVzdCBsaW5l VG8KK1BBU1MgZ2V0Q29sb3IoNTAsNTApIGlzIFswLDEyOCwwLDI1NV0KK1Rlc3QgbGluZVRvIHNl cXVlbmNlCitQQVNTIGdldENvbG9yKDUwLDUwKSBpcyBbMCwxMjgsMCwyNTVdCitUZXN0IHF1YWRy YXRpY0N1cnZlVG8KK1BBU1MgZ2V0Q29sb3IoNTAsNTApIGlzIFswLDEyOCwwLDI1NV0KK1Rlc3Qg YmV6aWVyQ3VydmVUbworUEFTUyBnZXRDb2xvcig1MCw1MCkgaXMgWzAsMTI4LDAsMjU1XQorUEFT UyBzdWNjZXNzZnVsbHlQYXJzZWQgaXMgdHJ1ZQorCitURVNUIENPTVBMRVRFCisKZGlmZiAtLWdp dCBhL0xheW91dFRlc3RzL2Zhc3QvY2FudmFzL2NhbnZhcy1tb2RpZnktZW1wdHlQYXRoLmh0bWwg Yi9MYXlvdXRUZXN0cy9mYXN0L2NhbnZhcy9jYW52YXMtbW9kaWZ5LWVtcHR5UGF0aC5odG1sCm5l dyBmaWxlIG1vZGUgMTAwNjQ0CmluZGV4IDAwMDAwMDAuLjY0ZmFhOTIKLS0tIC9kZXYvbnVsbAor KysgYi9MYXlvdXRUZXN0cy9mYXN0L2NhbnZhcy9jYW52YXMtbW9kaWZ5LWVtcHR5UGF0aC5odG1s CkBAIC0wLDAgKzEsMTMgQEAKKzwhRE9DVFlQRSBIVE1MIFBVQkxJQyAiLS8vSUVURi8vRFREIEhU TUwvL0VOIj4KKzxodG1sPgorPGhlYWQ+Cis8bGluayByZWw9InN0eWxlc2hlZXQiIGhyZWY9Ii4u L2pzL3Jlc291cmNlcy9qcy10ZXN0LXN0eWxlLmNzcyI+Cis8c2NyaXB0IHNyYz0iLi4vanMvcmVz b3VyY2VzL2pzLXRlc3QtcHJlLmpzIj48L3NjcmlwdD4KKzwvaGVhZD4KKzxib2R5PgorPHAgaWQ9 ImRlc2NyaXB0aW9uIj48L3A+Cis8ZGl2IGlkPSJjb25zb2xlIj48L2Rpdj4KKzxzY3JpcHQgc3Jj PSJyZXNvdXJjZXMvY2FudmFzLW1vZGlmeS1lbXB0eVBhdGguanMiPjwvc2NyaXB0PgorPHNjcmlw dCBzcmM9Ii4uL2pzL3Jlc291cmNlcy9qcy10ZXN0LXBvc3QuanMiPjwvc2NyaXB0PgorPC9ib2R5 PgorPC9odG1sPgpkaWZmIC0tZ2l0IGEvTGF5b3V0VGVzdHMvZmFzdC9jYW52YXMvcmVzb3VyY2Vz L2NhbnZhcy1tb2RpZnktZW1wdHlQYXRoLmpzIGIvTGF5b3V0VGVzdHMvZmFzdC9jYW52YXMvcmVz b3VyY2VzL2NhbnZhcy1tb2RpZnktZW1wdHlQYXRoLmpzCm5ldyBmaWxlIG1vZGUgMTAwNjQ0Cmlu ZGV4IDAwMDAwMDAuLjkwOTEwNzkKLS0tIC9kZXYvbnVsbAorKysgYi9MYXlvdXRUZXN0cy9mYXN0 L2NhbnZhcy9yZXNvdXJjZXMvY2FudmFzLW1vZGlmeS1lbXB0eVBhdGguanMKQEAgLTAsMCArMSw1 MiBAQAorZGVzY3JpcHRpb24oIlRoaXMgdGVzdHMgYmVoYXZpb3VyIG9mIHBhdGggbW9kaWZpY2F0 aW9uIEFQSXMgb24gYW4gZW1wdHkgcGF0aC4iKTsKK3ZhciBjYW52YXMgPSBkb2N1bWVudC5jcmVh dGVFbGVtZW50KCdjYW52YXMnKTsKK2NhbnZhcy53aWR0aD0zMDA7CitjYW52YXMuaGVpZ2h0PTMw MDsKK3ZhciBjdHggPSBjYW52YXMuZ2V0Q29udGV4dCgnMmQnKTsKKworZnVuY3Rpb24gZ2V0Q29s b3IoeCx5KSB7CisgICAgdmFyIGltYWdlRGF0YSA9IGN0eC5nZXRJbWFnZURhdGEoeCwgeSwgMSwg MSk7CisgICAgdmFyIGRhdGEgPSBpbWFnZURhdGEuZGF0YTsKKyAgICByZXR1cm4gW2RhdGFbMF0s IGRhdGFbMV0sIGRhdGFbMl0sIGRhdGFbM11dOworfQorCisvLyBUZXN0IG5vIGRyYXdpbmcgY2Fz ZXMKK2N0eC5maWxsU3R5bGUgPSAnZ3JlZW4nOworY3R4LnN0cm9rZVN0eWxlID0gJ3JlZCc7Citj dHgubGluZVdpZHRoID0gNTA7CisKK2RlYnVnKCJUZXN0IGxpbmVUbyIpCitjdHguYmVnaW5QYXRo KCk7CitjdHguZmlsbFJlY3QoMCwgMCwgMTAwLCAxMDApOworY3R4LmxpbmVUbyg1MCwgNTApOwor Y3R4LnN0cm9rZSgpOworc2hvdWxkQmUoImdldENvbG9yKDUwLDUwKSIsICJbMCwxMjgsMCwyNTVd Iik7CisKKworLy8gVGVzdCBwYXRoIG1vZGlmaWNhdGlvbnMgdGhhdCByZXN1bHQgaW4gZHJhd2lu ZworY3R4LmZpbGxTdHlsZSA9ICdyZWQnOworY3R4LnN0cm9rZVN0eWxlID0gJ2dyZWVuJzsKKwor ZGVidWcoIlRlc3QgbGluZVRvIHNlcXVlbmNlIikKK2N0eC5iZWdpblBhdGgoKTsKK2N0eC5maWxs UmVjdCgwLCAwLCAxMDAsIDEwMCk7CitjdHgubGluZVRvKDAsIDUwKTsKK2N0eC5saW5lVG8oMTAw LCA1MCk7CitjdHguc3Ryb2tlKCk7CitzaG91bGRCZSgiZ2V0Q29sb3IoNTAsNTApIiwgIlswLDEy OCwwLDI1NV0iKTsKKworZGVidWcoIlRlc3QgcXVhZHJhdGljQ3VydmVUbyIpCitjdHguYmVnaW5Q YXRoKCk7CitjdHguZmlsbFJlY3QoMCwgMCwgMTAwLCAxMDApOworY3R4LnF1YWRyYXRpY0N1cnZl VG8oMCwgNTAsIDEwMCwgNTApOworY3R4LnN0cm9rZSgpOworc2hvdWxkQmUoImdldENvbG9yKDUw LDUwKSIsICJbMCwxMjgsMCwyNTVdIik7CisKK2RlYnVnKCJUZXN0IGJlemllckN1cnZlVG8iKQor Y3R4LmJlZ2luUGF0aCgpOworY3R4LmZpbGxSZWN0KDAsIDAsIDEwMCwgMTAwKTsKK2N0eC5iZXpp ZXJDdXJ2ZVRvKDAsIDUwLCA1MCwgNTAsIDEwMCwgNTApOworY3R4LnN0cm9rZSgpOworc2hvdWxk QmUoImdldENvbG9yKDUwLDUwKSIsICJbMCwxMjgsMCwyNTVdIik7CisKK3ZhciBzdWNjZXNzZnVs bHlQYXJzZWQgPSB0cnVlOwpkaWZmIC0tZ2l0IGEvV2ViQ29yZS9DaGFuZ2VMb2cgYi9XZWJDb3Jl L0NoYW5nZUxvZwppbmRleCAwYTJlNDg5Li5hMDU4YzNkIDEwMDY0NAotLS0gYS9XZWJDb3JlL0No YW5nZUxvZworKysgYi9XZWJDb3JlL0NoYW5nZUxvZwpAQCAtMSwzICsxLDIxIEBACisyMDA5LTA3 LTExICBPbGl2ZXIgSHVudCAgPG9saXZlckBhcHBsZS5jb20+CisKKyAgICAgICAgUmV2aWV3ZWQg YnkgTk9CT0RZIChPT1BTISkuCisKKyAgICAgICAgQnVnIDI3MTg3IC0gTWF0Y2ggR2Vja28gYmVo YXZpb3VyIGZvciBjYW52YXMgcGF0aCBtdXRhdGlvbiBBUElzIG9uIGFuIGVtcHR5IHBhdGgKKyAg ICAgICAgPGh0dHBzOi8vYnVncy53ZWJraXQub3JnL3Nob3dfYnVnLmNnaT9pZD0yNzE4Nz4KKwor ICAgICAgICBTaW1wbGUgQVBJIGNoYW5nZSwgY2hlY2sgZm9yIHRoZSBtcHR5IHBhdGggYW5kIGFk ZCBhcHByb3ByaWF0ZSBwb2ludCBpZiBuZWNlc3NhcnkuCisKKyAgICAgICAgVGVzdDogZmFzdC9j YW52YXMvY2FudmFzLW1vZGlmeS1lbXB0eVBhdGguaHRtbAorCisgICAgICAgICogSW5mby5wbGlz dDoKKyAgICAgICAgKiBXZWJDb3JlLnhjb2RlcHJvai9wcm9qZWN0LnBieHByb2o6CisgICAgICAg ICogaHRtbC9DYW52YXNSZW5kZXJpbmdDb250ZXh0MkQuY3BwOgorICAgICAgICAoV2ViQ29yZTo6 Q2FudmFzUmVuZGVyaW5nQ29udGV4dDJEOjpsaW5lVG8pOgorICAgICAgICAoV2ViQ29yZTo6Q2Fu dmFzUmVuZGVyaW5nQ29udGV4dDJEOjpxdWFkcmF0aWNDdXJ2ZVRvKToKKyAgICAgICAgKFdlYkNv cmU6OkNhbnZhc1JlbmRlcmluZ0NvbnRleHQyRDo6YmV6aWVyQ3VydmVUbyk6CisKIDIwMDktMDct MTEgIEJyYWR5IEVpZHNvbiAgPGJlaWRzb25AYXBwbGUuY29tPgogCiAgICAgICAgIFJldmlld2Vk IGJ5IE1hcmsgUm93ZS4KZGlmZiAtLWdpdCBhL1dlYkNvcmUvaHRtbC9DYW52YXNSZW5kZXJpbmdD b250ZXh0MkQuY3BwIGIvV2ViQ29yZS9odG1sL0NhbnZhc1JlbmRlcmluZ0NvbnRleHQyRC5jcHAK aW5kZXggNTJiOGE3My4uN2EyNTVkOCAxMDA2NDQKLS0tIGEvV2ViQ29yZS9odG1sL0NhbnZhc1Jl bmRlcmluZ0NvbnRleHQyRC5jcHAKKysrIGIvV2ViQ29yZS9odG1sL0NhbnZhc1JlbmRlcmluZ0Nv bnRleHQyRC5jcHAKQEAgLTU0Niw2ICs1NDYsOCBAQCB2b2lkIENhbnZhc1JlbmRlcmluZ0NvbnRl eHQyRDo6bGluZVRvKGZsb2F0IHgsIGZsb2F0IHkpCiAgICAgICAgIHJldHVybjsKICAgICBpZiAo IXN0YXRlKCkubV9pbnZlcnRpYmxlQ1RNKQogICAgICAgICByZXR1cm47CisgICAgaWYgKG1fcGF0 aC5pc0VtcHR5KCkpCisgICAgICAgIG1fcGF0aC5tb3ZlVG8oRmxvYXRQb2ludCh4LCB5KSk7CiAg ICAgbV9wYXRoLmFkZExpbmVUbyhGbG9hdFBvaW50KHgsIHkpKTsKIH0KIApAQCAtNTU1LDYgKzU1 Nyw4IEBAIHZvaWQgQ2FudmFzUmVuZGVyaW5nQ29udGV4dDJEOjpxdWFkcmF0aWNDdXJ2ZVRvKGZs b2F0IGNweCwgZmxvYXQgY3B5LCBmbG9hdCB4LCBmCiAgICAgICAgIHJldHVybjsKICAgICBpZiAo IXN0YXRlKCkubV9pbnZlcnRpYmxlQ1RNKQogICAgICAgICByZXR1cm47CisgICAgaWYgKG1fcGF0 aC5pc0VtcHR5KCkpCisgICAgICAgIG1fcGF0aC5tb3ZlVG8oRmxvYXRQb2ludChjcHgsIGNweSkp OwogICAgIG1fcGF0aC5hZGRRdWFkQ3VydmVUbyhGbG9hdFBvaW50KGNweCwgY3B5KSwgRmxvYXRQ b2ludCh4LCB5KSk7CiB9CiAKQEAgLTU2NCw2ICs1NjgsOCBAQCB2b2lkIENhbnZhc1JlbmRlcmlu Z0NvbnRleHQyRDo6YmV6aWVyQ3VydmVUbyhmbG9hdCBjcDF4LCBmbG9hdCBjcDF5LCBmbG9hdCBj cDJ4LAogICAgICAgICByZXR1cm47CiAgICAgaWYgKCFzdGF0ZSgpLm1faW52ZXJ0aWJsZUNUTSkK ICAgICAgICAgcmV0dXJuOworICAgIGlmIChtX3BhdGguaXNFbXB0eSgpKQorICAgICAgICBtX3Bh dGgubW92ZVRvKEZsb2F0UG9pbnQoY3AxeCwgY3AxeSkpOwogICAgIG1fcGF0aC5hZGRCZXppZXJD dXJ2ZVRvKEZsb2F0UG9pbnQoY3AxeCwgY3AxeSksIEZsb2F0UG9pbnQoY3AyeCwgY3AyeSksIEZs b2F0UG9pbnQoeCwgeSkpOwogfQogCg==