25961 2009-05-22 07:13:43 -0700 wx port measures character widths incorrectly (Windows specific) 2009-05-22 13:12:44 -0700 1 1 1 Unclassified WebKit WebKit wx 528+ (Nightly build) PC Windows XP RESOLVED FIXED P2 Minor --- 1 kevinwatters webkit-unassigned oldest_to_newest 122652 0 kevinwatters 2009-05-22 07:13:43 -0700 The GetTextExtent function in platform/wx/wxcode/win/fontprops.cpp measures font character widths incorrectly. Code copied from wx internals assumes that you are measuring font strings of more then one character, and accounts for overhang by subtracting values obtained from the GetCharABCWidths function. This is incorrect for measuring the width of individual characters, like WebKit asks for. 122653 1 30579 kevinwatters 2009-05-22 07:14:48 -0700 Created attachment 30579 Do not use GetCharABCWidths for single character measurements Fixes the single character measuring problem and makes GDI font rendering match with Firefox/IE. 122658 2 30579 eric 2009-05-22 07:28:54 -0700 Comment on attachment 30579 Do not use GetCharABCWidths for single character measurements Where is this code used? Which multi-character strings does this affect? Test case? Also, the style is not webkit style in fontprops.cpp Normally all code in WebCore matches WK style, I guess we've made an exception for wx files? Please either add a test case or update the ChangeLog to explain why one is not possible (and if needed, file a bug requesting the additional features being added to wx's DumpRenderTree). Thanks for the patch! r- for lack of test. 122684 3 kevino 2009-05-22 10:11:44 -0700 Sorry, I know changing a review is unusual, but Eric didn't even give me an opportunity to review the patch myself, and given all the questions he asked I have to wonder why he didn't even give someone more qualified a chance to review it. In fact, I had seen this patch even before it was posted, and this is code that initially was going to go into wx, but now will probably be phased out in the not-too-distant future. Until we do so, though, we need this as a stopgap measure. In addition, we're still working on getting a complete testing framework up and running for wx, so if we were to r- patches on those grounds, we'd be unable to commit anything. Similarly, adding comments to every patch because our testing framework isn't yet ready for action seems a bit silly. By this rule, almost every wx patch ever committed should have this note, to the point that we should probably write a script adding it. ;-) wx is not in the same place, port-wise, as Win, Mac or Chromium and I'd appreciate it if in the future, patch reviewers would either take that into consideration before giving an r-, or preferably just leave port-specific reviews to port reviewers. I assure you, I very rarely let a patch sit around more than a few days in the queue without review! 122725 4 kevino 2009-05-22 13:12:44 -0700 Landed in r44057, thanks! 30579 2009-05-22 07:14:48 -0700 2009-05-22 08:56:32 -0700 Do not use GetCharABCWidths for single character measurements fontprops.patch text/plain 1098 kevinwatters SW5kZXg6IFdlYkNvcmUvQ2hhbmdlTG9nCj09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT0KLS0tIFdlYkNvcmUvQ2hhbmdlTG9n CShyZXZpc2lvbiA0NDAzMSkKKysrIFdlYkNvcmUvQ2hhbmdlTG9nCSh3b3JraW5nIGNvcHkpCkBA IC0xLDMgKzEsMTIgQEAKKzIwMDktMDUtMjEgIEtldmluIFdhdHRlcnMgIDxrZXZpbndhdHRlcnNA Z21haWwuY29tPgorCisgICAgICAgIFJldmlld2VkIGJ5IE5PQk9EWSAoT09QUyEpLgorCisgICAg ICAgIE9ubHkgYWNjb3VudCBmb3Igb3ZlcmhhbmcgZm9yIG11bHRpLWNoYXJhY3RlciBzdHJpbmdz LgorCisgICAgICAgICogcGxhdGZvcm0vd3gvd3hjb2RlL3dpbi9mb250cHJvcHMuY3BwOgorICAg ICAgICAoR2V0VGV4dEV4dGVudCk6CisKIDIwMDktMDUtMjEgIFN0ZXBoYW5pZSBMZXdpcyAgPHNs ZXdpc0BhcHBsZS5jb20+CiAKICAgICAgICAgUmV2aWV3ZWQgYnkgTWFyayBSb3dlLgpJbmRleDog V2ViQ29yZS9wbGF0Zm9ybS93eC93eGNvZGUvd2luL2ZvbnRwcm9wcy5jcHAKPT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PQot LS0gV2ViQ29yZS9wbGF0Zm9ybS93eC93eGNvZGUvd2luL2ZvbnRwcm9wcy5jcHAJKHJldmlzaW9u IDQ0MDE2KQorKysgV2ViQ29yZS9wbGF0Zm9ybS93eC93eGNvZGUvd2luL2ZvbnRwcm9wcy5jcHAJ KHdvcmtpbmcgY29weSkKQEAgLTkxLDcgKzkxLDcgQEAKICAgICAvLyBhY2NvdW50cyBmb3IgdW5k ZXIvb3Zlcmhhbmcgb2YgdGhlIGZpcnN0L2xhc3QgY2hhcmFjdGVyIHdoaWxlIHdlIHdhbnQKICAg ICAvLyBqdXN0IHRoZSBib3VuZGluZyByZWN0IGZvciB0aGlzIHN0cmluZyBzbyBhZGp1c3QgdGhl IHdpZHRoIGFzIG5lZWRlZAogICAgIC8vICh1c2luZyBBUEkgbm90IGF2YWlsYWJsZSBpbiAyMDAy IFNES3Mgb2YgV2luQ0UpCi0gICAgaWYgKCBsZW4gPiAwICkKKyAgICBpZiAoIGxlbiA+IDEgKQog ICAgIHsKICAgICAgICAgQUJDIHdpZHRoOwogICAgICAgICBjb25zdCB3eENoYXIgY2hGaXJzdCA9 ICpzdHIuYmVnaW4oKTsK