Bug 25961 – wx port measures character widths incorrectly (Windows specific)

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit wx

[Kevin Watters]

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.

[Kevin Watters]

Created attachment 30579 [details]
Do not use GetCharABCWidths for single character measurements

Fixes the single character measuring problem and makes GDI font rendering match with Firefox/IE.

[Eric Seidel (no email)]

Comment on attachment 30579 [details]
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.

[Kevin Ollivier]

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!

[Kevin Ollivier]

Landed in r44057, thanks!