[webkit-reviews] review denied: [Bug 62889] Add Font interface to support Skia on Mac Chrome port : [Attachment 97636] Patch

[bugzilla-daemon at webkit.org]

Eric Seidel <eric at webkit.org> has denied Cary Clark <caryclark at google.com>'s
request for review:
Bug 62889: Add Font interface to support Skia on Mac Chrome port
https://bugs.webkit.org/show_bug.cgi?id=62889

Attachment 97636: Patch
https://bugs.webkit.org/attachment.cgi?id=97636&action=review

------- Additional Comments from Eric Seidel <eric at webkit.org>
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?