IntersectionObserver: convert tests to testharness.js

third_party/WebKit/LayoutTests/intersection-observer/resources/intersection-observer-test-utils.js

+// Here's how waitForNotification works:

[foolip, 2017/01/11 11:42:33]

Great documentation! :)

+//
+// - myTestFunction0()
+//   - waitForNotification(myTestFunction1)
+//     - requestAnimationFrame()
+//   - Modify DOM in a way that should trigger an IntersectionObserver callback.
+// - BeginFrame
+//   - requestAnimationFrame handler runs
+//     - First setTimeout()
+//   - Style, layout, paint
+//   - IntersectionObserver generates new notifications
+//     - Posts a task to deliver notifications
+// - First setTimeout handler runs
+//   - Second setTimeout()
+// - Task to deliver IntersectionObserver notifications runs
+//   - IntersectionObserver callbacks run
+// - Second setTimeout handler runs
+//   - myTestFunction1()
+//     - waitForNotification(myTestFunction2)

[foolip, 2017/01/11 11:42:33]

I guess that this part is optional, and that the order relative to the two
points below isn't important? If so might want to put it as an "optional: ..."
at the end?

[szager1, 2017/01/23 23:18:04]

Done.

+//       - requestAnimationFrame()
+//     - Verify newly-arrived IntersectionObserver notifications
+//     - Modify DOM to trigger new notifications
+function waitForNotification(f, description) {
+  requestAnimationFrame(function() {
+    setTimeout(function() {
+      setTimeout(f);
+    });
+  });
+}
+
+function runTestCycle(f, description) {
+  async_test(function(t) {

[foolip, 2017/01/11 11:42:33]

Can you document that the timing of when runTestCycle is called matters? As long
as it's called before/during the window load event or from a step of another
test it should be fine, but if for any odd reason one were to to setTimeout from
one test to start another, then that second test may or may not run. Such a test
could use setup({ explicit_done: true }) though.

[szager1, 2017/01/23 23:18:04]

Done.

+    waitForNotification(t.step_func(function() {

[foolip, 2017/01/11 11:42:33]

Simplification nit: waitForNotification(t.step_func_done(f));

[szager1, 2017/01/23 23:18:04]

Done.

+      f();
+      t.done();
+    }));
+  }, description);
+}
+
+// Root bounds for a root with an overflow clip as defined by:
+//   http://wicg.github.io/IntersectionObserver/#intersectionobserver-root-intersection-rectangle
+function contentBounds(root) {
+  var left = root.offsetLeft + root.clientLeft;
+  var right = left + root.clientWidth;
+  var top = root.offsetTop + root.clientTop;
+  var bottom = top + root.clientHeight;
+  return [left, right, top, bottom];
+}
+
+// Root bounds for a root without an overflow clip as defined by:
+//   http://wicg.github.io/IntersectionObserver/#intersectionobserver-root-intersection-rectangle
+function borderBoxBounds(root) {
+  var left = root.offsetLeft;
+  var right = left + root.offsetWidth;
+  var top = root.offsetTop;
+  var bottom = top + root.offsetHeight;
+  return [left, right, top, bottom];
+}
+
+function rectArea(rect) {
+  return (rect.left - rect.right) * (rect.bottom - rect.top);
+}
+
+function checkRect(actual, expected, description) {
+  if (expected.length > 0)

[foolip, 2017/01/11 11:42:33]

Just skimming the call sites I don't quite see when expected.length is anything
other than 4. Can it be?

[szager1, 2017/01/23 23:18:04]

Yes, there are cases where it's called with expected.length==0.  I simplified
the length checking, since length will only ever be 0 or 4.

+    assert_equals(actual.left, expected[0], description + ".left == " + expected[0]);

[foolip, 2017/01/11 11:42:33]

testharness.js style nit: the assertion description could simply be `description
+ ".left"` result in a sensible failure message, there should be no reason to
include the expected value in the description, but simply to say what the actual
value is.

[szager1, 2017/01/23 23:18:04]

Done.

+  if (expected.length > 1)
+    assert_equals(actual.right, expected[1], description + ".right == " + expected[1]);
+  if (expected.length > 2)
+    assert_equals(actual.top, expected[2], description + ".top == " + expected[2]);
+  if (expected.length > 3)
+    assert_equals(actual.bottom, expected[3], description + ".bottom == " + expected[3]);
+}
+
+function checkEntry(entries, i, expected) {
+  assert_equals(entries.length, i+1, String(i+1) + " notification(s).");

[foolip, 2017/01/11 11:42:33]

Nit: Here the description "number of notifications" or "entries.length" would
result in a sensible failure message.

[foolip, 2017/01/11 13:42:53]

Actually, why can checkEntry only be used to check the last entry? Should it be
called checkLastEntry?

[szager1, 2017/01/23 23:18:04]

That's a good idea, changed to checkLastEntry.

+  if (expected) {
+    checkRect(entries[i].boundingClientRect, expected.slice(0, 4),
+              "entries[" + i + "].boundingClientRect");
+    checkRect(entries[i].intersectionRect, expected.slice(4, 8),
+              "entries[" + i + "].intersectionRect");
+    checkRect(entries[i].rootBounds, expected.slice(8, 12),
+              "entries[" + i + "].rootBounds");
+  }
+}
+
+function coordinatesToClientRectJson(top, right, bottom, left) {

[foolip, 2017/01/11 11:42:33]

This appears to be unused now.

[szager1, 2017/01/23 23:18:04]

It's used in resources/cross-origin-subframe.html

+  return {
+    top: top,
+    right: right,
+    bottom: bottom,
+    left: left
+  };
+}
+
+function clientRectToJson(rect) {
+  if (!rect)
+    return "null";
+  return {
+    top: rect.top,
+    right: rect.right,
+    bottom: rect.bottom,
+    left: rect.left
+  };
+}
+
+function entryToJson(entry) {
+  return {
+    boundingClientRect: clientRectToJson(entry.boundingClientRect),
+    intersectionRect: clientRectToJson(entry.intersectionRect),
+    rootBounds: clientRectToJson(entry.rootBounds),
+    target: entry.target.id
+  };
+}
+
+function checkJsonEntry(actual, expected) {
+  checkRect(actual.boundingClientRect, expected.boundingClientRect, "entry.boundingClientRect");
+  checkRect(actual.intersectionRect, expected.intersectionRect, "entry.intersectionRect");
+  if (actual.rootBounds == "null")

[foolip, 2017/01/11 11:42:33]

I've argued against requiring === everywhere on blink-dev, but here it would
actually help my understanding. Can actual.rootBounds be the null literal, or
should only the string "null" work here?

[szager1, 2017/01/23 23:18:04]

This value is encoded by clientRectToJson, which uses a literal "null" string.

+    assert_equals(expected.rootBounds, "null", "rootBounds is null");
+  else
+    checkRect(actual.rootBounds, expected.rootBounds, "entry.rootBounds");
+  assert_equals(actual.target, expected.target);
+}
+
+function checkJsonEntries(actual, expected, description) {
+  test(function() {

[foolip, 2017/01/11 11:42:33]

Shouldn't this already be inside a test? Will comment on
cross-origin-iframe.html

[szager1, 2017/01/23 23:18:04]

I addressed this in the test, but: this is a case where a failure should not
halt execution of the entire test run.

+    assert_equals(actual.length, expected.length);
+    for (var i = 0; i < actual.length; i++)
+      checkJsonEntry(actual[i], expected[i]);
+  }, description);
+}