Summary: | CG and ATSUI give different width to the same text | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | mitz | ||||||||
Component: | Layout and Rendering | Assignee: | Dave Hyatt <hyatt> | ||||||||
Status: | VERIFIED FIXED | ||||||||||
Severity: | Normal | ||||||||||
Priority: | P2 | ||||||||||
Version: | 420+ | ||||||||||
Hardware: | Mac | ||||||||||
OS: | OS X 10.4 | ||||||||||
Bug Depends on: | |||||||||||
Bug Blocks: | 4844 | ||||||||||
Attachments: |
|
Description
mitz
2005-09-11 14:29:26 PDT
Created attachment 3865 [details]
testcase
Created attachment 3922 [details]
patch to implement the rounding hacks in the ATSUI code path
I tried to sneak letter-spacing support in with this patch, but I didn't do it right. I'm not submitting a revised patch now, but after this one is reviewed, I'll take the letter-spacing part out and deal with it under bug 3922. Comment on attachment 3922 [details]
patch to implement the rounding hacks in the ATSUI code path
ATSULayoutParameters should not be in the header -- it's not used in any
declarations outside the file so it doesn't need to be.
Mitz said he'd remove the letter-spacing bits.
Is it right to use a copy of an ATSUTextLayout rather than a pointer in
ATSUTextLayoutParameters?
Please don't use ``'' quoting in WebKit code.
There should not be a space after the word initializeATSULayoutParameters and
before the opening parenthesis.
The overrideUPP should not be in a global variable. Since WebKit is never
compiled with CFM, the NewATSUDirectLayoutOperationOverrideUPP function is
simply
Comment on attachment 3922 [details]
patch to implement the rounding hacks in the ATSUI code path
hit submit by accident, more comments coming
Comment on attachment 3922 [details]
patch to implement the rounding hacks in the ATSUI code path
The overrideUPP should not be in a global variable. Since WebKit is never
compiled with CFM, the ATSUDirectLayoutOperationOverrideUPP is the same thing
as ATSUDirectLayoutOperationOverrideProcPtr and there's no reason to call
NewATSUDirectLayoutOperationOverrideUPP at all.
There are a couple cases of "if(status" in here with no space before the
parenthesis.
I think that given that initializeATSULayoutParameters creates the layout, it
should have a name more like "create" and there should be a corresponding
"dispose" function.
Otherwise, looks good. (Too bad my synthesized bold/oblique patch isn't landed
yet. Merging is going to be a pain.)
Created attachment 4020 [details]
patch to implement the rounding hacks in the ATSUI code path
Comment on attachment 4020 [details] patch to implement the rounding hacks in the ATSUI code path Revised patch addressing Darin's comments: > ATSULayoutParameters should not be in the header Removed (I based ATSULayoutParameters on CharacterWidthIterator, which currently is in the header, apparently for no good reason). > letter-spacing bits Removed. > Is it right to use a copy of an ATSUTextLayout rather than a pointer in > ATSUTextLayoutParameters? ATSUTextLayout is itself a pointer to an opaque struct. > there's no reason to call NewATSUDirectLayoutOperationOverrideUPP at all. Changed to use the procptr directly. Also changed to install the override only if word rounding is required (since otherwise it doesn't do anything). > I think that given that initializeATSULayoutParameters creates the layout, it > should have a name more like "create" and there should be a corresponding > "dispose" function. Renamed to createATSULayoutParameters and added disposeATSULayoutParameters. Note that they don't allocate/free the params structure itself. Also corrected coding style mistakes pointed out. > Too bad my synthesized bold/oblique patch isn't landed > yet. Merging is going to be a pain. Indeed... Comment on attachment 4020 [details]
patch to implement the rounding hacks in the ATSUI code path
r=me
I just noticed that the patch doesn't handle the case where font fallback occurs and the fallback font's rendering mode isn't the same as the original font's. Another couple of things not addressed by the patch: 1. ATSU applies kerning when the font specifies it but CG doesn't. 2. ATSU uses ligatures such as "fi" but CG doesn't. Both affect width. I should open a new bug about all the things this patch didn't fix. Opened bug 5166 about remaining mismatches. |