[webkit-reviews] review denied: [Bug 66684] Implement high-resolution time via window.performance.now() : [Attachment 138256] Patch

[bugzilla-daemon at webkit.org]

Tony Gentilcore <tonyg at chromium.org> has denied Nat Duca <nduca at chromium.org>'s
request for review:
Bug 66684: Implement high-resolution time via window.performance.now()
https://bugs.webkit.org/show_bug.cgi?id=66684

Attachment 138256: Patch
https://bugs.webkit.org/attachment.cgi?id=138256&action=review

------- Additional Comments from Tony Gentilcore <tonyg at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=138256&action=review


Thanks for the updates. Looking good, just a few minor comments to address.

> Source/WebCore/page/Performance.cpp:75
> +    return 1000.0 *
m_frame->document()->loader()->timing()->convertMonotonicTimeToDocumentTime(mon
otonicallyIncreasingTime());

The spec says to return milliseconds, but if I'm reading this right, you are
converting to microseconds.

Since Date.now() and everything in performance.timing use ms, we should
probably stick to that, right? This is a double, so the microsecond precision
will be maintained.

> Source/WebCore/page/Performance.idl:43
> +	   double now();

According to the spec, this should be:
typedef double DOMHighResTimeStamp;
DOMHighResTimeStamp now();

That's analogous to the DOMTimeStamp used by other APIs.

Also, I've lost track, what is our current stance on vendor prefixing? Should
this be webkitNow() until the spec reaches CR?

> LayoutTests/fast/performance/script-tests/performance-now-timestamps.js:12
> +busyWait(10);

Because the condition below uses ">=", this wait doesn't add any value to the
test.

You could either just remove the wait or else change the test below to
something like "secondTimestamp > (firstTimestamp + (WAIT_TIME / 2))"

> LayoutTests/fast/performance/script-tests/performance-now-timestamps.js:16
> +shouldBeTrue("secondTimestamp >= firstTimestamp");

Any reason to use shouldBeGreaterThanOrEqual above but ">=" directly here?

> LayoutTests/fast/performance/script-tests/performance-now-timestamps.js:17
> +

Can you add a reasonable but generous <= condition to verify this is in the
correct range (ie. ms since navigationStart as opposed to ms since epoch or
microseconds since anything)?

> LayoutTests/fast/performance/script-tests/performance-now-timestamps.js:19
> +    layoutTestController.waitUntilDone();

There is no reason for this test to waitUntilDone. Please make it synchronous.
That will also fix the double TEST_COMPLETE message in your expectations.