[chromium] Scale all compositor output by the defaultDeviceScaleFactor

• Component: New Bugs
• Status: RESOLVED FIXED

[Dana Jansens]

[chromium] Scale all compositor output by deviceScaleFactor

[Fady Samuel]

(In reply to comment #0)
> [chromium] Scale all compositor output by deviceScaleFactor

Please use defaultDeviceScaleFactor so that we can make target-densitydpi in the viewport tag work in the near future. :-)

Please see Defining the viewport target density here: http://developer.android.com/guide/webapps/targeting.html

[Dana Jansens]

What we're doing in my CL is using the current deviceScaleFactor. Without viewport, this is the defaultDeviceScaleFactor. With viewport, it's whatever gets computed as the deviceScaleFactor.

Isn't this right?

[Fady Samuel]

(In reply to comment #2)
> What we're doing in my CL is using the current deviceScaleFactor. Without viewport, this is the defaultDeviceScaleFactor. With viewport, it's whatever gets computed as the deviceScaleFactor.
> 
> Isn't this right?

But when we go to compute the pageScaleFactor in WebCore, it should be (deviceScaleFactor / defaultDeviceScaleFactor * initialscale)? No?

[Dana Jansens]

Offline convo: Seems this is the approach, and we may need to fix some viewport stuff later.

[Dana Jansens]

Created attachment 141259 [details]
Patch

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

[Alexandre Elias]

WebCore scroll offsets are integers.  So at deviceScaleFactor=2, it looks like this change would snap scroll offsets to 2-pixel boundaries.

[Dana Jansens]

Rob, can you comment on scrolling?

[Dana Jansens]

Created attachment 141290 [details]
Patch

GLC::pageScaleFactor() called GLC::pageScaleFactor(), oops. Now GLC::computePageScaleFactor() will call GL::pageScaleFactor().

[James Robinson]

I don't really see a need for the compositor to know about device scale.  Why not just set page scale, which the compositor already knows about?

[Dana Jansens]

There is a good reason - we want to avoid all the bugs that occur on desktop sites in fixed layout mode, because they are not fixable in our timeframe.

I attempted to scale using the pageScale instead, with input from Alex. We scaled up the FrameView size to physical pixels. And scaled down the ScrollView::layoutWidth and layoutHeight, to CSS pixels.

The contents end up being scaled twice, and events end up getting scaled too much (probably page * device scale somewhere), but we could fix that stuff.

However, because the FrameView and the layoutWidth/Height are not the same we hit all the same layout problems that fixed layout mode has. And that makes this solution a no-go for us right now. For instance, the desktop site for docs.google.com is completely unusable.

It is a requirement for us that FrameView be sized in CSS pixels to avoid these bugs in layout/rendering. In order to do that, we need to scale to physical pixels in the compositor.

- pageScale will be used for making the contents larger within the viewport.
- deviceScale will be used for making the scale of the display and the viewport larger.

The issue with scrolling right now is that WebCore has scroll offsets in CSS pixel integers. We can solve this a number of ways:
1. Make WebCore use floats.
2. Make compositor snap scroll to CSS pixels.
3. Make compositor drop scrolls from WebCore that are < 1 CSS pixel (the rounding jitter).

We could long-term fix layout/rendering for FrameView size != layout size, but it's not something we can do right now, so I think we should fix scrolling instead. This seems like a more reasonable goal for M21.

[Alexandre Elias]

OK, that sounds like a real reason to go this route.  I'd like to learn the root causes of the fixed layout problems, but I can believe they are too hairy to fix in the short run.

There is another workaround 4. to the scroll snapping problem.  We can pre-snap it in the impl thread and send that down along with setting m_sentScrollDelta appropriately.  In other words, we can exploit the mechanism to keep track of differences between threads for in-progress scrolls, for this difference as well.  This fix would mean we don't lose any information due to rounding.  The main ugliness is that it means scroll delta will never converge to zero, and that absent more hacks, the 1-px delta would be preserved across navigations as well.  We should attempt 1. change WebCore scroll offsets to floats first, but that would be a fallback workaround.

[Alexandre Elias]

OK, after more thinking about this, I have a new proposal: fold deviceScaleFactor into pageScaleFactor as before -- but, create a new ENABLE flag or WebSetting that lets us suppress the default behavior of applying pageScaleFactor as 2D CSS transform changing the bounds on the root document, and instead applies it using the calculateDrawTransformsAndVisibility method you created in this patch.

Here is the reasoning behind this:

- There's no way in which applying pageScaleFactor and deviceScaleFactor using different transformations makes sense.  They should be folded in together.

- I think the bugs you are seeing are not so much due to fixed layout, as to the pageScaleFactor 2D CSS transform implementation.  So, there are bugs in both the CSS-transform approach (particularly around fixed position, and sublayers in general), and with your calculateDrawTransformsAndVisibility approach (particularly around pixel snapping w.r.t. scroll offsets and possibly other things).  The bugs on both ends appear somewhat nontrivial to fix, and they are different priority to Android and ChromeOS, so it's reasonable for the platforms to diverge in the short term via a setting.

- Fundamentally, I think your approach points to a fundamentally cleaner way to implement pageScaleFactor.  I was never happy with the frequent scroll-offset coordinate space changes and with the different treatment of root and sublayers.  The great things about having WebCore live entirely in CSS pixels would be, our page-scale-related sublayer bugs would go away, and scroll offsets would become independent of page scale.  The only problem is that there are many questions around floating point conversion and pixel snapping that need to be better understood.  So it can act as a forward-looking setting, that Android would also be happy to switch to if/when all the bugs are resolved.

[Robert Kroeger]

(In reply to comment #7)
> WebCore scroll offsets are integers.  So at deviceScaleFactor=2, it looks like this change would snap scroll offsets to 2-pixel boundaries.

It would snap scroll to 2-physical-pixel boundaries. I believe that this is desired behaviour: otherwise I think we would have a situation where user scrolls (snapped to physical) could not be necessarily be replicated by JavaScript scrollTo (snapped to even physical pixels)

[Fady Samuel]

> - Fundamentally, I think your approach points to a fundamentally cleaner way to implement pageScaleFactor.  I was never happy with the frequent scroll-offset coordinate space changes and with the different treatment of root and sublayers.  The great things about having WebCore live entirely in CSS pixels would be, our page-scale-related sublayer bugs would go away, and scroll offsets would become independent of page scale.  The only problem is that there are many questions around floating point conversion and pixel snapping that need to be better understood.  So it can act as a forward-looking setting, that Android would also be happy to switch to if/when all the bugs are resolved.

Two concerns:

Will web developers ever be able to develop high-DPI aware apps that layout boxes at physical pixel positions or produce one pixel lines?

How this will interact with the viewport tag?

[Alexandre Elias]

Hmm, sorry to reverse myself, but I'm having second thoughts about my proposal to try out a different coordinate space in a setting.  I'm realizing in that space there would be pixel snapping problems with not only scroll offset, but also layer positions and viewport size, and the viewport size would have to constantly change.  So it becomes a bit of a wash which space has more fundamental problems.  At that point, we should go with what already is in place and similar to the other WebKit ports.

It seems like the main problem is the fixed-position bug.  Let's dig into that further and see if we can fix it first.

[Alexandre Elias]

OK, leviw has a good description of the fixed-layout, fixed-position element bug: http://code.google.com/p/chromium/issues/detail?id=123624  .  It applies to fixed-position elements which are not composited layers, and it does seem difficult to address in the short term.

The benefit of this patch is that it makes non-composited fixed position elements appear correct at (dynamic) page scale 1.0.  Android sees no benefit from this change, as all our fixed position elements are composited, and our page scale is rarely 1.0.  I don't think ChromeOS will really need it in the long term either, once all the deeper bugs are fixed.

But I understand ChromeOS is under some time pressure and I haven't found a better compromise than this patch after a couple of days of debate.  So I'm okay with this patch going in more or less as is, as long as all behavior diffs are in if (!isFixedLayoutModeEnabled()) blocks, so it's a no-op on mobile.  Then we can make progress over the deeper issues over a longer time period.

[Dana Jansens]

Created attachment 141543 [details]
Patch

[Dana Jansens]

The deviceScaleFactor is given to the compositor only when fixed-layout mode is not enabled now. Simplied things a bunch, keeping layer contentsScale() how it was previously, where it uses the deviceScaleFactor and pageScaleFactor given to us from the WebCore::Page.

[WebKit Review Bot]

Comment on attachment 141543 [details]
Patch

Attachment 141543 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/12665816

New failing tests:
fast/events/page-scaled-mouse-click-iframe.html
compositing/geometry/fixed-position-transform-composited-page-scale.html
http/tests/security/sandboxed-iframe-origin-add.html
fast/frames/lots-of-objects.html
fast/loader/text-document-wrapping.html
compositing/geometry/fixed-position-iframe-composited-page-scale.html
compositing/overflow/overflow-scaled-descendant-overlapping.html
fast/events/page-scaled-mouse-click.html
fast/events/scale-and-scroll-iframe-body.html
WebFrameTest.FAILS_DivAutoZoomParamsTest
fast/canvas/webgl/shader-precision-format.html
fast/repaint/background-scaling.html
fast/events/touch/send-oncancel-event.html
compositing/geometry/fixed-position-composited-page-scale.html
fast/events/touch/page-scaled-touch-gesture-click.html
fast/events/scale-and-scroll-iframe-window.html
http/tests/xmlhttprequest/xmlhttprequest-unsafe-redirect.html
fast/frames/frame-set-scaling-hit.html
compositing/geometry/fixed-position-iframe-composited-page-scale-down.html
fast/frames/frame-set-rotation-hit.html
compositing/geometry/fixed-position-composited-page-scale-down.html
fast/loader/javascript-url-in-object.html
compositing/geometry/fixed-position-transform-composited-page-scale-down.html
http/tests/inspector/resource-har-pages.html
fast/repaint/scale-page-shrink.html

[WebKit Review Bot]

Created attachment 141556 [details]
Archive of layout-test-results from ec2-cr-linux-02

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-02  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

[Dana Jansens]

Created attachment 141760 [details]
Patch

Fixing layout tests with page scale:
- don't limit the page scale while setting the device scale factor.
- The NCCH appliesPageScale(). But it did not set the flag to indicate this. Instead of just didn't return the real page/display scale factors. Made this code more obvious by setting the appliesPageScale() flag and returning real values (which GraphicsLayerChromium can ignore as it wishes).

[Alexandre Elias]

Comment on attachment 141760 [details]
Patch

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

Looks fine in general, comments:

> Source/WebCore/platform/graphics/chromium/cc/CCLayerImpl.h:171
> +    float contentsScale() const { return m_contentsScale; }

Is this contentsScale change needed for this change?  If not, can it be removed/moved to another patch for clarity?

> Source/WebKit/chromium/src/NonCompositedContentHost.h:88
> +    float m_deviceScaleFactor;

As far as I see, NCCH::m_deviceScaleFactor is not used anywhere, can it be removed?

> Source/WebKit/chromium/tests/CCLayerTreeHostTest.cpp:1794
> +        m_layerTreeHost->setDeviceScaleFactor(2);

Please add a test for non-integral deviceScaleFactor as I understand all platform variants may run into this eventually.

[WebKit Review Bot]

Comment on attachment 141760 [details]
Patch

Attachment 141760 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/12705060

New failing tests:
WebFrameTest.FAILS_DivAutoZoomParamsTest

[WebKit Review Bot]

Created attachment 141779 [details]
Archive of layout-test-results from ec2-cr-linux-02

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-02  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

[Alexandre Elias]

> > Source/WebKit/chromium/src/NonCompositedContentHost.h:88
> > +    float m_deviceScaleFactor;
> 
> As far as I see, NCCH::m_deviceScaleFactor is not used anywhere, can it be removed?

OK, I see this is overriding a GraphicsLayerClient function.  In that case, please make it 1 in fixed layout mode for consistency.

[Dana Jansens]

Created attachment 141792 [details]
Patch

Thanks Alex, a couple changes here:

- Made the NCCH deviceScaleFactor == 1 when in fixed-layout mode (since it is baked into the page scale here, right?).
- In non-fixed-layout, the pageScaleFactor is baked into the NCCH contentsSize, but the deviceScaleFactor is not. So make the pageScaleFactor == 1 on the NCCH, but make the deviceScaleFactor the real scale value.
- GLC expects to get a resize after the "pageOrDeviceScaleChanged" notification, so re-ordering the calls in NCCH.
- Made the unit test use 1.5 scale factor!
- Removed contentsScale from CCLayerImpl for now. It can go in after with the changes to fix render surfaces under scale.

[WebKit Review Bot]

Comment on attachment 141792 [details]
Patch

Attachment 141792 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/12694317

New failing tests:
WebFrameTest.FAILS_DivAutoZoomParamsTest

[WebKit Review Bot]

Created attachment 141799 [details]
Archive of layout-test-results from ec2-cr-linux-04

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-04  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

[Adrienne Walker]

Comment on attachment 141792 [details]
Patch

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

> Source/WebCore/ChangeLog:43
> +        * platform/graphics/chromium/cc/CCLayerImpl.h:
> +        (WebCore::CCLayerImpl::contentsScale):
> +        (WebCore::CCLayerImpl::setContentsScale):

Your ChangeLog lies.  :)

> Source/WebKit/chromium/src/NonCompositedContentHost.cpp:114
> +        m_graphicsLayer->deviceOrPageScaleFactorChanged();

How does this get called for non-root layers to push contents scale down for sublayers? (Or, are sublayers explicitly not part of this patch?)

> Source/WebKit/chromium/tests/CCLayerTreeHostTest.cpp:1814
> +        EXPECT_EQ(1, m_childLayer->contentsScale());

This is a little bit unexpected, but am I understanding correctly that this is 1 because there's no GraphicsLayerChromium involved, and that's currently the only class responsible for pushing down the contents scale to the LayerChromium when the device scale factor is changed?

How are Aura layers going to interact with different device scales, since there's no GraphicsLayerChromium involved there? Are we going to expose content scale on WebContentLayer and Aura will have to manage this more explicitly? (Should the compositor just be handling all these content scale changes internally from the device scale on layerTreeHost?)

[Dana Jansens]

Comment on attachment 141792 [details]
Patch

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

>> Source/WebCore/ChangeLog:43
>> +        (WebCore::CCLayerImpl::setContentsScale):
> 
> Your ChangeLog lies.  :)

Oops, yeh. I've regenerated the ChangeLogs.

>> Source/WebKit/chromium/src/NonCompositedContentHost.cpp:114
>> +        m_graphicsLayer->deviceOrPageScaleFactorChanged();
> 
> How does this get called for non-root layers to push contents scale down for sublayers? (Or, are sublayers explicitly not part of this patch?)

Other layers get notified when we call setPageScaleFactor (or setDeviceScaleFactor) on the WebView's Page object. This runs thru the hierarchy calling deviceOrPageScaleFactorChanged() for all graphics layers that it knows about (all but this one).

>> Source/WebKit/chromium/tests/CCLayerTreeHostTest.cpp:1814
>> +        EXPECT_EQ(1, m_childLayer->contentsScale());
> 
> This is a little bit unexpected, but am I understanding correctly that this is 1 because there's no GraphicsLayerChromium involved, and that's currently the only class responsible for pushing down the contents scale to the LayerChromium when the device scale factor is changed?
> 
> How are Aura layers going to interact with different device scales, since there's no GraphicsLayerChromium involved there? Are we going to expose content scale on WebContentLayer and Aura will have to manage this more explicitly? (Should the compositor just be handling all these content scale changes internally from the device scale on layerTreeHost?)

Yeh this is kind of bogus. If we had a RLCompositor and a GraphicsLayer, then it would be == deviceScaleFactor() due to GLC::contentsScale(). I should probably just remove these checks, it is not what the test is checking for. It is supposed to be testing the resulting drawTransforms etc.

Aura is doing something of its own. It makes compositor layers in physical pixels all AIUI, just using larger sizes and assets instead.

[Dana Jansens]

Created attachment 141825 [details]
Patch

- Adding OVERRIDE to new overriding methods in NCCH.
- Use fixed layout mode in WebFrameTest.DivAutoZoomParamsTest and make the test function again.
- Drop mouse events in the event handler before leaving the test, and remove the FAILS_.
- Adding test WebFrameTest.DeviceScaleFactorUsesDefaultWithoutFixedLayout which verifies that deviceScale is not a component of pageScale when not in fixed-layout mode.

[Alexandre Elias]

Comment on attachment 141825 [details]
Patch

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

> Source/WebKit/chromium/src/WebViewImpl.cpp:3481
> +    float pageScale = 1;

There is no longer any code here to set pageScale to non-1.

[Dana Jansens]

Comment on attachment 141825 [details]
Patch

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

>> Source/WebKit/chromium/src/WebViewImpl.cpp:3481
>> +    float pageScale = 1;
> 
> There is no longer any code here to set pageScale to non-1.

There never was any value except 1 used for the NCCH. It was just super unclear before, unless I am confused somewhere?

It used to pass pageScale in here. Then say "if pageScale != pageScaleFactor(), displayOrPageScaleFactorChanged()". But NCCH always returns "1" from its pageScaleFactor() method.

Now we pass in "1" explicitly so that it is more clear what is going on, and we don't need to call displayOrPageScaleFactorChanged() when the page scale is not changing in the NCCH.

[Adrienne Walker]

(In reply to comment #31)
> (From update of attachment 141792 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=141792&action=review
> 
> >> Source/WebKit/chromium/src/NonCompositedContentHost.cpp:114
> >> +        m_graphicsLayer->deviceOrPageScaleFactorChanged();
> > 
> > How does this get called for non-root layers to push contents scale down for sublayers? (Or, are sublayers explicitly not part of this patch?)
> 
> Other layers get notified when we call setPageScaleFactor (or setDeviceScaleFactor) on the WebView's Page object. This runs thru the hierarchy calling deviceOrPageScaleFactorChanged() for all graphics layers that it knows about (all but this one).

Ah, thanks for the explanation.
 
> >> Source/WebKit/chromium/tests/CCLayerTreeHostTest.cpp:1814
> >> +        EXPECT_EQ(1, m_childLayer->contentsScale());
> > 
> > This is a little bit unexpected, but am I understanding correctly that this is 1 because there's no GraphicsLayerChromium involved, and that's currently the only class responsible for pushing down the contents scale to the LayerChromium when the device scale factor is changed?
> > 
> > How are Aura layers going to interact with different device scales, since there's no GraphicsLayerChromium involved there? Are we going to expose content scale on WebContentLayer and Aura will have to manage this more explicitly? (Should the compositor just be handling all these content scale changes internally from the device scale on layerTreeHost?)
> 
> Yeh this is kind of bogus. If we had a RLCompositor and a GraphicsLayer, then it would be == deviceScaleFactor() due to GLC::contentsScale(). I should probably just remove these checks, it is not what the test is checking for. It is supposed to be testing the resulting drawTransforms etc.
> 
> Aura is doing something of its own. It makes compositor layers in physical pixels all AIUI, just using larger sizes and assets instead.

Ok, thanks for the update on how Aura is handling this.  It's not unreasonable to have the semantics for changing deviceScale to mean that a compositor automatically has its content scaled from the original DIP contents unless it explicitly provides a different content scale.  And, jamesr just added contentsScale to WebContentLayer, so that answers that question too.


On an unrelated note, I need some convincing about how device scale works for events.  It looks like CCLayerTreeHostImpl::scrollBegin assumes that WebInputEvent positions are in device viewport space since it uses the screen space draw transform inverse for mapping, but some of the event handling in WebViewImpl looks like it is assuming DIP viewport space.  I'm not sure what the browser process is sending, but it seems like one of these needs to change.

[Alexandre Elias]

Comment on attachment 141825 [details]
Patch

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

OK, I tested the patch out on the Android branch and discovered some bugs, here are some comments to address them.

> Source/WebKit/chromium/src/ChromeClientImpl.cpp:647
> +    if (!m_webView->isFixedLayoutModeEnabled()) {

Now that it's untied from pinch zoom and viewport tag, this isn't a very natural place to put this; it will be called multiple times all over initialization and on resizes, and potentially it can happen in our fixed-layout configuration if the WebKit side initializes too early. I'd suggest putting it somewhere that's only called once after fixed layout is guaranteed to have been initialized (or not), such as RenderViewImpl::ProcessViewLayoutFlags.

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

Please add a clause:
else
   m_layerTreeView.setDeviceScaleFactor(1);

so that we are not stuck with the deviceScaleFactor if fixed-layout mode is turned on a bit late in initialization.

>>> Source/WebKit/chromium/src/WebViewImpl.cpp:3481
>>> +    float pageScale = 1;
>> 
>> There is no longer any code here to set pageScale to non-1.
> 
> There never was any value except 1 used for the NCCH. It was just super unclear before, unless I am confused somewhere?
> 
> It used to pass pageScale in here. Then say "if pageScale != pageScaleFactor(), displayOrPageScaleFactorChanged()". But NCCH always returns "1" from its pageScaleFactor() method.
> 
> Now we pass in "1" explicitly so that it is more clear what is going on, and we don't need to call displayOrPageScaleFactorChanged() when the page scale is not changing in the NCCH.

OK, but now displayOrPageScaleFactorChanged() is never getting called on pageScaleFactor changes, and we do rely on that for pinch zoom.  I also tried setting pageScale to pageScaleFactor() here to address that, but the non-1 return value is also causing a weird bug I don't fully understand yet (blank screen when maximally pinch-zoomed in).

Could we remove the NCCH-related changes from this patch?  It looks like unrelated cleanup and I'd rather deal with it separately.

[Dana Jansens]

(In reply to comment #36)
> (From update of attachment 141825 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=141825&action=review
> 
> OK, I tested the patch out on the Android branch and discovered some bugs, here are some comments to address them.
> 
> > Source/WebKit/chromium/src/ChromeClientImpl.cpp:647
> > +    if (!m_webView->isFixedLayoutModeEnabled()) {
> 
> Now that it's untied from pinch zoom and viewport tag, this isn't a very natural place to put this; it will be called multiple times all over initialization and on resizes, and potentially it can happen in our fixed-layout configuration if the WebKit side initializes too early. I'd suggest putting it somewhere that's only called once after fixed layout is guaranteed to have been initialized (or not), such as RenderViewImpl::ProcessViewLayoutFlags.
> 
> > Source/WebKit/chromium/src/WebViewImpl.cpp:2358
> > +        m_layerTreeView.setDeviceScaleFactor(scaleFactor);
> 
> Please add a clause:
> else
>    m_layerTreeView.setDeviceScaleFactor(1);

Ok great, I had no idea where else might even be appropriate. If it moves, then the else clause isn't required anymore right?

> so that we are not stuck with the deviceScaleFactor if fixed-layout mode is turned on a bit late in initialization.
> 
> >>> Source/WebKit/chromium/src/WebViewImpl.cpp:3481
> >>> +    float pageScale = 1;
> >> 
> >> There is no longer any code here to set pageScale to non-1.
> > 
> > There never was any value except 1 used for the NCCH. It was just super unclear before, unless I am confused somewhere?
> > 
> > It used to pass pageScale in here. Then say "if pageScale != pageScaleFactor(), displayOrPageScaleFactorChanged()". But NCCH always returns "1" from its pageScaleFactor() method.
> > 
> > Now we pass in "1" explicitly so that it is more clear what is going on, and we don't need to call displayOrPageScaleFactorChanged() when the page scale is not changing in the NCCH.
> 
> OK, but now displayOrPageScaleFactorChanged() is never getting called on pageScaleFactor changes, and we do rely on that for pinch zoom.  I also tried setting pageScale to pageScaleFactor() here to address that, but the non-1 return value is also causing a weird bug I don't fully understand yet (blank screen when maximally pinch-zoomed in).

So displayOrPageScaleFactorChanged() should be called even when pageScaleFactor() doesn't change, but the contentsSize() do I guess! Will fix.

> Could we remove the NCCH-related changes from this patch?  It looks like unrelated cleanup and I'd rather deal with it separately.

Not entirely.. It needs to return a deviceScaleFactor in order to have the contents scaled properly when not in fixed-layout mode (since contentsSize() is only scaled by pageScaleFactor).

[Dana Jansens]

(In reply to comment #35)
> On an unrelated note, I need some convincing about how device scale works for events.  It looks like CCLayerTreeHostImpl::scrollBegin assumes that WebInputEvent positions are in device viewport space since it uses the screen space draw transform inverse for mapping, but some of the event handling in WebViewImpl looks like it is assuming DIP viewport space.  I'm not sure what the browser process is sending, but it seems like one of these needs to change.

I tested and played with this a lot but yeh, I will spend some time convincing myself today as well.

[Alexandre Elias]

(In reply to comment #37)
> > > Source/WebKit/chromium/src/WebViewImpl.cpp:2358
> > > +        m_layerTreeView.setDeviceScaleFactor(scaleFactor);
> > 
> > Please add a clause:
> > else
> >    m_layerTreeView.setDeviceScaleFactor(1);
> 
> Ok great, I had no idea where else might even be appropriate. If it moves, then the else clause isn't required anymore right?

Correct, but I would still prefer to have the else clause there for safety, as relying on initialization order can be brittle.

> 
> > so that we are not stuck with the deviceScaleFactor if fixed-layout mode is turned on a bit late in initialization.
> > 
> > >>> Source/WebKit/chromium/src/WebViewImpl.cpp:3481
> > >>> +    float pageScale = 1;
> > >> 
> > >> There is no longer any code here to set pageScale to non-1.
> > > 
> > > There never was any value except 1 used for the NCCH. It was just super unclear before, unless I am confused somewhere?
> > > 
> > > It used to pass pageScale in here. Then say "if pageScale != pageScaleFactor(), displayOrPageScaleFactorChanged()". But NCCH always returns "1" from its pageScaleFactor() method.
> > > 
> > > Now we pass in "1" explicitly so that it is more clear what is going on, and we don't need to call displayOrPageScaleFactorChanged() when the page scale is not changing in the NCCH.
> > 
> > OK, but now displayOrPageScaleFactorChanged() is never getting called on pageScaleFactor changes, and we do rely on that for pinch zoom.  I also tried setting pageScale to pageScaleFactor() here to address that, but the non-1 return value is also causing a weird bug I don't fully understand yet (blank screen when maximally pinch-zoomed in).
> 
> So displayOrPageScaleFactorChanged() should be called even when pageScaleFactor() doesn't change, but the contentsSize() do I guess! Will fix.

It needs to be called only when pageScaleFactor changes.  Previously it was called too much, almost all the time (as pageScaleFactor is almost never exactly 1 on Clank), but in your current patch it is never called in fixed-layout mode, which is worse.

> 
> > Could we remove the NCCH-related changes from this patch?  It looks like unrelated cleanup and I'd rather deal with it separately.
> 
> Not entirely.. It needs to return a deviceScaleFactor in order to have the contents scaled properly when not in fixed-layout mode (since contentsSize() is only scaled by pageScaleFactor).

OK.  Then I would be fine with keeping your current patch, but setting pageScale = pageScaleFactor in WebViewImpl and changing NCCH::pageScaleFactor() to always return 1 as before.

[Alexandre Elias]

> > > Could we remove the NCCH-related changes from this patch?  It looks like unrelated cleanup and I'd rather deal with it separately.
> > 
> > Not entirely.. It needs to return a deviceScaleFactor in order to have the contents scaled properly when not in fixed-layout mode (since contentsSize() is only scaled by pageScaleFactor).
> 
> OK.  Then I would be fine with keeping your current patch, but setting pageScale = pageScaleFactor in WebViewImpl and changing NCCH::pageScaleFactor() to always return 1 as before.

On second thought, even better would be just to remove all lines in NCCH and updateLayerTreeViewport with the word "pageScale" entirely, as that's hard for you to test for yourself if any changes with are correct, and I don't think you need to touch any of it (since it will always be 1 in your world assuming no pinch zoom).  I would like for this patch to be 100% no-op in fixed-layout mode.

[Dana Jansens]

Created attachment 142122 [details]
Patch

Alright, this is getting into a good state now. Testing with pinch/scroll on device.

There's some issues with partial swap: http://code.google.com/p/chromium/issues/detail?id=128268

- Scroll and pinch seem to be working well! Scaling things from physical pixels to DIP when making scroll positions.
- Returning 1 as pageScaleFactor() on NCCH always, but calling dOrPScaleFactorChanged() when it is changed in the WebView.
- Moving the code to set the deviceScaleFactor out of ChromeClientImpl and into RenderViewImpl in chromium (separate CL). It will happen in the same place that fixed-layout mode is enabled/disabled. This makes lots of sense since setting of the deviceScaleFactor in this way is tied to not being in fixed-layout mode.

[Dana Jansens]

FYI https://chromiumcodereview.appspot.com/10386160/

[Alexandre Elias]

Comment on attachment 142122 [details]
Patch

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

The logic is looking good now, I'll give it another spin on Android tomorrow to make sure, but I don't expect any more bugs with this.

> Source/WebKit/chromium/src/WebViewImpl.cpp:2362
> +    printf("page %ld\n", (long)page());

Leftover debug code

> Source/WebKit/chromium/src/WebViewImpl.cpp:2369
> +    if (!m_layerTreeView.isNull() && !deviceScaleFactorIsComponentOfPageScaleFactor)

I'm still thinking we want an else block setting 1 here... particularly now that you're considering dynamically turning fixed-layout on and off with viewport tag.  It can't hurt to have it.

> Source/WebKit/chromium/src/WebViewImpl.cpp:2396
> +    if (!enable) {

Leftover debug code

[James Robinson]

Comment on attachment 142122 [details]
Patch

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

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

> Source/WebKit/chromium/src/NonCompositedContentHost.cpp:42
> +    , m_pageScaleFactor(1.0)

why would NonCompositedContentHost have a pageScaleFactor? that doesn't seem necessary

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

> Source/WebKit/chromium/src/NonCompositedContentHost.h:69
> +    // The pageScaleFactor is always baked into the GraphicsLayer's size, so its scale factor should always be 1.
> +    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.

> Source/WebKit/chromium/src/NonCompositedContentHost.h:89
> +    float m_pageScaleFactor;

remove

> Source/WebKit/chromium/src/WebViewImpl.cpp:2362
> +    printf("page %ld\n", (long)page());

doubt you meant to check this in

> Source/WebKit/chromium/src/WebViewImpl.cpp:2369
> +    if (!m_layerTreeView.isNull() && !deviceScaleFactorIsComponentOfPageScaleFactor)

I think that if possible we should enforce that the device scale is set before any other initialization happens and ASSERT() here.  If we can't, i'd like to know why this is further upstream.

It seems weird to have device scale be a setter on WebView instead of a getter on the client, but oh well

> Source/WebKit/chromium/src/WebViewImpl.cpp:2397
> +    if (!enable) {
> +    }

huh?

> Source/WebKit/chromium/src/WebViewImpl.cpp:-2391
> -        m_nonCompositedContentHost->topLevelRootLayer()->deviceOrPageScaleFactorChanged();

are you deleting this call or did you move it somewhere else?

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

> Source/WebKit/chromium/src/WebViewImpl.cpp:3495
> +    // When in fixed-layout mode, the deviceScale is baked into pageScaleFactor,
> +    // so it should be treated as 1.

aelias@ - can you check on this?  are you setting deviceScale to non-1.0 in Android, and if so is this comment write?

as a purely style thing I think it'd be better to initialize deviceScale when you declare it and then override it later if you want, so it's obvious that it's always initialized and not stack garbage

[Alexandre Elias]

(In reply to comment #44)
> (From update of attachment 142122 [details])
> 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...

> > Source/WebKit/chromium/src/WebViewImpl.cpp:2369
> > +    if (!m_layerTreeView.isNull() && !deviceScaleFactorIsComponentOfPageScaleFactor)
> 
> I think that if possible we should enforce that the device scale is set before any other initialization happens and ASSERT() here.  If we can't, i'd like to know why this is further upstream.

Yeah, this kind of strict enforcement would be an alternative to the else block I suggested.

> 
> It seems weird to have device scale be a setter on WebView instead of a getter on the client, but oh well

We have such a client getter in Android branch, I'll make a note to push it upstream soon.

> 
> > Source/WebKit/chromium/src/WebViewImpl.cpp:3495
> > +    // When in fixed-layout mode, the deviceScale is baked into pageScaleFactor,
> > +    // so it should be treated as 1.
> 
> aelias@ - can you check on this?  are you setting deviceScale to non-1.0 in Android, and if so is this comment write?

On Android, the deviceScale that will make it to the compositor will always be 1.0.  But we do have some concept of device scale factor that's folded into page scale.

[James Robinson]

(In reply to comment #45)
> (In reply to comment #44)
> > (From update of attachment 142122 [details] [details])
> > 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.

[Dana Jansens]

Comment on attachment 142122 [details]
Patch

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.

[Dana Jansens]

Comment on attachment 142122 [details]
Patch

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.

I find this problematic actually. The LayerTreeView is initialized before WebViewImpl knows if it will be in fixed layout mode or not. So it doesn't know if it should be giving the defaultDeviceScaleFactor to the LayerTreeView or not at initialization time.

[James Robinson]

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

I am sure and I do want you to break it.

[Dana Jansens]

(In reply to comment #49)
> > > 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.
> 
> I am sure and I do want you to break it.

Ok maybe the guard's purpose is not clear. I think that we need it here regardless of pageScale==1. What we have is something like:

pageScale = 1
deviceScale = 2

There are a few places in WebViewImpl that multiply by the deviceScale because they assume that pageScale is scaled by it. When we are not using fixed-layout, then this assumption is false.

If we want to limit pageScale to always be 1, then we would set the limits to [1,1], not [1*deviceScale, 1*deviceScale].

[James Robinson]

Maybe the problem is just the name - as implemented, the limits appear to actually be limits on pageScale * deviceScale.  When dynamically changing the pageScale it makes sense the combination (since that's what the user sees).

In this case the deviceScale is something non-1 but we aren't changing the pageScale, so it seems that we shouldn't have to worry about these limits.  I think the intent of the [1,1] limit is to clamp pageScale, not really both.  The code isn't super clear here.  I'm not sure how to implement this.

[Alexandre Elias]

I'm okay with the change in computePageScaleFactorLimits if nothing else because it's logically consistent with the transition in the rest of the code, even if currently an unused codepath in non-fixed-layout.  As you suggest, in fixed-layout the value of pageScaleFactor always represents the product of viewport-tag-space-scale * deviceScaleFactor, whereas in the new non-fixedLayout world they are separate values and only implicitly multiplied at rendering time.

[Alexandre Elias]

Also, if we choose to disable pinch zoom, then I think it makes sense to set the limits to [1,1] explicitly instead of ignoring the whole question.  We may eventually find our pageScale accidentally changed on desktop from some random code that calls setPageScaleFactor -- some example scenarios would be a HistoryItem synced from a mobile device, or find-in-page logic upstreamed from Android (where we tend to call setPageScaleFactor to change scroll offset and scale in one step).

[Dana Jansens]

Created attachment 142387 [details]
Patch

Alright new patch!

- The deviceScaleFactor in the compositor is set during initialization through CCSettings.
- We save this deviceScaleFactor in WebViewImpl as m_deviceScaleInCompositor. Then we can do deviceScaleFactor()/m_deviceScaleInCompositor, and that tells us the deviceScaleFactor component of the pageScaleFactor. Right now that's going to be either equal to 1 (on ChromeOS) or deviceScaleFactor() (on Android).
- Also the above lets us get rid of all the if (isFixedLayoutModeEnabled()) crap.
- NCCH is to its GraphicsLayer what RLB is to other GraphicsLayers. It should know the scale at which the contents of the GraphicsLayer will be displayed, so I am keeping the pageScale and deviceScale factors in the NCCH, so it knows when to tell the GraphicsLayer things changed, and so it can give the deviceScale and pageScale (1) that it should use.

[James Robinson]

Comment on attachment 142387 [details]
Patch

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

> Source/WebKit/chromium/src/NonCompositedContentHost.h:79
> +    // size, so it is always 1 for the GraphicsLayer.

so if it's always 1, why do we have a m_pageScaleFactor member variable on NCCH? I can't find anything that uses it.

[James Robinson]

WebViewImpl has to track changes to pageScale, so I think it should send the deviceOrPageScaleChanged notification like it used to.  It's a bit silly to have NCCH track pageScale only to do notifications since it's not otherwise going to use it.

[Dana Jansens]

(In reply to comment #56)
> WebViewImpl has to track changes to pageScale, so I think it should send the deviceOrPageScaleChanged notification like it used to.  It's a bit silly to have NCCH track pageScale only to do notifications since it's not otherwise going to use it.

Oh.. What I was trying to do was to only set the notification and make us invalidate the layer when the page scale has changed, rather than every time we come to this code. But I guess we can tell if things changed in the WebViewImpl also, and make the calls from there. Okay I'll move that over tomorrow, thanks.

[Dana Jansens]

Created attachment 142533 [details]
Patch

Removed the pageScaleFactor from NCCH. It called deviceOrPageScaleFactorChanged() whenever its viewport is set, as it did before.

Doing this from WebViewImpl instead doesn't buy us anything really.. we should do in order:
1. change the deviceScaleFactor() reported from the NCCH
2. report the change to the GraphicsLayer
3. change the size of the GraphicsLayer

1 and 3 happen in NCCH::setViewport, so it's simplest to do 2 there as well. Else we have to add API to make 1 happen from WebViewImpl separately.

[Dana Jansens]

Created attachment 142559 [details]
Patch

rebased.

[James Robinson]

Comment on attachment 142559 [details]
Patch

This looks good to me.  I'm sorry we had to go so many rounds on this :/

[Dana Jansens]

Yay, thanks!

[Alexandre Elias]

Please hold off a bit before committing, I'll give it another test run on Android.

[Dana Jansens]

Excellent thanks! Waiting for EWS as well

[Alexandre Elias]

I ran into an issue in the Clank branch where defaultDeviceScaleFactor setting is currently zero, probably due to a not-yet-merged patch related to that.  I'll force it to 1 on Android, but please add an ASSERT that it's not zero, otherwise the symptom is a black screen.  Also make sure this ASSERT doesn't fire on Mac and Windows (I tested Linux, that one's fine).

LGTM otherwise.

[Alexandre Elias]

Comment on attachment 142559 [details]
Patch

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

Sorry, I have more review comments after all.

The main thing is, looking further at all the implications, it will be really confusing to everyone to have a deviceScaleFactor value which is sometimes 1 on Android.  It would be much better to plumb the true value everywhere.  Could you introduce a new setting compositorAppliesDeviceScaleFactor?  Then add an if() for that setting around all the relevant scaling operations.

> Source/WebKit/chromium/src/WebViewImpl.cpp:399
> +    , m_deviceScaleInCompositor(1)

Then with the other setting compositorAppliesDeviceScale(), we can just remove this and use the original settings value of defaultDeviceScaleFactor instead.

> Source/WebKit/chromium/src/WebViewImpl.cpp:NaN
>  void WebViewImpl::computeScaleAndScrollForHitRect(const WebRect& hitRect, AutoZo

You can leave this function unchanged, it's Android-specific and we'll upstream different stuff later anyway.

[Dana Jansens]

Created attachment 142600 [details]
Patch

- Gating scaling in the compositor on a new Settings::applyDefaultDeviceScaleFactorInCompositor().
- Remove changes to computeScaleAndScrollForHitRect.
- Fix up/remove changes to unit tests after the above gating.

@jamesr Can you please verify you agree with this change? The small diff is http://pastebin.com/FtghKdvc

[Alexandre Elias]

That looks great, thanks.  Now Android can use the same --default-device-scale-factor flag and setting that other platforms do and we can remove our forked plumbing from other places.

[James Robinson]

Comment on attachment 142600 [details]
Patch

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

> Source/WebCore/page/Settings.cpp:133
> +    , m_applyDefaultDeviceScaleFactorInCompositor(false)

you should not have to touch the cross-platform WebCore::Settings for this, this is just a chromium thing.

Plumb it through the WebSettings API - WebViewImpl has access to that.

[Dana Jansens]

(In reply to comment #68)
> you should not have to touch the cross-platform WebCore::Settings for this, this is just a chromium thing.
> 
> Plumb it through the WebSettings API - WebViewImpl has access to that.

Oh! Yes thanks.

[Dana Jansens]

Created attachment 142607 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 142607 [details]
Patch for landing

Clearing flags on attachment: 142607

Committed r117535: <http://trac.webkit.org/changeset/117535>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.