Bug 24947 – Special-case chromium drawing text-shadow when opaque color & no shadow blur

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[Rafael Weinstein]

This patch special-cases drawing of text-shadows on the chromium port to let GDI draw the text and text shadow, by passing Skia. This only occurs when both the text fill color and shadow color are opaque and the text shadow is 0 blur.

[Rafael Weinstein]

Created attachment 29088 [details]
Let GDI draw text-shadow on opaque colors and 0 blur

[Mark Rowe (bdash)]

Comment on attachment 29088 [details]
Let GDI draw text-shadow on opaque colors and 0 blur

Clearing review flag as it was set to + by a non-reviewer.

[Darin Fisher (:fishd, Google)]

Comment on attachment 29088 [details]
Let GDI draw text-shadow on opaque colors and 0 blur

>Index: ChangeLog
...
>+        Reviewed by NOBODY (OOPS!).
>+
>+	Special-case drawing text-shadow on win32, to let GDI draw in cases when both fill & 
>+	shadow color are opaque and the shadow has no blur.

replace tabs with spaces.


>Index: platform/graphics/chromium/FontChromiumWin.cpp

>+    IntSize shadow_size;
>+    int shadow_blur;
>+    Color shadow_color;

please use webkit style for naming variables:
http://webkit.org/coding/coding-style.html


>Index: platform/graphics/skia/SkiaFontWin.cpp

>+bool windowsCanHandleDrawTextShadow(WebCore::GraphicsContext *context) {
>+    IntSize shadow_size;
>+    int shadow_blur;
>+    Color shadow_color;

ditto

[Eric Seidel (no email)]

See:
http://webkit.org/coding/coding-style.html
for information on WebKit coding style.

[Rafael Weinstein]

Created attachment 29092 [details]
Spaces removed, variables now reflect webkit style guide

[Eric Seidel (no email)]

Comment on attachment 29092 [details]
Spaces removed, variables now reflect webkit style guide

The bug url should be in the ChangeLog.

Much stronger would be to ASSERT these conditions instead of just makign a comment:

+    // Note: If the fillColor or shadowColor are ever non-opaque, we should 
+    // never reach this code. SkiaFontWin::windowsCanHandleTextDrawing() will
+    // have set m_useGDI == false and the above check will have passed and 
+    // the this function returned.

tempColor isn't a very descriptiveName.

textColor would probably be better.

savedColor or savedTextColor would be better than "saveColor".

+bool windowsCanHandleDrawTextShadow(WebCore::GraphicsContext *context) {

{ should be on its own line.

"context" is not needed here (nor on the line after) as it provides no additional description to purpose of the argument (See style guide).

+bool windowsCanHandleDrawTextShadow(GraphicsContext *context);

[Eric Seidel (no email)]

Comment on attachment 29092 [details]
Spaces removed, variables now reflect webkit style guide

I can't really comment on if this is the right approach or not.  Maybe fishd or brettw can.

[Rafael Weinstein]

Created attachment 29101 [details]
Changes from Comment #6 (Eric Seidel)

-ASSERT()s used rather than second check of windowCanHandleDrawTextShadow()
-tempColor -> textColor
-saveColor -> savedTextColor
-formatting
-"context" removed from function declaration of both *canHandle*Text functions

[Rafael Weinstein]

I pre-reviewed this approach with brettw and made his corrections before submitting here.

(In reply to comment #7)
> (From update of attachment 29092 [details] [review])
> I can't really comment on if this is the right approach or not.  Maybe fishd or
> brettw can.
> 

[Eric Seidel (no email)]

Comment on attachment 29101 [details]
Changes from Comment #6 (Eric Seidel)

Looks fine.  Thanks!

[Darin Fisher (:fishd, Google)]

Landed as:  http://trac.webkit.org/changeset/42131