Bug 107342 – Clean up device scale factor with Skia GraphicsContext

• Component: New Bugs
• Status: RESOLVED WONTFIX

[Tien-Ren Chen]

Clean up device scale factor with Skia GraphicsContext

[Tien-Ren Chen]

Created attachment 183580 [details]
Patch

[Tien-Ren Chen]

Created attachment 183581 [details]
Patch

[WebKit Review Bot]

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.

[Nico Weber]

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?

[Tien-Ren Chen]

(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.

[Nico Weber]

> 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!

[Tien-Ren Chen]

Created attachment 183589 [details]
Patch

[WebKit Review Bot]

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.

[Nico Weber]

Comment on attachment 183589 [details]
Patch

This looks correct. lgtm, but I'm not a reviewer.

[Nico Weber]

(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
)

[Dana Jansens]

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?

[Tien-Ren Chen]

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?

[Tien-Ren Chen]

(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. :)

[Dana Jansens]

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.

[Darin Adler]

Comment on attachment 183589 [details]
Patch

Skia-specific.