[webkit-reviews] review canceled: [Bug 86051] [chromium] Scale all compositor output by deviceScaleFactor : [Attachment 142122] Patch

[bugzilla-daemon at webkit.org]

Dana Jansens <danakj at chromium.org> has canceled Dana Jansens
<danakj at chromium.org>'s request for review:
Bug 86051: [chromium] Scale all compositor output by deviceScaleFactor
https://bugs.webkit.org/show_bug.cgi?id=86051

Attachment 142122: Patch
https://bugs.webkit.org/attachment.cgi?id=142122&action=review

------- Additional Comments from Dana Jansens <danakj at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=142122&action=review


>>>> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHost.cpp:368
>>>> +void CCLayerTreeHost::setDeviceScaleFactor(float deviceScaleFactor)
>>> 
>>> This seems fishy - are you expecting us to be able to handle the device
scale factor changing dynamically (like if you drag a window from a high-dpi
monitor to a low-dpi monitor on a multi-monitor setup)?  That sounds really
complicated and means that all viewport-size-dependent code needs to be aware
of dynamic changes.
>>> 
>>> I would much prefer you make the device scale factor a creation-time
invariant and not have any code to try to handle dynamic changing unless that's
a hard requirement
>> 
>> It's a creation-time invariant on ChromeOS and also mostly on Android.  The
wrinkle is that there's support in the viewport tag to allow web pages to turn
off CSS-pixel scaling and that's what ends up possibly calling this function
multiple times.  I also think this is a questionable way of doing it (not even
sure it really works, the feature is very rarely used in the wild), but fixing
it is somewhat outside the scope of this patch.
>> 
>> The current terminology is that "default device scale factor" is the actual
physical screen device scale factor, and "device scale factor" is usually the
same thing but can be overriden by the viewport tag.  These names are poorly
chosen...
> 
> OK.  Seems a bit fishy to me as well.  Since it's just for the viewport tag,
which CrOS isn't supporting for now, I would really prefer not to add any new
code for dynamically changing this.  When we get to that bit of the Android
code we can think about how to handle it and what we want to actually do.

We do plan to support viewport at some point. So that would mean changing the
deviceScaleFactor in the LayerTreeHost to 1 on some pages, and scaling up the
frame instead. But we can cross that bridge when we get to it. There are other
changes that will need to be made as well.

I can stick this in WebLayerTreeSettings::deviceScaleFactor but I feel like the
logic is a little less clean when WebViewImpl isn't aware of and deciding how
the deviceScale and pageScale are interacting. See contentsScale() comments
below.

>> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.cpp:650
>> +	// maxScroll is computed in physical pixels, but scroll positions are
in DIP.
> 
> DIP is an an acronym that seems to only be used in this patch and not
defined. expand, please?

k. i think it can be better described as "layout pixels".

>> Source/WebKit/chromium/src/NonCompositedContentHost.cpp:42
>> +	, m_pageScaleFactor(1.0)
> 
> why would NonCompositedContentHost have a pageScaleFactor? that doesn't seem
necessary

Then we will call pageOrDeviceScaleFactorChanged() on the GraphicsLayer every
time NCCH::setViewport() is called, as we did before. Is that what you are
intending?

>> Source/WebKit/chromium/src/NonCompositedContentHost.cpp:111
>> +	if (m_pageScaleFactor != pageScale || m_deviceScaleFactor !=
deviceScale) {
> 
> This page scale logic seems wrong
> 
> I'm quite skeptical of dynamically changing deviceScale, as I mentioned
above. Would be much happier without this.

The deviceScaleFactor needs to be factored into the GraphicsLayer's
contentsScale() in order to render at the scale things will be put on the
screen.

In a previous patch I did this by using the deviceScaleFactor on the
LayerTreeHost within LayerChromium, but this code was more complex. This way we
give it to the GraphicsLayer and it sets the contentsScale() appropriately.

But it seems like you want to pull all the logic related to this out of
WebViewImpl - moving it to the initialization of WebLayerTreeView for instance.
It's getting a bit weird to have this here if WebViewImpl doesn't know and
control this distinction of when the deviceScaleFactor is part of the
pageScaleFactor.

Do you want the LayerChromium::contentsScale() to be multiplied by the LTH's
deviceScale again then?

>> Source/WebKit/chromium/src/NonCompositedContentHost.h:69
>> +	virtual float pageScaleFactor() const OVERRIDE { return 1; }
> 
> Why is this here?  From GraphicsLayerClient.h:
> 
> virtual float pageScaleFactor() const { return 1; }
> 
> The rest of the GraphicsLayerClient overrides are below, in the private
section.  Please put this with those.

Oh.. I figured it would make them inaccessible. k!

>> Source/WebKit/chromium/src/WebViewImpl.cpp:-2391
>> -	   
m_nonCompositedContentHost->topLevelRootLayer()->deviceOrPageScaleFactorChanged
();
> 
> are you deleting this call or did you move it somewhere else?

Deleting. updateLayerTreeViewport() calls NCCH::setViewport() which calls
deviceOrPageScaleFactorChanged() on its GraphicsLayer. This looks redundant to
me.

>> Source/WebKit/chromium/src/WebViewImpl.cpp:2438
>> +	bool deviceScaleFactorIsComponentOfPageScaleFactor =
isFixedLayoutModeEnabled();
> 
> it feels like something dodgy is going on here.  you should never have a
non-1 pageScale without being in FixedLayoutMode, so why do you need this
guard?

I'm not sure it doesn't make sense to let you zoom in with pinch in non-fixed
layout. I'd rather not break pageScale in non-fixed layout.