Introducing LayoutObject::mapToVisibleRectInContainerSpace

This is a rename and slight modification to mapRectToPaintInvalidationBacking.

The rename is for clarity (not for brevity, clearly).

There are two changes in functionality:

- Overflow clipping is done for all layers in the intermediate hierarchy; previously, only non-composited layers applied clipping.

- Overflow clipping is *not* done for the paint invalidation container itself; previously it was.

The reason for the change in functionality is to support the IntersectionObserver use case (see bug).

BUG=540528
Committed: https://crrev.com/a7fdc1aef78ed0f3ea6d66351498b4f3e4a9c3b5
Cr-Commit-Position: refs/heads/master@{#364567}

[szager1, 2015-12-09 22:39:36]

Description was changed from

==========
Don't do overflow clipping on paint invalidation container.

That's what the usesCompositedScrolling() clause effectively did;
this patch makes mapRectToPaintInvalidationBacking suitable to use
for Intersection Observer.

BUG=
==========

to

==========
Don't do overflow clipping on paint invalidation container.

That's what the usesCompositedScrolling() clause effectively did;
this patch makes mapRectToPaintInvalidationBacking suitable to use
for Intersection Observer.

BUG=
==========

[szager1, 2015-12-09 22:39:36]

szager@chromium.org changed reviewers:
+ chrishtr@chromium.org, leviw@chromium.org

[leviw_travelin_and_unemployed, 2015-12-09 23:06:26]

I'd put the IntersectionObserver bug into BUG=

https://codereview.chromium.org/1516683002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/layout/LayoutBox.cpp (left):

https://codereview.chromium.org/1516683002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/LayoutBox.cpp:950: // Do not clip scroll
layer contents because the compositor expects the whole layer
You said there's a use that prevents us from asserting don't cross a composited
scrolling boundary, no? Otherwise we should have that assert.

https://codereview.chromium.org/1516683002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/layout/LayoutBox.cpp (right):

https://codereview.chromium.org/1516683002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/LayoutBox.cpp:1928: if (o !=
paintInvalidationContainer && o->hasOverflowClip()) {
May be worthy of a comment?

[szager1, 2015-12-09 23:21:26]

https://codereview.chromium.org/1516683002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/layout/LayoutBox.cpp (left):

https://codereview.chromium.org/1516683002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/LayoutBox.cpp:950: // Do not clip scroll
layer contents because the compositor expects the whole layer
On 2015/12/09 23:06:26, leviw wrote:
> You said there's a use that prevents us from asserting don't cross a
composited
> scrolling boundary, no? Otherwise we should have that assert.

The use is IntersectionObserver :)

https://codereview.chromium.org/1516683002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/layout/LayoutBox.cpp (right):

https://codereview.chromium.org/1516683002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/LayoutBox.cpp:1928: if (o !=
paintInvalidationContainer && o->hasOverflowClip()) {
On 2015/12/09 23:06:26, leviw wrote:
> May be worthy of a comment?

I'm not exactly sure what the comment should be.  I could copy the comment which
I deleted from applyCachedClipAndScrollOffsetForPaintInvalidation, but that kind
of obscures the fact that we actually do this because it's what works for the
two known applications of this method.  Maybe just a top-level comment on the
method that describe what it does?

[chrishtr, 2015-12-09 23:40:34]

Let's rename this method in the same patch, and specify the semantics w.r.t.
clipping of the
root element clipping.

[szager1, 2015-12-10 06:29:54]

Description was changed from

==========
Don't do overflow clipping on paint invalidation container.

That's what the usesCompositedScrolling() clause effectively did;
this patch makes mapRectToPaintInvalidationBacking suitable to use
for Intersection Observer.

BUG=
==========

to

==========
Don't do overflow clipping on paint invalidation container.

That's what the usesCompositedScrolling() clause effectively did;
this patch makes mapRectToPaintInvalidationBacking suitable to use
for Intersection Observer.

BUG=540528
==========

[szager1, 2015-12-10 18:45:03]

Description was changed from

==========
Don't do overflow clipping on paint invalidation container.

That's what the usesCompositedScrolling() clause effectively did;
this patch makes mapRectToPaintInvalidationBacking suitable to use
for Intersection Observer.

BUG=540528
==========

to

==========
Also, don't do overflow clipping on paint invalidation container.  That's what
the usesCompositedScrolling() clause effectively did;
this patch makes mapRectToPaintInvalidationBacking suitable to use
for Intersection Observer.

BUG=540528
==========

[szager1, 2015-12-10 18:52:24]

PTAL

[leviw_travelin_and_unemployed, 2015-12-10 19:05:50]

The code looks good. I'm happy with the name.

Can you clean up the description a bit? The subject line needs to be copied to
the description and the whole thing should be wrapped to the magical number 72.
Not 71 shall you wrap, and 80 is right out!

I'd also change the description to not imply there are no functional changes.

[chrishtr, 2015-12-10 19:13:36]

https://codereview.chromium.org/1516683002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/LayoutObject.h (right):

https://codereview.chromium.org/1516683002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutObject.h:1089: virtual void
visibleRectInContainerSpace(const LayoutBoxModelObject*
paintInvalidationContainer, LayoutRect&, const PaintInvalidationState*) const;
s/paintInvalidationContainer/container/

Since this method does not return a value, and instead mutates one of them,
maybe a better name is mapToVisibleRectInContainerSpace().

[chrishtr, 2015-12-10 19:17:13]

https://codereview.chromium.org/1516683002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/LayoutObject.h (right):

https://codereview.chromium.org/1516683002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutObject.h:1087: // rect in the
coordinate space of paintInvalidationContainer.  If intermediate containers have
overflow
clipping or scrolling of any kind

https://codereview.chromium.org/1516683002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutObject.h:1088: // clipping, it is
applied; but overflow clipping is *not* applied for paintInvalidationContainer
itself.
But the scroll offset of paintInvalidationContainer does get applied.

[szager1, 2015-12-10 19:29:42]

Description was changed from

==========
Also, don't do overflow clipping on paint invalidation container.  That's what
the usesCompositedScrolling() clause effectively did;
this patch makes mapRectToPaintInvalidationBacking suitable to use
for Intersection Observer.

BUG=540528
==========

to

==========
This is a rename and slight modification to mapRectToPaintInvalidationBacking.

The rename is for clarity (not for brevity, clearly).

There are two changes in functionality:

- Overflow clipping is done for all layers in the intermediate hierarchy;
previously, only non-composited layers applied clipping.

- Overflow clipping is *not* done for the paint invalidation container itself;
previously it was.


The reason for the change in functionality is to support the
IntersectionObserver use case (see bug).

BUG=540528
==========

[szager1, 2015-12-10 19:30:12]

Updated description and renamed to mapToVisibleRectInContainerSpace.

[szager1, 2015-12-10 20:25:09]

https://codereview.chromium.org/1516683002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/LayoutObject.h (right):

https://codereview.chromium.org/1516683002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutObject.h:1087: // rect in the
coordinate space of paintInvalidationContainer.  If intermediate containers have
overflow
On 2015/12/10 19:17:13, chrishtr wrote:
> clipping or scrolling of any kind

Done.

https://codereview.chromium.org/1516683002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutObject.h:1088: // clipping, it is
applied; but overflow clipping is *not* applied for paintInvalidationContainer
itself.
On 2015/12/10 19:17:13, chrishtr wrote:
> But the scroll offset of paintInvalidationContainer does get applied.

Isn't that redundant?  The method name already indicates that it maps the rect
to the container space.

https://codereview.chromium.org/1516683002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutObject.h:1089: virtual void
visibleRectInContainerSpace(const LayoutBoxModelObject*
paintInvalidationContainer, LayoutRect&, const PaintInvalidationState*) const;
On 2015/12/10 19:13:35, chrishtr wrote:
> s/paintInvalidationContainer/container/
> 
> Since this method does not return a value, and instead mutates one of them,
> maybe a better name is mapToVisibleRectInContainerSpace().

Done.

[chrishtr, 2015-12-10 21:19:40]

lgtm

https://codereview.chromium.org/1516683002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/LayoutObject.h (right):

https://codereview.chromium.org/1516683002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutObject.h:1088: // clipping, it is
applied; but overflow clipping is *not* applied for paintInvalidationContainer
itself.
On 2015/12/10 at 20:25:09, szager1 wrote:
> On 2015/12/10 19:17:13, chrishtr wrote:
> > But the scroll offset of paintInvalidationContainer does get applied.
> 
> Isn't that redundant?  The method name already indicates that it maps the rect
to the container space.

I guess. Seems harmless to me but ok.

[szager1, 2015-12-10 23:45:42]

The CQ bit was checked by szager@chromium.org

[szager1, 2015-12-10 23:45:43]

The patchset sent to the CQ was uploaded after l-g-t-m from
chrishtr@chromium.org
Link to the patchset: https://codereview.chromium.org/1516683002/#ps110001
(title: "Fix flipping logic")

[commit-bot: I haz the power, 2015-12-10 23:46:16]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1516683002/110001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1516683002/110001

[commit-bot: I haz the power, 2015-12-11 01:28:50]

Description was changed from

==========
This is a rename and slight modification to mapRectToPaintInvalidationBacking.

The rename is for clarity (not for brevity, clearly).

There are two changes in functionality:

- Overflow clipping is done for all layers in the intermediate hierarchy;
previously, only non-composited layers applied clipping.

- Overflow clipping is *not* done for the paint invalidation container itself;
previously it was.


The reason for the change in functionality is to support the
IntersectionObserver use case (see bug).

BUG=540528
==========

to

==========
This is a rename and slight modification to mapRectToPaintInvalidationBacking.

The rename is for clarity (not for brevity, clearly).

There are two changes in functionality:

- Overflow clipping is done for all layers in the intermediate hierarchy;
previously, only non-composited layers applied clipping.

- Overflow clipping is *not* done for the paint invalidation container itself;
previously it was.


The reason for the change in functionality is to support the
IntersectionObserver use case (see bug).

BUG=540528
==========

[commit-bot: I haz the power, 2015-12-11 01:28:51]

Committed patchset #7 (id:110001)

[commit-bot: I haz the power, 2015-12-11 01:29:54]

Description was changed from

==========
This is a rename and slight modification to mapRectToPaintInvalidationBacking.

The rename is for clarity (not for brevity, clearly).

There are two changes in functionality:

- Overflow clipping is done for all layers in the intermediate hierarchy;
previously, only non-composited layers applied clipping.

- Overflow clipping is *not* done for the paint invalidation container itself;
previously it was.


The reason for the change in functionality is to support the
IntersectionObserver use case (see bug).

BUG=540528
==========

to

==========
This is a rename and slight modification to mapRectToPaintInvalidationBacking.

The rename is for clarity (not for brevity, clearly).

There are two changes in functionality:

- Overflow clipping is done for all layers in the intermediate hierarchy;
previously, only non-composited layers applied clipping.

- Overflow clipping is *not* done for the paint invalidation container itself;
previously it was.

The reason for the change in functionality is to support the
IntersectionObserver use case (see bug).

BUG=540528

Committed: https://crrev.com/a7fdc1aef78ed0f3ea6d66351498b4f3e4a9c3b5
Cr-Commit-Position: refs/heads/master@{#364567}
==========

[commit-bot: I haz the power, 2015-12-11 01:29:55]

Patchset 7 (id:??) landed as
https://crrev.com/a7fdc1aef78ed0f3ea6d66351498b4f3e4a9c3b5
Cr-Commit-Position: refs/heads/master@{#364567}