[Webkit-unassigned] [Bug 55609] option to use skia's font backend when drawing text on windows

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Thu Mar 3 12:43:32 PST 2011


James Robinson <jamesr at chromium.org> changed:

           What    |Removed                     |Added
  Attachment #84462|review?, commit-queue?      |review-
               Flag|                            |

--- Comment #4 from James Robinson <jamesr at chromium.org>  2011-03-03 12:43:32 PST ---
(From update of attachment 84462)
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
> +    #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

Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.

More information about the webkit-unassigned mailing list