Bug 95094 – [Chromium] Remove contentsScale from GraphicsLayerChromium and WebContentLayer

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[Jeff Timanus]

Issue tracking refactoring work to remove the contentsScale behaviour from GraphicsLayerChromium and WebContentsLayer.

The contents scale code is being relocated to the CCLayerTreeHostClass, in preparation for a page-scale-factor-driven pinch zoom implementation.

See related discussion on bug:  https://bugs.webkit.org/show_bug.cgi?id=93292

[Jeff Timanus]

Created attachment 160764 [details]
First patch to observe EWS bots.

[Jeff Timanus]

Created attachment 161337 [details]
Piping appliesPageScale to LayerChromium from GraphicsLayer.

This patch implements the basic idea of how we can move the contentsScale code from GraphicsLayer{Chromium} to LayerChromium.

Essentially this change removes the assignment of contents scale at the GraphicsLayer level, and instead performs it during CCLayerTreeHost::updateLayers.

The GraphicsLayer::appliesPageScale property is pushed to the LayerChromium owned by the GraphicsLayer, and used to control the page scale semantics at the LayerChromium level.

There are a few issues I wonder about with this change:
 - I'm currently assigning the page scale to all LayerChromium instances in the tree, where as the old path only assigned the scale to the layer associated with the path.  I had been hoping that the appliesPageScale selective application of the contentsScale would filter out layers that should not be scaled.
 - There are several layout tests that fail with a result of this patch.  (Attachment forthcoming.)

Is this approach at all sane?  I feel that the complexity of all the different zoom/scale factors is an indication that this is not the right direction.

[Jeff Timanus]

Created attachment 161346 [details]
Failing layout tests from patch 161337.

I'm trying to understand the nature of the failures captured in these test results.

In particular, the compositing/geometry failures indicate problems with:
 - Scroll bars are not being resized to compensate for the new scale factor.  I'm presuming that this is a timing issue.  The scale factor is being installed such that it is not caught by the scroll bar code.
 - The root layer seems to have a small scale problem, as shown by the slight text differences.

Any help or intuition on either of these problems would be appreciated.

[Dana Jansens]

Comment on attachment 161337 [details]
Piping appliesPageScale to LayerChromium from GraphicsLayer.

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

Re the failures.

Tests with a pageScale > 1 have a contentsScale on the NCCH that is too large (scrollbars get smaller). Tests with a pageScale < 1 have a contentsScale on the NCCH that is too small (scollbars get larger). So I suspect the NCCH GraphicsLayer does not have setAppliesPageScale set on it, and is being scaled in terms of bounds, and then scaled again by contentsScale.

> WebCore/platform/graphics/chromium/LayerChromium.cpp:93
> +    , m_appliesPageScale(false)

I would personally like to see this get a more descriptive name in LayerChromium cuz this name has always been so opaque. GraphicsLayerChromium can do the translation from one name to the other.

> WebCore/platform/graphics/chromium/LayerChromium.cpp:630
> +    if (appliesPageScale())
> +        return;

If applies page scale is something the API lets you change, maybe drop this piece in setContentsScale. Then if you setAppliesPS(false) you will have the right contents scale already there.

> WebCore/platform/graphics/chromium/LayerChromium.cpp:640
> +    m_appliesPageScale = appliesScale;

early-out if matches already. setNeedsDisplay() otherwise.

> WebCore/platform/graphics/chromium/LayerChromium.cpp:646
> +bool LayerChromium::appliesPageScale() const
> +{
> +    return m_appliesPageScale;
> +}

move to header

> WebCore/platform/graphics/chromium/cc/CCLayerTreeHost.cpp:481
> +static void updateLayerScale(LayerChromium* layer, float pageScale)

nit: rename pageScale to something like "scaleFactor"? it's the product of all scale factors.

> WebCore/platform/graphics/chromium/cc/CCLayerTreeHost.cpp:486
> +    Vector<RefPtr<LayerChromium> >::const_iterator iter(children.begin()), end(children.end());
> +    for (; iter != end; ++iter) {

Style guide says to use "for (i = 0; i < children.size(); ++i)" instead of iterators here.

> WebCore/platform/graphics/chromium/GraphicsLayerChromium.cpp:815
>  void GraphicsLayerChromium::deviceOrPageScaleFactorChanged()

Can this function go away entirely? We should be invalidating everything when we setContentsScale() on the layer with a new value.

[Dana Jansens]

Comment on attachment 161337 [details]
Piping appliesPageScale to LayerChromium from GraphicsLayer.

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

> WebCore/platform/graphics/chromium/cc/CCLayerTreeHost.cpp:497
> +    updateLayerScale(rootLayer, m_pageScaleFactor * m_deviceScaleFactor);

Design-wise, I think you could call layer->setContentsScale() just before painting each layer in paintLayerContents() and paintMasksForRenderSurface() and not require an extra tree-walk at all.

Also, this walk is missing masks layers (see paintMasksForRenderSurface()), which I think we want contentsScale applied to as well so they are nice and crisp! (DSF or PSF layout test for this?)

[Jeff Timanus]

(In reply to comment #4)
> (From update of attachment 161337 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=161337&action=review
> 
> Re the failures.
> 
> Tests with a pageScale > 1 have a contentsScale on the NCCH that is too large (scrollbars get smaller). Tests with a pageScale < 1 have a contentsScale on the NCCH that is too small (scollbars get larger). So I suspect the NCCH GraphicsLayer does not have setAppliesPageScale set on it, and is being scaled in terms of bounds, and then scaled again by contentsScale.

I think your haunch is correct.  As an experiment, I tried assigning setAppliesPageScale() in the NCCH constructor.  This corrects the problematic behaviour wrt the scroll bars in chrome, but the same layout test failures are present.

Is the NCCH handled differently in DRT?

> 
> > WebCore/platform/graphics/chromium/LayerChromium.cpp:93
> > +    , m_appliesPageScale(false)
> 
> I would personally like to see this get a more descriptive name in LayerChromium cuz this name has always been so opaque. GraphicsLayerChromium can do the translation from one name to the other.
> 
> > WebCore/platform/graphics/chromium/LayerChromium.cpp:630
> > +    if (appliesPageScale())
> > +        return;
> 
> If applies page scale is something the API lets you change, maybe drop this piece in setContentsScale. Then if you setAppliesPS(false) you will have the right contents scale already there.
> 
> > WebCore/platform/graphics/chromium/LayerChromium.cpp:640
> > +    m_appliesPageScale = appliesScale;
> 
> early-out if matches already. setNeedsDisplay() otherwise.
> 
> > WebCore/platform/graphics/chromium/LayerChromium.cpp:646
> > +bool LayerChromium::appliesPageScale() const
> > +{
> > +    return m_appliesPageScale;
> > +}
> 
> move to header
> 
> > WebCore/platform/graphics/chromium/cc/CCLayerTreeHost.cpp:481
> > +static void updateLayerScale(LayerChromium* layer, float pageScale)
> 
> nit: rename pageScale to something like "scaleFactor"? it's the product of all scale factors.
> 
> > WebCore/platform/graphics/chromium/cc/CCLayerTreeHost.cpp:486
> > +    Vector<RefPtr<LayerChromium> >::const_iterator iter(children.begin()), end(children.end());
> > +    for (; iter != end; ++iter) {
> 
> Style guide says to use "for (i = 0; i < children.size(); ++i)" instead of iterators here.
> 
> > WebCore/platform/graphics/chromium/GraphicsLayerChromium.cpp:815
> >  void GraphicsLayerChromium::deviceOrPageScaleFactorChanged()
> 
> Can this function go away entirely? We should be invalidating everything when we setContentsScale() on the layer with a new value.

[Jeff Timanus]

(In reply to comment #6)
> (In reply to comment #4)
> > (From update of attachment 161337 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=161337&action=review
> > 
> > Re the failures.
> > 
> > Tests with a pageScale > 1 have a contentsScale on the NCCH that is too large (scrollbars get smaller). Tests with a pageScale < 1 have a contentsScale on the NCCH that is too small (scollbars get larger). So I suspect the NCCH GraphicsLayer does not have setAppliesPageScale set on it, and is being scaled in terms of bounds, and then scaled again by contentsScale.
> 
> I think your haunch is correct.  As an experiment, I tried assigning setAppliesPageScale() in the NCCH constructor.  This corrects the problematic behaviour wrt the scroll bars in chrome, but the same layout test failures are present.
> 
> Is the NCCH handled differently in DRT?

Nevermind, I had accidentally run a stale build.  Setting setAppliesPageScale in the constructor of NCCH fixes the layout test failures.

However, other tests fail with what appears to be unrelated output.  Text diff here from failure in compositing/geometry/outline-change:

--- /usr/local/google/home/twiz/src/src1/src/webkit/Release/layout-test-results/compositing/geometry/outline-change-expected.txt 
+++ /usr/local/google/home/twiz/src/src1/src/webkit/Release/layout-test-results/compositing/geometry/outline-change-actual.txt 
@@ -1,3 +1,13 @@
+** (DumpRenderTree:14063): DEBUG: 0xd2a500: "NewStream mimetype 'video/mp4' URL 'file:///usr/local/google/home/twiz/src/src1/src/third_party/WebKit/LayoutTests/compositing/resources/video.mp4'"
+** (DumpRenderTree:14063): DEBUG: 0xd2a500: "Not expecting a new stream; aborting stream"
+** (DumpRenderTree:14063): DEBUG: 0xd2a500: "Viewer DBus interface name is 'org.gnome.totem.PluginViewer_14074'"
+** (DumpRenderTree:14063): DEBUG: 0xd2a500: "NameOwnerChanged old-owner '' new-owner ':1.327'"
+** (DumpRenderTree:14063): DEBUG: 0xd2a500: "Viewer now connected to the bus"
+** (DumpRenderTree:14063): DEBUG: 0xd2a500: "ViewerSetup"
+** (DumpRenderTree:14063): DEBUG: 0xd2a500: "Calling SetWindow"
+Viewer: SetWindow XID 88080418 size 100:100
+** (DumpRenderTree:14063): DEBUG: ~totemPlugin [0xd2a500]
+** (DumpRenderTree:14063): DEBUG: NP_Shutdown
 layer at (0,0) size 800x600
   RenderView at (0,0) size 800x600
 layer at (0,0) size 800x256

> 
> > 
> > > WebCore/platform/graphics/chromium/LayerChromium.cpp:93
> > > +    , m_appliesPageScale(false)
> > 
> > I would personally like to see this get a more descriptive name in LayerChromium cuz this name has always been so opaque. GraphicsLayerChromium can do the translation from one name to the other.
> > 
> > > WebCore/platform/graphics/chromium/LayerChromium.cpp:630
> > > +    if (appliesPageScale())
> > > +        return;
> > 
> > If applies page scale is something the API lets you change, maybe drop this piece in setContentsScale. Then if you setAppliesPS(false) you will have the right contents scale already there.
> > 
> > > WebCore/platform/graphics/chromium/LayerChromium.cpp:640
> > > +    m_appliesPageScale = appliesScale;
> > 
> > early-out if matches already. setNeedsDisplay() otherwise.
> > 
> > > WebCore/platform/graphics/chromium/LayerChromium.cpp:646
> > > +bool LayerChromium::appliesPageScale() const
> > > +{
> > > +    return m_appliesPageScale;
> > > +}
> > 
> > move to header
> > 
> > > WebCore/platform/graphics/chromium/cc/CCLayerTreeHost.cpp:481
> > > +static void updateLayerScale(LayerChromium* layer, float pageScale)
> > 
> > nit: rename pageScale to something like "scaleFactor"? it's the product of all scale factors.
> > 
> > > WebCore/platform/graphics/chromium/cc/CCLayerTreeHost.cpp:486
> > > +    Vector<RefPtr<LayerChromium> >::const_iterator iter(children.begin()), end(children.end());
> > > +    for (; iter != end; ++iter) {
> > 
> > Style guide says to use "for (i = 0; i < children.size(); ++i)" instead of iterators here.
> > 
> > > WebCore/platform/graphics/chromium/GraphicsLayerChromium.cpp:815
> > >  void GraphicsLayerChromium::deviceOrPageScaleFactorChanged()
> > 
> > Can this function go away entirely? We should be invalidating everything when we setContentsScale() on the layer with a new value.

[Dana Jansens]

(In reply to comment #7)
> (In reply to comment #6)
> > (In reply to comment #4)
> > > (From update of attachment 161337 [details] [details] [details])
> > > View in context: https://bugs.webkit.org/attachment.cgi?id=161337&action=review
> > > 
> > > Re the failures.
> > > 
> > > Tests with a pageScale > 1 have a contentsScale on the NCCH that is too large (scrollbars get smaller). Tests with a pageScale < 1 have a contentsScale on the NCCH that is too small (scollbars get larger). So I suspect the NCCH GraphicsLayer does not have setAppliesPageScale set on it, and is being scaled in terms of bounds, and then scaled again by contentsScale.
> > 
> > I think your haunch is correct.  As an experiment, I tried assigning setAppliesPageScale() in the NCCH constructor.  This corrects the problematic behaviour wrt the scroll bars in chrome, but the same layout test failures are present.
> > 
> > Is the NCCH handled differently in DRT?
> 
> Nevermind, I had accidentally run a stale build.  Setting setAppliesPageScale in the constructor of NCCH fixes the layout test failures.

Cool!

> However, other tests fail with what appears to be unrelated output.  Text diff here from failure in compositing/geometry/outline-change:
> 
> --- /usr/local/google/home/twiz/src/src1/src/webkit/Release/layout-test-results/compositing/geometry/outline-change-expected.txt 
> +++ /usr/local/google/home/twiz/src/src1/src/webkit/Release/layout-test-results/compositing/geometry/outline-change-actual.txt 
> @@ -1,3 +1,13 @@
> +** (DumpRenderTree:14063): DEBUG: 0xd2a500: "NewStream mimetype 'video/mp4' URL 'file:///usr/local/google/home/twiz/src/src1/src/third_party/WebKit/LayoutTests/compositing/resources/video.mp4'"
> +** (DumpRenderTree:14063): DEBUG: 0xd2a500: "Not expecting a new stream; aborting stream"
> +** (DumpRenderTree:14063): DEBUG: 0xd2a500: "Viewer DBus interface name is 'org.gnome.totem.PluginViewer_14074'"
> +** (DumpRenderTree:14063): DEBUG: 0xd2a500: "NameOwnerChanged old-owner '' new-owner ':1.327'"
> +** (DumpRenderTree:14063): DEBUG: 0xd2a500: "Viewer now connected to the bus"
> +** (DumpRenderTree:14063): DEBUG: 0xd2a500: "ViewerSetup"
> +** (DumpRenderTree:14063): DEBUG: 0xd2a500: "Calling SetWindow"
> +Viewer: SetWindow XID 88080418 size 100:100
> +** (DumpRenderTree:14063): DEBUG: ~totemPlugin [0xd2a500]
> +** (DumpRenderTree:14063): DEBUG: NP_Shutdown
>  layer at (0,0) size 800x600
>    RenderView at (0,0) size 800x600
>  layer at (0,0) size 800x256

That doesn't sound like it's your patch. Does the same output occur without your patch?

[Jeff Timanus]

Created attachment 161550 [details]
Addressing code review comments.

[Jeff Timanus]

Comment on attachment 161337 [details]
Piping appliesPageScale to LayerChromium from GraphicsLayer.

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

>>>> WebCore/platform/graphics/chromium/LayerChromium.cpp:93
>>>> +    , m_appliesPageScale(false)
>>> 
>>> I would personally like to see this get a more descriptive name in LayerChromium cuz this name has always been so opaque. GraphicsLayerChromium can do the translation from one name to the other.
>> 
>> 
> 
> 

Done.

Renamed to m_disregardsContentsScale.  Also renamed the accessor routines.

>> WebCore/platform/graphics/chromium/LayerChromium.cpp:630
>> +        return;
> 
> If applies page scale is something the API lets you change, maybe drop this piece in setContentsScale. Then if you setAppliesPS(false) you will have the right contents scale already there.

Is this safe?  I'm concerned about spurious setNeedsDisplay calls.

>> WebCore/platform/graphics/chromium/LayerChromium.cpp:640
>> +    m_appliesPageScale = appliesScale;
> 
> early-out if matches already. setNeedsDisplay() otherwise.

Done.

>> WebCore/platform/graphics/chromium/LayerChromium.cpp:646
>> +}
> 
> move to header

Done.

>> WebCore/platform/graphics/chromium/cc/CCLayerTreeHost.cpp:481
>> +static void updateLayerScale(LayerChromium* layer, float pageScale)
> 
> nit: rename pageScale to something like "scaleFactor"? it's the product of all scale factors.

Done.

>> WebCore/platform/graphics/chromium/cc/CCLayerTreeHost.cpp:497
>> +    updateLayerScale(rootLayer, m_pageScaleFactor * m_deviceScaleFactor);
> 
> Design-wise, I think you could call layer->setContentsScale() just before painting each layer in paintLayerContents() and paintMasksForRenderSurface() and not require an extra tree-walk at all.
> 
> Also, this walk is missing masks layers (see paintMasksForRenderSurface()), which I think we want contentsScale applied to as well so they are nice and crisp! (DSF or PSF layout test for this?)

Done.

>> WebCore/platform/graphics/chromium/GraphicsLayerChromium.cpp:815
>>  void GraphicsLayerChromium::deviceOrPageScaleFactorChanged()
> 
> Can this function go away entirely? We should be invalidating everything when we setContentsScale() on the layer with a new value.

I can't speak with authority, but is calling Layer::setNeedsDisplay sufficient to perform these invalidations?

[Dana Jansens]

Comment on attachment 161337 [details]
Piping appliesPageScale to LayerChromium from GraphicsLayer.

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

>>> WebCore/platform/graphics/chromium/LayerChromium.cpp:630
>>> +        return;
>> 
>> If applies page scale is something the API lets you change, maybe drop this piece in setContentsScale. Then if you setAppliesPS(false) you will have the right contents scale already there.
> 
> Is this safe?  I'm concerned about spurious setNeedsDisplay calls.

Hm, right, extra invalidates would happen. How about saving the m_contentsScale, and then early-out/don't setNeedsDisplay() if appliesPageScale() is true?

>>> WebCore/platform/graphics/chromium/GraphicsLayerChromium.cpp:815
>>>  void GraphicsLayerChromium::deviceOrPageScaleFactorChanged()
>> 
>> Can this function go away entirely? We should be invalidating everything when we setContentsScale() on the layer with a new value.
> 
> I can't speak with authority, but is calling Layer::setNeedsDisplay sufficient to perform these invalidations?

Yeh that invalidates the whole layer. And we will do that when there's a new value passed to setContentsScale() from CCLTH. So I don't think that GraphicsLayerChromium needs to worry itself with invalidating based on scale factors at all now! (if it ever did, we were invalidating multiple times)

[Jeff Timanus]

(In reply to comment #8)
> (In reply to comment #7)
> > (In reply to comment #6)
> > > (In reply to comment #4)
> > > > (From update of attachment 161337 [details] [details] [details] [details])
> > > > View in context: https://bugs.webkit.org/attachment.cgi?id=161337&action=review
> > > > 
> > > > Re the failures.
> > > > 
> > > > Tests with a pageScale > 1 have a contentsScale on the NCCH that is too large (scrollbars get smaller). Tests with a pageScale < 1 have a contentsScale on the NCCH that is too small (scollbars get larger). So I suspect the NCCH GraphicsLayer does not have setAppliesPageScale set on it, and is being scaled in terms of bounds, and then scaled again by contentsScale.
> > > 
> > > I think your haunch is correct.  As an experiment, I tried assigning setAppliesPageScale() in the NCCH constructor.  This corrects the problematic behaviour wrt the scroll bars in chrome, but the same layout test failures are present.
> > > 
> > > Is the NCCH handled differently in DRT?
> > 
> > Nevermind, I had accidentally run a stale build.  Setting setAppliesPageScale in the constructor of NCCH fixes the layout test failures.
> 
> Cool!
> 
> > However, other tests fail with what appears to be unrelated output.  Text diff here from failure in compositing/geometry/outline-change:
> > 
> > --- /usr/local/google/home/twiz/src/src1/src/webkit/Release/layout-test-results/compositing/geometry/outline-change-expected.txt 
> > +++ /usr/local/google/home/twiz/src/src1/src/webkit/Release/layout-test-results/compositing/geometry/outline-change-actual.txt 
> > @@ -1,3 +1,13 @@
> > +** (DumpRenderTree:14063): DEBUG: 0xd2a500: "NewStream mimetype 'video/mp4' URL 'file:///usr/local/google/home/twiz/src/src1/src/third_party/WebKit/LayoutTests/compositing/resources/video.mp4'"
> > +** (DumpRenderTree:14063): DEBUG: 0xd2a500: "Not expecting a new stream; aborting stream"
> > +** (DumpRenderTree:14063): DEBUG: 0xd2a500: "Viewer DBus interface name is 'org.gnome.totem.PluginViewer_14074'"
> > +** (DumpRenderTree:14063): DEBUG: 0xd2a500: "NameOwnerChanged old-owner '' new-owner ':1.327'"
> > +** (DumpRenderTree:14063): DEBUG: 0xd2a500: "Viewer now connected to the bus"
> > +** (DumpRenderTree:14063): DEBUG: 0xd2a500: "ViewerSetup"
> > +** (DumpRenderTree:14063): DEBUG: 0xd2a500: "Calling SetWindow"
> > +Viewer: SetWindow XID 88080418 size 100:100
> > +** (DumpRenderTree:14063): DEBUG: ~totemPlugin [0xd2a500]
> > +** (DumpRenderTree:14063): DEBUG: NP_Shutdown
> >  layer at (0,0) size 800x600
> >    RenderView at (0,0) size 800x600
> >  layer at (0,0) size 800x256
> 
> That doesn't sound like it's your patch. Does the same output occur without your patch?

Yes, the same failures happen without my patch.  I wonder if it's a stale build issue - I'm running the tests in release, so the presence of DEBUG: messages is suspicious.

[Jeff Timanus]

(In reply to comment #11)
> (From update of attachment 161337 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=161337&action=review
> 
> >>> WebCore/platform/graphics/chromium/LayerChromium.cpp:630
> >>> +        return;
> >> 
> >> If applies page scale is something the API lets you change, maybe drop this piece in setContentsScale. Then if you setAppliesPS(false) you will have the right contents scale already there.
> > 
> > Is this safe?  I'm concerned about spurious setNeedsDisplay calls.
> 
> Hm, right, extra invalidates would happen. How about saving the m_contentsScale, and then early-out/don't setNeedsDisplay() if appliesPageScale() is true?
> 
> >>> WebCore/platform/graphics/chromium/GraphicsLayerChromium.cpp:815
> >>>  void GraphicsLayerChromium::deviceOrPageScaleFactorChanged()
> >> 
> >> Can this function go away entirely? We should be invalidating everything when we setContentsScale() on the layer with a new value.
> > 
> > I can't speak with authority, but is calling Layer::setNeedsDisplay sufficient to perform these invalidations?
> 
> Yeh that invalidates the whole layer. And we will do that when there's a new value passed to setContentsScale() from CCLTH. So I don't think that GraphicsLayerChromium needs to worry itself with invalidating based on scale factors at all now! (if it ever did, we were invalidating multiple times)

Ian added code depending on deviceOrPageScaleFactorChanged in https://bugs.webkit.org/show_bug.cgi?id=89676.

Ian, can you comment on if it is safe to remove the test and code path?  Is there an alternative location for the same test in this new design?

[Dana Jansens]

Ohh overlays. Basically that code was to hook up different GraphicsLayers (other than RenderLayerBacking and NonCompositedContentHost) so that the GraphicsLayer would report the right contentsScale to the compositor. Since the compositor is getting these values to all layers via its own means now it should be safe to remove!

The test should still be possible to pass, but instead of the MockClient returning a deviceScaleFactor of 2, the WebLayerTreeView needs to have its deviceScaleFactor set to 2.

[Dana Jansens]

(In reply to comment #14)
> Ohh overlays. Basically that code was to hook up different GraphicsLayers (other than RenderLayerBacking and NonCompositedContentHost) so that the GraphicsLayer would report the right contentsScale to the compositor. Since the compositor is getting these values to all layers via its own means now it should be safe to remove!
> 
> The test should still be possible to pass, but instead of the MockClient returning a deviceScaleFactor of 2, the WebLayerTreeView needs to have its deviceScaleFactor set to 2.

In order to verify it's working just open the webpage inspector with --force-compositing-mode on and --force-device-scale-factor=2 in an aura/chromeos build. The inspector's text should not be pixel doubled.

[Jeff Timanus]

Created attachment 161716 [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.

(In reply to comment #13)
> (In reply to comment #11)
> > (From update of attachment 161337 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=161337&action=review
> > 
> > >>> WebCore/platform/graphics/chromium/LayerChromium.cpp:630
> > >>> +        return;
> > >> 
> > >> If applies page scale is something the API lets you change, maybe drop this piece in setContentsScale. Then if you setAppliesPS(false) you will have the right contents scale already there.
> > > 
> > > Is this safe?  I'm concerned about spurious setNeedsDisplay calls.
> > 
> > Hm, right, extra invalidates would happen. How about saving the m_contentsScale, and then early-out/don't setNeedsDisplay() if appliesPageScale() is true?
> > 
> > >>> WebCore/platform/graphics/chromium/GraphicsLayerChromium.cpp:815
> > >>>  void GraphicsLayerChromium::deviceOrPageScaleFactorChanged()
> > >> 
> > >> Can this function go away entirely? We should be invalidating everything when we setContentsScale() on the layer with a new value.
> > > 
> > > I can't speak with authority, but is calling Layer::setNeedsDisplay sufficient to perform these invalidations?
> > 
> > Yeh that invalidates the whole layer. And we will do that when there's a new value passed to setContentsScale() from CCLTH. So I don't think that GraphicsLayerChromium needs to worry itself with invalidating based on scale factors at all now! (if it ever did, we were invalidating multiple times)
> 
> Ian added code depending on deviceOrPageScaleFactorChanged in https://bugs.webkit.org/show_bug.cgi?id=89676.
> 
> Ian, can you comment on if it is safe to remove the test and code path?  Is there an alternative location for the same test in this new design?

My response was going to be very similar to Dana's below. The point of my change was only to ensure that a layer has the correct content scale when it needs it. If CCLayerTreeHost now guarantees that layers have valid contents scales before they're ever used, then I think we're good. The old unit test is invalid, but it would be nice if a new unit test could prove that we're guaranteed to have correct page scales before they are ever used. It would also be nice to confirm that overlays are still crisp in the web inspector with --force-device-scale-factor=2.

[Dana Jansens]

Comment on attachment 161716 [details]
Patch

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

This is shaping up very nice! It definitely needs some tests, and the link highlighting interactions need to be looked at further/better understood.

> Source/Platform/chromium/public/WebContentLayer.h:49
> +    virtual void setDisregardsContentsScale(bool) = 0;
> +    virtual bool disregardsContentsScale() const = 0;

I like this name a lot more, thanks.

> Source/WebCore/platform/graphics/chromium/GraphicsLayerChromium.cpp:-231
> -        if (m_linkHighlight)
> -            m_linkHighlight->invalidate();

I'm not sure about interaction with link highlight or why this call was inside the if{} block here. wjmaclean or jamesr will have to weigh in.

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHost.cpp:614
> +        it->setContentsScale(m_pageScaleFactor * m_deviceScaleFactor);
> +

Move this into the elseif block for representsItself. No need to set it on layers that don't draw.

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

Move this up one line with the other m_graphicsLayer setters? I'd prefer to pass in true explicitly also.

[W. James MacLean]

(In reply to comment #19)
> (From update of attachment 161716 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=161716&action=review
> 
> > Source/WebCore/platform/graphics/chromium/GraphicsLayerChromium.cpp:-231
> > -        if (m_linkHighlight)
> > -            m_linkHighlight->invalidate();
> 
> I'm not sure about interaction with link highlight or why this call was inside the if{} block here. wjmaclean or jamesr will have to weigh in.

As long as the associated m_layer->layer()->invalidate() is going at the same time (and everything still works), then this is fine. 

Basically, any time the GLC's layer gets invalidated, the link highlight (should one exist), needs to check in case the invalidation affects it too).

[Dana Jansens]

(In reply to comment #20)
> (In reply to comment #19)
> > (From update of attachment 161716 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=161716&action=review
> > 
> > > Source/WebCore/platform/graphics/chromium/GraphicsLayerChromium.cpp:-231
> > > -        if (m_linkHighlight)
> > > -            m_linkHighlight->invalidate();
> > 
> > I'm not sure about interaction with link highlight or why this call was inside the if{} block here. wjmaclean or jamesr will have to weigh in.
> 
> As long as the associated m_layer->layer()->invalidate() is going at the same time (and everything still works), then this is fine. 
> 
> Basically, any time the GLC's layer gets invalidated, the link highlight (should one exist), needs to check in case the invalidation affects it too).

Ok thanks. To summarize an offline clarifying convo: Since changing contentsScale doesn't affect layout we should be fine. And if it did, then invalidations should bubble up out of WebCore and catch the link highlight at that time.

[Jeff Timanus]

Created attachment 161732 [details]
Screen-shot of device-scale-factor=2.

Confirmation of the behaviour of the device scaling when using --force-device-scale-factor=2.

From my observations, this looks like it is the correct behaviour.

[Jeff Timanus]

Created attachment 161741 [details]
Patch

[Jeff Timanus]

Comment on attachment 161337 [details]
Piping appliesPageScale to LayerChromium from GraphicsLayer.

I addressed those comments, but found that my change was introducing crashes:

../../third_party/WebKit/Source/WebCore/platform/graphics/chromium/TiledLayerChromium.cpp(399) : void WebCore::TiledLayerChromium::markOcclusionsAndRequestTextures(int, int, int, int, const WebCore::CCOcclusionTracker*)

From a discussion with danakj, this was due to the timing/placement of the setContentsScale calls.  Will post an updated patch that explicitly walks the layers to update their scale in CCLayerTreeHost::updateLayers(....).

[Jeff Timanus]

Created attachment 161743 [details]
Screen-shot of sharp scaled web-inspector text, but fuzzy content text.

Patch 161741 is not implementing the contentsScale behaviour properly.  It can clearly be seen in this screen capture that the web-contents text is not being rasterized at the correct scale.

More debugging/investigation will be required.  

Note:  The compositing layout tests still pass against patch 161741.

[Jeff Timanus]

Comment on attachment 161741 [details]
Patch

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

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHost.cpp:497
> +}

I believe that this approach of applying the scale is not correct - as shown in the screen-shot attachment.  

Can any of the reviewers comment on why this may be the case?  The only change between this patch and the previous is that these calls were moved here, instead of in CCLayerTreeHost::paintLayerContents & CCLayerTreeHost::paintMasksForRenderSurface.

[Dana Jansens]

Comment on attachment 161741 [details]
Patch

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

> Source/WebCore/platform/graphics/chromium/LayerChromium.cpp:621
> +    if (disregardsContentsScale())

nit: use m_disregardsContentsScale directly when it is available.

> Source/WebCore/platform/graphics/chromium/LayerChromium.cpp:633
> +    if (disregardsContentsScale())

and here.

>> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHost.cpp:497
>> +}
> 
> I believe that this approach of applying the scale is not correct - as shown in the screen-shot attachment.  
> 
> Can any of the reviewers comment on why this may be the case?  The only change between this patch and the previous is that these calls were moved here, instead of in CCLayerTreeHost::paintLayerContents & CCLayerTreeHost::paintMasksForRenderSurface.

In the screenshot you posted, were there any composited layers besides the NCCH? I think I might get what is going on here. Device scale is a bit different than page scale, as always...

NonCompositedContentHost::deviceScaleFactor() returns a non-1 value for the NCCH in ChromeOS. This translates to a non-1 contentsScale. So what we should have is a "disregardsPageScaleFactor" not a "disregardsScaleFactor". The bounds of the NCCH layer are not scaled by the part of the deviceScaleFactor that is given to the compositor. Thus it still needs to appear in the contentBounds() for that layer.

That explains why the pageScale tests still passed :) This is right for page scale currently just not device scale.

[WebKit Review Bot]

Comment on attachment 161741 [details]
Patch

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

New failing tests:
CCLayerTreeHostTestDeviceScaleFactorScalesViewportAndLayers.runMultiThread

[Jeff Timanus]

Created attachment 162066 [details]
Patch

[Jeff Timanus]

Created attachment 162091 [details]
Patch

[Jeff Timanus]

(In reply to comment #27)
> (From update of attachment 161741 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=161741&action=review
> 
> > Source/WebCore/platform/graphics/chromium/LayerChromium.cpp:621
> > +    if (disregardsContentsScale())
> 
> nit: use m_disregardsContentsScale directly when it is available.
> 
> > Source/WebCore/platform/graphics/chromium/LayerChromium.cpp:633
> > +    if (disregardsContentsScale())
> 
> and here.
> 
> >> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHost.cpp:497
> >> +}
> > 
> > I believe that this approach of applying the scale is not correct - as shown in the screen-shot attachment.  
> > 
> > Can any of the reviewers comment on why this may be the case?  The only change between this patch and the previous is that these calls were moved here, instead of in CCLayerTreeHost::paintLayerContents & CCLayerTreeHost::paintMasksForRenderSurface.
> 
> In the screenshot you posted, were there any composited layers besides the NCCH? I think I might get what is going on here. Device scale is a bit different than page scale, as always...
> 
> NonCompositedContentHost::deviceScaleFactor() returns a non-1 value for the NCCH in ChromeOS. This translates to a non-1 contentsScale. So what we should have is a "disregardsPageScaleFactor" not a "disregardsScaleFactor". The bounds of the NCCH layer are not scaled by the part of the deviceScaleFactor that is given to the compositor. Thus it still needs to appear in the contentBounds() for that layer.

The patch in comment #30 implements this suggestion, and appears to work, at least from my initial unit-test runs, and experimentation with --force-device-scale-factor=2.

I renamed the disregardsContentsScale set of routines to disregardsPageScale, and selectively apply either page-scale * device-scale, or device-scale in CCLayerTreeHost::updateLayers(...).

> 
> That explains why the pageScale tests still passed :) This is right for page scale currently just not device scale.

[Alexandre Elias]

I like the direction of this change, but please hold off on commit until I manually test Android behavior doesn't change (I'll try the patch out later today).

[Jeff Timanus]

Created attachment 162094 [details]
Screen-shot of fuzzy tooltip box.

Ian, could you look at this image to verify if I have introduce a regression.  Both the resolution of the text of the tool-tip, and the text of the page looks correct, but the actual tool-tip box appears to be rasterized at the wrong scale. 

This image is generated with patch from comment #30 applied.

Can you please confirm?

[Alexandre Elias]

Comment on attachment 162091 [details]
Patch

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

> Source/WebCore/platform/graphics/chromium/GraphicsLayerChromium.h:136
> +    virtual void setAppliesPageScale(bool appliesScale) OVERRIDE;

Seems these methods also need to be renamed to "disregards"?

[Jeff Timanus]

Comment on attachment 162091 [details]
Patch

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

>> Source/WebCore/platform/graphics/chromium/GraphicsLayerChromium.h:136
>> +    virtual void setAppliesPageScale(bool appliesScale) OVERRIDE;
> 
> Seems these methods also need to be renamed to "disregards"?

I had not changed these because they are overloading routines exposed outside of chromium-specific code.

GraphicsLayer initially exported these routines, and I did not want to modify non-chromium ports with this change.  However, there are not very many call-sites for these routines. 
  setAppliesPageScale is only called from RenderLayerBacking::createPrimaryGraphicsLayer.  

  appliesPageScale is called from the Core-Animation port:  GraphicsLayerCA::computePositionRelativeToBase.

Reviewing the CA code, it looks like it is using the appliesPageScale for the exact same purpose as the chromium port, so the semantics of the routine will remain consistent after the name change.

Would it be reasonable to rename the routines in a subsequent patch?  Or should I just add it in here?

[Jeff Timanus]

Created attachment 162249 [details]
Rebaselined to 127510.

[Dana Jansens]

Comment on attachment 162249 [details]
Rebaselined to 127510.

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

I like this patch! Thanks for looking into the tooltip fuzziness, looking forward to hearing how we can fix that.

Since the NCCH::deviceScaleFactor() method will no longer be used, can we remove that entirely now? As well as the also-unused deviceScale in the setViewport() method and the call to deviceOrPageScaleFactorChanged().

> Source/WebCore/platform/graphics/chromium/LayerChromium.h:263
> +    // Invoke this routine to instruct layer hosts to skip application of page scale..

nit: This can be more terse I think, something like: // When true, the layer's contents are not scaled by the current page scale factor.

[Jeff Timanus]

(In reply to comment #33)
> Created an attachment (id=162094) [details]
> Screen-shot of fuzzy tooltip box.
> 
> Ian, could you look at this image to verify if I have introduce a regression.  Both the resolution of the text of the tool-tip, and the text of the page looks correct, but the actual tool-tip box appears to be rasterized at the wrong scale. 
> 
> This image is generated with patch from comment #30 applied.
> 
> Can you please confirm?

I believe that I found the cause of the fuzzy tool-tip in the overlay.  The overlay boxes are now drawn via a canvas2d tag.  This was recently changed by pfeldman@chromium in r127240 (https://bugs.webkit.org/show_bug.cgi?id=95456).  As I understand things, canvas2d tags are not up-rezzed in response to scale changes, and so will appear pixelated.

I double checked, and the blurry box is present without this patch.

[Jeff Timanus]

(In reply to comment #38)
> (In reply to comment #33)
> > Created an attachment (id=162094) [details] [details]
> > Screen-shot of fuzzy tooltip box.
> > 
> > Ian, could you look at this image to verify if I have introduce a regression.  Both the resolution of the text of the tool-tip, and the text of the page looks correct, but the actual tool-tip box appears to be rasterized at the wrong scale. 
> > 
> > This image is generated with patch from comment #30 applied.
> > 
> > Can you please confirm?
> 
> I believe that I found the cause of the fuzzy tool-tip in the overlay.  The overlay boxes are now drawn via a canvas2d tag.  This was recently changed by pfeldman@chromium in r127240 (https://bugs.webkit.org/show_bug.cgi?id=95456).  As I understand things, canvas2d tags are not up-rezzed in response to scale changes, and so will appear pixelated.
> 
> I double checked, and the blurry box is present without this patch.
New bug opened to track this separate issue:  https://bugs.webkit.org/show_bug.cgi?id=95875

[Jeff Timanus]

Created attachment 162313 [details]
Address review comments, and remove deviceScaleFactor from NCCH.

[Dana Jansens]

Comment on attachment 162313 [details]
Address review comments, and remove deviceScaleFactor from NCCH.

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

Thanks for bearing with me for all these changes. This LGTM with nits. /me tosses it at jamesr and enne.

> Source/WebKit/chromium/src/NonCompositedContentHost.cpp:91
> -void NonCompositedContentHost::setViewport(const WebCore::IntSize& viewportSize, const WebCore::IntSize& contentsSize, const WebCore::IntPoint& scrollPosition, const WebCore::IntPoint& scrollOrigin, float deviceScale)
> +void NonCompositedContentHost::setViewport(const WebCore::IntSize& viewportSize, const WebCore::IntSize& contentsSize, const WebCore::IntPoint& scrollPosition, const WebCore::IntPoint& scrollOrigin/*, float deviceScale*/)

nit: Looks like you left a comment in here by mistake.

> Source/WebKit/chromium/src/WebViewImpl.cpp:3985
>      float deviceScale = m_deviceScaleInCompositor;

nit: unused var now?

[Jeff Timanus]

Created attachment 162323 [details]
Addressed review comments.  Rebaselined to 127637.

[Jeff Timanus]

Comment on attachment 162313 [details]
Address review comments, and remove deviceScaleFactor from NCCH.

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

>> Source/WebKit/chromium/src/NonCompositedContentHost.cpp:91
>> +void NonCompositedContentHost::setViewport(const WebCore::IntSize& viewportSize, const WebCore::IntSize& contentsSize, const WebCore::IntPoint& scrollPosition, const WebCore::IntPoint& scrollOrigin/*, float deviceScale*/)
> 
> nit: Looks like you left a comment in here by mistake.

Done.

>> Source/WebKit/chromium/src/WebViewImpl.cpp:3985
>>      float deviceScale = m_deviceScaleInCompositor;
> 
> nit: unused var now?

It is still used in calls that are hidden by the code-review UI.  

From a few lines down:
    IntSize layoutViewportSize = size();
    IntSize deviceViewportSize = size();
    deviceViewportSize.scale(deviceScale);

[Adrienne Walker]

Comment on attachment 162323 [details]
Addressed review comments.  Rebaselined to 127637.

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

> Source/WebCore/platform/graphics/chromium/GraphicsLayerChromium.cpp:858
> +    m_layer->setDisregardsPageScale(appliesScale);

I'm trying and failing to resist the bikeshed here, but I don't find this name any clearer.  If anything, this line in particular that does the conversion only exacerbates it.

Can I suggest boundsContainPageScale, which mentions the target that has the page scale applied to it? (Or some other name that includes a noun to identify the target or actor?)

I'd really love to just get rid of this entirely and have GraphicsLayerChromium apply whatever transforms it needs such that all LayerChromiums can always apply both DSF and PSF.

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHost.cpp:501
> +    LayerChromium* maskLayer = layer->maskLayer();
> +    if (maskLayer)
> +        setScale(maskLayer, deviceScaleFactor, pageScaleFactor);
> +
> +    LayerChromium* replicaMaskLayer = layer->replicaLayer() ? layer->replicaLayer()->maskLayer() : 0;
> +    if (replicaMaskLayer)
> +        setScale(replicaMaskLayer, deviceScaleFactor, pageScaleFactor);

Does the replicaLayer need its scale set as well?

[Dana Jansens]

Comment on attachment 162323 [details]
Addressed review comments.  Rebaselined to 127637.

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

>> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHost.cpp:501
>> +        setScale(replicaMaskLayer, deviceScaleFactor, pageScaleFactor);
> 
> Does the replicaLayer need its scale set as well?

Replicas don't have contents do they?

[Alexandre Elias]

I confirmed this works fine on Android.

[Jeff Timanus]

(In reply to comment #45)
> (From update of attachment 162323 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=162323&action=review
> 
> >> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHost.cpp:501
> >> +        setScale(replicaMaskLayer, deviceScaleFactor, pageScaleFactor);
> > 
> > Does the replicaLayer need its scale set as well?
> 
> Replicas don't have contents do they?

I wrote this routine using my changes from a early patch on this CL [1], as recommended by Dana previously in this review in comment #5.

[1] https://bug-95094-attachments.webkit.org/attachment.cgi?id=161550

I can't really answer this question with authority.  Is there a good way to test if replicas need special handling?

[Jeff Timanus]

Created attachment 162378 [details]
Rename disregardsPageScale to boundsContainPageScale.

[James Robinson]

(In reply to comment #47)
> (In reply to comment #45)
> > (From update of attachment 162323 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=162323&action=review
> > 
> > >> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHost.cpp:501
> > >> +        setScale(replicaMaskLayer, deviceScaleFactor, pageScaleFactor);
> > > 
> > > Does the replicaLayer need its scale set as well?
> > 
> > Replicas don't have contents do they?
> 
> I wrote this routine using my changes from a early patch on this CL [1], as recommended by Dana previously in this review in comment #5.
> 
> [1] https://bug-95094-attachments.webkit.org/attachment.cgi?id=161550
> 
> I can't really answer this question with authority.  Is there a good way to test if replicas need special handling?

Replicas can have contents layers.  To test, try making a webgl <canvas> or <video> and put a reflection on it.

[Adrienne Walker]

Comment on attachment 162378 [details]
Rename disregardsPageScale to boundsContainPageScale.

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

Dana convinced me that replica layers are always !drawsContent() and are just a transform, essentially.  Looking at other code, we'd have other problems if replica layers had contents.

R=me.  Thanks for wrestling with this mess.  :)

> Source/WebCore/platform/graphics/chromium/GraphicsLayerChromium.cpp:-234
> -        if (m_linkHighlight)
> -            m_linkHighlight->invalidate();

I was a little dubious about this, but I think I've convinced myself that because the m_linkHighlight layer is in the LayerChromium tree, that it'll get setContentsScale() called on it, and that'll do the right invalidation.  And, on top of that, the compositor properly handles any ordering of setContentsScale / setNeedsDisplay.

[Jeff Timanus]

(In reply to comment #49)
> (In reply to comment #47)
> > (In reply to comment #45)
> > > (From update of attachment 162323 [details] [details] [details])
> > > View in context: https://bugs.webkit.org/attachment.cgi?id=162323&action=review
> > > 
> > > >> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHost.cpp:501
> > > >> +        setScale(replicaMaskLayer, deviceScaleFactor, pageScaleFactor);
> > > > 
> > > > Does the replicaLayer need its scale set as well?
> > > 
> > > Replicas don't have contents do they?
> > 
> > I wrote this routine using my changes from a early patch on this CL [1], as recommended by Dana previously in this review in comment #5.
> > 
> > [1] https://bug-95094-attachments.webkit.org/attachment.cgi?id=161550
> > 
> > I can't really answer this question with authority.  Is there a good way to test if replicas need special handling?
> 
> Replicas can have contents layers.  To test, try making a webgl <canvas> or <video> and put a reflection on it.

I double checked this assumption, and found an existing test [1] that should fail when device scale factor is non-1.

When loading this page in ChromeOS with a boosted device scale factor, the video reflection is also scaled properly.  Is this sufficient to demonstrate that our assumptions are correct?

[1] WebKit/LayoutTests/compositing/reflections/load-video-in-reflection.html

[Jeff Timanus]

(In reply to comment #50)
> (From update of attachment 162378 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=162378&action=review
> 
> Dana convinced me that replica layers are always !drawsContent() and are just a transform, essentially.  Looking at other code, we'd have other problems if replica layers had contents.
> 
> R=me.  Thanks for wrestling with this mess.  :)

Yay!  Let's hope that this change sticks, and that we can keep the momentum on cleaning this up.  

> 
> > Source/WebCore/platform/graphics/chromium/GraphicsLayerChromium.cpp:-234
> > -        if (m_linkHighlight)
> > -            m_linkHighlight->invalidate();
> 
> I was a little dubious about this, but I think I've convinced myself that because the m_linkHighlight layer is in the LayerChromium tree, that it'll get setContentsScale() called on it, and that'll do the right invalidation.  And, on top of that, the compositor properly handles any ordering of setContentsScale / setNeedsDisplay.

[James Robinson]

(In reply to comment #51)
> 
> I double checked this assumption, and found an existing test [1] that should fail when device scale factor is non-1.
> 
> When loading this page in ChromeOS with a boosted device scale factor, the video reflection is also scaled properly.  Is this sufficient to demonstrate that our assumptions are correct?
> 
> [1] WebKit/LayoutTests/compositing/reflections/load-video-in-reflection.html

Sounds like it.  Could you contribute a test that has a reflection and a non-1 scale factor so we can make sure we don't bust this in the future?  It doesn't have to block this patch landing since it seems that we're good.

[WebKit Review Bot]

Comment on attachment 162378 [details]
Rename disregardsPageScale to boundsContainPageScale.

Rejecting attachment 162378 [details] from commit-queue.

New failing tests:
CCLayerTreeHostTestDeviceScaleFactorScalesViewportAndLayers.runMultiThread
Full output: http://queues.webkit.org/results/13771295

[Jeff Timanus]

Created attachment 162532 [details]
Correct failing CC unit test, and rebaselined to 127747.

[Jeff Timanus]

Comment on attachment 162532 [details]
Correct failing CC unit test, and rebaselined to 127747.

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

> Source/WebKit/chromium/tests/CCLayerTreeHostTest.cpp:1369
> +        // The child is at position 2,2, which is transformed to 3,3 after the scale

This test has been rebaselined due to the change in behaviour of the draw transforms.

The child layer now has a contents-scale applied to the bounds, so the transformation has an identity scale.  From a conversation with danakj@chromium, it is correct that the translational component still has the scale factor applied.

[Dana Jansens]

Comment on attachment 162532 [details]
Correct failing CC unit test, and rebaselined to 127747.

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

>> Source/WebKit/chromium/tests/CCLayerTreeHostTest.cpp:1369
>> +        // The child is at position 2,2, which is transformed to 3,3 after the scale
> 
> This test has been rebaselined due to the change in behaviour of the draw transforms.
> 
> The child layer now has a contents-scale applied to the bounds, so the transformation has an identity scale.  From a conversation with danakj@chromium, it is correct that the translational component still has the scale factor applied.

Yay, cool thanks.

Would you mind adding an EXPECT_EQ(1.5*child->bounds(), child->contentBounds()) to make sure the scale is ending up in the right place in the future?

[Jeff Timanus]

Created attachment 162537 [details]
Addressing comments.

[Adrienne Walker]

Comment on attachment 162537 [details]
Addressing comments.

R=me.

[WebKit Review Bot]

Comment on attachment 162537 [details]
Addressing comments.

Clearing flags on attachment: 162537

Committed r127789: <http://trac.webkit.org/changeset/127789>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Adam Klein]

compositing/overflow/overflow-scaled-descendant-overlapping.html began failing on cr-mac after this change. I've filed bug 96416 to track that.