Bug 23861 - Stroke font outlines on chromium linux
Summary: Stroke font outlines on chromium linux
Alias: None
Product: WebKit
Classification: Unclassified
Component: Text (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC Linux
: P2 Normal
Assignee: Evan Stade
Depends on:
Reported: 2009-02-09 18:08 PST by Evan Stade
Modified: 2009-02-18 14:07 PST (History)
0 users

See Also:

patch (2.38 KB, patch)
2009-02-09 18:10 PST, Evan Stade
eric: review-
Details | Formatted Diff | Diff
try2 (3.14 KB, patch)
2009-02-13 15:24 PST, Evan Stade
eric: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Evan Stade 2009-02-09 18:08:00 PST
See http://codereview.chromium.org/18176
Comment 1 Evan Stade 2009-02-09 18:10:32 PST
Created attachment 27512 [details]

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.

Comment 2 Eric Seidel (no email) 2009-02-13 14:29:30 PST
Comment on attachment 27512 [details]

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:

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.
Comment 3 Evan Stade 2009-02-13 14:44:53 PST
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.
Comment 4 Evan Stade 2009-02-13 15:24:45 PST
Created attachment 27670 [details]

updated comment, made |paint| local to if blocks, added changelog
Comment 5 Eric Seidel (no email) 2009-02-18 12:09:24 PST
Comment on attachment 27670 [details]

Comment 6 Darin Fisher (:fishd, Google) 2009-02-18 14:07:29 PST
Landed as http://trac.webkit.org/changeset/41065