Avoid compositing fixed-position elements if they cannot scroll

Avoid compositing fixed-position elements if they cannot scroll

If there is actually no scrollable element between a fixed-position layer and
its container, then there is no reason to composite the fixed-position
element. In fact, it is more wasteful in memory resources and computation
resources to make it composited. This patch avoids compositing in those cases.
Note that if there is another reason that the fixed-position element needs to
be composited, such as overlap, it will still do so.

BUG=222110
R=dglazkov@chromium.org, enne@chromium.org, trchen@chromium.org, vangelis@chromium.org

Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=149624

[shawnsingh, 2013-04-10 01:15:04]

Not for review yet.  I just ported the WebKit version of this patch to Blink.

Before review I will try to generalize this patch to work for any fixed-position
container, not just the viewport/RenderView.

[trchen, 2013-04-10 01:24:03]

https://codereview.chromium.org/13828004/diff/1/Source/WebCore/rendering/Rend...
File Source/WebCore/rendering/RenderLayerCompositor.cpp (right):

https://codereview.chromium.org/13828004/diff/1/Source/WebCore/rendering/Rend...
Source/WebCore/rendering/RenderLayerCompositor.cpp:1988: noScrollableAncestor =
false;
I'm concerned about the platforms that use impl-side pinch zoom (Android &
CrOS). FrameView::isScrollable becomes a dynamic property. In that case I think
we should always create composited layers for RenderView-contained
fixed-position elements.

[shawnsingh, 2013-04-10 01:55:47]

Can you please explain the example you think may go wrong?   I'm not seeing any
problem.

I think we can be pretty certain that if FrameView::isScrollable changes value,
it would have changed for reasons that would also require re-computing
compositing requirements, which would be when this code runs.  At that point
it's conceivable that the layer can change to become composited.  For pinch-zoom
with a non-composited fixed-pos layer, I would expect it will work just fine, as
it's effectively scaling the contents of only one layer.

[trchen, 2013-04-10 02:05:52]

On 2013/04/10 01:55:47, shawnsingh wrote:
> Can you please explain the example you think may go wrong?   I'm not seeing
any
> problem.
> 
> I think we can be pretty certain that if FrameView::isScrollable changes
value,
> it would have changed for reasons that would also require re-computing
> compositing requirements, which would be when this code runs.  At that point
> it's conceivable that the layer can change to become composited.  For
pinch-zoom
> with a non-composited fixed-pos layer, I would expect it will work just fine,
as
> it's effectively scaling the contents of only one layer.

There will be a short while that the main frame suddenly becomes scrollable in
the compositor, before the latest page scale factor gets committed to main
thread. How do we apply scroll compensation when fixed-position elements don't
have its own layers?

[shawnsingh, 2013-04-10 02:33:28]

I see the problem.  We have to think carefully about how to resolve this.  
Chrome OS especially would benefit greatly from reducing gmail layer explosion,
and I don't want to make patch active for only some platforms and not others =(

[trchen, 2013-04-10 02:54:47]

On 2013/04/10 02:33:28, shawnsingh wrote:
> I see the problem.  We have to think carefully about how to resolve this.  
> Chrome OS especially would benefit greatly from reducing gmail layer
explosion,
> and I don't want to make patch active for only some platforms and not others
=(

I'm thinking main frames should be always considered as scrollable when pinch
gesture is enabled. Not only for fixed-position layer requirement purpose, this
is also the right behavior for overlay scrollbar existence. However I also think
modifying the semantics of FrameView::isScrollable could be dangerous.

[shawnsingh, 2013-04-15 23:58:26]

Reviewers, PTAL

trchen@ and I talked at the end of last week, and we agreed that we can proceed
with this patch without disrupting the nice behavior that was achieved on
pinch-zoom recently.  So I believe this patch is good for landing after
reviewers approve.  Thanks!

[vangelis, 2013-04-16 06:54:23]

https://codereview.chromium.org/13828004/diff/7001/Source/WebCore/rendering/R...
File Source/WebCore/rendering/RenderLayerCompositor.cpp (right):

https://codereview.chromium.org/13828004/diff/7001/Source/WebCore/rendering/R...
Source/WebCore/rendering/RenderLayerCompositor.cpp:1939: if (container ==
m_renderView && m_renderView->frameView() &&
(m_renderView->frameView()->isScrollable()))
Because of the code right above, we're guaranteed that (container ==
m_renderView) if we get here.

https://codereview.chromium.org/13828004/diff/7001/Source/WebCore/rendering/R...
Source/WebCore/rendering/RenderLayerCompositor.cpp:1940: noScrollableAncestor =
false;
Should you set the value of viewportConstrainedNotCompositedReason here and
below?

https://codereview.chromium.org/13828004/diff/7001/Source/WebCore/rendering/R...
Source/WebCore/rendering/RenderLayerCompositor.cpp:1944: if
(ancestor->hasHorizontalScrollbar() || ancestor->hasVerticalScrollbar())
Since you know you need to iterate until you get back to the container which is
the m_renderView, it seems that you can eliminate the code in lines 1939-1940
and just use this loop. 

For the RenderView, is ancestor->hasHorizontalScrollbar() equivalent to
m_renderView->frameView()->isScrollable() ?

[vangelis, 2013-04-16 06:55:34]

On 2013/04/15 23:58:26, shawnsingh wrote:
> Reviewers, PTAL
> 
> trchen@ and I talked at the end of last week, and we agreed that we can
proceed
> with this patch without disrupting the nice behavior that was achieved on
> pinch-zoom recently.  So I believe this patch is good for landing after
> reviewers approve.  Thanks!

How do we get around the problem that we need fixed position elements to be
composited in order to properly handle pinch-zoom, even in pages that don't have
scrollable content?

[trchen, 2013-04-16 19:32:59]

On 2013/04/16 06:55:34, vangelis wrote:
> On 2013/04/15 23:58:26, shawnsingh wrote:
> > Reviewers, PTAL
> > 
> > trchen@ and I talked at the end of last week, and we agreed that we can
> proceed
> > with this patch without disrupting the nice behavior that was achieved on
> > pinch-zoom recently.  So I believe this patch is good for landing after
> > reviewers approve.  Thanks!
> 
> How do we get around the problem that we need fixed position elements to be
> composited in order to properly handle pinch-zoom, even in pages that don't
have
> scrollable content?

I'm working on desktop compatibility viewport. Basically it is an outer viewport
in addition to the visible(inner) viewport, that will be used by WebCore for all
layout purpose (includes fixed position). The desired behavior is that pinch
zoom doesn't affect outer viewport so scrollability won't be affected by
compositor activities.

[trchen, 2013-04-16 19:36:21]

https://codereview.chromium.org/13828004/diff/7001/Source/WebCore/rendering/R...
File Source/WebCore/rendering/RenderLayerCompositor.cpp (right):

https://codereview.chromium.org/13828004/diff/7001/Source/WebCore/rendering/R...
Source/WebCore/rendering/RenderLayerCompositor.cpp:1944: if
(ancestor->hasHorizontalScrollbar() || ancestor->hasVerticalScrollbar())
On 2013/04/16 06:54:23, vangelis wrote:
> Since you know you need to iterate until you get back to the container which
is
> the m_renderView, it seems that you can eliminate the code in lines 1939-1940
> and just use this loop. 
> 
> For the RenderView, is ancestor->hasHorizontalScrollbar() equivalent to
> m_renderView->frameView()->isScrollable() ? 

I have the same concern. For historical reason we often use scrollbar existance
as scrollability. For example, overflow:hidden doesn't have scrollbars, but they
are actually scrollable (you can set scrollLeft/scrollTop with javascript). In
contrast, overflow:visible can be never scrolled, even if you assign a
scrollLeft/scrollTop, the readback will be always 0.

Another commonly confused concepts are scrollability and user-scrollability. For
example, although overflow:hidden can be scrolled by javascript, you can't
scroll those with mouse or gesture.

In this case, I believe what you want is user-scrollability. Looking at
scrollbar existance doesn't look right.

[jamesr, 2013-04-16 19:41:41]

On 2013/04/16 19:36:21, trchen wrote:
>
https://codereview.chromium.org/13828004/diff/7001/Source/WebCore/rendering/R...
> File Source/WebCore/rendering/RenderLayerCompositor.cpp (right):
> 
>
https://codereview.chromium.org/13828004/diff/7001/Source/WebCore/rendering/R...
> Source/WebCore/rendering/RenderLayerCompositor.cpp:1944: if
> (ancestor->hasHorizontalScrollbar() || ancestor->hasVerticalScrollbar())
> On 2013/04/16 06:54:23, vangelis wrote:
> > Since you know you need to iterate until you get back to the container which
> is
> > the m_renderView, it seems that you can eliminate the code in lines
1939-1940
> > and just use this loop. 
> > 
> > For the RenderView, is ancestor->hasHorizontalScrollbar() equivalent to
> > m_renderView->frameView()->isScrollable() ? 
> 
> I have the same concern. For historical reason we often use scrollbar
existance
> as scrollability. For example, overflow:hidden doesn't have scrollbars, but
they
> are actually scrollable (you can set scrollLeft/scrollTop with javascript). In
> contrast, overflow:visible can be never scrolled, even if you assign a
> scrollLeft/scrollTop, the readback will be always 0.

That's for correctness.  Scrollbars existing === user-scrollability in the given
axis.

> 
> Another commonly confused concepts are scrollability and user-scrollability.
For
> example, although overflow:hidden can be scrolled by javascript, you can't
> scroll those with mouse or gesture.
> 
> In this case, I believe what you want is user-scrollability. Looking at
> scrollbar existance doesn't look right.

Looking at the scrollbar existence is the correct way to determine if something
is user-scrollable in a given axis.  It doesn't tell you if something can be
programmatically scrolled in that axis.

[trchen, 2013-04-16 19:49:01]

On 2013/04/16 19:41:41, jamesr wrote:
> On 2013/04/16 19:36:21, trchen wrote:
> >
>
https://codereview.chromium.org/13828004/diff/7001/Source/WebCore/rendering/R...
> > File Source/WebCore/rendering/RenderLayerCompositor.cpp (right):
> > 
> >
>
https://codereview.chromium.org/13828004/diff/7001/Source/WebCore/rendering/R...
> > Source/WebCore/rendering/RenderLayerCompositor.cpp:1944: if
> > (ancestor->hasHorizontalScrollbar() || ancestor->hasVerticalScrollbar())
> > On 2013/04/16 06:54:23, vangelis wrote:
> > > Since you know you need to iterate until you get back to the container
which
> > is
> > > the m_renderView, it seems that you can eliminate the code in lines
> 1939-1940
> > > and just use this loop. 
> > > 
> > > For the RenderView, is ancestor->hasHorizontalScrollbar() equivalent to
> > > m_renderView->frameView()->isScrollable() ? 
> > 
> > I have the same concern. For historical reason we often use scrollbar
> existance
> > as scrollability. For example, overflow:hidden doesn't have scrollbars, but
> they
> > are actually scrollable (you can set scrollLeft/scrollTop with javascript).
In
> > contrast, overflow:visible can be never scrolled, even if you assign a
> > scrollLeft/scrollTop, the readback will be always 0.
> 
> That's for correctness.  Scrollbars existing === user-scrollability in the
given
> axis.
> 
> > 
> > Another commonly confused concepts are scrollability and user-scrollability.
> For
> > example, although overflow:hidden can be scrolled by javascript, you can't
> > scroll those with mouse or gesture.
> > 
> > In this case, I believe what you want is user-scrollability. Looking at
> > scrollbar existance doesn't look right.
> 
> Looking at the scrollbar existence is the correct way to determine if
something
> is user-scrollable in a given axis.  It doesn't tell you if something can be
> programmatically scrolled in that axis.

I see, but shouldn't we make scrollbar existance and user scrollability
different concepts?
For example, a overflow:scroll without enough contents isn't scrollable, but
will create disabled scrollbars anyway.

[jamesr, 2013-04-16 19:52:23]

On 2013/04/16 19:49:01, trchen wrote:
> On 2013/04/16 19:41:41, jamesr wrote:
> > On 2013/04/16 19:36:21, trchen wrote:
> > >
> >
>
https://codereview.chromium.org/13828004/diff/7001/Source/WebCore/rendering/R...
> > > File Source/WebCore/rendering/RenderLayerCompositor.cpp (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/13828004/diff/7001/Source/WebCore/rendering/R...
> > > Source/WebCore/rendering/RenderLayerCompositor.cpp:1944: if
> > > (ancestor->hasHorizontalScrollbar() || ancestor->hasVerticalScrollbar())
> > > On 2013/04/16 06:54:23, vangelis wrote:
> > > > Since you know you need to iterate until you get back to the container
> which
> > > is
> > > > the m_renderView, it seems that you can eliminate the code in lines
> > 1939-1940
> > > > and just use this loop. 
> > > > 
> > > > For the RenderView, is ancestor->hasHorizontalScrollbar() equivalent to
> > > > m_renderView->frameView()->isScrollable() ? 
> > > 
> > > I have the same concern. For historical reason we often use scrollbar
> > existance
> > > as scrollability. For example, overflow:hidden doesn't have scrollbars,
but
> > they
> > > are actually scrollable (you can set scrollLeft/scrollTop with
javascript).
> In
> > > contrast, overflow:visible can be never scrolled, even if you assign a
> > > scrollLeft/scrollTop, the readback will be always 0.
> > 
> > That's for correctness.  Scrollbars existing === user-scrollability in the
> given
> > axis.
> > 
> > > 
> > > Another commonly confused concepts are scrollability and
user-scrollability.
> > For
> > > example, although overflow:hidden can be scrolled by javascript, you can't
> > > scroll those with mouse or gesture.
> > > 
> > > In this case, I believe what you want is user-scrollability. Looking at
> > > scrollbar existance doesn't look right.
> > 
> > Looking at the scrollbar existence is the correct way to determine if
> something
> > is user-scrollable in a given axis.  It doesn't tell you if something can be
> > programmatically scrolled in that axis.
> 
> I see, but shouldn't we make scrollbar existance and user scrollability
> different concepts?
> For example, a overflow:scroll without enough contents isn't scrollable, but
> will create disabled scrollbars anyway.

Well, then it won't be scollable at all so it definitely won't be
user-scrollable.

Scrollable: Have overflow
User-scrollable in a given axis: Scrollable, and have scrollbar for that axis

[shawnsingh, 2013-04-16 20:20:06]

Thanks for reviewing - responses below.

The goal of this patch is to avoid compositing fixed-position layers if
*nothing* will scroll at all.  I wonder if we should choose to composite for the
javascript-scrollable case, too?  The current patch I think would opt-out of
fixed-position compositing only if the user cannot scroll, but if javascript
scrolls, it will still opt-out and require more repaints.

https://codereview.chromium.org/13828004/diff/7001/Source/WebCore/rendering/R...
File Source/WebCore/rendering/RenderLayerCompositor.cpp (right):

https://codereview.chromium.org/13828004/diff/7001/Source/WebCore/rendering/R...
Source/WebCore/rendering/RenderLayerCompositor.cpp:1939: if (container ==
m_renderView && m_renderView->frameView() &&
(m_renderView->frameView()->isScrollable()))
On 2013/04/16 06:54:23, vangelis wrote:
> Because of the code right above, we're guaranteed that (container ==
> m_renderView) if we get here.
> 

Oops - forgot to remove this based on your previous review.  Will be gone in
next patch.

https://codereview.chromium.org/13828004/diff/7001/Source/WebCore/rendering/R...
Source/WebCore/rendering/RenderLayerCompositor.cpp:1940: noScrollableAncestor =
false;
On 2013/04/16 06:54:23, vangelis wrote:
> Should you set the value of viewportConstrainedNotCompositedReason here and
> below? 

Thanks for catching this - will do it.

https://codereview.chromium.org/13828004/diff/7001/Source/WebCore/rendering/R...
Source/WebCore/rendering/RenderLayerCompositor.cpp:1944: if
(ancestor->hasHorizontalScrollbar() || ancestor->hasVerticalScrollbar())
On 2013/04/16 06:54:23, vangelis wrote:
> Since you know you need to iterate until you get back to the container which
is
> the m_renderView, it seems that you can eliminate the code in lines 1939-1940
> and just use this loop. 
> 
> For the RenderView, is ancestor->hasHorizontalScrollbar() equivalent to
> m_renderView->frameView()->isScrollable() ? 
> 

I don't think we can eliminate those lines of code.  I found that the FrameView
can have scrollbars while the rest of the RenderLayer hierarchy does not.  We
want to make sure we catch these cases, or else we will opt-out of compositing
too often.

Another option is to revert back to the RenderObject version of this patch,
which checks RenderStyle directly for the overflow status.  That version works
without needing to specially check the FrameView as well.

[shawnsingh, 2013-04-16 21:55:43]

New patch uploaded, PTAL.   Thanks!

[shawnsingh, 2013-04-17 18:37:08]

On 2013/04/16 21:55:43, shawnsingh wrote:
> New patch uploaded, PTAL.   Thanks!

vangelis@ and trchen@  do you feel your questions were answered already in the
discussion?

summary:
  - FrameView check is needed since it may have scrollbars when nothing else
does
  - The pinch-zoom WIP by trchen means that this patch will not affect the
pinch-zoom case.
  - Latest patch does pass local testing manual and layout tests, I tried both
linux desktop and mac hi-dpi.

[vangelis, 2013-04-17 19:53:15]

Just a comment on the implementation.  Happy with the rest of it!

https://codereview.chromium.org/13828004/diff/15002/Source/WebCore/rendering/...
File Source/WebCore/rendering/RenderLayerCompositor.cpp (right):

https://codereview.chromium.org/13828004/diff/15002/Source/WebCore/rendering/...
Source/WebCore/rendering/RenderLayerCompositor.cpp:1944: if
(ancestor->hasScrollbars())
I'm wondering whether ancestor->hasScrollbars() will return the same value as
m_renderView->frameView()->isScrollable() when ancestor == container  .  If
that's the case then you could remove lines 1939-1940 .

[shawnsingh, 2013-04-17 20:15:57]

I'll add a comment to clarify this in the patch.

https://codereview.chromium.org/13828004/diff/15002/Source/WebCore/rendering/...
File Source/WebCore/rendering/RenderLayerCompositor.cpp (right):

https://codereview.chromium.org/13828004/diff/15002/Source/WebCore/rendering/...
Source/WebCore/rendering/RenderLayerCompositor.cpp:1944: if
(ancestor->hasScrollbars())
I had empirically checked, with printf debugging on layout tests: 
When (ancestor == container == the RenderView), FrameView::isScrollable() often
returns "true" when ancestor->hasScrollbars() returns "false".

As I understand the code, the FrameView itself is a ScrollView which takes the
responsibility of having the scrollbars associated with the viewport -- the
RenderView (container) doesn't get those scrollbars.  The implementation of
FrameView::isScrollable() does not explicitly check if the FrameView has
scrollbars, but it does cover that condition indirectly, I believe based on
"condition #1" in that code -
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...

[trchen, 2013-04-17 20:38:14]

https://codereview.chromium.org/13828004/diff/15002/Source/WebCore/rendering/...
File Source/WebCore/rendering/RenderLayerCompositor.cpp (right):

https://codereview.chromium.org/13828004/diff/15002/Source/WebCore/rendering/...
Source/WebCore/rendering/RenderLayerCompositor.cpp:1939: if
(m_renderView->frameView() && (m_renderView->frameView()->isScrollable()))
Do we have to check whether container is a transformed element, not the frame?
If true then we don't need this condition.

https://codereview.chromium.org/13828004/diff/15002/Source/WebCore/rendering/...
Source/WebCore/rendering/RenderLayerCompositor.cpp:1943: while (ancestor &&
noScrollableAncestor) {
nits: I prefer to use for loop:
for (RenderLayer* ancestor = layer->parent(); ancestor && anceator->renderer()
!= container; ancestor = ancestor->parent())
IMO it is cleaner to see the terminal condition and you won't forget to iterate
when using continue keyword.
Don't have strong opinion though. Either convention is popular.

https://codereview.chromium.org/13828004/diff/15002/Source/WebCore/rendering/...
Source/WebCore/rendering/RenderLayerCompositor.cpp:1944: if
(ancestor->hasScrollbars())
On 2013/04/17 20:15:57, shawnsingh wrote:
> I had empirically checked, with printf debugging on layout tests: 
> When (ancestor == container == the RenderView), FrameView::isScrollable()
often
> returns "true" when ancestor->hasScrollbars() returns "false".
> 
> As I understand the code, the FrameView itself is a ScrollView which takes the
> responsibility of having the scrollbars associated with the viewport -- the
> RenderView (container) doesn't get those scrollbars.  The implementation of
> FrameView::isScrollable() does not explicitly check if the FrameView has
> scrollbars, but it does cover that condition indirectly, I believe based on
> "condition #1" in that code -
>
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...

Shawn is right. RenderView itself, as a RenderBlock, is never scrollable.
RenderLayerCompositor, which is owned by RenderView, handles the top level
scrolling of a frame.

[shawnsingh, 2013-04-17 20:57:36]

responses below - please let me know if you still want me to change those, given
my responses.  thanks!

https://codereview.chromium.org/13828004/diff/15002/Source/WebCore/rendering/...
File Source/WebCore/rendering/RenderLayerCompositor.cpp (right):

https://codereview.chromium.org/13828004/diff/15002/Source/WebCore/rendering/...
Source/WebCore/rendering/RenderLayerCompositor.cpp:1939: if
(m_renderView->frameView() && (m_renderView->frameView()->isScrollable()))
On 2013/04/17 20:38:14, trchen wrote:
> Do we have to check whether container is a transformed element, not the frame?
> If true then we don't need this condition.

That's exactly what the if-statement above this new code checks for.  So I feel
it is clean and correct to not worry about it here.  How's that sound?

https://codereview.chromium.org/13828004/diff/15002/Source/WebCore/rendering/...
Source/WebCore/rendering/RenderLayerCompositor.cpp:1943: while (ancestor &&
noScrollableAncestor) {
The for-loop version skips the case where ancestor == container.  We need this
loop to include that case, but exit immediately after that.  Rewriting the
for-loop to accomodate that makes it a bit messier, and at that point I think
while loop is actually clearer =)

[trchen, 2013-04-17 21:03:03]

lgtm

https://codereview.chromium.org/13828004/diff/15002/Source/WebCore/rendering/...
File Source/WebCore/rendering/RenderLayerCompositor.cpp (right):

https://codereview.chromium.org/13828004/diff/15002/Source/WebCore/rendering/...
Source/WebCore/rendering/RenderLayerCompositor.cpp:1939: if
(m_renderView->frameView() && (m_renderView->frameView()->isScrollable()))
On 2013/04/17 20:57:36, shawnsingh wrote:
> On 2013/04/17 20:38:14, trchen wrote:
> > Do we have to check whether container is a transformed element, not the
frame?
> > If true then we don't need this condition.
> 
> That's exactly what the if-statement above this new code checks for.  So I
feel
> it is clean and correct to not worry about it here.  How's that sound?

Hmm I didn't realize that transformed container never promote fixed position
layers. I think that's very wrong and needs to be corrected. Anyway it will be
out of scope of this patch. Let's file a bug for later.

https://codereview.chromium.org/13828004/diff/15002/Source/WebCore/rendering/...
Source/WebCore/rendering/RenderLayerCompositor.cpp:1943: while (ancestor &&
noScrollableAncestor) {
On 2013/04/17 20:57:36, shawnsingh wrote:
> The for-loop version skips the case where ancestor == container.  We need this
> loop to include that case, but exit immediately after that.  Rewriting the
> for-loop to accomodate that makes it a bit messier, and at that point I think
> while loop is actually clearer =)

Ah ha you're right. Got your point. :)

[vangelis, 2013-04-17 22:05:29]

https://codereview.chromium.org/13828004/diff/15002/Source/WebCore/rendering/...
File Source/WebCore/rendering/RenderLayerCompositor.cpp (right):

https://codereview.chromium.org/13828004/diff/15002/Source/WebCore/rendering/...
Source/WebCore/rendering/RenderLayerCompositor.cpp:1939: if
(m_renderView->frameView() && (m_renderView->frameView()->isScrollable()))
On 2013/04/17 21:03:03, trchen wrote:
> On 2013/04/17 20:57:36, shawnsingh wrote:
> > On 2013/04/17 20:38:14, trchen wrote:
> > > Do we have to check whether container is a transformed element, not the
> frame?
> > > If true then we don't need this condition.
> > 
> > That's exactly what the if-statement above this new code checks for.  So I
> feel
> > it is clean and correct to not worry about it here.  How's that sound?
> 
> Hmm I didn't realize that transformed container never promote fixed position
> layers. I think that's very wrong and needs to be corrected. Anyway it will be
> out of scope of this patch. Let's file a bug for later.

Actually that's not necessarily bad. If an element is fixed in a transformed
container then it shouldn't really be fixed to the scrolling viewport so it
doesn't need a separate layer.

https://codereview.chromium.org/13828004/diff/15002/Source/WebCore/rendering/...
Source/WebCore/rendering/RenderLayerCompositor.cpp:1944: if
(ancestor->hasScrollbars())
On 2013/04/17 20:38:14, trchen wrote:
> On 2013/04/17 20:15:57, shawnsingh wrote:
> > I had empirically checked, with printf debugging on layout tests: 
> > When (ancestor == container == the RenderView), FrameView::isScrollable()
> often
> > returns "true" when ancestor->hasScrollbars() returns "false".
> > 
> > As I understand the code, the FrameView itself is a ScrollView which takes
the
> > responsibility of having the scrollbars associated with the viewport -- the
> > RenderView (container) doesn't get those scrollbars.  The implementation of
> > FrameView::isScrollable() does not explicitly check if the FrameView has
> > scrollbars, but it does cover that condition indirectly, I believe based on
> > "condition #1" in that code -
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...
> 
> Shawn is right. RenderView itself, as a RenderBlock, is never scrollable.
> RenderLayerCompositor, which is owned by RenderView, handles the top level
> scrolling of a frame.

Ah, ok!

[shawnsingh, 2013-04-17 23:51:05]

jamesr@, vangelis@ PTAL -- same patch as previous with clearer comments.  I
think this patch will meet everyone's approval for landing.

[Vangelis Kokkevis, 2013-04-18 07:34:47]

lgtm

[trchen, 2013-04-19 07:49:58]

On 2013/04/17 22:05:29, vangelis wrote:
>
https://codereview.chromium.org/13828004/diff/15002/Source/WebCore/rendering/...
> File Source/WebCore/rendering/RenderLayerCompositor.cpp (right):
> 
>
https://codereview.chromium.org/13828004/diff/15002/Source/WebCore/rendering/...
> Source/WebCore/rendering/RenderLayerCompositor.cpp:1939: if
> (m_renderView->frameView() && (m_renderView->frameView()->isScrollable()))
> On 2013/04/17 21:03:03, trchen wrote:
> > On 2013/04/17 20:57:36, shawnsingh wrote:
> > > On 2013/04/17 20:38:14, trchen wrote:
> > > > Do we have to check whether container is a transformed element, not the
> > frame?
> > > > If true then we don't need this condition.
> > > 
> > > That's exactly what the if-statement above this new code checks for.  So I
> > feel
> > > it is clean and correct to not worry about it here.  How's that sound?
> > 
> > Hmm I didn't realize that transformed container never promote fixed position
> > layers. I think that's very wrong and needs to be corrected. Anyway it will
be
> > out of scope of this patch. Let's file a bug for later.
> 
> Actually that's not necessarily bad. If an element is fixed in a transformed
> container then it shouldn't really be fixed to the scrolling viewport so it
> doesn't need a separate layer.

It is bad since the transformed container can have threaded scrolling. (Not
enabled by default for most platforms except Android)
In fact I think it is perfect time to remove this restriction after this patch
lands -- no unnecessary layers when none of the container and intermediary
layers can scroll.

However, now I have a question: do we really want to promote fixed-position
layers, when any of the container and intermediary blocks can scroll? I'm
thinking maybe we want to promote fixed-position layers only if some of the
intermediary layers have threaded scrolling. Well, without threaded-scrolling we
still benefit from the fact that scrolling can be done without repainting, but I
don't know if it worth the price of higher memory usage.

[vangelis, 2013-04-19 17:07:40]

On 2013/04/19 07:49:58, trchen wrote:
> On 2013/04/17 22:05:29, vangelis wrote:
> >
>
https://codereview.chromium.org/13828004/diff/15002/Source/WebCore/rendering/...
> > File Source/WebCore/rendering/RenderLayerCompositor.cpp (right):
> > 
> >
>
https://codereview.chromium.org/13828004/diff/15002/Source/WebCore/rendering/...
> > Source/WebCore/rendering/RenderLayerCompositor.cpp:1939: if
> > (m_renderView->frameView() && (m_renderView->frameView()->isScrollable()))
> > On 2013/04/17 21:03:03, trchen wrote:
> > > On 2013/04/17 20:57:36, shawnsingh wrote:
> > > > On 2013/04/17 20:38:14, trchen wrote:
> > > > > Do we have to check whether container is a transformed element, not
the
> > > frame?
> > > > > If true then we don't need this condition.
> > > > 
> > > > That's exactly what the if-statement above this new code checks for.  So
I
> > > feel
> > > > it is clean and correct to not worry about it here.  How's that sound?
> > > 
> > > Hmm I didn't realize that transformed container never promote fixed
position
> > > layers. I think that's very wrong and needs to be corrected. Anyway it
will
> be
> > > out of scope of this patch. Let's file a bug for later.
> > 
> > Actually that's not necessarily bad. If an element is fixed in a transformed
> > container then it shouldn't really be fixed to the scrolling viewport so it
> > doesn't need a separate layer.
> 
> It is bad since the transformed container can have threaded scrolling. (Not
> enabled by default for most platforms except Android)
> In fact I think it is perfect time to remove this restriction after this patch
> lands -- no unnecessary layers when none of the container and intermediary
> layers can scroll.

This discussion is tangential to this patch but I'd like to see an example of
what you mean. I tried a few
different experiments with fixed position elements in overflow containers but
I'm not sure I see how to get fixed pos to stick to an overflow container if
there's a webkit-transform in it as well. 

I put in an example here:
http://www.corp.google.com/~vangelis/overflow.html

> 
> However, now I have a question: do we really want to promote fixed-position
> layers, when any of the container and intermediary blocks can scroll? I'm
> thinking maybe we want to promote fixed-position layers only if some of the
> intermediary layers have threaded scrolling. Well, without threaded-scrolling
we
> still benefit from the fact that scrolling can be done without repainting, but
I
> don't know if it worth the price of higher memory usage.

Since we union damage rects, simple fixed-pos elements can very easily end up
causing full page invalidations. Paint and record times are big performance
bottlenecks so I would say that, yes, we want to composite fixed-pos even if its
scrolling ancestor doesn't do threaded scrolling.

[trchen, 2013-04-19 18:50:09]

On 2013/04/19 17:07:40, vangelis wrote:
> On 2013/04/19 07:49:58, trchen wrote:
> > On 2013/04/17 22:05:29, vangelis wrote:
> > >
> >
>
https://codereview.chromium.org/13828004/diff/15002/Source/WebCore/rendering/...
> > > File Source/WebCore/rendering/RenderLayerCompositor.cpp (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/13828004/diff/15002/Source/WebCore/rendering/...
> > > Source/WebCore/rendering/RenderLayerCompositor.cpp:1939: if
> > > (m_renderView->frameView() && (m_renderView->frameView()->isScrollable()))
> > > On 2013/04/17 21:03:03, trchen wrote:
> > > > On 2013/04/17 20:57:36, shawnsingh wrote:
> > > > > On 2013/04/17 20:38:14, trchen wrote:
> > > > > > Do we have to check whether container is a transformed element, not
> the
> > > > frame?
> > > > > > If true then we don't need this condition.
> > > > > 
> > > > > That's exactly what the if-statement above this new code checks for. 
So
> I
> > > > feel
> > > > > it is clean and correct to not worry about it here.  How's that sound?
> > > > 
> > > > Hmm I didn't realize that transformed container never promote fixed
> position
> > > > layers. I think that's very wrong and needs to be corrected. Anyway it
> will
> > be
> > > > out of scope of this patch. Let's file a bug for later.
> > > 
> > > Actually that's not necessarily bad. If an element is fixed in a
transformed
> > > container then it shouldn't really be fixed to the scrolling viewport so
it
> > > doesn't need a separate layer.
> > 
> > It is bad since the transformed container can have threaded scrolling. (Not
> > enabled by default for most platforms except Android)
> > In fact I think it is perfect time to remove this restriction after this
patch
> > lands -- no unnecessary layers when none of the container and intermediary
> > layers can scroll.
> 
> This discussion is tangential to this patch but I'd like to see an example of
> what you mean. I tried a few
> different experiments with fixed position elements in overflow containers but
> I'm not sure I see how to get fixed pos to stick to an overflow container if
> there's a webkit-transform in it as well. 
> 
> I put in an example here:
> http://www.corp.google.com/%7Evangelis/overflow.html

Here's an example that needs fixed-position compensation for threaded scrolling:
http://pastebin.com/Tnk5caFT
But you're right, the position:fixed doesn't stick to the container's client
rect, instead, it sticks to the container's content (just like
position:absolute). I think the behavior is very confusing. We should open a
separate thread to discuss fixed-position behavior in general.

> > 
> > However, now I have a question: do we really want to promote fixed-position
> > layers, when any of the container and intermediary blocks can scroll? I'm
> > thinking maybe we want to promote fixed-position layers only if some of the
> > intermediary layers have threaded scrolling. Well, without
threaded-scrolling
> we
> > still benefit from the fact that scrolling can be done without repainting,
but
> I
> > don't know if it worth the price of higher memory usage.
> 
> Since we union damage rects, simple fixed-pos elements can very easily end up
> causing full page invalidations. Paint and record times are big performance
> bottlenecks so I would say that, yes, we want to composite fixed-pos even if
its
> scrolling ancestor doesn't do threaded scrolling.

I thought damage rect is managed per-tile. I'd prefer to add a flag for
different behavior for benchmark purpose, but I don't have a strong opinion
about it. Either way is fine to me.

[shawnsingh, 2013-04-20 00:09:09]

jamesr - can you please give OWNERS review?  Its pretty clear to me (cross
sectioning try bot failures) that the try-bot failures are not from this patch. 
I would like to land this as soon as possible.  Thanks.

[jamesr, 2013-04-20 01:24:10]

https://codereview.chromium.org/13828004/diff/39001/Source/core/rendering/Ren...
File Source/core/rendering/RenderLayerCompositor.cpp (right):

https://codereview.chromium.org/13828004/diff/39001/Source/core/rendering/Ren...
Source/core/rendering/RenderLayerCompositor.cpp:1948: while (ancestor &&
noScrollableAncestor) {
this seems too slow.  This function is called from RenderLayer::styleChanged ->
RenderLayerCompositor::updateCompositingState ->
RenderLayerCompositor::updateBacking ->
RenderLayerCompositor::needsToBeComposited ->
RenderLayerCompositor::requiresCompositingLayer

In other words, it's called for every RenderObject that has a RenderLayer
whenever the style changes.  For a full document style recalc, this means that
on every element we're going to do a walk potentially back up to the root of the
render layer tree for every render layer making the operation O(N * depth).  Is
there no better way to figure this out?  Could this be done at a later step
instead of on every style change?

I'm working on trying to defer this work more in general but that's not in place
yet.

https://codereview.chromium.org/13828004/diff/39001/Source/core/rendering/Ren...
Source/core/rendering/RenderLayerCompositor.cpp:1951: if (ancestor->renderer()
== container)
since we've checked explicitly for transformed containers on line 1929, can this
check ever return for something other than the RenderView?

[shawnsingh, 2013-04-21 00:22:21]

Thanks for review, responses below

https://codereview.chromium.org/13828004/diff/39001/Source/core/rendering/Ren...
File Source/core/rendering/RenderLayerCompositor.cpp (right):

https://codereview.chromium.org/13828004/diff/39001/Source/core/rendering/Ren...
Source/core/rendering/RenderLayerCompositor.cpp:1948: while (ancestor &&
noScrollableAncestor) {
> this seems too slow.  

This only occurs for fixed-position elements. The assumption is that (1) there
will be relatively few fixed-pos elements on a page, and (2) They are unlikely
to be too deep in the RenderObject / RenderLayer hierarchy.

Does that change your opinion about slowness?  Do you want me to get some
numbers?

The more general comment you're making is that computeCompositingRequirements is
itself fairly slow, and likely called far more often than needed (whenever style
changes).  In addition to the work you're proposing to call it fewer times per
update, we also have a bug to look at how to improve its performance each time
its called.

https://codereview.chromium.org/13828004/diff/39001/Source/core/rendering/Ren...
Source/core/rendering/RenderLayerCompositor.cpp:1951: if (ancestor->renderer()
== container)
On 2013/04/20 01:24:10, jamesr wrote:
> since we've checked explicitly for transformed containers on line 1929, can
this
> check ever return for something other than the RenderView?

No, it can't.  You're right that container is the renderView at this point in
code.  Did you want me to use isRenderView() instead?  I felt this is more
general.

[shawnsingh, 2013-04-23 21:34:18]

jamesr@  -  I ran some quick numbers.  The conclusion is that this patch is
elegantly negligible.

I checked on google plus, gmail, facebook, cnn, cnet, techcrunch, digg, and
dev.verticalsection.com.

The code which I've added only seems to get invoked about 10-20 times at most
for any given page.  The function requiresCompositingForPosition, does get
called far more often (once per layer, basically), but generally early exits
because the layers are not fixed position.

The tree walk was mostly always skipped, actually.  It got triggered only on
gmail.  In almost every other case, noScrollableAncestor was already true due to
checking the FrameView, so the walk gets skipped for efficiency.

When the walk did execute, it took on the order of 1-2 microseconds, according
to event traces (I assume that's about the limit of its resolution, so it may be
faster, even).

I hope this is compelling enough that performance is totally not an issue here.
=)

[shawnsingh, 2013-04-24 17:47:45]

Still waiting for review please - thanks.

Performance concerns are addressed in comment #32.

[shawnsingh, 2013-04-29 14:35:32]

Still waiting for review.   Performance concerns are addressed in comment #32.

We really should get this done so that we can turn on fixed-position compositing
again for high DPI for m28.  That was a pretty significant performance boost.

[Tom Hudson, 2013-04-29 14:50:24]

https://codereview.chromium.org/13828004/diff/47001/Source/core/rendering/Ren...
File Source/core/rendering/RenderLayerCompositor.cpp (right):

https://codereview.chromium.org/13828004/diff/47001/Source/core/rendering/Ren...
Source/core/rendering/RenderLayerCompositor.cpp:1937: RenderLayer* ancestor =
layer->parent();
Naive question: this function assumes layer is non-NULL (not just here, but
above too)?

[shawnsingh, 2013-04-29 15:57:13]

https://codereview.chromium.org/13828004/diff/47001/Source/core/rendering/Ren...
File Source/core/rendering/RenderLayerCompositor.cpp (right):

https://codereview.chromium.org/13828004/diff/47001/Source/core/rendering/Ren...
Source/core/rendering/RenderLayerCompositor.cpp:1937: RenderLayer* ancestor =
layer->parent();
On 2013/04/29 14:50:24, Tom Hudson wrote:
> Naive question: this function assumes layer is non-NULL (not just here, but
> above too)?

It's a "reasonable assumption" that it will not be null, because the code paths
that call this function are iterating over lists of layers, which are not
intended to have null values when passed to this function.  But it is probably a
good idea to at least add an assertion at the beginning of this function.

Could I please do that in a separate trivial patch?  If there was a case where
layer is null, it would be a bug beyond this patch, anyway.

[jamesr, 2013-04-29 23:20:05]

On 2013/04/29 14:35:32, shawnsingh wrote:
> Still waiting for review.   Performance concerns are addressed in comment #32.

OK, that sounds good for now.  I'm not sure whether this patch actually does the
right thing, but if anyone is feel free to review it without waiting for me.

[shawnsingh, 2013-04-29 23:38:02]

vollick@ could you please review this?  I think your review can help boost
OWNERS comfort level for reviewing.   Otherwise I'm blocked on this for no
technical reasons. =)  Thanks!

[Ian Vollick, 2013-04-30 00:22:21]

Do we need to modify things like RenderLayer::updateScrollableAreaSet to rerun
this check when layers (or the main frame) become or stop being scrollable? Are
we already guaranteed that we'll re-check in this case? Do we have layout tests
that demonstrate this? (Sorry if I missed them).

(For those following the thread -- I was initially concerned that we could have
scrolling, non-ancestor content behind the fixed pos element and we'd end up
doing too much painting. I spoke with Shawn about this offline and he pointed
out that in the future, these bits of scrolling content should be promoted
anyhow, so the fixed pos elements will get promoted due to overlap.)

https://codereview.chromium.org/13828004/diff/47001/Source/core/rendering/Ren...
File Source/core/rendering/RenderLayerCompositor.cpp (right):

https://codereview.chromium.org/13828004/diff/47001/Source/core/rendering/Ren...
Source/core/rendering/RenderLayerCompositor.cpp:1939: if
(ancestor->hasScrollbars())
I believe the check is frameView->containsScrollableArea(layer)

[enne (OOO), 2013-04-30 01:51:35]

https://codereview.chromium.org/13828004/diff/47001/LayoutTests/compositing/g...
File LayoutTests/compositing/geometry/fixed-position-composited-switch.html
(right):

https://codereview.chromium.org/13828004/diff/47001/LayoutTests/compositing/g...
LayoutTests/compositing/geometry/fixed-position-composited-switch.html:61: <body
style="height: 4000px;">
I think this could be more elegantly done with overflow-y: scroll

https://codereview.chromium.org/13828004/diff/47001/Source/core/rendering/Ren...
File Source/core/rendering/RenderLayerCompositor.cpp (right):

https://codereview.chromium.org/13828004/diff/47001/Source/core/rendering/Ren...
Source/core/rendering/RenderLayerCompositor.cpp:1930: bool noScrollableAncestor
= true;
style nit: bools are always easier to understand when phrased positively, i.e.
"hasScrollableAncestor".

https://codereview.chromium.org/13828004/diff/47001/Source/core/rendering/Ren...
Source/core/rendering/RenderLayerCompositor.cpp:1932: // The FrameView has the
scrollbars associated with the top level viewport, so we have to
I'm not sure what you mean by "top level" here.  This could be the FrameView for
an iframe.  Do you have any tests that make sure the right thing is done when
there are multiple FrameViews in play?

https://codereview.chromium.org/13828004/diff/47001/Source/core/rendering/Ren...
Source/core/rendering/RenderLayerCompositor.cpp:1935: noScrollableAncestor =
false;
I don't quite understand how you can set this to false here.  What happens if
there is a scrollable div between this layer and its frame in the RenderLayer
ancestor chain, but the frame is not scrollable?

Is the implicit argument here that if the overflow div is not composited that it
will fully invalidate when scrolling, so promoting the fixed pos layer is a
waste.  If the overflow div is composited then the fixed pos layer will only get
composited due to overlap? (Although, what code prevents it from incorrectly
sharing the same backing if it does overlap?)

https://codereview.chromium.org/13828004/diff/47001/Source/core/rendering/Ren...
Source/core/rendering/RenderLayerCompositor.cpp:1941: if (ancestor->renderer()
== container)
I think it would be clearer to call container m_renderView here.

[shawnsingh, 2013-04-30 20:53:51]

Thanks for the reviews!   A few responses below.   I'm working on another patch
to address the feedback from both of you.

https://codereview.chromium.org/13828004/diff/47001/Source/core/rendering/Ren...
File Source/core/rendering/RenderLayerCompositor.cpp (right):

https://codereview.chromium.org/13828004/diff/47001/Source/core/rendering/Ren...
Source/core/rendering/RenderLayerCompositor.cpp:1935: noScrollableAncestor =
false;
On 2013/04/30 01:51:35, enne wrote:
> I don't quite understand how you can set this to false here.  What happens if
> there is a scrollable div between this layer and its frame in the RenderLayer
> ancestor chain, but the frame is not scrollable?
> 


Setting it to false means that we should still composite the fixed-position
element... It won't matter if there is any additional scrolling div in the
RenderLayer ancestor change, since we already find that the FrameView is
scrollable.

You're right - "noScrollableAncestor" should be flipped to
"hasScrollableAncestor", and that should make this clearer.

Does that clear up the confusion?


> Is the implicit argument here that if the overflow div is not composited that
it
> will fully invalidate when scrolling, so promoting the fixed pos layer is a
> waste.  If the overflow div is composited then the fixed pos layer will only
get
> composited due to overlap? (Although, what code prevents it from incorrectly
> sharing the same backing if it does overlap?)


That's almost my argument, yes - but I should clarify where we want to opt-in. 
Here's the way I see it:

CASE 1: fixed-pos elements that have scrollable ancestors.  we definitely want
to composite the fixed-pos element.

CASE 2: fixed-pos elements on top of scrollable siblings.  eventually we should
rely on compositing the scroll div, and then getting overlap, just like you
said.  Until then, we won't benefit much from compositing the fixed-pos element
in the common case anyway, since the scrolling div will still have to repaint.

CASE 3: fixed-pos elements that have other non-scroll related things changing
underneath.  It seems we should also rely on overlap here, too.  If any good
reason requires us to opt-in here because the changing content underneath is not
composited, we can handle it when it comes up.

Case #1 is the one we will always opt-in.  So we will not incorrectly sharing
the same backing.  Case #2 will definitely have a different backing.  Case #3 I
feel is premature to optimize for, unless we already have use cases to worry
about.

[shawnsingh, 2013-04-30 22:49:44]

https://codereview.chromium.org/13828004/diff/47001/LayoutTests/compositing/g...
File LayoutTests/compositing/geometry/fixed-position-composited-switch.html
(right):

https://codereview.chromium.org/13828004/diff/47001/LayoutTests/compositing/g...
LayoutTests/compositing/geometry/fixed-position-composited-switch.html:61: <body
style="height: 4000px;">
On 2013/04/30 01:51:35, enne wrote:
> I think this could be more elegantly done with overflow-y: scroll

Unfortunately, this doesn't work.  The FrameView doesn't consider itself
scrollable unless it has content that exceeds the size of the viewport.  Also
tried this with additional <div> tags and same issue.

[shawnsingh, 2013-05-01 00:40:48]

PTAL
- addressed all feedback except the overflow-y: scroll suggestion (see comment
#42)
- added one more layout test,
fixed-position-nonscrollable-iframes-in-scrollable-page.html to check that it
works as intended with multiple FrameViews.

thanks!

[shawnsingh, 2013-05-01 22:45:44]

PTAL - Branch point is less than 5 days away.  I still need to land one more
patch after this, before branch, to re-enable fixed-position compositing on
high-DPI.  I totally understand that reviewers may be quite saturated at this
busy time, but it should only take 10 minutes to re-review, and it will be a
solid performance gain for high-DPI users on many common pages to re-enable
fixed-position compositing.

[enne (OOO), 2013-05-02 01:04:06]

lgtm

https://codereview.chromium.org/13828004/diff/61001/Source/core/rendering/Ren...
File Source/core/rendering/RenderLayerCompositor.cpp (right):

https://codereview.chromium.org/13828004/diff/61001/Source/core/rendering/Ren...
Source/core/rendering/RenderLayerCompositor.cpp:1979: hasScrollableAncestor =
true;
Thanks, this is way clearer.

[shawnsingh, 2013-05-02 21:04:53]

Hi Dimitri - would you please be willing to give an OWNERS review for 10 lines
of change in Source/WebKit/chromium/tests/ ?

James has already said he's too busy to review (comment #37).

Enne has already reviewed similar code changes in their OWNER portion of this
patch.

We would really benefit from this patch landing by tomorrow to make m28 branch
for performance gains on fixed-position elements.

Either way, thank you very much.

[dglazkov, 2013-05-02 21:28:52]

LGTM to Source/WebKit/chromium/tests changes.

[shawnsingh, 2013-05-03 00:14:03]

Committed patchset #9 manually as r149624 (presubmit successful).