Support edge-inclusive intersections in mapToVisibleRectInAncestorSpace

Support edge-inclusive intersections in mapToVisibleRectInAncestorSpace

Edge-inclusive geometry means that if two rects have a zero-area intersection -- either because they overlap at an edge or point, or because one or both of the rects is zero-area -- the zero-area intersection will be used and propagated.  Previously, a zero-area intersection was always treated as not intersecting.

BUG=540428
R=chrishtr@chromium.org,ojan@chromium.org,esprehn@chromium.org

Committed: https://crrev.com/f790fe75a34b71516f68b7600ba11b9e9f0d6167
Cr-Commit-Position: refs/heads/master@{#383186}

[szager1, 2016-03-21 18:15:12]

ping

[chrishtr, 2016-03-21 22:14:50]

https://codereview.chromium.org/1817693002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/LayoutBox.h (right):

https://codereview.chromium.org/1817693002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutBox.h:642: bool
mapToVisibleRectInAncestorSpace(const LayoutBoxModelObject* ancestor,
LayoutRect&, const PaintInvalidationState*, bool edgeInclusive) const override;
Upgrade this bool to a bitfield, and default it to 0.

https://codereview.chromium.org/1817693002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/geometry/LayoutRect.cpp (right):

https://codereview.chromium.org/1817693002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/geometry/LayoutRect.cpp:83: bool
LayoutRect::inclusiveIntersect(const LayoutRect& other)
Unittest this.

https://codereview.chromium.org/1817693002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/geometry/LayoutRect.h (right):

https://codereview.chromium.org/1817693002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/geometry/LayoutRect.h:167: bool
inclusiveIntersect(const LayoutRect&);
Document this.

[ojan, 2016-03-21 22:23:04]

ojan@chromium.org changed reviewers:
+ eae@chromium.org, leviw@chromium.org

[ojan, 2016-03-21 22:23:05]

eae/leviw might be the best reviewers for this. If you all have any time, we're
trying to land this code by Thursday. Given vacation schedules and because we're
trying to get into M51. So, please prioritize this review if you can.

https://codereview.chromium.org/1817693002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/dom/IntersectionObservation.cpp (right):

https://codereview.chromium.org/1817693002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/IntersectionObservation.cpp:42:
geometry.doesIntersect = true;
Why do you need this line? It looks like it always gets overridden later.

https://codereview.chromium.org/1817693002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/IntersectionObservation.cpp:179:
geometry.intersectionRect = LayoutRect();
Doesn't this get the x/y coordinates of the intersectionRect wrong?

https://codereview.chromium.org/1817693002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/IntersectionObservation.cpp:199:
newVisibleRatio = std::numeric_limits<float>::denorm_min();
This can't be 0? /me is probably ignorant about working with floats.

https://codereview.chromium.org/1817693002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/LayoutBox.h (right):

https://codereview.chromium.org/1817693002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutBox.h:642: bool
mapToVisibleRectInAncestorSpace(const LayoutBoxModelObject* ancestor,
LayoutRect&, const PaintInvalidationState*, bool edgeInclusive) const override;
Use an enum instead of a bool argument.

[eae, 2016-03-22 00:13:50]

The functionality and inclusiveIntersects method make sense to me. I'm afraid
I'm not a fan of the changes to  mapToVisibleRectInAncestorSpace though. It's
performance sensetive and complicated enough as is. 

Furthermore I don't understand why we need to change the way coordinate
translation works for edge inclusion.

Finally, please update the CL description to include a description of what edge
inclusion is, what it does and then detail the changes you're making.

https://codereview.chromium.org/1817693002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/LayoutBox.cpp (right):

https://codereview.chromium.org/1817693002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutBox.cpp:992: doesIntersect =
!rect.isEmpty();
LayoutRect::intersects will only ever return true for an empty rect if the other
rect is also empty.

Is an intersection between two empty rects really a use-case we need to care
about? I understand supporting zero sized rects to track things like scroll
position but when would the other rect ever be zero sized?

https://codereview.chromium.org/1817693002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/LayoutInline.cpp (right):

https://codereview.chromium.org/1817693002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutInline.cpp:1084: bool
LayoutInline::mapToVisibleRectInAncestorSpace(const LayoutBoxModelObject*
ancestor, LayoutRect& rect, const PaintInvalidationState*
paintInvalidationState, bool edgeInclusive) const
It's unclear to me why these method would need to be made aware of
edge-inclusion. They're supposed to map the x/y coordinates of the supplied rect
which shouldn't be the same regardless of whether the edge is included or not.
Perhaps I'm missing something here?

Having it return a value is also a bit odd.

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

https://codereview.chromium.org/1817693002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutObject.h:1154: virtual bool
mapToVisibleRectInAncestorSpace(const LayoutBoxModelObject* ancestor,
LayoutRect&, const PaintInvalidationState*, bool edgeInclusive) const;
Adding a second capability (intersection detection) to a translation method
seems really odd to me and I don't understand why that is needed. Shouldn't the
intersection check be done on the resulting rects _after_ translation?

[szager1, 2016-03-22 00:55:49]

https://codereview.chromium.org/1817693002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/dom/IntersectionObservation.cpp (right):

https://codereview.chromium.org/1817693002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/IntersectionObservation.cpp:42:
geometry.doesIntersect = true;
On 2016/03/21 22:23:05, ojan wrote:
> Why do you need this line? It looks like it always gets overridden later.

No reason other than my instinctive aversion to leaving struct fields
uninitialized in a method with "initialize" in the name.

https://codereview.chromium.org/1817693002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/IntersectionObservation.cpp:179:
geometry.intersectionRect = LayoutRect();
On 2016/03/21 22:23:05, ojan wrote:
> Doesn't this get the x/y coordinates of the intersectionRect wrong?

If there's no intersection, then there are no right or wrong coordinates.  The
spec doesn't say anything about what intersectionRect should be when root and
target don't overlap.  I consider it safer to do this, rather than possibly leak
information about the root viewport.

Currently, this line is effectively a no-op, since
LayoutRect::inclusiveIntersect will set the rect to (0, 0, 0, 0) if there's no
intersection.  But I still think it's prudent to clear the rect here.

https://codereview.chromium.org/1817693002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/IntersectionObservation.cpp:199:
newVisibleRatio = std::numeric_limits<float>::denorm_min();
On 2016/03/21 22:23:04, ojan wrote:
> This can't be 0? /me is probably ignorant about working with floats.

Zero is a special value that means "not intersecting," which is distinct from
"has a non-zero intersection."

Basically, we want a value here that is greater than zero, but guaranteed to be
less than or equal to any other threshold value.

https://codereview.chromium.org/1817693002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/LayoutBox.cpp (right):

https://codereview.chromium.org/1817693002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutBox.cpp:992: doesIntersect =
!rect.isEmpty();
On 2016/03/22 00:13:50, eae wrote:
> LayoutRect::intersects will only ever return true for an empty rect if the
other
> rect is also empty.

That's not true:

bool LayoutRect::intersects(const LayoutRect& other) const                      
                                                           
{                                                                               
                                                           
    // Checking emptiness handles negative widths as well as zero.              
                                                           
    return !isEmpty() && !other.isEmpty()                                       
                                                           
        && x() < other.maxX() && other.x() < maxX()                             
                                                           
        && y() < other.maxY() && other.y() < maxY();                            
                                                           
} 

If either rect is empty, then intersects() returns false.

https://codereview.chromium.org/1817693002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/LayoutBox.h (right):

https://codereview.chromium.org/1817693002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutBox.h:642: bool
mapToVisibleRectInAncestorSpace(const LayoutBoxModelObject* ancestor,
LayoutRect&, const PaintInvalidationState*, bool edgeInclusive) const override;
On 2016/03/21 22:14:50, chrishtr wrote:
> Upgrade this bool to a bitfield, and default it to 0.

Done.

https://codereview.chromium.org/1817693002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/LayoutInline.cpp (right):

https://codereview.chromium.org/1817693002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutInline.cpp:1084: bool
LayoutInline::mapToVisibleRectInAncestorSpace(const LayoutBoxModelObject*
ancestor, LayoutRect& rect, const PaintInvalidationState*
paintInvalidationState, bool edgeInclusive) const
On 2016/03/22 00:13:50, eae wrote:
> It's unclear to me why these method would need to be made aware of
> edge-inclusion. They're supposed to map the x/y coordinates of the supplied
rect
> which shouldn't be the same regardless of whether the edge is included or not.
> Perhaps I'm missing something here?
> 
> Having it return a value is also a bit odd.

mapToVisibleRectInAncestorSpace is used by IntersectionObservation to compute
the intersection between to LayoutObject's, and that intersection needs to be
edge-inclusive (required by the spec).  This is the least intrusive way I could
think of for this method to report "the rects intersect, but the intersection is
zero area," as distinct from "the rects do not intersect."

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

https://codereview.chromium.org/1817693002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutObject.h:1154: virtual bool
mapToVisibleRectInAncestorSpace(const LayoutBoxModelObject* ancestor,
LayoutRect&, const PaintInvalidationState*, bool edgeInclusive) const;
On 2016/03/22 00:13:50, eae wrote:
> Adding a second capability (intersection detection) to a translation method
> seems really odd to me and I don't understand why that is needed. Shouldn't
the
> intersection check be done on the resulting rects _after_ translation?

One of the core features of this method is that it applies clipping up the
hierarchy, but prior to this patch the clipping was not edge-aware, as required
by IntersectionObserver.  Adding a flag and a bool return value was the least
intrusive way I could think of to add that capability.

[ojan, 2016-03-22 04:24:47]

https://codereview.chromium.org/1817693002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/dom/IntersectionObservation.cpp (right):

https://codereview.chromium.org/1817693002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/IntersectionObservation.cpp:179:
geometry.intersectionRect = LayoutRect();
On 2016/03/22 at 00:55:48, szager1 wrote:
> On 2016/03/21 22:23:05, ojan wrote:
> > Doesn't this get the x/y coordinates of the intersectionRect wrong?
> 
> If there's no intersection, then there are no right or wrong coordinates.  The
spec doesn't say anything about what intersectionRect should be when root and
target don't overlap.  I consider it safer to do this, rather than possibly leak
information about the root viewport.
> 
> Currently, this line is effectively a no-op, since
LayoutRect::inclusiveIntersect will set the rect to (0, 0, 0, 0) if there's no
intersection.  But I still think it's prudent to clear the rect here.

Oh, I misread this code. This is only in the non-intersecting case. As far as
the clearing of the rect for prudency, can you verify the correct behavior via
asserts instead of something that will use cycles at runtime?

https://codereview.chromium.org/1817693002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/LayoutBox.h (right):

https://codereview.chromium.org/1817693002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutBox.h:642: bool
mapToVisibleRectInAncestorSpace(const LayoutBoxModelObject* ancestor,
LayoutRect&, const PaintInvalidationState*, bool edgeInclusive) const override;
On 2016/03/22 at 00:55:48, szager1 wrote:
> On 2016/03/21 22:14:50, chrishtr wrote:
> > Upgrade this bool to a bitfield, and default it to 0.
> 
> Done.

Why a bitfield instead of an enum? In my experience, the places we use bitfield
arguments often lead to hard to understand, spaghetti code.

[chrishtr, 2016-03-22 15:04:16]

On 2016/03/22 at 04:24:47, ojan wrote:
>
https://codereview.chromium.org/1817693002/diff/40001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/core/dom/IntersectionObservation.cpp (right):
> 
>
https://codereview.chromium.org/1817693002/diff/40001/third_party/WebKit/Sour...
> third_party/WebKit/Source/core/dom/IntersectionObservation.cpp:179:
geometry.intersectionRect = LayoutRect();
> On 2016/03/22 at 00:55:48, szager1 wrote:
> > On 2016/03/21 22:23:05, ojan wrote:
> > > Doesn't this get the x/y coordinates of the intersectionRect wrong?
> > 
> > If there's no intersection, then there are no right or wrong coordinates. 
The spec doesn't say anything about what intersectionRect should be when root
and target don't overlap.  I consider it safer to do this, rather than possibly
leak information about the root viewport.
> > 
> > Currently, this line is effectively a no-op, since
LayoutRect::inclusiveIntersect will set the rect to (0, 0, 0, 0) if there's no
intersection.  But I still think it's prudent to clear the rect here.
> 
> Oh, I misread this code. This is only in the non-intersecting case. As far as
the clearing of the rect for prudency, can you verify the correct behavior via
asserts instead of something that will use cycles at runtime?
> 
>
https://codereview.chromium.org/1817693002/diff/40001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/core/layout/LayoutBox.h (right):
> 
>
https://codereview.chromium.org/1817693002/diff/40001/third_party/WebKit/Sour...
> third_party/WebKit/Source/core/layout/LayoutBox.h:642: bool
mapToVisibleRectInAncestorSpace(const LayoutBoxModelObject* ancestor,
LayoutRect&, const PaintInvalidationState*, bool edgeInclusive) const override;
> On 2016/03/22 at 00:55:48, szager1 wrote:
> > On 2016/03/21 22:14:50, chrishtr wrote:
> > > Upgrade this bool to a bitfield, and default it to 0.
> > 
> > Done.
> 
> Why a bitfield instead of an enum? In my experience, the places we use
bitfield arguments often lead to hard to understand, spaghetti code.

Because I anticipate adding more options in the future (I have one in mind
actually).

[chrishtr, 2016-03-22 15:04:35]

On 2016/03/22 at 15:04:16, chrishtr wrote:
> On 2016/03/22 at 04:24:47, ojan wrote:
> >
https://codereview.chromium.org/1817693002/diff/40001/third_party/WebKit/Sour...
> > File third_party/WebKit/Source/core/dom/IntersectionObservation.cpp (right):
> > 
> >
https://codereview.chromium.org/1817693002/diff/40001/third_party/WebKit/Sour...
> > third_party/WebKit/Source/core/dom/IntersectionObservation.cpp:179:
geometry.intersectionRect = LayoutRect();
> > On 2016/03/22 at 00:55:48, szager1 wrote:
> > > On 2016/03/21 22:23:05, ojan wrote:
> > > > Doesn't this get the x/y coordinates of the intersectionRect wrong?
> > > 
> > > If there's no intersection, then there are no right or wrong coordinates. 
The spec doesn't say anything about what intersectionRect should be when root
and target don't overlap.  I consider it safer to do this, rather than possibly
leak information about the root viewport.
> > > 
> > > Currently, this line is effectively a no-op, since
LayoutRect::inclusiveIntersect will set the rect to (0, 0, 0, 0) if there's no
intersection.  But I still think it's prudent to clear the rect here.
> > 
> > Oh, I misread this code. This is only in the non-intersecting case. As far
as the clearing of the rect for prudency, can you verify the correct behavior
via asserts instead of something that will use cycles at runtime?
> > 
> >
https://codereview.chromium.org/1817693002/diff/40001/third_party/WebKit/Sour...
> > File third_party/WebKit/Source/core/layout/LayoutBox.h (right):
> > 
> >
https://codereview.chromium.org/1817693002/diff/40001/third_party/WebKit/Sour...
> > third_party/WebKit/Source/core/layout/LayoutBox.h:642: bool
mapToVisibleRectInAncestorSpace(const LayoutBoxModelObject* ancestor,
LayoutRect&, const PaintInvalidationState*, bool edgeInclusive) const override;
> > On 2016/03/22 at 00:55:48, szager1 wrote:
> > > On 2016/03/21 22:14:50, chrishtr wrote:
> > > > Upgrade this bool to a bitfield, and default it to 0.
> > > 
> > > Done.
> > 
> > Why a bitfield instead of an enum? In my experience, the places we use
bitfield arguments often lead to hard to understand, spaghetti code.
> 
> Because I anticipate adding more options in the future (I have one in mind
actually).

(and those options can be in combination with this one)

[ojan, 2016-03-22 16:49:41]

On 2016/03/22 at 15:04:35, chrishtr wrote:
> On 2016/03/22 at 15:04:16, chrishtr wrote:
> > On 2016/03/22 at 04:24:47, ojan wrote:
> > >
https://codereview.chromium.org/1817693002/diff/40001/third_party/WebKit/Sour...
> > > File third_party/WebKit/Source/core/dom/IntersectionObservation.cpp
(right):
> > > 
> > >
https://codereview.chromium.org/1817693002/diff/40001/third_party/WebKit/Sour...
> > > third_party/WebKit/Source/core/dom/IntersectionObservation.cpp:179:
geometry.intersectionRect = LayoutRect();
> > > On 2016/03/22 at 00:55:48, szager1 wrote:
> > > > On 2016/03/21 22:23:05, ojan wrote:
> > > > > Doesn't this get the x/y coordinates of the intersectionRect wrong?
> > > > 
> > > > If there's no intersection, then there are no right or wrong
coordinates.  The spec doesn't say anything about what intersectionRect should
be when root and target don't overlap.  I consider it safer to do this, rather
than possibly leak information about the root viewport.
> > > > 
> > > > Currently, this line is effectively a no-op, since
LayoutRect::inclusiveIntersect will set the rect to (0, 0, 0, 0) if there's no
intersection.  But I still think it's prudent to clear the rect here.
> > > 
> > > Oh, I misread this code. This is only in the non-intersecting case. As far
as the clearing of the rect for prudency, can you verify the correct behavior
via asserts instead of something that will use cycles at runtime?
> > > 
> > >
https://codereview.chromium.org/1817693002/diff/40001/third_party/WebKit/Sour...
> > > File third_party/WebKit/Source/core/layout/LayoutBox.h (right):
> > > 
> > >
https://codereview.chromium.org/1817693002/diff/40001/third_party/WebKit/Sour...
> > > third_party/WebKit/Source/core/layout/LayoutBox.h:642: bool
mapToVisibleRectInAncestorSpace(const LayoutBoxModelObject* ancestor,
LayoutRect&, const PaintInvalidationState*, bool edgeInclusive) const override;
> > > On 2016/03/22 at 00:55:48, szager1 wrote:
> > > > On 2016/03/21 22:14:50, chrishtr wrote:
> > > > > Upgrade this bool to a bitfield, and default it to 0.
> > > > 
> > > > Done.
> > > 
> > > Why a bitfield instead of an enum? In my experience, the places we use
bitfield arguments often lead to hard to understand, spaghetti code.
> > 
> > Because I anticipate adding more options in the future (I have one in mind
actually).
> 
> (and those options can be in combination with this one)

Is there a forum other than this codereview we can discuss this? I don't want to
block this codereview on this. Either thing can land for now, although I'd
prefer using an enum until we proove the need for a bitfield instead of adding a
bitfield because we might need it in the future. My experience is that what
starts out as reasonable code with bitfields incrementally becomes more and more
spaghetti because it's very easy to add new values to the bitfields instead of
thinking about how to structure the code to avoid the need for the flags.

[chrishtr, 2016-03-22 17:25:49]

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

https://codereview.chromium.org/1817693002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutObject.h:1153: // resulting rect is
zero-area.
"intersection" might lead to the wrong conclusions here. Maybe better is: the
return
value indicates whether the visible rect computed is not "empty". If
EdgeInclusive is
false, this is the same as !rect.isEmpty(); if EdgeInclusive is true it can be
true
if the rect has zero area but its bounds intersected all clips. The latter is
still
kind of confusing, an example would be good.

https://codereview.chromium.org/1817693002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutObject.h:1154: virtual bool
mapToVisibleRectInAncestorSpace(const LayoutBoxModelObject* ancestor,
LayoutRect&, const PaintInvalidationState*, int flags = 0) const;
IntersectionFlags instead of int

https://codereview.chromium.org/1817693002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/LayoutView.cpp (right):

https://codereview.chromium.org/1817693002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutView.cpp:519:
rect.intersect(viewRectangle);
return false here if it's empty? Early out code?

https://codereview.chromium.org/1817693002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/PaintInvalidationState.cpp (right):

https://codereview.chromium.org/1817693002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/PaintInvalidationState.cpp:185:
rect.intersect(m_clipRect);
also return false here if it's empty?

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

https://codereview.chromium.org/1817693002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/PaintInvalidationState.h:24: };
Add typedef unsigned IntersectionFlags

https://codereview.chromium.org/1817693002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/svg/LayoutSVGRoot.cpp (right):

https://codereview.chromium.org/1817693002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/svg/LayoutSVGRoot.cpp:366:
rect.intersect(LayoutRect(pixelSnappedBorderBoxRect()));
early out?

[eae, 2016-03-22 17:54:16]

LGTM based on in person discussions

https://codereview.chromium.org/1817693002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/LayoutBox.cpp (right):

https://codereview.chromium.org/1817693002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutBox.cpp:992: doesIntersect =
!rect.isEmpty();
On 2016/03/22 00:55:48, szager1 wrote:
> On 2016/03/22 00:13:50, eae wrote:
> > LayoutRect::intersects will only ever return true for an empty rect if the
> other
> > rect is also empty.
> 
> That's not true:
> 
> bool LayoutRect::intersects(const LayoutRect& other) const                    
 
>                                                            
> {                                                                             
 
>                                                            
>     // Checking emptiness handles negative widths as well as zero.            
 
>                                                            
>     return !isEmpty() && !other.isEmpty()                                     
 
>                                                            
>         && x() < other.maxX() && other.x() < maxX()                           
 
>                                                            
>         && y() < other.maxY() && other.y() < maxY();                          
 
>                                                            
> } 
> 
> If either rect is empty, then intersects() returns false.

Oh, my bad. You're right. Never mind!

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

https://codereview.chromium.org/1817693002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutObject.h:1154: virtual bool
mapToVisibleRectInAncestorSpace(const LayoutBoxModelObject* ancestor,
LayoutRect&, const PaintInvalidationState*, bool edgeInclusive) const;
On 2016/03/22 00:55:48, szager1 wrote:
> On 2016/03/22 00:13:50, eae wrote:
> > Adding a second capability (intersection detection) to a translation method
> > seems really odd to me and I don't understand why that is needed. Shouldn't
> the
> > intersection check be done on the resulting rects _after_ translation?
> 
> One of the core features of this method is that it applies clipping up the
> hierarchy, but prior to this patch the clipping was not edge-aware, as
required
> by IntersectionObserver.  Adding a flag and a bool return value was the least
> intrusive way I could think of to add that capability.

Oh, I see. Didn't realize it did clipping until I start digging into the code
this morning. We might want to rename mapToVisibleRectInAncestorSpace to include
the word clipping (in a subsequent patch, not as a part of this one).

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

https://codereview.chromium.org/1817693002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutObject.h:1154: virtual bool
mapToVisibleRectInAncestorSpace(const LayoutBoxModelObject* ancestor,
LayoutRect&, const PaintInvalidationState*, int flags = 0) const;
Use unsigned (or typedef for unsigned as Chris suggested) for flags.

https://codereview.chromium.org/1817693002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/LayoutTableCell.cpp (right):

https://codereview.chromium.org/1817693002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutTableCell.cpp:938:
std::sort(borderValues.begin(), borderValues.end(), compareBorders);
Nice!

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

https://codereview.chromium.org/1817693002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/PaintInvalidationState.h:22: EdgeExclusive
= 0,
We generally do not have a name for the 0 flag and if we do it's generally
called something like DefaultIntersectionFlags or NoIntersectionFlags.

Having the 0 value called EdgeExclusive makes it quite confusing if we ever add
another flag.

[szager1, 2016-03-22 18:33:35]

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

https://codereview.chromium.org/1817693002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutObject.h:1153: // resulting rect is
zero-area.
On 2016/03/22 17:25:49, chrishtr wrote:
> "intersection" might lead to the wrong conclusions here. Maybe better is: the
> return
> value indicates whether the visible rect computed is not "empty". If
> EdgeInclusive is
> false, this is the same as !rect.isEmpty(); if EdgeInclusive is true it can be
> true
> if the rect has zero area but its bounds intersected all clips. The latter is
> still
> kind of confusing, an example would be good.

I think the best documentation is what I wrote for
LayoutRect::inclusiveIntersect:

"Set this rect to be the intersection of itself and the argument rect
using edge-inclusive geometry.  If the two rectangles overlap but the
overlap region is zero-area (either because one of the two rectangles
is zero-area, or because the rectangles overlap at an edge or a corner),
the result is the zero-area intersection.  The return value indicates
whether the two rectangle actually have an intersection, since checking
the result for isEmpty() is not conclusive."

I can copy-paste that here, or I can just say, "EdgeInclusive means clipping
will be calculated using LayoutRect::inclusiveIntersect; see documentation for
that method."

WDYT?

[chrishtr, 2016-03-22 18:44:17]

On 2016/03/22 at 18:33:35, szager wrote:
>
https://codereview.chromium.org/1817693002/diff/60001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/core/layout/LayoutObject.h (right):
> 
>
https://codereview.chromium.org/1817693002/diff/60001/third_party/WebKit/Sour...
> third_party/WebKit/Source/core/layout/LayoutObject.h:1153: // resulting rect
is zero-area.
> On 2016/03/22 17:25:49, chrishtr wrote:
> > "intersection" might lead to the wrong conclusions here. Maybe better is:
the
> > return
> > value indicates whether the visible rect computed is not "empty". If
> > EdgeInclusive is
> > false, this is the same as !rect.isEmpty(); if EdgeInclusive is true it can
be
> > true
> > if the rect has zero area but its bounds intersected all clips. The latter
is
> > still
> > kind of confusing, an example would be good.
> 
> I think the best documentation is what I wrote for
LayoutRect::inclusiveIntersect:
> 
> "Set this rect to be the intersection of itself and the argument rect
> using edge-inclusive geometry.  If the two rectangles overlap but the
> overlap region is zero-area (either because one of the two rectangles
> is zero-area, or because the rectangles overlap at an edge or a corner),
> the result is the zero-area intersection.  The return value indicates
> whether the two rectangle actually have an intersection, since checking
> the result for isEmpty() is not conclusive."
> 
> I can copy-paste that here, or I can just say, "EdgeInclusive means clipping
will be calculated using LayoutRect::inclusiveIntersect; see documentation for
that method."
> 
> WDYT?

Referring to the documentation of inclusiveIntersect SGTM.

[szager1, 2016-03-22 19:42:25]

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

https://codereview.chromium.org/1817693002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutObject.h:1154: virtual bool
mapToVisibleRectInAncestorSpace(const LayoutBoxModelObject* ancestor,
LayoutRect&, const PaintInvalidationState*, int flags = 0) const;
On 2016/03/22 17:54:16, eae wrote:
> Use unsigned (or typedef for unsigned as Chris suggested) for flags.

Done.

https://codereview.chromium.org/1817693002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/PaintInvalidationState.cpp (right):

https://codereview.chromium.org/1817693002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/PaintInvalidationState.cpp:185:
rect.intersect(m_clipRect);
On 2016/03/22 17:25:49, chrishtr wrote:
> also return false here if it's empty?

Done.

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

https://codereview.chromium.org/1817693002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/PaintInvalidationState.h:22: EdgeExclusive
= 0,
On 2016/03/22 17:54:16, eae wrote:
> We generally do not have a name for the 0 flag and if we do it's generally
> called something like DefaultIntersectionFlags or NoIntersectionFlags.
> 
> Having the 0 value called EdgeExclusive makes it quite confusing if we ever
add
> another flag.

Changed to DefaultVisibleRectFlags.

https://codereview.chromium.org/1817693002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/PaintInvalidationState.h:24: };
On 2016/03/22 17:25:49, chrishtr wrote:
> Add typedef unsigned IntersectionFlags

I changed it to:

enum VisibleRectFlags {
  ...

https://codereview.chromium.org/1817693002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/svg/LayoutSVGRoot.cpp (right):

https://codereview.chromium.org/1817693002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/svg/LayoutSVGRoot.cpp:366:
rect.intersect(LayoutRect(pixelSnappedBorderBoxRect()));
On 2016/03/22 17:25:49, chrishtr wrote:
> early out?

I'm hesitant to do that, because the call to
LayoutReplaced::mapToVisibleRectInAncestorSpace dispatches to
LayoutBox::mapVisibleRectToAncestorSpace, which does this at the top:

inflatePaintInvalidationRectForReflectionAndFilter(rect);

I'm concerned that this could take a zero-area rect and expand it to a non-zero
rect.  So an early out here might be incorrect behavior.

[chrishtr, 2016-03-22 19:54:26]

https://codereview.chromium.org/1817693002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/dom/IntersectionObservation.cpp (right):

https://codereview.chromium.org/1817693002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/IntersectionObservation.cpp:37: void
IntersectionObservation::initializeGeometry(IntersectionGeometry& geometry)
const
Prefer to remove changes to this class from the CL and make it just about adding
the
inclusive feature.

I haven't reviewed this class for example..

https://codereview.chromium.org/1817693002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/LayoutBox.h (right):

https://codereview.chromium.org/1817693002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutBox.h:846: // Returns true if the
rect actually intersects the clipping region.
Add a comment that this is not an "iff" - i.e. there may be false positives.

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

https://codereview.chromium.org/1817693002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutObject.h:1152: // If
visibleRectFlags is EdgeInclusive, clipping operations will use
LayoutRect::inclusiveIntersect,
s/is/has/

https://codereview.chromium.org/1817693002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/LayoutObjectTest.cpp (right):

https://codereview.chromium.org/1817693002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutObjectTest.cpp:126: EXPECT_TRUE(rect
== LayoutRect(0, 10, 20, 0));
Why does this example yield a zero-area rect? Isn't it a repeat of the example
on
line 121 with a weaker test?

Also, this amount of testing is a little weak sauce. e.g. not tested for
iframes, etc.
Can you add those?

[szager1, 2016-03-23 23:26:59]

https://codereview.chromium.org/1817693002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/dom/IntersectionObservation.cpp (right):

https://codereview.chromium.org/1817693002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/IntersectionObservation.cpp:37: void
IntersectionObservation::initializeGeometry(IntersectionGeometry& geometry)
const
On 2016/03/22 19:54:26, chrishtr wrote:
> Prefer to remove changes to this class from the CL and make it just about
adding
> the
> inclusive feature.
> 
> I haven't reviewed this class for example..

Done.

https://codereview.chromium.org/1817693002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/LayoutBox.h (right):

https://codereview.chromium.org/1817693002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutBox.h:846: // Returns true if the
rect actually intersects the clipping region.
On 2016/03/22 19:54:26, chrishtr wrote:
> Add a comment that this is not an "iff" - i.e. there may be false positives.

I don't think that's true.  applyOverflowClip always calls either
LayoutRect::intersect or LayoutRect::inclusiveIntersect, and the return value is
set on basis of that call.  If this method returns 'true', it must be the case
that the argument rect actually intersects the clip region.

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

https://codereview.chromium.org/1817693002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutObject.h:1152: // If
visibleRectFlags is EdgeInclusive, clipping operations will use
LayoutRect::inclusiveIntersect,
On 2016/03/22 19:54:26, chrishtr wrote:
> s/is/has/

Done.

https://codereview.chromium.org/1817693002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/LayoutObjectTest.cpp (right):

https://codereview.chromium.org/1817693002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutObjectTest.cpp:126: EXPECT_TRUE(rect
== LayoutRect(0, 10, 20, 0));
On 2016/03/22 19:54:26, chrishtr wrote:
> Why does this example yield a zero-area rect? Isn't it a repeat of the example
> on
> line 121 with a weaker test?

No; the input rect has zero height, so the result should also be zero area.

> Also, this amount of testing is a little weak sauce. e.g. not tested for
> iframes, etc.
> Can you add those?

grumble grumble

[szager1, 2016-03-24 19:13:52]

Description was changed from

==========
IntersectionObserver: use edge-inclusive geometry for intersections.

BUG=540428
R=chrishtr@chromium.org,ojan@chromium.org,esprehn@chromium.org
==========

to

==========
Support edge-inclusive intersections in mapToVisibleRectInAncestorSpace

Edge-inclusive geometry means that if two rects have a zero-area intersection --
either because they overlap at an edge or point, or because one or both of the
rects is zero-area -- the zero-area intersection will be used and propagated. 
Previously, a zero-area intersection was always treated as not intersecting.

BUG=540428
R=chrishtr@chromium.org,ojan@chromium.org,esprehn@chromium.org
==========

[szager1, 2016-03-24 19:15:10]

Added another test case that exercises the LayoutView code, removed the
IntersectionObserver bits, and updated the description.

PTAL

[chrishtr, 2016-03-24 21:18:43]

https://codereview.chromium.org/1817693002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/LayoutBox.h (right):

https://codereview.chromium.org/1817693002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutBox.h:846: // Returns true if the
rect actually intersects the clipping region.
On 2016/03/23 at 23:26:59, szager1 wrote:
> On 2016/03/22 19:54:26, chrishtr wrote:
> > Add a comment that this is not an "iff" - i.e. there may be false positives.
> 
> I don't think that's true.  applyOverflowClip always calls either
LayoutRect::intersect or LayoutRect::inclusiveIntersect, and the return value is
set on basis of that call.  If this method returns 'true', it must be the case
that the argument rect actually intersects the clip region.

Ok you win. :)

https://codereview.chromium.org/1817693002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/LayoutObjectTest.cpp (right):

https://codereview.chromium.org/1817693002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutObjectTest.cpp:126: EXPECT_TRUE(rect
== LayoutRect(0, 10, 20, 0));
On 2016/03/23 at 23:26:59, szager1 wrote:
> On 2016/03/22 19:54:26, chrishtr wrote:
> > Why does this example yield a zero-area rect? Isn't it a repeat of the
example
> > on
> > line 121 with a weaker test?
> 
> No; the input rect has zero height, so the result should also be zero area.
> 
> > Also, this amount of testing is a little weak sauce. e.g. not tested for
> > iframes, etc.
> > Can you add those?
> 
> grumble grumble

?

https://codereview.chromium.org/1817693002/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/LayoutObject.cpp (right):

https://codereview.chromium.org/1817693002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutObject.cpp:1641: bool
LayoutObject::mapToVisibleRectInAncestorSpace(const LayoutBoxModelObject*
ancestor, LayoutRect& rect, const PaintInvalidationState*
paintInvalidationState, VisibleRectFlags visibleRectFlags) const
"When this method is called, the coordinate space of |rect| is assumed to be
that of the parent."

https://codereview.chromium.org/1817693002/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/LayoutObjectTest.cpp (right):

https://codereview.chromium.org/1817693002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutObjectTest.cpp:117: "<div
id='container' style='overflow: scroll; will-change: transform; width: 50px;
height: 50px'>"
will-change: transform; seems irrelevant

https://codereview.chromium.org/1817693002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutObjectTest.cpp:132: LayoutText*
frameText = toLayoutText(frameBody->lastChild());
Separate these two test types into two unittests (one for the text mapping, one
for iframe)

https://codereview.chromium.org/1817693002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutObjectTest.cpp:145:
frameDocument.view()->setScrollPosition(DoublePoint(0, 47), ProgrammaticScroll);
Make one test for LayoutInline, one for LayoutText (which this test already
exercises)

[szager1, 2016-03-24 21:52:28]

https://codereview.chromium.org/1817693002/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/LayoutObject.cpp (right):

https://codereview.chromium.org/1817693002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutObject.cpp:1641: bool
LayoutObject::mapToVisibleRectInAncestorSpace(const LayoutBoxModelObject*
ancestor, LayoutRect& rect, const PaintInvalidationState*
paintInvalidationState, VisibleRectFlags visibleRectFlags) const
On 2016/03/24 21:18:43, chrishtr wrote:
> "When this method is called, the coordinate space of |rect| is assumed to be
> that of the parent."

Done.

https://codereview.chromium.org/1817693002/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/LayoutObjectTest.cpp (right):

https://codereview.chromium.org/1817693002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutObjectTest.cpp:117: "<div
id='container' style='overflow: scroll; will-change: transform; width: 50px;
height: 50px'>"
On 2016/03/24 21:18:43, chrishtr wrote:
> will-change: transform; seems irrelevant

Done.

https://codereview.chromium.org/1817693002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutObjectTest.cpp:132: LayoutText*
frameText = toLayoutText(frameBody->lastChild());
On 2016/03/24 21:18:43, chrishtr wrote:
> Separate these two test types into two unittests (one for the text mapping,
one
> for iframe)

Done.

https://codereview.chromium.org/1817693002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutObjectTest.cpp:145:
frameDocument.view()->setScrollPosition(DoublePoint(0, 47), ProgrammaticScroll);
On 2016/03/24 21:18:43, chrishtr wrote:
> Make one test for LayoutInline, one for LayoutText (which this test already
> exercises)

Done.

[chrishtr, 2016-03-24 21:58:48]

The CQ bit was checked by chrishtr@chromium.org

[chrishtr, 2016-03-24 21:58:49]

lgtm

[chrishtr, 2016-03-24 21:58:49]

The patchset sent to the CQ was uploaded after l-g-t-m from eae@chromium.org
Link to the patchset: https://codereview.chromium.org/1817693002/#ps160001
(title: "more tests")

[commit-bot: I haz the power, 2016-03-24 21:59:23]

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

[commit-bot: I haz the power, 2016-03-24 23:33:37]

Description was changed from

==========
Support edge-inclusive intersections in mapToVisibleRectInAncestorSpace

Edge-inclusive geometry means that if two rects have a zero-area intersection --
either because they overlap at an edge or point, or because one or both of the
rects is zero-area -- the zero-area intersection will be used and propagated. 
Previously, a zero-area intersection was always treated as not intersecting.

BUG=540428
R=chrishtr@chromium.org,ojan@chromium.org,esprehn@chromium.org
==========

to

==========
Support edge-inclusive intersections in mapToVisibleRectInAncestorSpace

Edge-inclusive geometry means that if two rects have a zero-area intersection --
either because they overlap at an edge or point, or because one or both of the
rects is zero-area -- the zero-area intersection will be used and propagated. 
Previously, a zero-area intersection was always treated as not intersecting.

BUG=540428
R=chrishtr@chromium.org,ojan@chromium.org,esprehn@chromium.org
==========

[commit-bot: I haz the power, 2016-03-24 23:33:39]

Committed patchset #9 (id:160001)

[commit-bot: I haz the power, 2016-03-24 23:35:26]

Description was changed from

==========
Support edge-inclusive intersections in mapToVisibleRectInAncestorSpace

Edge-inclusive geometry means that if two rects have a zero-area intersection --
either because they overlap at an edge or point, or because one or both of the
rects is zero-area -- the zero-area intersection will be used and propagated. 
Previously, a zero-area intersection was always treated as not intersecting.

BUG=540428
R=chrishtr@chromium.org,ojan@chromium.org,esprehn@chromium.org
==========

to

==========
Support edge-inclusive intersections in mapToVisibleRectInAncestorSpace

Edge-inclusive geometry means that if two rects have a zero-area intersection --
either because they overlap at an edge or point, or because one or both of the
rects is zero-area -- the zero-area intersection will be used and propagated. 
Previously, a zero-area intersection was always treated as not intersecting.

BUG=540428
R=chrishtr@chromium.org,ojan@chromium.org,esprehn@chromium.org

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

[commit-bot: I haz the power, 2016-03-24 23:35:28]

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