[webkit-reviews] review denied: [Bug 55609] option to use skia's font backend when drawing text on windows : [Attachment 84462] fix style complaint

[bugzilla-daemon at webkit.org]

James Robinson <jamesr at chromium.org> has denied Mike Reed <reed at google.com>'s
request for review:
Bug 55609: option to use skia's font backend when drawing text on windows
https://bugs.webkit.org/show_bug.cgi?id=55609

Attachment 84462: fix style complaint
https://bugs.webkit.org/attachment.cgi?id=84462&action=review

------- Additional Comments from James Robinson <jamesr at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=84462&action=review

Why we would add another macro for this?  If this API only changes behavior
"significantly" (what does that mean?) in the SKIA_GPU path, why not tie this
behavior to the SKIA_GPU path directly and not have the extra macro? 
Maintaining a large set of combinations of macros is a code maintenance
headache.

Also, why is this change only for windows?  We use skia on linux as well.

> WebCore/ChangeLog:1
> +2011-03-02  Mike Reed  <reed at google.com>

This patch is rooted inside Source/ instead of the root of the repository. 
This still works, but it breaks several tools (for example expanding context in
the review tool).  Please generate patches from the root of the repository.

> WebCore/ChangeLog:10
> +	   No new tests. text output should be essitially identical. 

typo "essentially"

I'm curious what "essentially identical" means given that we do pixel-exact
testing.  If there are subtle differences, that means we'll have to maintain
another set of baselines (which is probably unavoidable for text in GPU
rendered cases, but worth keeping track of).

> WebCore/platform/graphics/skia/SkiaFontWin.cpp:47
> +extern SkTypeface* SkCreateTypefaceFromLOGFONT(const LOGFONT&);

this is odd - why don't you include the correct header for this function?

> WebCore/platform/graphics/skia/SkiaFontWin.cpp:50
> +#if ENABLE(SKIA_GPU)
> +    #define USE_SKIA_TEXT_API

Do we want to turn this on for all content whenever SKIA_GPU is set, or just
content rendered by a SKIA_GPU accelerated context?

> WebCore/platform/graphics/skia/SkiaFontWin.cpp:51
> +//  #define FORCE_SKIA_TEXT_FOR_ALL

why check in commented out code?

> WebCore/platform/graphics/skia/SkiaFontWin.cpp:296
> +    static const size_t kLocalGlyphMax = 64;
> +    SkAutoSTArray<kLocalGlyphMax, SkPoint> posStorage(numGlyphs);
> +    SkPoint* pos = posStorage.get();
> +    SkScalar x = point.fX;
> +    SkScalar y = point.fY;
> +    for (int i = 0; i < numGlyphs; i++) {

What happens when numGlyphs >= 64?

> WebCore/platform/graphics/skia/SkiaFontWin.cpp:333
> +    /*
> +	   Much of this logic could also happen in
> +	   FontCustomPlatformData::fontPlatformData and be cached,
> +	   allowing us to avoid talking to GDI at this point.
> +    */
> +    LOGFONT info;

In WebKit this sort of comment is normally a FIXME and written with // in front
of each line.

> WebCore/platform/graphics/skia/SkiaFontWin.cpp:337
> +    if (size < 0)
> +	   size = -size;

huh?  this deserves a comment at least