Bug 62889 – Add Font interface to support Skia on Mac Chrome port

• Component: New Bugs
• Status: RESOLVED FIXED

[Cary Clark]

Add Font interface to support Skia on Mac Chrome port

[Cary Clark]

Created attachment 97624 [details]
Patch

[WebKit Review Bot]

Comment on attachment 97624 [details]
Patch

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

[Cary Clark]

Created attachment 97634 [details]
Patch

[Cary Clark]

Created attachment 97636 [details]
Patch

[Cary Clark]

Eric, feel free to punt back if this isn't something you're comfortable reviewing -- I'm having trouble finding an appropriate reviewer.

[Eric Seidel (no email)]

Comment on attachment 97636 [details]
Patch

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

> Source/WebCore/platform/graphics/skia/FontSkia.cpp:51
> +#include "ComplexTextController.h"
> +#include "FloatRect.h"
> +#include "GlyphBuffer.h"
> +#include "GraphicsContext.h"
> +#include "NotImplemented.h"
> +#include "PlatformContextSkia.h"
> +#include "SimpleFontData.h"
> +
> +#include "SkCanvas.h"
> +#include "SkFontHost.h"
> +#include "SkPaint.h"
> +#include "SkTemplates.h"
> +#include "SkTypeface.h"
> +#include "SkTypeface_mac.h"
> +#include "SkUtils.h"
> +
> +#include "skia/ext/platform_canvas.h"
> +#include <wtf/unicode/Unicode.h>

Are all these really needed?  I didn't notice any notImplemented() calls at least.

> Source/WebCore/platform/graphics/skia/FontSkia.cpp:63
> +bool Font::canReturnFallbackFontsForComplexText()
> +{
> +    return false;
> +}
> +
> +bool Font::canExpandAroundIdeographsInComplexText()
> +{
> +    return false;
> +}

Why?  Both of these might deserve comments.

> Source/WebCore/platform/graphics/skia/FontSkia.cpp:85
> +    const float ts = platformData.m_size >= 0 ? platformData.m_size : 12;

ts?  I assume you mean textSize?  or fontSize?

> Source/WebCore/platform/graphics/skia/FontSkia.cpp:90
> +    SkTypeface* typeface = SkCreateTypefaceFromCTFont(platformData.ctFont());

Don't we have a smartpointer to use here?  Like RetainPtr with Skia specializations?

> Source/WebCore/platform/graphics/skia/FontSkia.cpp:102
> +    SkASSERT(sizeof(GlyphBufferGlyph) == sizeof(uint16_t)); // compile-time assert

I think WebKit has a COMPILE_ASSERT or similar, which could avoid the need for your comment. :)

> Source/WebCore/platform/graphics/skia/FontSkia.cpp:130
> +    for (int i = 0; i < numGlyphs; i++) {
> +        SkScalar myWidth = SkFloatToScalar(adv[i].width);
> +        pos[i].set(x, y);
> +        if (isVertical) {
> +            vPosBegin[i].set(x + myWidth, y);
> +            vPosEnd[i].set(x + myWidth, y - myWidth);
> +        }
> +        x += myWidth;
> +        y += SkFloatToScalar(adv[i].height);
> +    }

I would have probably made this a helper to better scope its inputs/outputs.  Also may better prepare you for later special-casing the default advances case you mentioned in the fixme.

> Source/WebCore/platform/graphics/skia/FontSkia.cpp:144
> +    // We draw text up to two times (once for fill, once for stroke).
> +    if (textMode & TextModeFill) {
> +        SkPaint paint;
> +        gc->platformContext()->setupPaintForFilling(&paint);
> +        setupPaint(&paint, font, this);
> +        adjustTextRenderMode(&paint, gc->platformContext());
> +        paint.setTextEncoding(SkPaint::kGlyphID_TextEncoding);
> +        paint.setColor(gc->fillColor().rgb());

So much code we could share between fill and stroke! no?

> Source/WebCore/platform/graphics/skia/FontSkia.cpp:155
> +        if (isVertical) {
> +            SkPath path;
> +            for (int i = 0; i < numGlyphs; ++i) {
> +                path.reset();
> +                path.moveTo(vPosBegin[i]);
> +                path.lineTo(vPosEnd[i]);
> +                canvas->drawTextOnPath(glyphs + i, 2, path, 0, paint);
> +            }
> +        } else
> +            canvas->drawPosText(glyphs, numGlyphs << 1, pos, paint);

This is repeated exactly twice, no?

> Source/WebCore/platform/graphics/skia/FontSkia.cpp:172
> +            SkSafeUnref(paint.setLooper(0));

Huh?  I assume we're trying to release the looper here?  If so, why doesn't SkPaint hold it in some sort of smart pointer/hold the reference itself?

> Source/WebCore/platform/graphics/skia/FontSkia.cpp:181
> +                canvas->drawTextOnPath(glyphs + i, 2, path, 0, paint);

What does the magic 2 and 0 do?

> Source/WebCore/platform/graphics/skia/FontSkia.cpp:184
> +            canvas->drawPosText(glyphs, numGlyphs << 1, pos, paint);

Why are we multiplying by 2 here?  And if so, why use << 1 to do it?

[Cary Clark]

Comment on attachment 97636 [details]
Patch

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

>> Source/WebCore/platform/graphics/skia/FontSkia.cpp:51
>> +#include <wtf/unicode/Unicode.h>
> 
> Are all these really needed?  I didn't notice any notImplemented() calls at least.

Removed a bunch. Hopefully the set is minimal now.

>> Source/WebCore/platform/graphics/skia/FontSkia.cpp:63
>> +}
> 
> Why?  Both of these might deserve comments.

The Windows and Linux ports do not support either, but on the other hand, they don't get their font data from CoreText. I don't know if it is correct to support these or not -- I assume it was safer not to. I have added comments and bugs to track this. (Is it correct to note the bug number in the comment?)

>> Source/WebCore/platform/graphics/skia/FontSkia.cpp:85
>> +    const float ts = platformData.m_size >= 0 ? platformData.m_size : 12;
> 
> ts?  I assume you mean textSize?  or fontSize?

Done.

>> Source/WebCore/platform/graphics/skia/FontSkia.cpp:90
>> +    SkTypeface* typeface = SkCreateTypefaceFromCTFont(platformData.ctFont());
> 
> Don't we have a smartpointer to use here?  Like RetainPtr with Skia specializations?

Done.

>> Source/WebCore/platform/graphics/skia/FontSkia.cpp:102
>> +    SkASSERT(sizeof(GlyphBufferGlyph) == sizeof(uint16_t)); // compile-time assert
> 
> I think WebKit has a COMPILE_ASSERT or similar, which could avoid the need for your comment. :)

Done.

>> Source/WebCore/platform/graphics/skia/FontSkia.cpp:130
>> +    }
> 
> I would have probably made this a helper to better scope its inputs/outputs.  Also may better prepare you for later special-casing the default advances case you mentioned in the fixme.

Eric, I'm uncomfortable rewriting the logic of this code. As uploaded here, it is the same as FontLinux.cpp. Since all of the Skia support has not yet been submitted, rewrites to improve the readability may introduce subtle bugs that will be difficult to find.

I am happy to make minor edits that I can ensure are correct, and have done so with all of your comments except this one and the two that follow. [e-mail] is working on fixing some unrelated bugs in FontLinux.cpp, and is happy to fix the code there to address your concerns (I copied him on this review).

I'll file a bug to track that these changes need to be made to both FontSkia.cpp and FontLinux.cpp, as well as add a TODO comment.

>> Source/WebCore/platform/graphics/skia/FontSkia.cpp:144
>> +        paint.setColor(gc->fillColor().rgb());
> 
> So much code we could share between fill and stroke! no?

See comment above.

>> Source/WebCore/platform/graphics/skia/FontSkia.cpp:155
>> +            canvas->drawPosText(glyphs, numGlyphs << 1, pos, paint);
> 
> This is repeated exactly twice, no?

See comment above.

>> Source/WebCore/platform/graphics/skia/FontSkia.cpp:172
>> +            SkSafeUnref(paint.setLooper(0));
> 
> Huh?  I assume we're trying to release the looper here?  If so, why doesn't SkPaint hold it in some sort of smart pointer/hold the reference itself?

The code was incorrect (if harmless). (Fixed.)

>> Source/WebCore/platform/graphics/skia/FontSkia.cpp:181
>> +                canvas->drawTextOnPath(glyphs + i, 2, path, 0, paint);
> 
> What does the magic 2 and 0 do?

Replaced the 2 with sizeof(uint16_t). The 0 is a pointer to the optional matrix. What's the best convention to express this? Would you prefer something like:

static const int noOptionalMatrix = 0;

>> Source/WebCore/platform/graphics/skia/FontSkia.cpp:184
>> +            canvas->drawPosText(glyphs, numGlyphs << 1, pos, paint);
> 
> Why are we multiplying by 2 here?  And if so, why use << 1 to do it?

Change to * sizeof(uint16_t). (Done.)

[Cary Clark]

Created attachment 97801 [details]
Patch

[Eric Seidel (no email)]

Comment on attachment 97801 [details]
Patch

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

OK.

> Source/WebCore/platform/graphics/skia/FontSkia.cpp:88
> +    SkTypeface* typeface = SkCreateTypefaceFromCTFont(platformData.ctFont());
> +    SkAutoUnref autoUnref(typeface);

I take it skia smart pointers don't work like webkit (or boost) ones with a .get() method?  If so, then there is no need to have a separate raw pointer.

[WebKit Review Bot]

Comment on attachment 97801 [details]
Patch

Clearing flags on attachment: 97801

Committed r89266: <http://trac.webkit.org/changeset/89266>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.