Implement SimpleFontDataSkia::platformBoundsForGlyph
Created attachment 172162 [details] Patch
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?
Comment on attachment 172162 [details] Patch Attachment 172162 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/14697815
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>.
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.
Timid is fine. :) One of the Skia guys can set us straight.
Created attachment 172168 [details] Patch
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.
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
Created attachment 172198 [details] Patch
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
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
Created attachment 172230 [details] Patch
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.
(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!
exactly! If it fails, then we won't draw the right glyph. In the current code, we will silently draw the wrong thing.
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);
I'm fine either way. Just wanted to mention the alternate style, but I do not want to force a new patch.
(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.)
Comment on attachment 172230 [details] Patch It sounds like Mike thinks it's OK from the Skia side. LGTM.
Comment on attachment 172230 [details] Patch Clearing flags on attachment: 172230 Committed r133494: <http://trac.webkit.org/changeset/133494>
All reviewed patches have been landed. Closing bug.