Add root margin support for IntersectionObserver.

Add root margin support for IntersectionObserver.

Depends on:

https://codereview.chromium.org/1553773002/

BUG=540528
R=ojan@chromium.org,esprehn@chromium.org

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

[szager1, 2016-01-08 23:54:21]

ping

[ojan, 2016-01-12 01:08:49]

Can you file github issues for all the TODO things if you agree they need doing?
That way we won't lose track of them.

https://codereview.chromium.org/1559593002/diff/60001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/intersection-observer/root-margin.html
(right):

https://codereview.chromium.org/1559593002/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/intersection-observer/root-margin.html:22:
rootMargin: "10px 20% 40% 30px"
Please add test coverage for the other parsing code paths:
-Different number of items in the list.
-Decimal values.
-More than 4 items.
-Non px/% values.

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

https://codereview.chromium.org/1559593002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/IntersectionObservation.cpp:68:
m_observer->applyRootMargin(geometry.rootRect);
Nit: you are repeating this line in both the if and else clauses. Can you have a
single line after the else clause on line 70?

https://codereview.chromium.org/1559593002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/dom/IntersectionObserver.cpp (right):

https://codereview.chromium.org/1559593002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/IntersectionObserver.cpp:25: static void
parseRootMargin(String rootMarginParameter, Vector<Length>& rootMargin,
ExceptionState& exceptionState)
TODO: We need to make sure this exact algorithm is specced somewhere.

https://codereview.chromium.org/1559593002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/IntersectionObserver.cpp:196:
exceptionState.throwDOMException(HierarchyRequestError, "Cannot observe a
cross-origin target because the observer has a root margin value specified as a
percent.");
TODO: I wonder if we should have a TODO or something to add a URL to this
description that can point people at some documentation explaining this. Or
maybe we should just make sure the IO documentation covers this.

https://codereview.chromium.org/1559593002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/IntersectionObserver.cpp:260: return
LayoutUnit(length.intValue());
ASSERT(length.type() == Fixed); ?

https://codereview.chromium.org/1559593002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/IntersectionObserver.cpp:266: LayoutUnit
rightMargin = computeMargin(m_rightMargin, rect.width());
TODO: We should make sure the spec is clear that the left/right margins resolve
with respect to the width and not the height (since with most css layouts they
resolve against the height).

https://codereview.chromium.org/1559593002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/dom/IntersectionObserver.h (right):

https://codereview.chromium.org/1559593002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/IntersectionObserver.h:67: Length
m_leftMargin;
This object is getting quite large. I don't think we need to do anything about
it at the moment, but when we think about perf/memory profiling, we should
consider the case that there are a lot of observers on the page. We might want
to move margins and thresholds to a rare data object in that case.

[szager1, 2016-01-12 19:24:44]

https://codereview.chromium.org/1559593002/diff/60001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/intersection-observer/root-margin.html
(right):

https://codereview.chromium.org/1559593002/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/intersection-observer/root-margin.html:22:
rootMargin: "10px 20% 40% 30px"
On 2016/01/12 01:08:49, ojan wrote:
> Please add test coverage for the other parsing code paths:
> -Different number of items in the list.
> -Decimal values.
> -More than 4 items.
> -Non px/% values.

I added tests to demonstrate that the parsing works or fails as expected; I'll
add more functional tests it a follow-up.

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

https://codereview.chromium.org/1559593002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/IntersectionObservation.cpp:68:
m_observer->applyRootMargin(geometry.rootRect);
On 2016/01/12 01:08:49, ojan wrote:
> Nit: you are repeating this line in both the if and else clauses. Can you have
a
> single line after the else clause on line 70?

applyRootMargin has to happen before the call to
geometry.intersectionRect.intersect(geometry.rootRect) in the 'if' clause, so I
don't think there's a clean way to refactor it.

https://codereview.chromium.org/1559593002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/dom/IntersectionObserver.cpp (right):

https://codereview.chromium.org/1559593002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/IntersectionObserver.cpp:25: static void
parseRootMargin(String rootMarginParameter, Vector<Length>& rootMargin,
ExceptionState& exceptionState)
On 2016/01/12 01:08:49, ojan wrote:
> TODO: We need to make sure this exact algorithm is specced somewhere.

Added TODO.

https://codereview.chromium.org/1559593002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/IntersectionObserver.cpp:196:
exceptionState.throwDOMException(HierarchyRequestError, "Cannot observe a
cross-origin target because the observer has a root margin value specified as a
percent.");
On 2016/01/12 01:08:49, ojan wrote:
> TODO: I wonder if we should have a TODO or something to add a URL to this
> description that can point people at some documentation explaining this. Or
> maybe we should just make sure the IO documentation covers this.

Added a TODO.

https://codereview.chromium.org/1559593002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/IntersectionObserver.cpp:260: return
LayoutUnit(length.intValue());
On 2016/01/12 01:08:49, ojan wrote:
> ASSERT(length.type() == Fixed); ?

Done.

https://codereview.chromium.org/1559593002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/IntersectionObserver.cpp:266: LayoutUnit
rightMargin = computeMargin(m_rightMargin, rect.width());
On 2016/01/12 01:08:49, ojan wrote:
> TODO: We should make sure the spec is clear that the left/right margins
resolve
> with respect to the width and not the height (since with most css layouts they
> resolve against the height).

Added TODO.

https://codereview.chromium.org/1559593002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/dom/IntersectionObserver.h (right):

https://codereview.chromium.org/1559593002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/IntersectionObserver.h:67: Length
m_leftMargin;
On 2016/01/12 01:08:49, ojan wrote:
> This object is getting quite large. I don't think we need to do anything about
> it at the moment, but when we think about perf/memory profiling, we should
> consider the case that there are a lot of observers on the page. We might want
> to move margins and thresholds to a rare data object in that case. 

Acknowledged.  I would expect that the number of observers would pretty much
always be small, although the number of observations might be much higher.  But
hard to predict how this might be used in the wild.

[ojan, 2016-01-12 21:13:48]

lgtm

I would have made the spec issues actual issues in the github. I'm a bit worried
we'll lose track of them in the code. But, if you make a point of keeping track
of them, then that's fine.

https://codereview.chromium.org/1559593002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/dom/IntersectionObservation.cpp (right):

https://codereview.chromium.org/1559593002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/IntersectionObservation.cpp:61:
m_observer->applyRootMargin(geometry.rootRect);
Do you remember why we decided to expand the rootRect by the margin? I can't
remember at the moment.

[szager1, 2016-01-12 21:18:38]

https://codereview.chromium.org/1559593002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/dom/IntersectionObservation.cpp (right):

https://codereview.chromium.org/1559593002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/IntersectionObservation.cpp:61:
m_observer->applyRootMargin(geometry.rootRect);
On 2016/01/12 21:13:48, ojan wrote:
> Do you remember why we decided to expand the rootRect by the margin? I can't
> remember at the moment.

I think that bit of spec pre-dated my involvement.

[szager1, 2016-01-12 21:18:44]

The CQ bit was checked by szager@chromium.org

[commit-bot: I haz the power, 2016-01-12 21:18:53]

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

[commit-bot: I haz the power, 2016-01-12 22:49:47]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2016-01-12 22:51:41]

Description was changed from

==========
Add root margin support for IntersectionObserver.

Depends on:

https://codereview.chromium.org/1553773002/

BUG=540528
R=ojan@chromium.org,esprehn@chromium.org
==========

to

==========
Add root margin support for IntersectionObserver.

Depends on:

https://codereview.chromium.org/1553773002/

BUG=540528
R=ojan@chromium.org,esprehn@chromium.org

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

[commit-bot: I haz the power, 2016-01-12 22:51:41]

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