[webkit-reviews] review denied: [Bug 23861] Stroke font outlines on chromium linux : [Attachment 27512] patch

[bugzilla-daemon at webkit.org]

Eric Seidel <eric at webkit.org> has denied Evan Stade <estade at chromium.org>'s
request for review:
Bug 23861: Stroke font outlines on chromium linux
https://bugs.webkit.org/show_bug.cgi?id=23861

Attachment 27512: patch
https://bugs.webkit.org/attachment.cgi?id=27512&action=review

------- Additional Comments from Eric Seidel <eric at webkit.org>
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.