Bug 45597 – [GTK] Complex text does not have full CSS text-shadow support

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKitGTK

[Martin Robinson]

The complex text path only support solid CSS shadows currently.

[Martin Robinson]

Created attachment 71211 [details]
Use ContextShadow in FontGtk

[Alejandro G. Castro]

Comment on attachment 71211 [details]
Use ContextShadow in FontGtk

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

> WebCore/platform/graphics/gtk/FontGtk.cpp:66
> +typedef GdkRegion* cairo_region_t*;

I think this definition is not correct, it should be:

typedef cairo_region_t* PangoRegionType;

> WebCore/platform/graphics/gtk/FontGtk.cpp:224
> +    FloatPoint totalOffset(point + shadow->m_offset);

Doesn't point include the shadow offset already?

> WebCore/platform/graphics/gtk/FontGtk.cpp:236
> +        cairo_translate(context, -totalOffset.x(), -totalOffset.y());

Is this required after doing the cairo_restore in the next line?

> WebCore/platform/graphics/gtk/FontGtk.cpp:-213
> -    if (to - from != run.length()) {
> -        // Clip the region of the run to be rendered

Before we were doing the clip just in case run does cover the whole line, is this intended?

[Martin Robinson]

(In reply to comment #2)
> (From update of attachment 71211 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=71211&action=review
> 
> > WebCore/platform/graphics/gtk/FontGtk.cpp:66
> > +typedef GdkRegion* cairo_region_t*;
> I think this definition is not correct, it should be:

Nice catch. I'll fix that.

> > WebCore/platform/graphics/gtk/FontGtk.cpp:224
> > +    FloatPoint totalOffset(point + shadow->m_offset);
> Doesn't point include the shadow offset already?

It doesn't. The point passed in is the origin of the baseline of the first solid character.

> > WebCore/platform/graphics/gtk/FontGtk.cpp:236
> > +        cairo_translate(context, -totalOffset.x(), -totalOffset.y());
> Is this required after doing the cairo_restore in the next line?

It isn't! I'll fix that.

> > WebCore/platform/graphics/gtk/FontGtk.cpp:-213
> > -    if (to - from != run.length()) {
> > -        // Clip the region of the run to be rendered
> Before we were doing the clip just in case run does cover the whole line, is this intended?

Doing the clipping unconditionally shouldn't hurt anything, but if performance is a concern I can re-add the condition.

[Martin Robinson]

Created attachment 71643 [details]
Patch incorporating Alex's suggestions

[Martin Robinson]

Comment on attachment 71643 [details]
Patch incorporating Alex's suggestions

Clearing flags on attachment: 71643

Committed r71750: <http://trac.webkit.org/changeset/71750>

[Martin Robinson]

All reviewed patches have been landed.  Closing bug.

[WebKit Review Bot]

http://trac.webkit.org/changeset/71750 might have broken GTK Linux 64-bit Debug