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.
Created attachment 200091 [details] patch
Adding mrobinson to CC
Does this change any test output?
All spelling related tests render similar outputs with and without this patch: Tools/Scripts/run-webkit-tests --gtk LayoutTests/editing/spelling/*
I meant identical, not similar :)
(In reply to comment #5) > I meant identical, not similar :) You'll need to run with -p.
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.
(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?
(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.
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.
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.
Created attachment 203606 [details] last patch with changelog corrected Last patch had an error in a ChangeLog, please ignore. Sorry for the noise.
Comment on attachment 203606 [details] last patch with changelog corrected Great!
Comment on attachment 203606 [details] last patch with changelog corrected Clearing flags on attachment: 203606 Committed r151125: <http://trac.webkit.org/changeset/151125>
All reviewed patches have been landed. Closing bug.