Allow paint invalidation containers to cross frame boundaries.

Allow paint invalidation containers to cross frame boundaries.

Previously, the *actual* paint invalidation container, meaning the enclosing
compositing layer / root RenderView, could already have been across a frame boundary. The logic to do this correctly was done via special code in RenderView.

Instead, generalize the existing mechanisms to find a paint invalidation container
and map rects to repaint container coordinate space to cross frame boundaries.
This simplifies the code, and also causes paint invalidation rects to always be stored
in the coordinate space of their graphics layer backing. The latter is important if we want
to use these rects for determining which parts of a graphics layer need to be painted.

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

[leviw_travelin_and_unemployed, 2014-08-06 18:54:00]

https://codereview.chromium.org/445793002/diff/60001/Source/core/rendering/Re...
File Source/core/rendering/RenderView.cpp (right):

https://codereview.chromium.org/445793002/diff/60001/Source/core/rendering/Re...
Source/core/rendering/RenderView.cpp:451: const RenderLayerModelObject*
paintInvalidationContainer = containerForPaintInvalidation();
Can we not use PaintInvalidationState::containerForPaintInvalidation() here?

[chrishtr, 2014-08-06 21:59:56]

https://codereview.chromium.org/445793002/diff/60001/Source/core/rendering/Re...
File Source/core/rendering/RenderView.cpp (right):

https://codereview.chromium.org/445793002/diff/60001/Source/core/rendering/Re...
Source/core/rendering/RenderView.cpp:451: const RenderLayerModelObject*
paintInvalidationContainer = containerForPaintInvalidation();
On 2014/08/06 18:54:00, leviw wrote:
> Can we not use PaintInvalidationState::containerForPaintInvalidation() here?

Done.

[leviw_travelin_and_unemployed, 2014-08-06 22:04:32]

lgtm

https://codereview.chromium.org/445793002/diff/80001/Source/core/rendering/Re...
File Source/core/rendering/RenderView.cpp (right):

https://codereview.chromium.org/445793002/diff/80001/Source/core/rendering/Re...
Source/core/rendering/RenderView.cpp:512: if (!paintInvalidationContainer ||
paintInvalidationContainer == this)
What's the case where paintInvalidationContainer is null here? Can we make that
case go away?

[chrishtr, 2014-08-06 22:54:21]

https://codereview.chromium.org/445793002/diff/80001/Source/core/rendering/Re...
File Source/core/rendering/RenderView.cpp (right):

https://codereview.chromium.org/445793002/diff/80001/Source/core/rendering/Re...
Source/core/rendering/RenderView.cpp:512: if (!paintInvalidationContainer ||
paintInvalidationContainer == this)
On 2014/08/06 22:04:32, leviw wrote:
> What's the case where paintInvalidationContainer is null here? Can we make
that
> case go away?

I don't think there is one. Fixed and added ASSERT(). 
RenderObject::adjustCompositedContainerForSpecialAncestors() has special code to
return view() if
the paintInvalidationContainer is 0. I'll try in another patch to unify that
logic into
RenderLayer::enclosingLayerForPaintInvalidationCrossingFrameBoundaries()
also.

[chrishtr, 2014-08-06 22:54:25]

The CQ bit was checked by chrishtr@chromium.org

[commit-bot: I haz the power, 2014-08-06 22:55:01]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/chrishtr@chromium.org/445793002/100001

[commit-bot: I haz the power, 2014-08-07 00:14:00]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  linux_blink_dbg on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/linux_blink_dbg/builds/2...)
  linux_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/linux_blink_rel/builds/1...)
  mac_blink_compile_dbg on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/mac_blink_compile_dbg/bu...)
  mac_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/mac_blink_rel/builds/18726)
  win_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/win_blink_rel/builds/21453)

[commit-bot: I haz the power, 2014-08-07 00:22:16]

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

[commit-bot: I haz the power, 2014-08-07 00:22:17]

Try jobs failed on following builders:
  linux_blink_dbg on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/linux_blink_dbg/builds/2...)

[chrishtr, 2014-08-07 18:49:24]

Updated to have RenderObject::absoluteClippedOverflowRect() use the view() as
its "paint invalidation container" (it's
abusing terminology to do this; I intend to fix this in a
subsequent patch).

Also updated to have
RenderObject::adjustCompositedContainerForSpecialAncestors() 
return the root document RenderView rather than the local frame in its final
fallback logic.

[leviw_travelin_and_unemployed, 2014-08-07 18:54:05]

lgtm

https://codereview.chromium.org/445793002/diff/120001/Source/core/rendering/R...
File Source/core/rendering/RenderObject.cpp (right):

https://codereview.chromium.org/445793002/diff/120001/Source/core/rendering/R...
Source/core/rendering/RenderObject.cpp:1453: while (!paintInvalidationContainer
&& renderView->frame()->ownerRenderer())
It seems like we'd really want to put this after the paintInvalidationContainer
null check.

if (paintInvalidationContainer)
    return paintInvalidationContainer;

RenderView*...

[chrishtr, 2014-08-07 20:20:27]

https://codereview.chromium.org/445793002/diff/120001/Source/core/rendering/R...
File Source/core/rendering/RenderObject.cpp (right):

https://codereview.chromium.org/445793002/diff/120001/Source/core/rendering/R...
Source/core/rendering/RenderObject.cpp:1453: while (!paintInvalidationContainer
&& renderView->frame()->ownerRenderer())
On 2014/08/07 18:54:05, leviw wrote:
> It seems like we'd really want to put this after the
paintInvalidationContainer
> null check.
> 
> if (paintInvalidationContainer)
>     return paintInvalidationContainer;
> 
> RenderView*...

Done.

[chrishtr, 2014-08-07 20:47:01]

Updated two ASSERTS to not trigger during document destruction. During such a
destruction, the render tree pointers are removed before document element
pointers, leading to a fails positive on the intent of the ASSERT.

[chrishtr, 2014-08-07 20:50:00]

The CQ bit was checked by chrishtr@chromium.org

[commit-bot: I haz the power, 2014-08-07 20:51:11]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/chrishtr@chromium.org/445793002/160001

[commit-bot: I haz the power, 2014-08-07 22:33:08]

Change committed as 179762

[abarth-chromium, 2014-08-14 22:34:52]

Could this issue have caused
https://code.google.com/p/chromium/issues/detail?id=403734#c4

(See comment #4 for a reduction.)

[leviw_travelin_and_unemployed, 2014-08-14 22:47:39]

A revert of this CL (patchset #10) has been created in
https://codereview.chromium.org/469313003/ by leviw@chromium.org.

The reason for reverting is: Broke iframe repainting:
https://code.google.com/p/chromium/issues/detail?id=403734.

[philipj_slow, 2014-08-28 14:02:28]

philipj@opera.com changed reviewers:
+ philipj@opera.com

[philipj_slow, 2014-08-28 14:02:28]

https://codereview.chromium.org/445793002/diff/80001/Source/core/rendering/Re...
File Source/core/rendering/RenderView.cpp (right):

https://codereview.chromium.org/445793002/diff/80001/Source/core/rendering/Re...
Source/core/rendering/RenderView.cpp:512: if (!paintInvalidationContainer ||
paintInvalidationContainer == this)
On 2014/08/06 22:54:20, chrishtr wrote:
> On 2014/08/06 22:04:32, leviw wrote:
> > What's the case where paintInvalidationContainer is null here? Can we make
> that
> > case go away?
> 
> I don't think there is one. Fixed and added ASSERT(). 
> RenderObject::adjustCompositedContainerForSpecialAncestors() has special code
to
> return view() if
> the paintInvalidationContainer is 0. I'll try in another patch to unify that
> logic into
> RenderLayer::enclosingLayerForPaintInvalidationCrossingFrameBoundaries()
> also.

There is such a case in RenderObject::selectionRect(), which simply passes 0 for
the paintInvalidationContainer which eventually ends up here and fails the
assert:
https://code.google.com/p/chromium/issues/detail?id=408495

[chrishtr, 2014-08-28 16:19:02]

On 2014/08/28 at 14:02:28, philipj wrote:
>
https://codereview.chromium.org/445793002/diff/80001/Source/core/rendering/Re...
> File Source/core/rendering/RenderView.cpp (right):
> 
>
https://codereview.chromium.org/445793002/diff/80001/Source/core/rendering/Re...
> Source/core/rendering/RenderView.cpp:512: if (!paintInvalidationContainer ||
paintInvalidationContainer == this)
> On 2014/08/06 22:54:20, chrishtr wrote:
> > On 2014/08/06 22:04:32, leviw wrote:
> > > What's the case where paintInvalidationContainer is null here? Can we make
> > that
> > > case go away?
> > 
> > I don't think there is one. Fixed and added ASSERT(). 
> > RenderObject::adjustCompositedContainerForSpecialAncestors() has special
code to
> > return view() if
> > the paintInvalidationContainer is 0. I'll try in another patch to unify that
> > logic into
> > RenderLayer::enclosingLayerForPaintInvalidationCrossingFrameBoundaries()
> > also.
> 
> There is such a case in RenderObject::selectionRect(), which simply passes 0
for the paintInvalidationContainer which eventually ends up here and fails the
assert:
> https://code.google.com/p/chromium/issues/detail?id=408495

Thanks for that pointer, will fix.

[philipj_slow, 2014-08-28 18:04:11]

I posted a review earlier today: https://codereview.chromium.org/516933002/