[webkit-reviews] review denied: [Bug 58354] [Navigation Timing] Use monotonicallyIncreasingTime() instead of currentTime() : [Attachment 103733] Patch

[bugzilla-daemon at webkit.org]

Pavel Feldman <pfeldman at chromium.org> has denied James Simonsen
<simonjam at chromium.org>'s request for review:
Bug 58354: [Navigation Timing] Use monotonicallyIncreasingTime() instead of
currentTime()
https://bugs.webkit.org/show_bug.cgi?id=58354

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

------- Additional Comments from Pavel Feldman <pfeldman at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=103733&action=review


Looks almost good.

> Source/WebCore/dom/DocumentTiming.h:40
> +	   , referenceStartTime(currentTime())

This might be not the best place to initialize these: document is created when
the provisional load commits. You could do a static lazy getter on
ResourceLoadTiming instead that would init those to emphasize that it does not
matter whether the time converted is before or after this initialization.

Also, it would be nice to see a comment explaining conversion taking place
there. The one that would explain how TimeTicks correlate with
monotonicallyIncreasingTime in case of Chromium so that other ports could jump
in.

> Source/WebCore/inspector/InspectorInstrumentation.cpp:512
> +    double finishTime = 0.0;

You should not do anything unless either of the agents below is activated.

> Source/WebCore/inspector/InspectorInstrumentation.cpp:516
> +	       finishTime = documentTiming->referenceStartTime +
monotonicFinishTime - documentTiming->monotonicReferenceStartTime;

Please add a FIXME suggesting that this conversion should be pushed into the
inspector itself.

> Source/WebCore/inspector/InspectorResourceAgent.cpp:126
> +    timingObject->setNumber("requestTime", referenceTime +
timing.requestTime);

Could you encapsulate this into the ResourceLoadTiming convenience getter?

> Source/WebCore/inspector/InspectorResourceAgent.cpp:167
> +	   responseObject->setObject("timing",
buildObjectForTiming(*response.resourceLoadTiming(), referenceTime));

That way you won't need to do this.

> Source/WebCore/inspector/InspectorResourceAgent.cpp:200
> +	   referenceTime = timing->referenceStartTime -
timing->monotonicReferenceStartTime;

And all this math syncing WebCore and platform time would live in a single
place...

> Source/WebCore/inspector/InspectorResourceAgent.cpp:246
> +    m_frontend->requestWillBeSent(resourceId,
m_pageAgent->frameId(loader->frame()), m_pageAgent->loaderId(loader),
loader->url().string(), buildObjectForResourceRequest(request), currentTime(),
initiatorObject, callStackValue,
buildObjectForResourceResponse(redirectResponse,
GetReferenceTimeFromLoader(loader)));

Like here, in case of the provisional load you will be using timing struct from
the current document.

> Source/WebCore/page/PerformanceTiming.cpp:370
> +    return toIntegerMilliseconds(timing->referenceStartTime +
monotonicSeconds - timing->monotonicReferenceStartTime);

Same math again.