Bug 129957

Summary: fast/css3-text/css3-text-decoration/text-decoration-thickness.html fails on GTK
Product: WebKit Reporter: Martin Robinson <mrobinson>
Component: Tools / TestsAssignee: Martin Robinson <mrobinson>
Status: RESOLVED FIXED    
Severity: Normal CC: commit-queue, dino, d-r, k.wolanski, mmaxfield, mpakulavelrutka, simon.fraser, zan
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch none

Description Martin Robinson 2014-03-07 20:42:31 PST
It looks like the font size is insufficient to make the test pass on GTK+. We can fix it by adding a few more orders of magnitude to the font size.
Comment 1 Martin Robinson 2014-03-08 10:14:56 PST
Looks like this might actually be a bug in the Cairo GraphicsContext. Investigating now.
Comment 2 Martin Robinson 2014-03-08 10:44:51 PST
fast/css3-text/css3-text-decoration/text-decoration-thickness.html and fast/css3-text/css3-text-decoration/text-decoration-style-double-space-scales.html are also failing because of this issue.
Comment 3 Martin Robinson 2014-03-08 10:47:18 PST
*** Bug 126879 has been marked as a duplicate of this bug. ***
Comment 4 Martin Robinson 2014-03-08 11:06:17 PST
Created attachment 226217 [details]
Patch
Comment 5 Martin Robinson 2014-03-08 11:25:46 PST
I have ported mmaxfield's original CG patch to the Cairo GraphicsContext, which seems to fix the tests.
Comment 6 Myles C. Maxfield 2014-03-10 09:24:53 PDT
Comment on attachment 226217 [details]
Patch

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

> Source/WebCore/platform/graphics/cairo/GraphicsContextCairo.cpp:617
> +static FloatRect computeLineBoundsAndAntialiasingModeForText(GraphicsContext& initialContext, const FloatPoint& point, float width, bool printing, bool& shouldAntialias, Color& color)

Perhaps there is some cause for duplicate code reuse. Is there enough commonality here that the implementations could be unified?

> LayoutTests/fast/css3-text/css3-text-decoration/text-decoration-skip/text-decoration-skip-tall-underlines.html:-10
> -<div style="left: -160px; top: -320px; position: absolute; -webkit-transform: scale(20); font-family: helvetica; -webkit-transform-origin: left top; display: inline-block; text-decoration: underline; -webkit-text-decoration-skip: ink;">gy</div>

Glad you caught this. This is good.
Comment 7 Martin Robinson 2014-03-10 09:33:54 PDT
(In reply to comment #6)
> (From update of attachment 226217 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=226217&action=review
> 
> > Source/WebCore/platform/graphics/cairo/GraphicsContextCairo.cpp:617
> > +static FloatRect computeLineBoundsAndAntialiasingModeForText(GraphicsContext& initialContext, const FloatPoint& point, float width, bool printing, bool& shouldAntialias, Color& color)
> 
> Perhaps there is some cause for duplicate code reuse. Is there enough commonality here that the implementations could be unified?

I'd be happy to move the code to a shared GraphicsContext method if you think it makes sense. I've tried to make most of the code platform-independent. Would getting the transformation matrix via GraphicsContext::getCTM be sufficient for iOS and Mac?
Comment 8 Myles C. Maxfield 2014-03-10 10:06:08 PDT
GraphicsContext::getCTM(DefinitelyIncludeDeviceScale) should do the trick.
Comment 9 Martin Robinson 2014-03-10 10:07:30 PDT
(In reply to comment #8)
> GraphicsContext::getCTM(DefinitelyIncludeDeviceScale) should do the trick.

Thanks for the information. I'll try to have a unified patch tomorrow.
Comment 10 Martin Robinson 2014-03-11 18:24:35 PDT
Created attachment 226460 [details]
Patch
Comment 11 Myles C. Maxfield 2014-03-11 20:34:52 PDT
r=me but i'm not a reviewer
Comment 12 Tim Horton 2014-04-07 12:42:49 PDT
Comment on attachment 226460 [details]
Patch

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

> Source/WebCore/platform/graphics/GraphicsContext.cpp:1012
> +    return FloatRect(FloatPoint(origin.x(), origin.y()), FloatSize(width, thickness));

There is a FloatRect constructor that takes all four.
Comment 13 Martin Robinson 2014-04-07 18:35:45 PDT
Committed r166902: <http://trac.webkit.org/changeset/166902>
Comment 14 Michal Pakula vel Rutka 2014-05-26 08:09:36 PDT
*** Bug 129734 has been marked as a duplicate of this bug. ***
Comment 15 Michal Pakula vel Rutka 2014-05-26 08:09:52 PDT
*** Bug 127282 has been marked as a duplicate of this bug. ***
Comment 16 Michal Pakula vel Rutka 2014-05-26 08:10:10 PDT
*** Bug 128664 has been marked as a duplicate of this bug. ***