83826 2012-04-12 15:40:00 -0700 REGRESSION (r114032): Layout Tests fast/text/drawBidiText.html fast/text/international/bidi-listbox-atsui.html are failing 2012-04-14 11:16:11 -0700 1 1 1 Unclassified WebKit Text 528+ (Nightly build) All All RESOLVED FIXED P2 Normal --- 1 scheib hyatt andersca ap eric hyatt leviw mitz rniwa tkent webkit.review.bot oldest_to_newest 601588 0 scheib 2012-04-12 15:40:00 -0700 http://trac.webkit.org/changeset/114032 This patch modifies RenderText so that it scans all of its characters up front... Caused these Layout Tests fast/text/drawBidiText.html fast/text/international/bidi-listbox-atsui.html are failing to start having pixel errors, visible here: http://test-results.appspot.com/dashboards/flakiness_dashboard.html#tests=fast%2Ftext%2FdrawBidiText.html%20fast%2Ftext%2Finternational%2Fbidi-listbox-atsui.html I can not tell if this is acceptable and should be rebaselined, or represents an unintended result. 602499 1 leviw 2012-04-13 16:27:39 -0700 The arabic is no longer being combined, but instead each character is being rendered individually. That's definitely wrong :( 602501 2 leviw 2012-04-13 16:30:48 -0700 Actually, likewise with the Hebrew. There are dots that are supposed to modify the adjacent character, but the extra spacing between the letters is the result of those dots being given the length of a character instead of the proper combined rendering. 602507 3 eric 2012-04-13 16:34:49 -0700 Presumably the simple-path is being taken for these runs when it shouldn't be. 602509 4 hyatt 2012-04-13 16:36:29 -0700 I will look into it. Optimized code path probably being incorrectly chosen for some reason. 602520 5 137178 hyatt 2012-04-13 16:47:51 -0700 Created attachment 137178 Patch 602522 6 137179 hyatt 2012-04-13 16:48:37 -0700 Created attachment 137179 Patch 602625 7 137179 eric 2012-04-13 22:11:01 -0700 Comment on attachment 137179 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=137179&action=review > Source/WebCore/html/canvas/CanvasRenderingContext2D.cpp:2025 > + TextRun textRun(string, length, false, 0, 0, TextRun::AllowTrailingExpansion, direction, override, true, TextRun::NoRounding); How could this have worked before? There are separate constructors for these cases? Clearly we need to clean up these constructors... probably using some sort of nicely named create variants, as well as enums instead of these true/flase/0 nonsense. 602626 8 eric 2012-04-13 22:12:12 -0700 I guess what I'm saying is that this mistake was not a product of Dave's coding skills, but of what appears to be horrible design on the part of TextRun constructors.. which we'll need to fix, or we'll just repeat this sort of mistake. :) 602678 9 hyatt 2012-04-14 11:06:32 -0700 What we need to do is remove the rounding hacks parameter from the constructors IMO. 602679 10 137179 webkit.review.bot 2012-04-14 11:16:06 -0700 Comment on attachment 137179 Patch Clearing flags on attachment: 137179 Committed r114205: <http://trac.webkit.org/changeset/114205> 602680 11 webkit.review.bot 2012-04-14 11:16:11 -0700 All reviewed patches have been landed. Closing bug. 137178 2012-04-13 16:47:51 -0700 2012-04-13 16:48:37 -0700 Patch patch.txt text/plain 4053 hyatt SW5kZXg6IFNvdXJjZS9XZWJDb3JlL0NoYW5nZUxvZwo9PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09Ci0tLSBTb3VyY2UvV2Vi Q29yZS9DaGFuZ2VMb2cJKHJldmlzaW9uIDExNDE4MykKKysrIFNvdXJjZS9XZWJDb3JlL0NoYW5n ZUxvZwkod29ya2luZyBjb3B5KQpAQCAtMSwzICsxLDE5IEBACisyMDEyLTA0LTEzICBEYXZpZCBI eWF0dCAgPGh5YXR0QGFwcGxlLmNvbT4KKworICAgICAgICBodHRwczovL2J1Z3Mud2Via2l0Lm9y Zy9zaG93X2J1Zy5jZ2k/aWQ9ODM4MjYKKyAgICAgICAgCisgICAgICAgIFdyb25nIEZvbnQgY29k ZSBwYXRoIHdhcyBjaG9zZW4gYWZ0ZXIgcjExNDAzMi4gVGhlIFRleHRSdW4gY29uc3RydWN0b3Ig aGFzIGFuIGFkZGl0aW9uYWwgYXJndW1lbnQgdGhhdAorICAgICAgICBuZWVkcyB0byBiZSBpbmNs dWRlZCBpZiByb3VuZGluZyBoYWNrcyBhcmUgbWVudGlvbmVkLgorCisgICAgICAgIFJldmlld2Vk IGJ5IE5PQk9EWSAoT09QUyEpLgorCisgICAgICAgICogaHRtbC9jYW52YXMvQ2FudmFzUmVuZGVy aW5nQ29udGV4dDJELmNwcDoKKyAgICAgICAgKFdlYkNvcmU6OkNhbnZhc1JlbmRlcmluZ0NvbnRl eHQyRDo6ZHJhd1RleHRJbnRlcm5hbCk6CisgICAgICAgICogcmVuZGVyaW5nL1JlbmRlckxpc3RC b3guY3BwOgorICAgICAgICAoV2ViQ29yZTo6UmVuZGVyTGlzdEJveDo6cGFpbnRJdGVtRm9yZWdy b3VuZCk6CisgICAgICAgICogcmVuZGVyaW5nL2JyZWFrX2xpbmVzLmNwcDoKKyAgICAgICAgKFdl YkNvcmU6Om5leHRCcmVha2FibGVQb3NpdGlvbik6CisKIDIwMTItMDQtMTMgIEplciBOb2JsZSAg PGplci5ub2JsZUBhcHBsZS5jb20+CiAKICAgICAgICAgVmlkZW8gYXQgYXBwbGUuY29tIGdldHMg c3RhbmRhcmQgY29udHJvbHMgaW4gYWRkaXRpb24gdG8gY3VzdG9tIGNvbnRyb2xzIGFmdGVyIHJl dHVybmluZyBmcm9tIGZ1bGwgc2NyZWVuCkluZGV4OiBTb3VyY2UvV2ViQ29yZS9odG1sL2NhbnZh cy9DYW52YXNSZW5kZXJpbmdDb250ZXh0MkQuY3BwCj09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT0KLS0tIFNvdXJjZS9XZWJD b3JlL2h0bWwvY2FudmFzL0NhbnZhc1JlbmRlcmluZ0NvbnRleHQyRC5jcHAJKHJldmlzaW9uIDEx NDAzMSkKKysrIFNvdXJjZS9XZWJDb3JlL2h0bWwvY2FudmFzL0NhbnZhc1JlbmRlcmluZ0NvbnRl eHQyRC5jcHAJKHdvcmtpbmcgY29weSkKQEAgLTIwMjIsNyArMjAyMiw3IEBAIHZvaWQgQ2FudmFz UmVuZGVyaW5nQ29udGV4dDJEOjpkcmF3VGV4dEkKIAogICAgIHVuc2lnbmVkIGxlbmd0aCA9IHRl eHQubGVuZ3RoKCk7CiAgICAgY29uc3QgVUNoYXIqIHN0cmluZyA9IHRleHQuY2hhcmFjdGVycygp OwotICAgIFRleHRSdW4gdGV4dFJ1bihzdHJpbmcsIGxlbmd0aCwgZmFsc2UsIDAsIDAsIFRleHRS dW46OkFsbG93VHJhaWxpbmdFeHBhbnNpb24sIGRpcmVjdGlvbiwgb3ZlcnJpZGUsIFRleHRSdW46 Ok5vUm91bmRpbmcpOworICAgIFRleHRSdW4gdGV4dFJ1bihzdHJpbmcsIGxlbmd0aCwgZmFsc2Us IDAsIDAsIFRleHRSdW46OkFsbG93VHJhaWxpbmdFeHBhbnNpb24sIGRpcmVjdGlvbiwgb3ZlcnJp ZGUsIHRydWUsIFRleHRSdW46Ok5vUm91bmRpbmcpOwogCiAgICAgLy8gRHJhdyB0aGUgaXRlbSB0 ZXh0IGF0IHRoZSBjb3JyZWN0IHBvaW50LgogICAgIEZsb2F0UG9pbnQgbG9jYXRpb24oeCwgeSk7 CkluZGV4OiBTb3VyY2UvV2ViQ29yZS9yZW5kZXJpbmcvUmVuZGVyTGlzdEJveC5jcHAKPT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PQotLS0gU291cmNlL1dlYkNvcmUvcmVuZGVyaW5nL1JlbmRlckxpc3RCb3guY3BwCShyZXZp c2lvbiAxMTQwMzEpCisrKyBTb3VyY2UvV2ViQ29yZS9yZW5kZXJpbmcvUmVuZGVyTGlzdEJveC5j cHAJKHdvcmtpbmcgY29weSkKQEAgLTQwOSw3ICs0MDksNyBAQCB2b2lkIFJlbmRlckxpc3RCb3g6 OnBhaW50SXRlbUZvcmVncm91bmQoCiAKICAgICB1bnNpZ25lZCBsZW5ndGggPSBpdGVtVGV4dC5s ZW5ndGgoKTsKICAgICBjb25zdCBVQ2hhciogc3RyaW5nID0gaXRlbVRleHQuY2hhcmFjdGVycygp OwotICAgIFRleHRSdW4gdGV4dFJ1bihzdHJpbmcsIGxlbmd0aCwgZmFsc2UsIDAsIDAsIFRleHRS dW46OkFsbG93VHJhaWxpbmdFeHBhbnNpb24sIGl0ZW1TdHlsZS0+ZGlyZWN0aW9uKCksIGlzT3Zl cnJpZGUoaXRlbVN0eWxlLT51bmljb2RlQmlkaSgpKSwgVGV4dFJ1bjo6Tm9Sb3VuZGluZyk7Cisg ICAgVGV4dFJ1biB0ZXh0UnVuKHN0cmluZywgbGVuZ3RoLCBmYWxzZSwgMCwgMCwgVGV4dFJ1bjo6 QWxsb3dUcmFpbGluZ0V4cGFuc2lvbiwgaXRlbVN0eWxlLT5kaXJlY3Rpb24oKSwgaXNPdmVycmlk ZShpdGVtU3R5bGUtPnVuaWNvZGVCaWRpKCkpLCB0cnVlLCBUZXh0UnVuOjpOb1JvdW5kaW5nKTsK ICAgICBGb250IGl0ZW1Gb250ID0gc3R5bGUoKS0+Zm9udCgpOwogICAgIExheW91dFJlY3QgciA9 IGl0ZW1Cb3VuZGluZ0JveFJlY3QocGFpbnRPZmZzZXQsIGxpc3RJbmRleCk7CiAgICAgci5tb3Zl KGl0ZW1PZmZzZXRGb3JBbGlnbm1lbnQodGV4dFJ1biwgaXRlbVN0eWxlLCBpdGVtRm9udCwgcikp OwpJbmRleDogU291cmNlL1dlYkNvcmUvcmVuZGVyaW5nL2JyZWFrX2xpbmVzLmNwcAo9PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09Ci0tLSBTb3VyY2UvV2ViQ29yZS9yZW5kZXJpbmcvYnJlYWtfbGluZXMuY3BwCShyZXZpc2lv biAxMTQwMzEpCisrKyBTb3VyY2UvV2ViQ29yZS9yZW5kZXJpbmcvYnJlYWtfbGluZXMuY3BwCSh3 b3JraW5nIGNvcHkpCkBAIC0yNyw2ICsyNyw3IEBACiAjaW5jbHVkZSAiY29uZmlnLmgiCiAjaW5j bHVkZSAiYnJlYWtfbGluZXMuaCIKIAorI2luY2x1ZGUgIkZvbnQuaCIKICNpbmNsdWRlICJUZXh0 QnJlYWtJdGVyYXRvci5oIgogI2luY2x1ZGUgPHd0Zi9BU0NJSUNUeXBlLmg+CiAjaW5jbHVkZSA8 d3RmL1N0ZExpYkV4dHJhcy5oPgpAQCAtMTcwLDkgKzE3MSwxMyBAQCBpbnQgbmV4dEJyZWFrYWJs ZVBvc2l0aW9uKExhenlMaW5lQnJlYWtJCiAKICAgICAgICAgaWYgKG5lZWRzTGluZUJyZWFrSXRl cmF0b3IoY2gpIHx8IG5lZWRzTGluZUJyZWFrSXRlcmF0b3IobGFzdENoKSkgewogICAgICAgICAg ICAgaWYgKG5leHRCcmVhayA8IGkgJiYgaSkgewotICAgICAgICAgICAgICAgIFRleHRCcmVha0l0 ZXJhdG9yKiBicmVha0l0ZXJhdG9yID0gbGF6eUJyZWFrSXRlcmF0b3IuZ2V0KCk7Ci0gICAgICAg ICAgICAgICAgaWYgKGJyZWFrSXRlcmF0b3IpCi0gICAgICAgICAgICAgICAgICAgIG5leHRCcmVh ayA9IHRleHRCcmVha0ZvbGxvd2luZyhicmVha0l0ZXJhdG9yLCBpIC0gMSk7CisgICAgICAgICAg ICAgICAgaWYgKEZvbnQ6OmlzQ0pLSWRlb2dyYXBoT3JTeW1ib2woY2gpICYmIEZvbnQ6OmlzQ0pL SWRlb2dyYXBoT3JTeW1ib2wobGFzdENoKSkgLy8gT3B0aW1pemF0aW9uIHRvIGF2b2lkIGZhbGxp bmcgaW50byBJQ1UgKHdoaWNoIGlzIHZlcnkgc2xvdyBmb3IgaWRlb2dyYXBocykuCisgICAgICAg ICAgICAgICAgICAgIG5leHRCcmVhayA9IGk7CisgICAgICAgICAgICAgICAgLyplbHNlIHsKKyAg ICAgICAgICAgICAgICAgICAgVGV4dEJyZWFrSXRlcmF0b3IqIGJyZWFrSXRlcmF0b3IgPSBsYXp5 QnJlYWtJdGVyYXRvci5nZXQoKTsKKyAgICAgICAgICAgICAgICAgICAgaWYgKGJyZWFrSXRlcmF0 b3IpCisgICAgICAgICAgICAgICAgICAgICAgICBuZXh0QnJlYWsgPSB0ZXh0QnJlYWtGb2xsb3dp bmcoYnJlYWtJdGVyYXRvciwgaSAtIDEpOworICAgICAgICAgICAgICAgIH0qLwogICAgICAgICAg ICAgfQogICAgICAgICAgICAgaWYgKGkgPT0gbmV4dEJyZWFrICYmICFpc0JyZWFrYWJsZVNwYWNl KGxhc3RDaCwgdHJlYXROb0JyZWFrU3BhY2VBc0JyZWFrKSkKICAgICAgICAgICAgICAgICByZXR1 cm4gaTsK 137179 2012-04-13 16:48:37 -0700 2012-04-14 11:16:06 -0700 Patch patch.txt text/plain 2681 hyatt SW5kZXg6IFNvdXJjZS9XZWJDb3JlL0NoYW5nZUxvZwo9PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09Ci0tLSBTb3VyY2UvV2Vi Q29yZS9DaGFuZ2VMb2cJKHJldmlzaW9uIDExNDE4MykKKysrIFNvdXJjZS9XZWJDb3JlL0NoYW5n ZUxvZwkod29ya2luZyBjb3B5KQpAQCAtMSwzICsxLDE5IEBACisyMDEyLTA0LTEzICBEYXZpZCBI eWF0dCAgPGh5YXR0QGFwcGxlLmNvbT4KKworICAgICAgICBodHRwczovL2J1Z3Mud2Via2l0Lm9y Zy9zaG93X2J1Zy5jZ2k/aWQ9ODM4MjYKKyAgICAgICAgCisgICAgICAgIFdyb25nIEZvbnQgY29k ZSBwYXRoIHdhcyBjaG9zZW4gYWZ0ZXIgcjExNDAzMi4gVGhlIFRleHRSdW4gY29uc3RydWN0b3Ig aGFzIGFuIGFkZGl0aW9uYWwgYXJndW1lbnQgdGhhdAorICAgICAgICBuZWVkcyB0byBiZSBpbmNs dWRlZCBpZiByb3VuZGluZyBoYWNrcyBhcmUgbWVudGlvbmVkLgorCisgICAgICAgIFJldmlld2Vk IGJ5IE5PQk9EWSAoT09QUyEpLgorCisgICAgICAgICogaHRtbC9jYW52YXMvQ2FudmFzUmVuZGVy aW5nQ29udGV4dDJELmNwcDoKKyAgICAgICAgKFdlYkNvcmU6OkNhbnZhc1JlbmRlcmluZ0NvbnRl eHQyRDo6ZHJhd1RleHRJbnRlcm5hbCk6CisgICAgICAgICogcmVuZGVyaW5nL1JlbmRlckxpc3RC b3guY3BwOgorICAgICAgICAoV2ViQ29yZTo6UmVuZGVyTGlzdEJveDo6cGFpbnRJdGVtRm9yZWdy b3VuZCk6CisgICAgICAgICogcmVuZGVyaW5nL2JyZWFrX2xpbmVzLmNwcDoKKyAgICAgICAgKFdl YkNvcmU6Om5leHRCcmVha2FibGVQb3NpdGlvbik6CisKIDIwMTItMDQtMTMgIEplciBOb2JsZSAg PGplci5ub2JsZUBhcHBsZS5jb20+CiAKICAgICAgICAgVmlkZW8gYXQgYXBwbGUuY29tIGdldHMg c3RhbmRhcmQgY29udHJvbHMgaW4gYWRkaXRpb24gdG8gY3VzdG9tIGNvbnRyb2xzIGFmdGVyIHJl dHVybmluZyBmcm9tIGZ1bGwgc2NyZWVuCkluZGV4OiBTb3VyY2UvV2ViQ29yZS9odG1sL2NhbnZh cy9DYW52YXNSZW5kZXJpbmdDb250ZXh0MkQuY3BwCj09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT0KLS0tIFNvdXJjZS9XZWJD b3JlL2h0bWwvY2FudmFzL0NhbnZhc1JlbmRlcmluZ0NvbnRleHQyRC5jcHAJKHJldmlzaW9uIDEx NDAzMSkKKysrIFNvdXJjZS9XZWJDb3JlL2h0bWwvY2FudmFzL0NhbnZhc1JlbmRlcmluZ0NvbnRl eHQyRC5jcHAJKHdvcmtpbmcgY29weSkKQEAgLTIwMjIsNyArMjAyMiw3IEBAIHZvaWQgQ2FudmFz UmVuZGVyaW5nQ29udGV4dDJEOjpkcmF3VGV4dEkKIAogICAgIHVuc2lnbmVkIGxlbmd0aCA9IHRl eHQubGVuZ3RoKCk7CiAgICAgY29uc3QgVUNoYXIqIHN0cmluZyA9IHRleHQuY2hhcmFjdGVycygp OwotICAgIFRleHRSdW4gdGV4dFJ1bihzdHJpbmcsIGxlbmd0aCwgZmFsc2UsIDAsIDAsIFRleHRS dW46OkFsbG93VHJhaWxpbmdFeHBhbnNpb24sIGRpcmVjdGlvbiwgb3ZlcnJpZGUsIFRleHRSdW46 Ok5vUm91bmRpbmcpOworICAgIFRleHRSdW4gdGV4dFJ1bihzdHJpbmcsIGxlbmd0aCwgZmFsc2Us IDAsIDAsIFRleHRSdW46OkFsbG93VHJhaWxpbmdFeHBhbnNpb24sIGRpcmVjdGlvbiwgb3ZlcnJp ZGUsIHRydWUsIFRleHRSdW46Ok5vUm91bmRpbmcpOwogCiAgICAgLy8gRHJhdyB0aGUgaXRlbSB0 ZXh0IGF0IHRoZSBjb3JyZWN0IHBvaW50LgogICAgIEZsb2F0UG9pbnQgbG9jYXRpb24oeCwgeSk7 CkluZGV4OiBTb3VyY2UvV2ViQ29yZS9yZW5kZXJpbmcvUmVuZGVyTGlzdEJveC5jcHAKPT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PQotLS0gU291cmNlL1dlYkNvcmUvcmVuZGVyaW5nL1JlbmRlckxpc3RCb3guY3BwCShyZXZp c2lvbiAxMTQwMzEpCisrKyBTb3VyY2UvV2ViQ29yZS9yZW5kZXJpbmcvUmVuZGVyTGlzdEJveC5j cHAJKHdvcmtpbmcgY29weSkKQEAgLTQwOSw3ICs0MDksNyBAQCB2b2lkIFJlbmRlckxpc3RCb3g6 OnBhaW50SXRlbUZvcmVncm91bmQoCiAKICAgICB1bnNpZ25lZCBsZW5ndGggPSBpdGVtVGV4dC5s ZW5ndGgoKTsKICAgICBjb25zdCBVQ2hhciogc3RyaW5nID0gaXRlbVRleHQuY2hhcmFjdGVycygp OwotICAgIFRleHRSdW4gdGV4dFJ1bihzdHJpbmcsIGxlbmd0aCwgZmFsc2UsIDAsIDAsIFRleHRS dW46OkFsbG93VHJhaWxpbmdFeHBhbnNpb24sIGl0ZW1TdHlsZS0+ZGlyZWN0aW9uKCksIGlzT3Zl cnJpZGUoaXRlbVN0eWxlLT51bmljb2RlQmlkaSgpKSwgVGV4dFJ1bjo6Tm9Sb3VuZGluZyk7Cisg ICAgVGV4dFJ1biB0ZXh0UnVuKHN0cmluZywgbGVuZ3RoLCBmYWxzZSwgMCwgMCwgVGV4dFJ1bjo6 QWxsb3dUcmFpbGluZ0V4cGFuc2lvbiwgaXRlbVN0eWxlLT5kaXJlY3Rpb24oKSwgaXNPdmVycmlk ZShpdGVtU3R5bGUtPnVuaWNvZGVCaWRpKCkpLCB0cnVlLCBUZXh0UnVuOjpOb1JvdW5kaW5nKTsK ICAgICBGb250IGl0ZW1Gb250ID0gc3R5bGUoKS0+Zm9udCgpOwogICAgIExheW91dFJlY3QgciA9 IGl0ZW1Cb3VuZGluZ0JveFJlY3QocGFpbnRPZmZzZXQsIGxpc3RJbmRleCk7CiAgICAgci5tb3Zl KGl0ZW1PZmZzZXRGb3JBbGlnbm1lbnQodGV4dFJ1biwgaXRlbVN0eWxlLCBpdGVtRm9udCwgcikp Owo=