Bug 104956 – [chromium] Fix PlatformContextSkia::setDrawingToImageBuffer abuse

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[Alok Priyadarshi]

We have been using PlatformContextSkia::setDrawingToImageBuffer to indirectly enable/disable LCD text. It was OK until only root layer had LCD text. But now to enable LCD text on composited layer, you have to call PlatformContextSkia::setDrawingToImageBuffer(false), which is wrong.

It turns out that we can use GraphicsContext::setShouldSmoothFonts() to do the same. We may also be able to deprecate PlatformContextSkia::setDrawingToImageBuffer().

[Alok Priyadarshi]

Created attachment 179350 [details]
Patch

[James Robinson]

What exactly does "setShouldSmoothFonts" mean?  Is it referring to subpixel AA, hinting, AA at all, something else?

[Alok Priyadarshi]

I was able to get rid of all setDrawingToImageBuffer calls except the ones in WebScrollbarThemePainter. It looks like it is using it to indirectly set a TransparencyWin::LayerMode in RenderThemeChromiumWin.cpp.

[Alok Priyadarshi]

(In reply to comment #2)
> What exactly does "setShouldSmoothFonts" mean?  Is it referring to subpixel AA, hinting, AA at all, something else?

From the current usage on other ports, I think it means subpixel AA.

[James Robinson]

(In reply to comment #4)
> (In reply to comment #2)
> > What exactly does "setShouldSmoothFonts" mean?  Is it referring to subpixel AA, hinting, AA at all, something else?
> 
> From the current usage on other ports, I think it means subpixel AA.

I think we want more than "I think it means".  In the past, we've definitely been burned by conflating multiple needs under a single flag.  Why not have a dedicated flag that means exactly what we want it to mean?

[Alok Priyadarshi]

The variable name is not explicit, but looking at Font::drawGlyphs in the mac port, it is pretty clear what it is being used for:

shouldAntiAlias: AA
shouldSmoothFonts: subpixel AA
shouldSubpixelQuantizeFonts: subpixel positioning

http://trac.webkit.org/browser/trunk/Source/WebCore/platform/graphics/mac/FontMac.mm#L144

I did not see those variables used in the chromium port.

[Justin Novosad]

(In reply to comment #6)
> The variable name is not explicit, but looking at Font::drawGlyphs in the mac port, it is pretty clear what it is being used for:
> 
> shouldAntiAlias: AA
> shouldSmoothFonts: subpixel AA
> shouldSubpixelQuantizeFonts: subpixel positioning
> 

If shouldSmoothFonts is in any way related to the webkit-font-smoothing CSS property, we probably want to leave it alone, since that property is now mostly used to control the boldness side-effect of subpixel AA on mac.

@bungeman?

[Alok Priyadarshi]

(In reply to comment #7)
> (In reply to comment #6)
> > The variable name is not explicit, but looking at Font::drawGlyphs in the mac port, it is pretty clear what it is being used for:
> > 
> > shouldAntiAlias: AA
> > shouldSmoothFonts: subpixel AA
> > shouldSubpixelQuantizeFonts: subpixel positioning
> > 
> 
> If shouldSmoothFonts is in any way related to the webkit-font-smoothing CSS property, we probably want to leave it alone, since that property is now mostly used to control the boldness side-effect of subpixel AA on mac.
> 
> @bungeman?

AFAICT they are not related. -webkit-font-smoothing is captured in Font::fontDescription().fontSmoothing()

See various implementations of Font::drawGlyphs().

(In reply to comment #8)
> (In reply to comment #7)
> > (In reply to comment #6)
> > > The variable name is not explicit, but looking at Font::drawGlyphs in the mac port, it is pretty clear what it is being used for:
> > > 
> > > shouldAntiAlias: AA
> > > shouldSmoothFonts: subpixel AA
> > > shouldSubpixelQuantizeFonts: subpixel positioning
> > > 
> > 
> > If shouldSmoothFonts is in any way related to the webkit-font-smoothing CSS property, we probably want to leave it alone, since that property is now mostly used to control the boldness side-effect of subpixel AA on mac.
> > 
> > @bungeman?
> 
> AFAICT they are not related. -webkit-font-smoothing is captured in Font::fontDescription().fontSmoothing()
> 
> See various implementations of Font::drawGlyphs().

Summary: Use shouldAntiAlias, shouldSmoothFonts, and shouldSubpixelQuantizeFonts. While I don't know all of the details, this patch looks correct to me.

The names AntiAlias, SmoothFonts, and SubpixelQuantizeFonts are all CoreGraphics-isms. In the world of CoreGraphics they mean what Alok says they mean in comment #6. That said, this used to directly interact with -webkit-font-smoothing, as the value of -webkit-font-smoothing basically translated into one of the above on Mac. The problem was that on Mac the weight of the text changes between lcd smoothed and regular anti-aliased. What we do now is use AntiAlias and SmoothFonts to choose the rendering method, and if -webkit-font-smoothing:antialised is used set the Skia paint's hinting to 'none' (instead of 'normal'). With AntiAlias and 'normal' hinting the regular anti-alias is back-formed from the lcd so that the weight doesn't change in transitions to and from lcd smoothed.

See http://trac.webkit.org/browser/trunk/Source/WebCore/platform/graphics/skia/FontSkia.cpp#L76

[James Robinson]

Comment on attachment 179350 [details]
Patch

OK, thanks for the background.  R=me

[WebKit Review Bot]

Comment on attachment 179350 [details]
Patch

Clearing flags on attachment: 179350

Committed r138987: <http://trac.webkit.org/changeset/138987>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.