Use DOMHighResTimeStamp for notification time.

Use DOMHighResTimeStamp for notification time.

Note that the time is relative to the execution context that created the observer, *not* the tracking document.

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

Committed: https://crrev.com/45eb90b43b0053f12da21b7a76c9ff6647942efb
Cr-Commit-Position: refs/heads/master@{#373324}

[szager1, 2016-01-21 22:57:57]

ping

[ojan, 2016-01-22 00:10:06]

1. I couldn't see anything in the spec about which context to use. Should there
be a bug on the spec to cover that?

2. Should be possible to write a test for this?

[szager1, 2016-01-22 00:22:55]

On 2016/01/22 00:10:06, ojan wrote:
> 1. I couldn't see anything in the spec about which context to use. Should
there
> be a bug on the spec to cover that?

I think this is the only thing that makes sense, but doesn't hurt to be
explicit.

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

> 2. Should be possible to write a test for this?

I have no idea how to do that; any pointers?

[ojan, 2016-01-22 19:24:10]

As far as testing, I was picturing that you could have an IO that is created in
a top-level frame and a callback out of an inner frame and verify that the time
origin is the one of the inner frame. Is that hard to do? This is an area I
don't know very well.

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

https://codereview.chromium.org/1613543002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/IntersectionObservation.cpp:152: timestamp,
I think you need to change IntersectionObserverEntry to store a
DOMHighResTimeStamp as well?

[szager1, 2016-02-01 23:52:30]

Description was changed from

==========
Use DOMHighResTimeStamp for notification time.

Note that the time is relative to the execution context of the
callback, *not* the tracking document.

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

to

==========
Use DOMHighResTimeStamp for notification time.

Note that the time is relative to the execution context that created the
observer, *not* the tracking document.

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

[szager1, 2016-02-02 18:07:51]

Added a test, PTAL.

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

https://codereview.chromium.org/1613543002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/IntersectionObservation.cpp:152: timestamp,
On 2016/01/22 19:24:10, ojan wrote:
> I think you need to change IntersectionObserverEntry to store a
> DOMHighResTimeStamp as well?

Done.

[ojan, 2016-02-03 01:46:28]

lgtm

Any tests with setTimeout are *very* likely to be flaky. In this case, I think
the setTimeout 100ms is sufficiently safe that we should be fine, but I don't
understand the other two setTimeouts. 

In either case, we should watch the flakiness dashboard in the couple days after
this lands to be sure it's not flaky.

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

https://codereview.chromium.org/1613543002/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/intersection-observer/timestamp.html:18:
setTimeout(function() {
Why do you need the setTimeout?

https://codereview.chromium.org/1613543002/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/intersection-observer/timestamp.html:34:
setTimeout(function() {
Ditto.

[szager1, 2016-02-03 01:54:35]

On 2016/02/03 01:46:28, ojan wrote:
> lgtm
> 
> Any tests with setTimeout are *very* likely to be flaky. In this case, I think
> the setTimeout 100ms is sufficiently safe that we should be fine, but I don't
> understand the other two setTimeouts. 
> 
> In either case, we should watch the flakiness dashboard in the couple days
after
> this lands to be sure it's not flaky.

ack

[szager1, 2016-02-03 01:54:45]

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

https://codereview.chromium.org/1613543002/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/intersection-observer/timestamp.html:18:
setTimeout(function() {
On 2016/02/03 01:46:28, ojan wrote:
> Why do you need the setTimeout?

The setTimeout's are to allow the IO notifications to be sent.  This first one
is not strictly necessary, but it's nice to start with an assert that no
notifications have been generated.

[szager1, 2016-02-03 01:54:58]

The CQ bit was checked by szager@chromium.org

[commit-bot: I haz the power, 2016-02-03 01:57:28]

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

[commit-bot: I haz the power, 2016-02-03 02:02:09]

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

[commit-bot: I haz the power, 2016-02-03 02:02:10]

Try jobs failed on following builders:
  android_clang_dbg_recipe on tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)
  android_compile_dbg on tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)
  cast_shell_linux on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
  chromeos_daisy_chromium_compile_only_ng on tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...)
  chromeos_x86-generic_chromium_compile_only_ng on tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_x86-ge...)
  linux_chromium_chromeos_compile_dbg_ng on tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_compile_dbg_32_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_gn_chromeos_rel on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)
  ios_rel_device_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)
  mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_gn_rel on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gn_r...)
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[ojan, 2016-02-03 02:51:50]

I just realized, this uses the execution context of the callback, not the IO
itself, no? They can be in different frames sadly. :(

[szager1, 2016-02-03 03:54:32]

On 2016/02/03 02:51:50, ojan wrote:
> I just realized, this uses the execution context of the callback, not the IO
> itself, no? They can be in different frames sadly. :(

Actually, no, it's the context that created the Observer constructor.

[ojan, 2016-02-03 05:04:18]

On 2016/02/03 at 03:54:32, szager wrote:
> On 2016/02/03 02:51:50, ojan wrote:
> > I just realized, this uses the execution context of the callback, not the IO
> > itself, no? They can be in different frames sadly. :(
> 
> Actually, no, it's the context that created the Observer constructor.

I'm confused. Aren't you getting the execution context off the callback?

[szager1, 2016-02-03 05:25:38]

On 2016/02/03 05:04:18, ojan wrote:
> On 2016/02/03 at 03:54:32, szager wrote:
> > On 2016/02/03 02:51:50, ojan wrote:
> > > I just realized, this uses the execution context of the callback, not the
IO
> > > itself, no? They can be in different frames sadly. :(
> > 
> > Actually, no, it's the context that created the Observer constructor.
> 
> I'm confused. Aren't you getting the execution context off the callback?

V8IntersectionObserverCallback takes its execution context from ScriptState of
the IO constructor, not from the actual v8::Function it invokes.

[ojan, 2016-02-03 05:28:17]

On 2016/02/03 at 05:25:38, szager wrote:
> On 2016/02/03 05:04:18, ojan wrote:
> > On 2016/02/03 at 03:54:32, szager wrote:
> > > On 2016/02/03 02:51:50, ojan wrote:
> > > > I just realized, this uses the execution context of the callback, not
the IO
> > > > itself, no? They can be in different frames sadly. :(
> > > 
> > > Actually, no, it's the context that created the Observer constructor.
> > 
> > I'm confused. Aren't you getting the execution context off the callback?
> 
> V8IntersectionObserverCallback takes its execution context from ScriptState of
the IO constructor, not from the actual v8::Function it invokes.

I see. Thx.

[szager1, 2016-02-03 05:32:46]

On 2016/02/03 05:25:38, szager1 wrote:
> On 2016/02/03 05:04:18, ojan wrote:
> > On 2016/02/03 at 03:54:32, szager wrote:
> > > On 2016/02/03 02:51:50, ojan wrote:
> > > > I just realized, this uses the execution context of the callback, not
the
> IO
> > > > itself, no? They can be in different frames sadly. :(
> > > 
> > > Actually, no, it's the context that created the Observer constructor.
> > 
> > I'm confused. Aren't you getting the execution context off the callback?
> 
> V8IntersectionObserverCallback takes its execution context from ScriptState of
> the IO constructor, not from the actual v8::Function it invokes.

void V8IntersectionObserver::constructorCustom(const
v8::FunctionCallbackInfo<v8:Value>& info) {
  ...
  new V8IntersectionObserverCallback(...,
ScriptState::current(info.GetIsolate());
}

V8IntersectionObserverCallback::V8IntersectionObserverCallback(..., ScriptState*
scriptState)
  : ActiveDOMCallback(scriptState->executionContext())
  ...

[szager1, 2016-02-03 05:51:30]

The CQ bit was checked by szager@chromium.org

[szager1, 2016-02-03 05:51:30]

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/1613543002/#ps80001
(title: "rebaseline")

[commit-bot: I haz the power, 2016-02-03 05:51:55]

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

[commit-bot: I haz the power, 2016-02-03 07:17:48]

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

[commit-bot: I haz the power, 2016-02-03 07:17:49]

Try jobs failed on following builders:
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[szager1, 2016-02-03 19:08:23]

The CQ bit was checked by szager@chromium.org

[szager1, 2016-02-03 19:08:24]

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/1613543002/#ps100001
(title: "syntax error in test")

[commit-bot: I haz the power, 2016-02-03 19:10:31]

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

[commit-bot: I haz the power, 2016-02-03 20:35:47]

Description was changed from

==========
Use DOMHighResTimeStamp for notification time.

Note that the time is relative to the execution context that created the
observer, *not* the tracking document.

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

to

==========
Use DOMHighResTimeStamp for notification time.

Note that the time is relative to the execution context that created the
observer, *not* the tracking document.

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

[commit-bot: I haz the power, 2016-02-03 20:35:48]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2016-02-03 20:36:47]

Description was changed from

==========
Use DOMHighResTimeStamp for notification time.

Note that the time is relative to the execution context that created the
observer, *not* the tracking document.

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

to

==========
Use DOMHighResTimeStamp for notification time.

Note that the time is relative to the execution context that created the
observer, *not* the tracking document.

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

Committed: https://crrev.com/45eb90b43b0053f12da21b7a76c9ff6647942efb
Cr-Commit-Position: refs/heads/master@{#373324}
==========

[commit-bot: I haz the power, 2016-02-03 20:36:50]

Patchset 6 (id:??) landed as
https://crrev.com/45eb90b43b0053f12da21b7a76c9ff6647942efb
Cr-Commit-Position: refs/heads/master@{#373324}