Bug 69530 – re-add support for GDI text behind a compile flag

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Mike Reed]

resupport GDI text behind a compile-flag[D[D[D[D[D[D[D[D[D[D[D[D[D[D[D[D[D[D[D[D[D[D[D[D[D[D[D[D[D[D[D[D[D[D[D[D[D[D[D[C-add support for GDI text behind a compile flag

[Mike Reed]

Created attachment 109968 [details]
Patch

[Mike Reed]

Comment on attachment 109968 [details]
Patch

not ready for review yet

[Mike Reed]

Created attachment 110144 [details]
Patch

[Mike Reed]

Created attachment 110146 [details]
Patch

[WebKit Review Bot]

Comment on attachment 110146 [details]
Patch

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

[Mike Reed]

Created attachment 110191 [details]
Patch

[Mike Reed]

Created attachment 110192 [details]
Patch

[James Robinson]

Comment on attachment 110192 [details]
Patch

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

Would you mind sending this to the win-layout trybot and posting a link? I want to be extra sure that this won't need a bunch of rebaselines before landing on a Friday afternoon.

> LayoutTests/platform/chromium/test_expectations.txt:1373
> +// Disable these while we're back to using GDI for text drawing. These give slightly
> +// different results than with Skia. When we switch back to Skia, we can
> +// reenable these.
> +BUGCR99500 WIN : media/audio-repaint.html = IMAGE
> +BUGCR99500 WIN : svg/as-background-image/animated-svg-as-background.html = IMAGE
> +BUGCR99500 WIN : svg/batik/text/textStyles.svg = IMAGE
> +BUGCR99500 WIN : svg/custom/simple-text-double-shadow.svg = IMAGE

these are the only layout tests that render differently on windows with this change? wow!

[Mike Reed]

http://build.chromium.org/p/tryserver.chromium/builders/win_layout_rel/builds/606

[James Robinson]

Comment on attachment 110192 [details]
Patch

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

Awesome! Thanks so much.

r=me with a few nits to address before landing

> Source/WebCore/ChangeLog:11
> +        Reverts back to using GDI for text (when possible)
> +        but keeps skia-text version behind a compile-flag. If/when we can
> +        resolve the outstanding soft-clip and intl-performance bugs with the
> +        skia version, we may change the compile-flag to reenable skia.

Please add a link to the commit where this code was deleted so people can look at the original history if they need to (you can link to revisions in WebKit with URLs of the form http://trac.webkit.org/changeset/12345).

> Source/WebCore/platform/graphics/chromium/FontChromiumWin.cpp:449
> +//    graphicsContext->platformContext()->prepareForSoftwareDraw();
> +

don't check this in commented out - I don't think you need it, so it can just be deleted.

> Source/WebCore/platform/graphics/chromium/FontChromiumWin.cpp:469
> +        int curLen = std::min(kMaxBufferLength, numGlyphs - glyphIndex);

nit: webkit style is to put a 'using namespace std' declaration at the top of the file and have this call be just min()

> Source/WebCore/platform/graphics/skia/SkiaFontWin.cpp:47
> +bool windowsCanHandleDrawTextShadow(GraphicsContext *context)

nit: WebKit style is the * goes with the type name, so GraphicsContext* context

[Mike Reed]

https://bugs.webkit.org/show_bug.cgi?id=65203

This was the change when GDI was initially removed.

[James Robinson]

Right, please add a link to that in the ChangeLog.

[Mike Reed]

Created attachment 110388 [details]
Patch

[James Robinson]

Comment on attachment 110388 [details]
Patch

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

> Source/WebCore/ChangeLog:7
> +        re-add support for GDI text behind a compile flag
> +        https://bugs.webkit.org/show_bug.cgi?id=69530
> +
> +        Reviewed by NOBODY (OOPS!).
> +

I was hoping for a link somewhere in here

[Mike Reed]

Created attachment 110390 [details]
Patch

[Mike Reed]

previous patch lost all ChangeLog comments (I do love cygwin so)

[James Robinson]

Comment on attachment 110390 [details]
Patch

R=me

[WebKit Review Bot]

Comment on attachment 110390 [details]
Patch

Clearing flags on attachment: 110390

Committed r97149: <http://trac.webkit.org/changeset/97149>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Adam Barth]

Comment on attachment 110390 [details]
Patch

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

> Source/WebCore/platform/graphics/skia/SkiaFontWin.cpp:46
> +#if !ENABLE(SKIA_TEXT)

For future reference, this should have been USE(SKIA_TEXT).  ENABLE is for features visible in the platform.  USE is for which libraries (and which parts of those libraries) we use.