Clean up device scale factor with Skia GraphicsContext
Created attachment 183580 [details] Patch
Created attachment 183581 [details] Patch
Attachment 183581 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/edit..." exit_code: 1 LayoutTests/platform/chromium-mac-snowleopard/editing/spelling/grammar-markers-hidpi-composited-expected.png:0: Have to enable auto props in the subversion config file (/home/alancutter/.subversion/config "enable-auto-props = yes"). Have to set the svn:mime-type in the subversion config file (/home/alancutter/.subversion/config "*.png = svn:mime-type=image/png"). [image/png] [5] Total errors found: 1 in 11 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 183581 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=183581&action=review > Source/WebCore/platform/graphics/skia/GraphicsContextSkia.cpp:615 > + int rasterScaleFactor = static_cast<int>(lroundf(effectiveScaleComponent(platformContext()->getTotalMatrix()))); This isn't able to differentiate between page zoom (user hits cmd-+ a few times) and device scale, is it? Like, if I zoom in often enough on a lowdpi screen to get >= 2x zoom, will spell marker size double all of a sudden?
(In reply to comment #4) > (From update of attachment 183581 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=183581&action=review > > > Source/WebCore/platform/graphics/skia/GraphicsContextSkia.cpp:615 > > + int rasterScaleFactor = static_cast<int>(lroundf(effectiveScaleComponent(platformContext()->getTotalMatrix()))); > > This isn't able to differentiate between page zoom (user hits cmd-+ a few times) and device scale, is it? Like, if I zoom in often enough on a lowdpi screen to get >= 2x zoom, will spell marker size double all of a sudden? Hmm, I think you're right. It would look bad if the appearance suddenly changes. My purpose was to make sure composited layer also get high resolution grammar markers. Then I guess the correct way to do it is to set the platform device scale factor somewhere near GraphicsLayerChromium::paint(). I'll discuss with compositor guys to figure out what is the best place.
> My purpose was to make sure composited layer also get high resolution grammar markers. Then I guess the correct way to do it is to set the platform device scale factor somewhere near GraphicsLayerChromium::paint(). I'll discuss with compositor guys to figure out what is the best place. That sounds like a better approach. Thanks!
Created attachment 183589 [details] Patch
Attachment 183589 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/edit..." exit_code: 1 LayoutTests/platform/chromium-mac-snowleopard/editing/spelling/grammar-markers-hidpi-composited-expected.png:0: Have to enable auto props in the subversion config file (/home/alancutter/.subversion/config "enable-auto-props = yes"). Have to set the svn:mime-type in the subversion config file (/home/alancutter/.subversion/config "*.png = svn:mime-type=image/png"). [image/png] [5] Total errors found: 1 in 13 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 183589 [details] Patch This looks correct. lgtm, but I'm not a reviewer.
(Make sure you appease the style bot though! Here's what it says: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/edit..." exit_code: 1 LayoutTests/platform/chromium-mac-snowleopard/editing/spelling/grammar-markers-hidpi-composited-expected.png:0: Have to enable auto props in the subversion config file (/home/alancutter/.subversion/config "enable-auto-props = yes"). Have to set the svn:mime-type in the subversion config file (/home/alancutter/.subversion/config "*.png = svn:mime-type=image/png"). [image/png] [5] Total errors found: 1 in 13 files )
Comment on attachment 183589 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=183589&action=review > Source/WebCore/platform/graphics/chromium/GraphicsLayerChromium.cpp:846 > + context.platformApplyDeviceScaleFactor(deviceScaleFactor()); Is this also correct when WebSettings::applyDeviceScaleFactorInCompositor is true?
Hi Enne, I'm trying to set correct platform context device scale factor for composited layers, which is necessary for some decorations(such as grammar markers) to render correctly in high-resolution. I'm setting it in GraphicsLayerChromium's implementation of GraphicsContextPainter::paint(), does that sounds reasonable for you? Personally I find it logically wrong because it should be the underlying backend(the compositor) to decide the device scale factor on the GraphicsContext, not the content being rasterized(the GraphicsLayer). However it is the most simplistic way to make it work (GraphicsLayer::deviceScaleFactor() comes in handy). Do you have a better idea?
(In reply to comment #11) > (From update of attachment 183589 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=183589&action=review > > > Source/WebCore/platform/graphics/chromium/GraphicsLayerChromium.cpp:846 > > + context.platformApplyDeviceScaleFactor(deviceScaleFactor()); > > Is this also correct when WebSettings::applyDeviceScaleFactorInCompositor is true? Yes, platformApplyDeviceScaleFactor doesn't change the matrix. It is its intended use. :)
Comment on attachment 183589 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=183589&action=review >>> Source/WebCore/platform/graphics/chromium/GraphicsLayerChromium.cpp:846 >>> + context.platformApplyDeviceScaleFactor(deviceScaleFactor()); >> >> Is this also correct when WebSettings::applyDeviceScaleFactorInCompositor is true? > > Yes, platformApplyDeviceScaleFactor doesn't change the matrix. It is its intended use. :) Ok thanks, this looks pretty reasonable to me.
Comment on attachment 183589 [details] Patch Skia-specific.