IntersectionObserver: Always send an initial notification.

IntersectionObserver: Always send an initial notification.

Generate a notification for the initial state of a target, even if
it's offscreen.  This implements a change to the spec:

https://github.com/WICG/IntersectionObserver/issues/165

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

Review-Url: https://codereview.chromium.org/2645283008
Cr-Commit-Position: refs/heads/master@{#447299}
Committed: https://chromium.googlesource.com/chromium/src/+/c9092d27fc367898890811f6bb75893bc2200755

[ojan, 2017-01-26 01:44:56]

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

https://codereview.chromium.org/2645283008/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/dom/IntersectionObservation.h:26: void
setLastThresholdIndex(int index) { m_lastThresholdIndex = index; }
I assume this is leftover from a previous iteration of the patch?

https://codereview.chromium.org/2645283008/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/dom/IntersectionObservation.h:40: // we can't
bit-pack it.  Instead, inflate all thresholdIndex values by 1.
Since we're going hacky, how about initializing m_lastThresholdIndex to
0x40000000 and leaving the rest of the code untouched? Then the hack is
localized to one place and there's no need for a RELEASE_ASSERT.

[szager1, 2017-01-30 19:37:41]

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

https://codereview.chromium.org/2645283008/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/dom/IntersectionObservation.h:26: void
setLastThresholdIndex(int index) { m_lastThresholdIndex = index; }
On 2017/01/26 01:44:56, ojan wrote:
> I assume this is leftover from a previous iteration of the patch?

Yep, thanks for the catch.

https://codereview.chromium.org/2645283008/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/dom/IntersectionObservation.h:40: // we can't
bit-pack it.  Instead, inflate all thresholdIndex values by 1.
On 2017/01/26 01:44:56, ojan wrote:
> Since we're going hacky, how about initializing m_lastThresholdIndex to
> 0x40000000 and leaving the rest of the code untouched? Then the hack is
> localized to one place and there's no need for a RELEASE_ASSERT.

The RELEASE_ASSERT is me being paranoid.  This patch doesn't make it any more
necessary than it was before; it just occurred to me that guarding against
overflow is probably a good idea.

0x40000000 doesn't fit in 30 bits.  I could initialize it to
kMaxThresholdIndex-1, if you think that's clearer.

[szager1, 2017-01-30 20:02:46]

PTAL; latest patch gets rid of the initialState machinery, which was added to
implement the same behavior.

[szager1, 2017-01-30 20:03:07]

Description was changed from

==========
IntersectionObserver: Always send a initial notification.

Generate a notification for the initial state of a target, even if
it's offscreen.  This implements a change to the spec:

https://github.com/WICG/IntersectionObserver/issues/165

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

to

==========
IntersectionObserver: Always send a initial notification.

Generate a notification for the initial state of a target, even if
it's offscreen.  This implements a change to the spec:

https://github.com/WICG/IntersectionObserver/issues/165

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

[szager1, 2017-01-30 21:01:44]

Description was changed from

==========
IntersectionObserver: Always send a initial notification.

Generate a notification for the initial state of a target, even if
it's offscreen.  This implements a change to the spec:

https://github.com/WICG/IntersectionObserver/issues/165

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

to

==========
IntersectionObserver: Always send an initial notification.

Generate a notification for the initial state of a target, even if
it's offscreen.  This implements a change to the spec:

https://github.com/WICG/IntersectionObserver/issues/165

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

[ojan, 2017-01-31 05:36:48]

Initializing to kMaxThresholdIndex-1 and then not needing the extra logic in
computeIntersectionObservations seems cleaner to me. Only one place to need to
document the hack instead of two and no extra branching.

But up to you. lgtm

[szager1, 2017-01-31 15:15:03]

On 2017/01/31 05:36:48, ojan wrote:
> Initializing to kMaxThresholdIndex-1 and then not needing the extra logic in
> computeIntersectionObservations seems cleaner to me. Only one place to need to
> document the hack instead of two and no extra branching.
> 
> But up to you. lgtm

Yeah, I suppose you're right; done.

[szager1, 2017-01-31 15:26:44]

The CQ bit was checked by szager@chromium.org

[szager1, 2017-01-31 15:26:46]

The patchset sent to the CQ was uploaded after l-g-t-m from ojan@chromium.org
Link to the patchset: https://codereview.chromium.org/2645283008/#ps40001
(title: "nit")

[commit-bot: I haz the power, 2017-01-31 15:27:01]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-31 16:48:13]

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

[commit-bot: I haz the power, 2017-01-31 16:48:14]

Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[esprehn, 2017-01-31 16:54:14]

Real failure of IntersectionObserverTest.ResumePostsTask

[szager1, 2017-01-31 17:02:19]

On 2017/01/31 16:54:14, esprehn wrote:
> Real failure of IntersectionObserverTest.ResumePostsTask

Whoops!  I forget that IntersectionObserver even had unit tests.

Fixed.

[szager1, 2017-01-31 17:02:36]

The CQ bit was checked by szager@chromium.org

[szager1, 2017-01-31 17:02:36]

The patchset sent to the CQ was uploaded after l-g-t-m from ojan@chromium.org
Link to the patchset: https://codereview.chromium.org/2645283008/#ps60001
(title: "Fix unit test")

[commit-bot: I haz the power, 2017-01-31 17:03:05]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-31 19:38:03]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1485882156174430,
"parent_rev": "9d72c664ac08275bbde7dbb4bfd350416302d4f4", "commit_rev":
"c9092d27fc367898890811f6bb75893bc2200755"}

[commit-bot: I haz the power, 2017-01-31 19:38:36]

Description was changed from

==========
IntersectionObserver: Always send an initial notification.

Generate a notification for the initial state of a target, even if
it's offscreen.  This implements a change to the spec:

https://github.com/WICG/IntersectionObserver/issues/165

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

to

==========
IntersectionObserver: Always send an initial notification.

Generate a notification for the initial state of a target, even if
it's offscreen.  This implements a change to the spec:

https://github.com/WICG/IntersectionObserver/issues/165

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

Review-Url: https://codereview.chromium.org/2645283008
Cr-Commit-Position: refs/heads/master@{#447299}
Committed:
https://chromium.googlesource.com/chromium/src/+/c9092d27fc367898890811f6bb75...
==========

[commit-bot: I haz the power, 2017-01-31 19:38:37]

Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/c9092d27fc367898890811f6bb75...