Support high DPI scrollbar on top level web frame.

Support high DPI scrollbar on top level web frame.

BUG=141389
TEST=Run chrome with --force-device-scale-factor=2 --force-compositing-mode and verify that the top-level scrollbar is high dpi.


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=158362

[flackr, 2012-09-15 02:43:31]

I changed layerRectToContentRect to accept a WebRect as we don't support the
implicit conversion from WebRect to IntRect. I'll update the webkit bug to just
have the fix for NonCompositedContentHost.

[flackr, 2012-09-17 19:25:55]

This fixes the layout test that was failing by only applying page scale to
layers below the root scroll layer in CCLayerTreeHost as suggested by Dana.

Dana, I currently just check for scrollable layer as it's traversing the layer
tree in updateLayerScale. Let me know if you think I should still make a
separate function do a search for the scrollable layer and try to generalize the
function findRootScrollLayer in CCLayerTreeHostImpl. These use separate class
types though (LayerChromium vs CCLayerImpl).

[danakj, 2012-09-17 19:34:14]

I think this is great as long as that logic is long-term correct. If
we're going to be changing how you find the root scroll layer in the
future, i'd prefer to do that search first in a templated way so the
logic isn't duplicated.

On Mon, Sep 17, 2012 at 3:25 PM,  <flackr@chromium.org> wrote:
> This fixes the layout test that was failing by only applying page scale to
> layers below the root scroll layer in CCLayerTreeHost as suggested by Dana.
>
> Dana, I currently just check for scrollable layer as it's traversing the
> layer
> tree in updateLayerScale. Let me know if you think I should still make a
> separate function do a search for the scrollable layer and try to generalize
> the
> function findRootScrollLayer in CCLayerTreeHostImpl. These use separate
> class
> types though (LayerChromium vs CCLayerImpl).
>
> https://codereview.chromium.org/10909255/

[flackr, 2012-09-17 19:37:51]

https://codereview.chromium.org/10909255/diff/2001/cc/CCLayerTreeHost.cpp
File cc/CCLayerTreeHost.cpp (right):

https://codereview.chromium.org/10909255/diff/2001/cc/CCLayerTreeHost.cpp#new...
cc/CCLayerTreeHost.cpp:481: inScrollLayer = true;
Alexandre, is the logic for finding the root scroll layer going to be changing?
This is the same logic as findRootScrollLayer in CCLayerTreeHostImpl.cpp.

[aelias_OOO_until_Jul13, 2012-09-17 19:45:48]

You're setting the value to true in the root scroll layer and all its sublayers,
is that the intent?  "inScrollLayer" does not really communicate that.

[flackr, 2012-09-17 19:48:43]

On 2012/09/17 19:45:48, aelias wrote:
> You're setting the value to true in the root scroll layer and all its
sublayers,
> is that the intent?  "inScrollLayer" does not really communicate that.

Yes, that's the intent. Suggestions? inRootScrollLayer? appliesPageScale?

[danakj, 2012-09-17 19:50:15]

inRootScrollLayerSubtree?

On Mon, Sep 17, 2012 at 3:48 PM,  <flackr@chromium.org> wrote:
> On 2012/09/17 19:45:48, aelias wrote:
>>
>> You're setting the value to true in the root scroll layer and all its
>
> sublayers,
>>
>> is that the intent?  "inScrollLayer" does not really communicate that.
>
>
> Yes, that's the intent. Suggestions? inRootScrollLayer? appliesPageScale?
>
> https://codereview.chromium.org/10909255/

[enne (OOO), 2012-09-17 20:37:00]

https://codereview.chromium.org/10909255/diff/2001/cc/CCLayerTreeHost.cpp
File cc/CCLayerTreeHost.cpp (right):

https://codereview.chromium.org/10909255/diff/2001/cc/CCLayerTreeHost.cpp#new...
cc/CCLayerTreeHost.cpp:481: inScrollLayer = true;
I think I'd prefer to keep this complexity outside of the compositor.  If a
browser compositor creates a layer tree, it's not necessarily going to want this
"outside root scroll layer" special case.  We already have a special case flag
to avoid this behavior: it's boundsContainPageScale.

I'd prefer if NonCompositedContentHost set this boundsContainPageScale flag on
itself and (recursively) all layers outside of the scroll layer, like
https://bugs.webkit.org/show_bug.cgi?id=77251 used to do for setting a different
flag on scrollbar layers.

That way the compositor doesn't have to know about this special case, and the
NonCompositedContentHost can contain all of this special case stuff about root
layers and root scrollbars.

[flackr, 2012-09-18 20:20:51]

Okay, this combined with the webkit patch
https://bugs.webkit.org/show_bug.cgi?id=95134 calls setBoundsContainPageScale on
the correct layers. The GraphicsLayers for the scrollbars point to the parents
of the ScrollbarLayerChromiums which is why I updated
setBoundsContainsScaleFactor to apply recursively on the LayerChromiumTree.

The root of the graphics layer tree contains the following:
GraphicsLayer* root
+ GraphicsLayer* clipLayer (children clipped for brevity)
+ GraphicsLayer* scrollbar (horizontal)
  -> LayerChromium* contentLayer()
     + ScrollbarLayerChromium*
+ GraphicsLayer* scrollbar (vertical)
  -> LayerChromium* contentLayer()
     + ScrollbarLayerChromium*
+ GraphicsLayer* scrollbar (corner)
  -> LayerChromium* contentLayer()

[enne (OOO), 2012-09-19 17:08:25]

LGTM

http://codereview.chromium.org/10909255/diff/10001/cc/LayerChromium.cpp
File cc/LayerChromium.cpp (right):

http://codereview.chromium.org/10909255/diff/10001/cc/LayerChromium.cpp#newco...
cc/LayerChromium.cpp:609: for (size_t i = 0; i < m_children.size(); ++i)
Can you add a comment to the header documenting that this function is now
recursive?

[flackr, 2012-09-24 17:19:21]

All of the test failures on the layout try jobs seem to be occurring in other
unrelated CL try runs as well.

http://codereview.chromium.org/10909255/diff/10001/cc/LayerChromium.cpp
File cc/LayerChromium.cpp (right):

http://codereview.chromium.org/10909255/diff/10001/cc/LayerChromium.cpp#newco...
cc/LayerChromium.cpp:609: for (size_t i = 0; i < m_children.size(); ++i)
On 2012/09/19 17:08:25, enne wrote:
> Can you add a comment to the header documenting that this function is now
> recursive?

Done.

[commit-bot: I haz the power, 2012-09-24 17:19:36]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/flackr@chromium.org/10909255/20001

[commit-bot: I haz the power, 2012-09-24 20:02:53]

Change committed as 158362