Bug 23861 – Stroke font outlines on chromium linux

• Status: RESOLVED
• Resolution: FIXED
• Component: Text

[Evan Stade]

See http://codereview.chromium.org/18176

[Evan Stade]

Created attachment 27512 [details]
patch

I'll make an entry on the ChangeLog later. The text of that entry will be as follows:

Stroke font outlines on chromium in Linux.

TEST=LayoutTests/svg/custom/pointer-events-text.svg

[Eric Seidel (no email)]

Comment on attachment 27512 [details]
patch

Since you can't yet land these yourself, it makes sense to include the ChangeLog in your posted patch.

It would help if this could have a bit more context:
98         if (textMode & cTextFill) {
 99             // See comment in FontChromiumWin.cpp::paintSkiaText()
 100             paint.setLooper(0)->safeUnref();

Some idea as to why we unref there.  For more context sure, point them to the original comment, but a small local explanation I think would help.

.rbg isn't needed here:
paint.setColor(gc->strokeColor().rgb());

It seems the shared code from the fill/stroke path could be abstracted into a small static function, where you pass a color.  Something like:

setupContentForTextPainting(gc, font, paint, color);

Maybe it's not worth it, but I think it would be nice to save the 6 lines from the file.

Is Paint creation expensive?  Seems we might just want to have a local SkPaint per block instead of using .reset()?

r- mostly for the lack of changelog.

[Evan Stade]

I actually only count 2 lines from each code path that could go into your proposed helper function. So probably not worth it :(

The reuse of |paint| doesn't save any computation. I will change that. Also I will add more commentary, and post the changelog.

[Evan Stade]

Created attachment 27670 [details]
try2

updated comment, made |paint| local to if blocks, added changelog

[Eric Seidel (no email)]

Comment on attachment 27670 [details]
try2

LGTM.

[Darin Fisher (:fishd, Google)]

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