Bug 24101 – Chromium Windows port should support semitransparent ClearType fonts

• Component: Platform
• Status: RESOLVED FIXED

[Brett Wilson (Google)]

Our incorrect handling of transparency on Windows causes a number of bugs, most notably incorrect semitransparet text handling.:
 http://code.google.com/p/chromium/issues/detail?id=559
 http://code.google.com/p/chromium/issues/detail?id=2791
 http://code.google.com/p/chromium/issues/detail?id=3229

Patch forthcoming.

[Brett Wilson (Google)]

Created attachment 27891 [details]
Patch

Fix Windows transparency for the Chromium port. Implement a helper class for handling transparency on Windows. It allows semitransparent ClearType and semitransparent form controls by making new layers in the background. 

It also replaces the "ThemeHelper" which allows better scaling and transforms on Windows form controls. In addition to the functionality that the ThemeHelper did, but additionally handles the antialiasing properly so that the form controls aren't composited on a white square.

This is covered by layout tests that previously had wrong results (requires re-baselining 71 Chromium-specific results, note that the Chromium layout test results are currently only in the Chromium tree). However, the majority of the layout test changes are a result of me now enabling filtering for more types of bitmap drawing that previously looked bad (this is what this patch uses to draw form controls transformed).

There is also a Google-style unit test for some of the basic functionality. This is a lower-level test than the layout tests that I used when developing this patch. You can see it here: http://codereview.chromium.org/21201/diff/5121/5140

[Eric Seidel (no email)]

Comment on attachment 27891 [details]
Patch

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.

[Brett Wilson (Google)]

(In reply to comment #2)
> 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 should be a pretty safe estimate. The ascent and decent don't change, so I don't see why this would affect partial painting. This is copied from http://trac.webkit.org/browser/trunk/WebCore/platform/graphics/win/FontCGWin.cpp#L168


> 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?
>
> 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.
>
> 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. ;)

Good ideas, I did all 3 suggestions and it improves things.


> I assume this is covered by existing tests?

Yes, see comment 1.

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

Ha! You should have seen it two cleanups ago.

I did all your other comments. New snap coming.

[Brett Wilson (Google)]

Created attachment 27982 [details]
Patch v2

[Eric Seidel (no email)]

Comment on attachment 27982 [details]
Patch v2

Unless I'm reading this wrong, there is no savings in the m_cachedBuffers logic.  I would suggest just removing it.  It seems you blast away the m_cachedBuffers every time, and never re-use them.  So might as well just always use m_ownedBuffers.

[Brett Wilson (Google)]

This line and its block
  if (m_ownedBuffers && m_ownedBuffers->canHandleSize(m_layerSize)) {
should have been m_cachedBuffers instead of m_ownedBuffers.

Do you have any other comments for the next round

[Eric Seidel (no email)]

Comment on attachment 27982 [details]
Patch v2

You also duplicated the ChangeLog.  You don't actually need to run prepare-ChagneLog more than once, it just for adding the initial boiler-plate.  You can run it again if you change a bunch more files or functions, but generally I only ever run it once per patch.

I would have broken the layerMode, layerRect detection off into its own static inline, but it's not a huge deal either way.

I'm surprised no one has written a SkiaBackToFrontLayerIterator and that you have to write your own reversal.

In general this patch looks fine.

[Brett Wilson (Google)]

(In reply to comment #7)
> (From update of attachment 27982 [details] [review])
> You also duplicated the ChangeLog.  You don't actually need to run
> prepare-ChagneLog more than once, it just for adding the initial boiler-plate. 
> You can run it again if you change a bunch more files or functions, but
> generally I only ever run it once per patch.

I added a whole bunch of functions that you asked for, so I wanted to get the updated function list.


> I would have broken the layerMode, layerRect detection off into its own static
> inline, but it's not a huge deal either way.

I assume you mean for the TransparencyAwareFontPainter? I did it this way because the layer mode and layer rect computation in this case go hand-in-hand, at least the way I think about it.


> I'm surprised no one has written a SkiaBackToFrontLayerIterator and that you
> have to write your own reversal.

Actually, the Skia LayerIterator was written specifically for this patch. It does front to back since that's the way Skia internally keeps the list, it's just a public wrapper around this API. If it comes up again (which I doubt), we should put it into Skia.

[Eric Seidel (no email)]

(In reply to comment #8)

> > I would have broken the layerMode, layerRect detection off into its own static
> > inline, but it's not a huge deal either way.
> 
> I assume you mean for the TransparencyAwareFontPainter? I did it this way
> because the layer mode and layer rect computation in this case go hand-in-hand,
> at least the way I think about it.

Yes, sorry I wasn't more clear.  That's what I meant.  That a single function would handle both computations, just that the computation as a whole woudl be broken out into its own separate function instead of part of initializeForGDI.  Again, not mission critical, but I personally am a huge fan of tiny, well-named functions. :)

[Brett Wilson (Google)]

Created attachment 28038 [details]
Patch v3

[Eric Seidel (no email)]

Comment on attachment 28038 [details]
Patch v3

So it looks like you removed the whole OwnedBuffers thing.  I skimmed it again, looks fine.  Your ChangeLog is now out of date, but you can fix that when landing.