IntersectionObserver: notify when root or target is removed.

IntersectionObserver: notify when root or target is removed.

If root and target were previously intersecting, and then DOM is
modified to remove root or target, or to make target not a
containing block descendant of root, send a notification.

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

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

[ojan, 2016-04-15 18:19:09]

lgtm

https://codereview.chromium.org/1889053002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/dom/IntersectionObservation.cpp (right):

https://codereview.chromium.org/1889053002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/dom/IntersectionObservation.cpp:152: return true;
Nit: I think this code would be more self-documenting if the bool were an
out-param instead of a return value. As it is, I had a hard time figuring out
what the return value meant. I think it means shouldAddEntry or something like
that?

https://codereview.chromium.org/1889053002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/dom/IntersectionObserver.cpp (left):

https://codereview.chromium.org/1889053002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/dom/IntersectionObserver.cpp:197: if (!m_root ||
!m_root->inShadowIncludingDocument())
Moving this makes me a bit sad because it means we potentially do a lot of work
for an observer whose root is not in the tree, but I see why this patch needs
it. I think it's fine though. If we see code in the wild doing this, we can
optimize it (e.g. only do this work the first time the root is removed from the
tree instead of each frame).

[szager1, 2016-04-15 19:47:23]

https://codereview.chromium.org/1889053002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/dom/IntersectionObservation.cpp (right):

https://codereview.chromium.org/1889053002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/dom/IntersectionObservation.cpp:152: return true;
On 2016/04/15 18:19:08, ojan wrote:
> Nit: I think this code would be more self-documenting if the bool were an
> out-param instead of a return value. As it is, I had a hard time figuring out
> what the return value meant. I think it means shouldAddEntry or something like
> that?

The return value means "should we *ever* send a notification."  It only returns
false when the observation is in an unrecoverable state (e.g., root or target
has been deleted, or root or target is an invalid node type).

[szager1, 2016-04-15 19:47:28]

The CQ bit was checked by szager@chromium.org

[commit-bot: I haz the power, 2016-04-15 19:47:39]

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

[commit-bot: I haz the power, 2016-04-15 22:13:39]

Committed patchset #1 (id:1)

[commit-bot: I haz the power, 2016-04-15 22:14:50]

Description was changed from

==========
IntersectionObserver: notify when root or target is removed.

If root and target were previously intersecting, and then DOM is
modified to remove root or target, or to make target not a
containing block descendant of root, send a notification.

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

to

==========
IntersectionObserver: notify when root or target is removed.

If root and target were previously intersecting, and then DOM is
modified to remove root or target, or to make target not a
containing block descendant of root, send a notification.

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

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

[commit-bot: I haz the power, 2016-04-15 22:14:51]

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

[ojan, 2016-04-15 22:40:52]

https://codereview.chromium.org/1889053002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/dom/IntersectionObservation.cpp (right):

https://codereview.chromium.org/1889053002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/dom/IntersectionObservation.cpp:152: return true;
On 2016/04/15 at 19:47:23, szager1 wrote:
> On 2016/04/15 18:19:08, ojan wrote:
> > Nit: I think this code would be more self-documenting if the bool were an
> > out-param instead of a return value. As it is, I had a hard time figuring
out
> > what the return value meant. I think it means shouldAddEntry or something
like
> > that?
> 
> The return value means "should we *ever* send a notification."  It only
returns false when the observation is in an unrecoverable state (e.g., root or
target has been deleted, or root or target is an invalid node type).

All the more reason to use an out param with a clear name. :)

[eae, 2016-04-17 18:14:13]

LGTM