[webkit-reviews] review denied: [Bug 24101] Chromium Windows port should support semitransparent ClearType fonts : [Attachment 27891] Patch

[bugzilla-daemon at webkit.org]

Eric Seidel <eric at webkit.org> has denied Brett Wilson (Google)
<brettw at chromium.org>'s request for review:
Bug 24101: Chromium Windows port should support semitransparent ClearType fonts
https://bugs.webkit.org/show_bug.cgi?id=24101

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

------- Additional Comments from Eric Seidel <eric at webkit.org>
It looks like TransparencyAwareFontPainter should be NonCopyable<T>

"Iterator is so hard to type" ;) SkCanvas::LayerIter  (yes, I know that's not
your class name).

estimateTextBounds?  Is this a high-estimate? low estimate?  What cases is it
safe to use this function?  For example, it doesn't look like it's safe to use
for repainting... since it averages the ascent/descent, which would be too
small in some cases, no?

It seems things like these:
 139	     m_graphicsContext->beginTransparencyLayer(SkColorGetA(color) /
255.0f);

Should be abstracted into functions so that people can't get them wrong. 
Converting from int to float is easy, but float to int conversions always go
wrong. ;)  Again, not really a comment on your change...

It seems the whole initialization block should be broken out into a separate
function outside the constructor.  That way if we later want to add stuff to
the constructor it's less confusing where "normal" initialziation ends.

Also determineLayerModeAndLayerRect(m_platformContext, mode, rect) would be a
static inline if I were writing this.

It seems maybe m_createdLayer should be m_createdTransparencyLayer, since then
it's not confused with any other type of layer in WebKit.

I would have written this:
194	    SkPoint origin = { SkFloatToScalar(m_point.x() + startAdvance),
 195				SkFloatToScalar(m_point.y()) };
as:
SkPoint origin = m_point;
origin.x += startAdvance;


I'm surprised you called the variable "helper" when the class is a "painter".
;)
 222	 TransparencyAwareFontPainter helper(graphicsContext, font,
glyphBuffer, from, numGlyphs, point);

{ goes on the same line as struct:
struct StaticBuffer
 51 {

Don't you need "static" before the struct to prevent staticBuffer from being
exported from the .o?  I think so.

C++ code in WK uses 0 instead of NULL.

I generally us "static" instead of inline for small functions and let the
compiler decide.
 66 inline skia::PlatformCanvas* canvasForContext(GraphicsContext* context)
The compiler of course doesn't always respect your "inline" hint either, so
they're probably equivalent.

Why not use const GraphicsContext& instead of GraphicsContext* to make clear
that you don't need to worry about NULL in all these functions? (yes, I know
&'s can still be NULL, but not intentionally).

WK generally uses one variable-per-line in declarations.

DeviceInfo

What are the x, y for?	I assume those coords are relative to the screen or
something?

No need to wrap to 80col here:
     return canvasForContext(context)->
 74	    getTopPlatformDevice().accessBitmap(false);

It seems TransparencyWin should also be NonCopyable<T>

I would break the:
// Compute the size of the layer we're making.
block out into its own static inline, making setupLayer shorter and more
readable, IMPO.

Seems this should just be a real class:
306	// Create a new cached buffer.
 307	 delete staticBuffer.destBitmap;
 308	 delete staticBuffer.referenceBitmap;

with destructor, instead of a struct with this ad-hoc memory management burried
in down in this "initialize new context" function.  Also, shouldn't this be a
class-static variable, instead of a file static?

No need to wrap:
 325	 SkBitmap* bitmap = const_cast<SkBitmap*>(
 326	     &bitmapForContext(m_layerBuffer->context()));

No need for {}
1     } else {
 362	     destRect.set(m_sourceRect.x(), m_sourceRect.y(),
m_sourceRect.right(), m_sourceRect.bottom());
 363	 }

I think this code could all be made more readable by abstracting more into
small static functions.  functions which individually have to handle these
if/switch statements for the 3 or 4 different cases which you're trying to
handle here.

Another great example of this:
68	   // Compute the transform mode.
 69	    TransformMode transformMode;
 70	    const TransformationMatrix& matrix = context->getCTM();
 71	    if (matrix.b() != 0 || matrix.c() != 0)  // Skew.
 72		transformMode = Untransform;
 73	    else if (matrix.a() != 1.0 || matrix.d() != 1.0)  // Scale.
 74		transformMode = ScaleTransform;
 75	    else  // Nothing interesting.
 76		transformMode = KeepTransform;

I would have made a transformModeForMatrix() static, same for layerMode.  Then
I, as a reviewer can more easily prove to myself that the individual
"transformModeForMatrix()" function makes sense w/o having to keep the question
of "does this constructor do what I expect it to" in my head. ;)

I assume this is covered by existing tests?

I think the patch in the end is probably fine.	But I think it can be worked
over more to be more readable.