Bug 4940 – CG and ATSUI give different width to the same text

• Status: VERIFIED
• Resolution: FIXED
• Component: Layout and Rendering

Summary: A given glyph sequence can have different width depending on the code path used to 
measure (or draw) it. This is especially problematic when the same sequence is measured by both code 
paths during the same layout operation (typically CG will measure a word that's CG-safe and then ATSUI 
will measure the entire non-CG-safe run containing that word).

To reproduce: Open the testcase. Select Use ATSU For All Text in the Debug menu and reload.

Expected: both red boxes to have the same width, text in both boxes to be the same width; same for 
the green boxes; in the lines of "mmm"s, all spaces between words to be the same width (up to a 
fraction of a pixel, at least); when switching to ATSU, ideally nothing should change.

Actual: boxes not the same width, text inside them not the same width, and the bottom boxes in each 
pair are too wide for their text; a wider space after the 4th "mmm" in the first line and after the 6th 
"mmm" in the second line; when switching to ATSU, everything is as expected but letter-spacing is 
smaller, so everything is narrower.

The measurements differ because the ATSUI code path uses device metrics in order to ensure that word 
boundaries occur at integer coordinates.

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.

[Darin Adler]

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

[Darin Adler]

Comment on attachment 3922 [details]
patch to implement the rounding hacks in the ATSUI code path

hit submit by accident, more comments coming

[Darin Adler]

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...

[Darin Adler]

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.