Bug 88916 – [chromium] Make the deviceScaleFactor dynamically adjustable.

• Component: Layout and Rendering
• Status: RESOLVED FIXED

[Robert Kroeger]

Make it possible for the embedder to specify the deviceScaleFactor dynamically.

[Robert Kroeger]

Created attachment 147203 [details]
Patch

[Robert Kroeger]

Created attachment 147378 [details]
Patch

[Robert Kroeger]

thakis@: This version seems to work properly in that it actually changes the displayed size of the webpage.

jamesr@: This is a first cut at implementing the dynamic deviceScaleFactor setting that we talked about last week. Could you take a look please?

[WebKit Review Bot]

Please wait for approval from [e-mail], [e-mail], [e-mail], [e-mail] or [e-mail] before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.

[WebKit Review Bot]

Attachment 147378 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/Platform/ChangeLog', u'Source/Platf..." exit_code: 1
Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHost.h:257:  The parameter name "deviceScaleFactor" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.h:154:  The parameter name "deviceScaleFactor" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/Platform/chromium/public/WebLayerTreeView.h:118:  The parameter name "deviceScaleFactor" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 3 in 10 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Dana Jansens]

Comment on attachment 147378 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=147378&action=review

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHost.cpp:693
> +    m_deviceViewportSize.scale(m_settings.deviceScaleFactor);

the deviceViewportSize should be scaled from the viewportSize, not from its last deviceViewportSize, right?

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.cpp:580
> +    m_settings.deviceScaleFactor = newDeviceScaleFactor;

Does it make sense to keep this in CCSettings at all then? This seems like it could default to 1, and be set after initialization instead. I don't think we use CCSettings for any settings that you can change post-initialization.

[Adam Barth]

Comment on attachment 147378 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=147378&action=review

> Source/WebKit/chromium/src/WebViewImpl.cpp:2485
> -void WebViewImpl::setDeviceScaleFactor(float scaleFactor)
> +void WebViewImpl::setDeviceScaleFactor(const float scaleFactor)

We don't usually mark scalar parameters as const.  Is there a benefit to this change?

[James Robinson]

Comment on attachment 147378 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=147378&action=review

>> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.cpp:580
>> +    m_settings.deviceScaleFactor = newDeviceScaleFactor;
> 
> Does it make sense to keep this in CCSettings at all then? This seems like it could default to 1, and be set after initialization instead. I don't think we use CCSettings for any settings that you can change post-initialization.

Indeed - this doesn't belong on m_settings at all

> Source/WebKit/chromium/src/WebViewImpl.cpp:2499
> +        m_layerTreeView.setDeviceScaleFactor(scaleFactor);

if the caller calls setDeviceScaleFactor before we initialize m_layerTreeView, are you sure we pass the right value in when initializing it?

[Nico Weber]

With this and http://codereview.chromium.org/10548026/ (patch set 9) patched in, switching dpiness on composited pages almost work: The only thing that seems to not get updated is the root layer text resolution. See http://i.imgur.com/kRrpT.png for how poster circle looks after lodpi->hidpi switch (poster circle itself is sharp; text isn't)

[Dana Jansens]

Perhaps we need to update the viewport in WebViewImpl

[James Robinson]

Comment on attachment 147378 [details]
Patch

Take deviceScale out of settings and I think this is good to go.

[James Robinson]

(In reply to comment #9)
> With this and http://codereview.chromium.org/10548026/ (patch set 9) patched in, switching dpiness on composited pages almost work: The only thing that seems to not get updated is the root layer text resolution. See http://i.imgur.com/kRrpT.png for how poster circle looks after lodpi->hidpi switch (poster circle itself is sharp; text isn't)

Could you open a new bug about this problem in particular?  The root layer is special in a few ways.

[Nico Weber]

My problem goes away with the following change on top of this bug's patch. Maybe this makes sense to someone on this bug. (Given the assert 2 lines above, something is wrong; chances are it's that assert?)

diff --git a/Source/WebKit/chromium/src/WebViewImpl.cpp b/Source/WebKit/chromium/src/WebViewImpl.cpp
index 3c558ea..8079780 100644
--- a/Source/WebKit/chromium/src/WebViewImpl.cpp
+++ b/Source/WebKit/chromium/src/WebViewImpl.cpp
@@ -2485,6 +2485,7 @@ void WebViewImpl::setDeviceScaleFactor(const float scaleFactor)
         // needs to match the one in the compositor.
         ASSERT(scaleFactor == m_deviceScaleInCompositor);
     }
+    m_deviceScaleInCompositor = scaleFactor;
     if (!m_layerTreeView.isNull())
         m_layerTreeView.setDeviceScaleFactor(scaleFactor);
 }

[Dana Jansens]

(In reply to comment #13)
> My problem goes away with the following change on top of this bug's patch. Maybe this makes sense to someone on this bug. (Given the assert 2 lines above, something is wrong; chances are it's that assert?)
> 
> diff --git a/Source/WebKit/chromium/src/WebViewImpl.cpp b/Source/WebKit/chromium/src/WebViewImpl.cpp
> index 3c558ea..8079780 100644
> --- a/Source/WebKit/chromium/src/WebViewImpl.cpp
> +++ b/Source/WebKit/chromium/src/WebViewImpl.cpp
> @@ -2485,6 +2485,7 @@ void WebViewImpl::setDeviceScaleFactor(const float scaleFactor)
>          // needs to match the one in the compositor.
>          ASSERT(scaleFactor == m_deviceScaleInCompositor);
>      }
> +    m_deviceScaleInCompositor = scaleFactor;
>      if (!m_layerTreeView.isNull())
>          m_layerTreeView.setDeviceScaleFactor(scaleFactor);
>  }

Yeh, that value should be == to what the layerTreeView is using as its scale factor. So it should be set inside the if() instead.

That said. Nothing should be passed to the compositor at all if WebSettings::applyDeviceScaleFactorInCompositor isn't true.

[Robert Kroeger]

Comment on attachment 147378 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=147378&action=review

>> Source/Platform/chromium/public/WebLayerTreeView.h:118
>> +    WEBKIT_EXPORT void setDeviceScaleFactor(const float deviceScaleFactor);
> 
> The parameter name "deviceScaleFactor" adds no information, so it should be removed.  [readability/parameter_name] [5]

done, next patch.

>> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHost.cpp:693
>> +    m_deviceViewportSize.scale(m_settings.deviceScaleFactor);
> 
> the deviceViewportSize should be scaled from the viewportSize, not from its last deviceViewportSize, right?

Good catch. I think so. done, next patch.

>> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHost.h:257
>> +    void setDeviceScaleFactor(const float deviceScaleFactor);
> 
> The parameter name "deviceScaleFactor" adds no information, so it should be removed.  [readability/parameter_name] [5]

done, next patch

>>> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.cpp:580
>>> +    m_settings.deviceScaleFactor = newDeviceScaleFactor;
>> 
>> Does it make sense to keep this in CCSettings at all then? This seems like it could default to 1, and be set after initialization instead. I don't think we use CCSettings for any settings that you can change post-initialization.
> 
> Indeed - this doesn't belong on m_settings at all

Done, next patch.

>> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.h:154
>> +    void setDeviceScaleFactor(const float deviceScaleFactor);
> 
> The parameter name "deviceScaleFactor" adds no information, so it should be removed.  [readability/parameter_name] [5]

done, next patch

>> Source/WebKit/chromium/src/WebViewImpl.cpp:2485
>> +void WebViewImpl::setDeviceScaleFactor(const float scaleFactor)
> 
> We don't usually mark scalar parameters as const.  Is there a benefit to this change?

This was an oversight. Corrected, next patch.

>> Source/WebKit/chromium/src/WebViewImpl.cpp:2499
>> +        m_layerTreeView.setDeviceScaleFactor(scaleFactor);
> 
> if the caller calls setDeviceScaleFactor before we initialize m_layerTreeView, are you sure we pass the right value in when initializing it?

Thanks for catching this. Fixed, next patch.

[Robert Kroeger]

Created attachment 147694 [details]
Patch

[James Robinson]

Comment on attachment 147694 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=147694&action=review

R=me, few nits to clean up (mostly unnecessary consts on scalar parameters).

> Source/Platform/chromium/public/WebLayerTreeView.h:116
> +    WEBKIT_EXPORT void setDeviceScaleFactor(const float);

no const

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHost.cpp:689
> +void CCLayerTreeHost::setDeviceScaleFactor(const float deviceScaleFactor)

no const

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHost.h:254
> +    // Dynamic setting of the deviceScaleFactor.

This comment's kind of silly, what does it tell the reader that they wouldn't already know? I would just remove it.

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHost.h:255
> +    void setDeviceScaleFactor(const float);

no const on a scalar parameter - it's passed by value anyway

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.cpp:577
> +void CCLayerTreeHostImpl::setDeviceScaleFactor(const float newDeviceScaleFactor)

no const

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.h:154
> +    void setDeviceScaleFactor(const float);

no const

[Nico Weber]

I patched the new patch in. Poster circle resizes better with it, but as said above the base layer isn't resized correctly. Something like this fixes it (on top of your patch):

diff --git a/Source/WebKit/chromium/src/WebViewImpl.cpp b/Source/WebKit/chromium/src/WebViewImpl.cpp
index 62258e7..a41aa6e 100644
--- a/Source/WebKit/chromium/src/WebViewImpl.cpp
+++ b/Source/WebKit/chromium/src/WebViewImpl.cpp
@@ -2480,14 +2480,16 @@ void WebViewImpl::setDeviceScaleFactor(float scaleFactor)
 
     page()->setDeviceScaleFactor(scaleFactor);
 
+    if (!m_layerTreeView.isNull()) {
+        m_deviceScaleInCompositor = scaleFactor;
+        m_layerTreeView.setDeviceScaleFactor(scaleFactor);
+    }
     if (m_deviceScaleInCompositor != 1) {
         // Don't allow page scaling when compositor scaling is being used,
         // as they are currently incompatible. This means the deviceScale
         // needs to match the one in the compositor.
         ASSERT(scaleFactor == m_deviceScaleInCompositor);
     }
-    if (!m_layerTreeView.isNull())
-        m_layerTreeView.setDeviceScaleFactor(scaleFactor);
 }
 
 bool WebViewImpl::isFixedLayoutModeEnabled() const

[Dana Jansens]

Comment on attachment 147694 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=147694&action=review

> Source/WebKit/chromium/src/WebViewImpl.cpp:2499
> +    if (!m_layerTreeView.isNull())
> +        m_layerTreeView.setDeviceScaleFactor(scaleFactor);

This should only be done if m_webSettings->applyDefaultDeviceScaleFactorInCompositor() is true. And m_deviceScaleInCompositor needs to be set accordingly, as Nico pointed out. It should basically mimic the code in setIsAcceleratedCompositingActive().

if (applyInCompositor && !ltv.isNull) {
  m_deviceScaleInCompositor = scale;
  m_layerTreeView.setDeviceScaleFactor(m_deviceScaleInCompositor);
}

> Source/WebKit/chromium/src/WebViewImpl.cpp:3504
>              m_deviceScaleInCompositor = page()->deviceScaleFactor();

Can this live beside the call to m_layerTreeView.setDeviceScaleFactor() to make their association more clear? This if-block would move with it.

[Robert Kroeger]

Comment on attachment 147694 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=147694&action=review

>> Source/Platform/chromium/public/WebLayerTreeView.h:116
>> +    WEBKIT_EXPORT void setDeviceScaleFactor(const float);
> 
> no const

done, p4

>> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHost.cpp:689
>> +void CCLayerTreeHost::setDeviceScaleFactor(const float deviceScaleFactor)
> 
> no const

done, p4

>> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHost.h:254
>> +    // Dynamic setting of the deviceScaleFactor.
> 
> This comment's kind of silly, what does it tell the reader that they wouldn't already know? I would just remove it.

removed, p4

>> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHost.h:255
>> +    void setDeviceScaleFactor(const float);
> 
> no const on a scalar parameter - it's passed by value anyway

done, p4

>> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.cpp:577
>> +void CCLayerTreeHostImpl::setDeviceScaleFactor(const float newDeviceScaleFactor)
> 
> no const

done, p4

>> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.h:154
>> +    void setDeviceScaleFactor(const float);
> 
> no const

done, p4

>> Source/WebKit/chromium/src/WebViewImpl.cpp:2499
>> +        m_layerTreeView.setDeviceScaleFactor(scaleFactor);
> 
> This should only be done if m_webSettings->applyDefaultDeviceScaleFactorInCompositor() is true. And m_deviceScaleInCompositor needs to be set accordingly, as Nico pointed out. It should basically mimic the code in setIsAcceleratedCompositingActive().
> 
> if (applyInCompositor && !ltv.isNull) {
>   m_deviceScaleInCompositor = scale;
>   m_layerTreeView.setDeviceScaleFactor(m_deviceScaleInCompositor);
> }

fixed, p4

>> Source/WebKit/chromium/src/WebViewImpl.cpp:3504
>>              m_deviceScaleInCompositor = page()->deviceScaleFactor();
> 
> Can this live beside the call to m_layerTreeView.setDeviceScaleFactor() to make their association more clear? This if-block would move with it.

adjusted, p4

[Robert Kroeger]

Created attachment 147872 [details]
Patch

[Dana Jansens]

Comment on attachment 147872 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=147872&action=review

Thanks for the changes, some nits for consistency, but overall this LGTM.

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.cpp:579
> +    if (newDeviceScaleFactor == deviceScaleFactor())

nit: m_deviceScaleFactor?

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.cpp:583
> +    m_deviceViewportSize = viewportSize();

nit: m_viewportSize?

> Source/WebKit/chromium/src/WebViewImpl.cpp:3509
> +                setDeviceScaleFactor(m_deviceScaleInCompositor);

This is basically setDeviceScaleFactor(deviceScaleFactor()). You can just remove this call here.

[Robert Kroeger]

Comment on attachment 147872 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=147872&action=review

>> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.cpp:579
>> +    if (newDeviceScaleFactor == deviceScaleFactor())
> 
> nit: m_deviceScaleFactor?

Is this really a good idea? It reduces the possible future benefit of having accessors.

>> Source/WebKit/chromium/src/WebViewImpl.cpp:3509
>> +                setDeviceScaleFactor(m_deviceScaleInCompositor);
> 
> This is basically setDeviceScaleFactor(deviceScaleFactor()). You can just remove this call here.

done, p5

[Dana Jansens]

Comment on attachment 147872 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=147872&action=review

>>> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.cpp:579
>>> +    if (newDeviceScaleFactor == deviceScaleFactor())
>> 
>> nit: m_deviceScaleFactor?
> 
> Is this really a good idea? It reduces the possible future benefit of having accessors.

I'm not sure I'm just going on what I've seen, as we do this everywhere else (and you did as well in CCLTH).

[Robert Kroeger]

Created attachment 147884 [details]
Patch

[James Robinson]

Comment on attachment 147884 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=147884&action=review

R=me

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.cpp:579
> +void CCLayerTreeHostImpl::setDeviceScaleFactor(float newDeviceScaleFactor)
> +{
> +    if (newDeviceScaleFactor == deviceScaleFactor())

IMO this is bad style - what you had in CCLayerTreeHost::setDeviceScaleFactor() is better. using the member var directly makes it much clearer what is going on to readers and avoids having the ugly "new" prefix on a variable name

[Nico Weber]

Comment on attachment 147884 [details]
Patch

I patched this in and it seems to work great (on OS X). Let's land this, so that there's enough time for a follow-up before the branch should there be issues on CrOs (will check that monday morning).

Thanks, Rob!

[WebKit Review Bot]

Comment on attachment 147884 [details]
Patch

Clearing flags on attachment: 147884

Committed r120544: <http://trac.webkit.org/changeset/120544>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Nico Weber]

Just checked: This seems to work in ash too.