Bug 69530 - re-add support for GDI text behind a compile flag
Summary: re-add support for GDI text behind a compile flag
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Mike Reed
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-10-06 09:04 PDT by Mike Reed
Modified: 2011-10-13 15:07 PDT (History)
3 users (show)

See Also:


Attachments
Patch (32.57 KB, patch)
2011-10-06 09:38 PDT, Mike Reed
no flags Details | Formatted Diff | Diff
Patch (11.92 KB, patch)
2011-10-07 07:50 PDT, Mike Reed
no flags Details | Formatted Diff | Diff
Patch (32.49 KB, patch)
2011-10-07 08:00 PDT, Mike Reed
no flags Details | Formatted Diff | Diff
Patch (35.52 KB, patch)
2011-10-07 12:14 PDT, Mike Reed
no flags Details | Formatted Diff | Diff
Patch (35.80 KB, patch)
2011-10-07 12:17 PDT, Mike Reed
no flags Details | Formatted Diff | Diff
Patch (35.44 KB, patch)
2011-10-10 13:21 PDT, Mike Reed
no flags Details | Formatted Diff | Diff
Patch (35.82 KB, patch)
2011-10-10 13:30 PDT, Mike Reed
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Mike Reed 2011-10-06 09:04:33 PDT
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
Comment 1 Mike Reed 2011-10-06 09:38:52 PDT
Created attachment 109968 [details]
Patch
Comment 2 Mike Reed 2011-10-06 09:40:04 PDT
Comment on attachment 109968 [details]
Patch

not ready for review yet
Comment 3 Mike Reed 2011-10-07 07:50:36 PDT
Created attachment 110144 [details]
Patch
Comment 4 Mike Reed 2011-10-07 08:00:47 PDT
Created attachment 110146 [details]
Patch
Comment 5 WebKit Review Bot 2011-10-07 08:19:03 PDT
Comment on attachment 110146 [details]
Patch

Attachment 110146 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/10006221
Comment 6 Mike Reed 2011-10-07 12:14:09 PDT
Created attachment 110191 [details]
Patch
Comment 7 Mike Reed 2011-10-07 12:17:45 PDT
Created attachment 110192 [details]
Patch
Comment 8 James Robinson 2011-10-07 15:25:50 PDT
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!
Comment 10 James Robinson 2011-10-10 12:57:32 PDT
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
Comment 11 Mike Reed 2011-10-10 13:10:13 PDT
https://bugs.webkit.org/show_bug.cgi?id=65203

This was the change when GDI was initially removed.
Comment 12 James Robinson 2011-10-10 13:13:19 PDT
Right, please add a link to that in the ChangeLog.
Comment 13 Mike Reed 2011-10-10 13:21:47 PDT
Created attachment 110388 [details]
Patch
Comment 14 James Robinson 2011-10-10 13:24:20 PDT
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
Comment 15 Mike Reed 2011-10-10 13:30:09 PDT
Created attachment 110390 [details]
Patch
Comment 16 Mike Reed 2011-10-10 13:31:48 PDT
previous patch lost all ChangeLog comments (I do love cygwin so)
Comment 17 James Robinson 2011-10-10 13:55:53 PDT
Comment on attachment 110390 [details]
Patch

R=me
Comment 18 WebKit Review Bot 2011-10-11 06:10:42 PDT
Comment on attachment 110390 [details]
Patch

Clearing flags on attachment: 110390

Committed r97149: <http://trac.webkit.org/changeset/97149>
Comment 19 WebKit Review Bot 2011-10-11 06:10:48 PDT
All reviewed patches have been landed.  Closing bug.
Comment 20 Adam Barth 2011-10-13 15:07:57 PDT
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.