Bug 101115 – Implement SimpleFontData::platformBoundsForGlyph on skia

• Component: Platform
• Status: RESOLVED FIXED

[Dave Barton]

Implement SimpleFontDataSkia::platformBoundsForGlyph

[Dave Barton]

Created attachment 172162 [details]
Patch

[Eric Seidel (no email)]

Comment on attachment 172162 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=172162&action=review

Seems reasonable.  One of hte skia guys should say LGTM first.

> Source/WebCore/platform/graphics/skia/SimpleFontDataSkia.cpp:242
> +    SkASSERT(sizeof(glyph) == 2); // compile-time assert

Curious why this ASSERT is needed?  Could we just pass sizeof(glyph) instead of 2 to measureText?

[WebKit Review Bot]

Comment on attachment 172162 [details]
Patch

Attachment 172162 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/14697815

[Dave Barton]

This is needed for MathML in Chrome on linux. It's the most important patch to include now in Chrome 24 (other than security ones). Without this, it would be better to not enable MathML in Chrome on linux.

This is a duplicate of <http://crbug.com/77095>.

[Dave Barton]

Comment on attachment 172162 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=172162&action=review

>> Source/WebCore/platform/graphics/skia/SimpleFontDataSkia.cpp:242
>> +    SkASSERT(sizeof(glyph) == 2); // compile-time assert
> 
> Curious why this ASSERT is needed?  Could we just pass sizeof(glyph) instead of 2 to measureText?

I just copied this from platformWidthForGlyph which comes next. I guess we could change both of them. This is my first time with skia, so I am being timid.

[Eric Seidel (no email)]

Timid is fine. :)  One of the Skia guys can set us straight.

[Dave Barton]

Created attachment 172168 [details]
Patch

[Dave Barton]

Comment on attachment 172162 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=172162&action=review

Sorry to spam you all with my dumbness. I can't compile this on my Mac, so I was going to cc you all after I got it working on cr-linux, but forgot.

>>> Source/WebCore/platform/graphics/skia/SimpleFontDataSkia.cpp:242
>>> +    SkASSERT(sizeof(glyph) == 2); // compile-time assert
>> 
>> Curious why this ASSERT is needed?  Could we just pass sizeof(glyph) instead of 2 to measureText?
> 
> I just copied this from platformWidthForGlyph which comes next. I guess we could change both of them. This is my first time with skia, so I am being timid.

Actually I now think it's good to assert that |glyph| is 2 bytes, like skia expects. If it was stored in a 4-byte int, then passing that as a 4-byte argument to skia might be bad - it'd be a null (0) character followed by a real one, or the other way around for little-endian machines.

[WebKit Review Bot]

Comment on attachment 172168 [details]
Patch

Attachment 172168 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/14692999

New failing tests:
fast/repaint/stacked-diacritics.html
mathml/xHeight.xhtml
fast/block/lineboxcontain/block-glyphs.html

[Dave Barton]

Created attachment 172198 [details]
Patch

[WebKit Review Bot]

Comment on attachment 172198 [details]
Patch

Attachment 172198 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/14720108

New failing tests:
fast/repaint/stacked-diacritics.html

[WebKit Review Bot]

Comment on attachment 172198 [details]
Patch

Attachment 172198 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/14720117

New failing tests:
fast/repaint/stacked-diacritics.html

[Dave Barton]

Created attachment 172230 [details]
Patch

[Mike Reed]

We could replace the assert with an assignment if that is preferrable.

uint16_t glyphID16 = SkToU16(glyph);

and then pass &glyphID16 to the paint API. Not saying its better, its just a different way to be sure we pass the right thing to skia.

[Dave Barton]

(In reply to comment #14)
> We could replace the assert with an assignment if that is preferrable.
> 
> uint16_t glyphID16 = SkToU16(glyph);
> 
> and then pass &glyphID16 to the paint API. Not saying its better, its just a different way to be sure we pass the right thing to skia.

This apparently fails in debug mode if glyph ≥ 2^16, and succeeds otherwise. Actually I prefer the current code, because if we ever change Glyph to be an unsigned int instead of unsigned short, e.g. to handle Unicode characters ≥ 2^16, then I think it's better to get the failure as soon as possible (not wait until we actually pass in a non-BMP codepoint). On the other hand, you are the expert on this. :) Thanks for the help!

[Mike Reed]

exactly! If it fails, then we won't draw the right glyph. In the current code, we will silently draw the wrong thing.

[Dave Barton]

In the current code, we assert sizeof(glyph) == 2, so we can't pass a number ≥ 2^16.

What if I leave the assert, and also add your line:
uint16_t glyphID16 = SkToU16(glyph);

[Mike Reed]

I'm fine either way. Just wanted to mention the alternate style, but I do not want to force a new patch.

[Dave Barton]

(In reply to comment #18)
> I'm fine either way. Just wanted to mention the alternate style, but I do not want to force a new patch.

I can live with the current patch. :) So is this ok for Eric or Tony or someone to r+, or do you want to? Or would reviewers prefer a new patch? (I don't mind making one if necessary.)

[Eric Seidel (no email)]

Comment on attachment 172230 [details]
Patch

It sounds like Mike thinks it's OK from the Skia side.  LGTM.

[WebKit Review Bot]

Comment on attachment 172230 [details]
Patch

Clearing flags on attachment: 172230

Committed r133494: <http://trac.webkit.org/changeset/133494>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.