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
Created attachment 109968 [details] Patch
Comment on attachment 109968 [details] Patch not ready for review yet
Created attachment 110144 [details] Patch
Created attachment 110146 [details] Patch
Comment on attachment 110146 [details] Patch Attachment 110146 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/10006221
Created attachment 110191 [details] Patch
Created attachment 110192 [details] Patch
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!
http://build.chromium.org/p/tryserver.chromium/builders/win_layout_rel/builds/606
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
https://bugs.webkit.org/show_bug.cgi?id=65203 This was the change when GDI was initially removed.
Right, please add a link to that in the ChangeLog.
Created attachment 110388 [details] Patch
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
Created attachment 110390 [details] Patch
previous patch lost all ChangeLog comments (I do love cygwin so)
Comment on attachment 110390 [details] Patch R=me
Comment on attachment 110390 [details] Patch Clearing flags on attachment: 110390 Committed r97149: <http://trac.webkit.org/changeset/97149>
All reviewed patches have been landed. Closing bug.
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.