Summary: | [chromium] Fix PlatformContextSkia::setDrawingToImageBuffer abuse | ||||||
---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Alok Priyadarshi <alokp> | ||||
Component: | Layout and Rendering | Assignee: | Alok Priyadarshi <alokp> | ||||
Status: | RESOLVED FIXED | ||||||
Severity: | Normal | CC: | bungeman, cc-bugs, enne, jamesr, junov, reed, senorblanco, webkit.review.bot | ||||
Priority: | P2 | ||||||
Version: | 528+ (Nightly build) | ||||||
Hardware: | Unspecified | ||||||
OS: | Unspecified | ||||||
Bug Depends on: | |||||||
Bug Blocks: | 105051 | ||||||
Attachments: |
|
Description
Alok Priyadarshi
2012-12-13 14:40:33 PST
Created attachment 179350 [details]
Patch
What exactly does "setShouldSmoothFonts" mean? Is it referring to subpixel AA, hinting, AA at all, something else? 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. (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. (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? 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. (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? (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 Comment on attachment 179350 [details]
Patch
OK, thanks for the background. R=me
Comment on attachment 179350 [details] Patch Clearing flags on attachment: 179350 Committed r138987: <http://trac.webkit.org/changeset/138987> All reviewed patches have been landed. Closing bug. |