Simply rules for compositing fixed position elements

Simply rules for compositing fixed position elements

Previously we had many complex conditions for determining whether a fixed
position element ought to be composited. This CL simplifies those conditions to
just checking whether the fixed position element's container is actually the
viewport.

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

[abarth-chromium, 2014-08-07 06:20:36]

@vollick: I'd like to get your opinion on this CL.  Do you think this change is
reasonable?  If not, is there some other way we could simplify this code that
would be reasonable?  Thanks!!

[Ian Vollick, 2014-08-07 15:56:16]

This seems reasonable, but could cause an increase in layer count. Have you
tested gmail after this change?

https://codereview.chromium.org/449723003/diff/20001/Source/core/rendering/co...
File Source/core/rendering/compositing/CompositingReasonFinder.cpp (left):

https://codereview.chromium.org/449723003/diff/20001/Source/core/rendering/co...
Source/core/rendering/compositing/CompositingReasonFinder.cpp:220:
*viewportConstrainedNotCompositedReason =
RenderLayer::NotCompositedForUnscrollableAncestors;
The CompositingInputsUpdater could be trivially updated to compute
has-a-scroller-on-the-parent-chain. Maybe we could add this and still avoid
promoting when when nothing's scrolling?

https://codereview.chromium.org/449723003/diff/20001/Source/core/rendering/co...
Source/core/rendering/compositing/CompositingReasonFinder.cpp:232:
*viewportConstrainedNotCompositedReason =
RenderLayer::NotCompositedForNoVisibleContent;
I'm glad this is going - I think this should have been handled elsewhere.
There's no reason that fixed position elements should be special wrt visibility.

https://codereview.chromium.org/449723003/diff/20001/Source/core/rendering/co...
Source/core/rendering/compositing/CompositingReasonFinder.cpp:236: // Fixed
position elements that are invisible in the current view don't get their own
layer.
This also seems like a nice chunk to remove -- I think that viewporting is the
correct, general solution for this special case.

[abarth-chromium, 2014-08-07 17:32:05]

https://codereview.chromium.org/449723003/diff/20001/Source/core/rendering/co...
File Source/core/rendering/compositing/CompositingReasonFinder.cpp (left):

https://codereview.chromium.org/449723003/diff/20001/Source/core/rendering/co...
Source/core/rendering/compositing/CompositingReasonFinder.cpp:220:
*viewportConstrainedNotCompositedReason =
RenderLayer::NotCompositedForUnscrollableAncestors;
This check doesn't really make sense.  We only composite fixed position elements
that have the viewport as their container.  Why does it matter whether there's
anything scrollable on their RenderLayer parent chain?  We could check whether
the viewport scrolls overflow if you like.  That makes sense.

[Ian Vollick, 2014-08-07 17:44:26]

https://codereview.chromium.org/449723003/diff/20001/Source/core/rendering/co...
File Source/core/rendering/compositing/CompositingReasonFinder.cpp (left):

https://codereview.chromium.org/449723003/diff/20001/Source/core/rendering/co...
Source/core/rendering/compositing/CompositingReasonFinder.cpp:220:
*viewportConstrainedNotCompositedReason =
RenderLayer::NotCompositedForUnscrollableAncestors;
On 2014/08/07 17:32:04, abarth wrote:
> This check doesn't really make sense.  We only composite fixed position
elements
> that have the viewport as their container.  Why does it matter whether there's
> anything scrollable on their RenderLayer parent chain?  We could check whether
> the viewport scrolls overflow if you like.  That makes sense.

Sorry, yup, you're right - the situation I was imagining is bogus. I do like the
idea of avoiding promotion when the viewport can't scroll.

[abarth-chromium, 2014-08-07 18:15:49]

On 2014/08/07 at 17:44:26, vollick wrote:
> Sorry, yup, you're right - the situation I was imagining is bogus. I do like
the idea of avoiding promotion when the viewport can't scroll.

Done.

[Ian Vollick, 2014-08-07 18:18:16]

lgtm

https://codereview.chromium.org/449723003/diff/40001/LayoutTests/TestExpectat...
File LayoutTests/TestExpectations (right):

https://codereview.chromium.org/449723003/diff/40001/LayoutTests/TestExpectat...
LayoutTests/TestExpectations:486: # crbug.com/307781 [ Android ]
svg/overflow/overflow-on-outermost-svg-element-in-xhtml-defaults.xhtml [ Failure
]
Should these lines just be deleted?

https://codereview.chromium.org/449723003/diff/40001/Source/core/rendering/co...
File Source/core/rendering/compositing/CompositingReasonFinder.cpp (right):

https://codereview.chromium.org/449723003/diff/40001/Source/core/rendering/co...
Source/core/rendering/compositing/CompositingReasonFinder.cpp:179: &&
m_renderView.frameView()->isScrollable();
Gorgeous.

[abarth-chromium, 2014-08-07 18:19:11]

https://codereview.chromium.org/449723003/diff/40001/LayoutTests/TestExpectat...
File LayoutTests/TestExpectations (right):

https://codereview.chromium.org/449723003/diff/40001/LayoutTests/TestExpectat...
LayoutTests/TestExpectations:486: # crbug.com/307781 [ Android ]
svg/overflow/overflow-on-outermost-svg-element-in-xhtml-defaults.xhtml [ Failure
]
Yeah, probably.

[abarth-chromium, 2014-08-07 18:19:21]

The CQ bit was checked by abarth@chromium.org

[commit-bot: I haz the power, 2014-08-07 18:20:27]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/abarth@chromium.org/449723003/60001

[commit-bot: I haz the power, 2014-08-07 20:19:02]

Change committed as 179749