Remove call to setMayNeedPaintInvalidation in RenderLayer::updateLayerPositionRecursive

Remove call to setMayNeedPaintInvalidation in RenderLayer::updateLayerPositionRecursive

The call to setMayNeedPaintInvalidation was compensating for a bug in RenderBox::clippedOverflowRectForPaintInvalidation.
Previously, that code considered only whether the current RenderLayer had visible
contents instead of asking whether any descendant was visible.  This change will
cause us to issue more invalidations in cases where some parts of the subtree are
visible, but they'll be the same ones we issue in the normal case when elements
are visible.

Removing this call will let us remove the RenderGeometryMap in
updateLayerPositionsAfterLayout because it's now almost entirely unused.

R=esprehn@chromium.org
BUG=383636
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=176060

[esprehn, 2014-06-12 21:00:34]

lgtm

[commit-bot: I haz the power, 2014-06-12 21:01:26]

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

[commit-bot: I haz the power, 2014-06-12 22:36:24]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  win_gpu_triggered_tests on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu_triggered...)

[commit-bot: I haz the power, 2014-06-12 22:36:38]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-06-12 22:36:39]

Try jobs failed on following builders:
  win_gpu_triggered_tests on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu_triggered...)

[esprehn, 2014-06-12 23:09:20]

The CQ bit was checked by esprehn@chromium.org

[commit-bot: I haz the power, 2014-06-12 23:09:37]

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

[commit-bot: I haz the power, 2014-06-12 23:43:51]

Change committed as 176060

[ojan, 2014-06-13 02:07:21]

Amazing. Can you explain more what the case is where we now do more
invalidations?

https://codereview.chromium.org/337593002/diff/20001/LayoutTests/TestExpectat...
File LayoutTests/TestExpectations (right):

https://codereview.chromium.org/337593002/diff/20001/LayoutTests/TestExpectat...
LayoutTests/TestExpectations:1012: Bug(abarth)
fast/repaint/abspos-shift-image-incorrect-repaint.html [ NeedsRebaseline ]
In the future, it's best if you rebaseline for the platform you're working on
and NeedsRebaseline the tests that have platform-specific expectations. This
lets the reviewer see what's actually changed.

[abarth-chromium, 2014-06-13 05:47:46]

On 2014/06/13 at 02:07:21, ojan wrote:
> Amazing. Can you explain more what the case is where we now do more
invalidations?

The case is when you have a visibility: hidden subtree that contains some
visibility: visible descendants.  Previously, we'd cut off the search for
invalidations if the visibility: visible descendants were in a separate
RenderLayer.  The tree walk in updateLayerPositionsAfterLayout would then find
them again and mark them as setMayNeedPaintInvalidation.  Now, we don't cut off
the search for invalidations unless you have no visibility: visible descendants.
 That means as long as you have at least one visibility: visible descendant,
you'll generate invalidations as if your subtree was visibility: visible.

>
https://codereview.chromium.org/337593002/diff/20001/LayoutTests/TestExpectat...
> File LayoutTests/TestExpectations (right):
> 
>
https://codereview.chromium.org/337593002/diff/20001/LayoutTests/TestExpectat...
> LayoutTests/TestExpectations:1012: Bug(abarth)
fast/repaint/abspos-shift-image-incorrect-repaint.html [ NeedsRebaseline ]
> In the future, it's best if you rebaseline for the platform you're working on
and NeedsRebaseline the tests that have platform-specific expectations. This
lets the reviewer see what's actually changed.

Sorry, Elliott and looked at these tests in detail in person, but I realize
that's hard to follow along via the code review tool.  The actual text diffs
were a bit hard to decipher because we're generating different sets of rects
that cover the same pixels, but we looked at each test individually to verify
that we didn't regress.

[ojan, 2014-06-13 05:58:46]

On 2014/06/13 05:47:46, abarth wrote:
> On 2014/06/13 at 02:07:21, ojan wrote:
> > Amazing. Can you explain more what the case is where we now do more
> invalidations?
> 
> The case is when you have a visibility: hidden subtree that contains some
> visibility: visible descendants.  Previously, we'd cut off the search for
> invalidations if the visibility: visible descendants were in a separate
> RenderLayer.  The tree walk in updateLayerPositionsAfterLayout would then find
> them again and mark them as setMayNeedPaintInvalidation.  Now, we don't cut
off
> the search for invalidations unless you have no visibility: visible
descendants.
>  That means as long as you have at least one visibility: visible descendant,
> you'll generate invalidations as if your subtree was visibility: visible.

Thanks for the explanation. This is clearly the right trade-off for now, but I
wonder if it's worth putting a FIXME in the appropriate place to avoid doing
invalidations for the non-visible parts of the subtree. As silly as visibility
inheriting is, some very prominent sites make heavy use of this (e.g. gmail).

>
https://codereview.chromium.org/337593002/diff/20001/LayoutTests/TestExpectat...
> > File LayoutTests/TestExpectations (right):
> > 
> >
>
https://codereview.chromium.org/337593002/diff/20001/LayoutTests/TestExpectat...
> > LayoutTests/TestExpectations:1012: Bug(abarth)
> fast/repaint/abspos-shift-image-incorrect-repaint.html [ NeedsRebaseline ]
> > In the future, it's best if you rebaseline for the platform you're working
on
> and NeedsRebaseline the tests that have platform-specific expectations. This
> lets the reviewer see what's actually changed.
> 
> Sorry, Elliott and looked at these tests in detail in person, but I realize
> that's hard to follow along via the code review tool.  The actual text diffs
> were a bit hard to decipher because we're generating different sets of rects
> that cover the same pixels, but we looked at each test individually to verify
> that we didn't regress.

No worries. I was confident you did a thorough job in evaluating the new
results. It just makes it easier to understand the patch if I can look at test
cases and see what actually changed. Which will also be true when looking at
this patch a year from now.

[abarth-chromium, 2014-06-13 06:35:29]

On 2014/06/13 at 05:58:46, ojan wrote:
> Thanks for the explanation. This is clearly the right trade-off for now, but I
wonder if it's worth putting a FIXME in the appropriate place to avoid doing
invalidations for the non-visible parts of the subtree. As silly as visibility
inheriting is, some very prominent sites make heavy use of this (e.g. gmail).

Sure, adding a FIXME makes sense.  Thanks!