Bug 115406 – [GTK] Use drawErrorUnderline() instead of Pango API for highlighting misspelled words

• Component: Layout and Rendering
• Status: RESOLVED FIXED

[Eduardo Lima Mitev]

GraphicsContextCairo currently uses Pango only to draw the underline error hint on misspelled words when platform is GTK. However, the drawErrorUnderline() method that is used by the Win platform, uses Cairo directly and works well for GTK.

This bug is about using drawErrorUnderline() instead of pango's API and completely remove Pango from GraphicsContextCairo.

[Eduardo Lima Mitev]

Created attachment 200091 [details]
patch

[Eduardo Lima Mitev]

Adding mrobinson to CC

[Martin Robinson]

Does this change any test output?

[Eduardo Lima Mitev]

All spelling related tests render similar outputs with and without this patch:

Tools/Scripts/run-webkit-tests --gtk LayoutTests/editing/spelling/*

[Eduardo Lima Mitev]

I meant identical, not similar :)

[Martin Robinson]

(In reply to comment #5)
> I meant identical, not similar :)

You'll need to run with -p.

[Eduardo Lima Mitev]

Created attachment 203567 [details]
updated patch including new baseline PNGs

This patch adds 3 new baseline PNG images for spell checking tests that were not passing pixel hash comparison.

It also wraps drawErrorUnderline() logic with new filled cairo path.

[Martin Robinson]

(In reply to comment #7)

> It also wraps drawErrorUnderline() logic with new filled cairo path.

Was there a test failing because a path was remaining on the context?

[Eduardo Lima Mitev]

(In reply to comment #8)
> 
> Was there a test failing because a path was remaining on the context?

Without the calls to new_path() and fill(), the drawings are never committed so the underline is never visible. Implementation of drawErrorUnderline() expected the path to be opened and closed outside, but I don't think the Win port was doing that, as far as I could research.

pango_cairo_show_error_underline() calls new_path() and fill() at the beginning and ending (respectively) of the draw_error_underline() method <https://git.gnome.org/browse/pango/tree/pango/pangocairo-render.c#n614>, conditionally. After the replacement by the custom drawErrorUnderline(), I had to make these calls explicit. First I though adding them in GraphicsContext::drawLineForDocumentMarker(), but I then decided to put them in drawErrorUnderline() itself for better encapsulation, and also because I think the Win port needs that as well.

[Martin Robinson]

If it's required, it seems that some other code (whatever used GraphicsContext before this call) is leaving a stale path on the context. It might be better to ensure that the other code doesn't leave the path hanging around, rather than having to manually clear the path in every method called afterward. I'm worried about this bug causing us to add cairo_new_path to a lot of places over time when the real fix is to prevent the situation.

[Eduardo Lima Mitev]

Created attachment 203603 [details]
last patch removing the cairo_new_path(), which was not required

After discussing a bit with mrobinson, I learned that it should not be required to cairo_new_path() at the beginning of drawErrorUnderline(). These routines should assume and respect a clean path.

This new patch just removes the call to cairo_new_path() in drawErrorUnderline(), and just add the cairo_fill() at the end.

Tests are passing and all looks ok to me.

[Eduardo Lima Mitev]

Created attachment 203606 [details]
last patch with changelog corrected

Last patch had an error in a ChangeLog, please ignore.

Sorry for the noise.

[Martin Robinson]

Comment on attachment 203606 [details]
last patch with changelog corrected

Great!

[WebKit Commit Bot]

Comment on attachment 203606 [details]
last patch with changelog corrected

Clearing flags on attachment: 203606

Committed r151125: <http://trac.webkit.org/changeset/151125>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.