Make it possible for the embedder to specify the deviceScaleFactor dynamically.
Created attachment 147203 [details] Patch
Created attachment 147378 [details] Patch
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?
Please wait for approval from abarth@webkit.org, dglazkov@chromium.org, fishd@chromium.org, jamesr@chromium.org or tkent@chromium.org before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.
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.
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.
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?
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?
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)
Perhaps we need to update the viewport in WebViewImpl
Comment on attachment 147378 [details] Patch Take deviceScale out of settings and I think this is good to go.
(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.
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); }
(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.
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.
Created attachment 147694 [details] Patch
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
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
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.
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
Created attachment 147872 [details] Patch
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.
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
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).
Created attachment 147884 [details] Patch
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
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!
Comment on attachment 147884 [details] Patch Clearing flags on attachment: 147884 Committed r120544: <http://trac.webkit.org/changeset/120544>
All reviewed patches have been landed. Closing bug.
Just checked: This seems to work in ash too.