[webkit-reviews] review denied: [Bug 58354] [Navigation Timing] Use monotonicallyIncreasingTime() instead of currentTime() : [Attachment 103328] 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 103328: Patch
https://bugs.webkit.org/attachment.cgi?id=103328&action=review

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


> Source/WebCore/loader/MainResourceLoader.cpp:485
> +	   documentLoader()->timing()->responseEnd = finishTime;

I don't think you should mutate WebKit-provided values from within the WebCore
even for convenience. (I realize that both timing and finishTime originate from
WebKit). You've done the job for the main resource loader, but same timing
struct + finishTime are provided for subresources.

>> Source/WebCore/platform/network/ResourceLoadTiming.h:46
>> +	    timing->monotonicRequestTime = monotonicRequestTime;
> 
> Since this is a platform API, I want to be a little bit careful with it.
> 
> Is it really necessary to have a requestTime and monotonicRequestTime? I
don't understand why we can't put the monotonic version into requestTime. If
this is being overloaded to determine whether the platform provided monotonic
times, could we instead add a boolean timesAreMonotonicallyIncreasing or
something? If all ports used monotonic, could we get rid of this? Perhaps we
can just file bugs and not leave a trail in the API?

ResourceLoadTiming data originates from the port (WebKit). It does not really
matter whether we use currentTime or monotonicTime in WebCore since it is
likely that port's time will differ anyways. In chromium's case (and Chromium
is the only port providing this data), the time is taken in the network stack,
in the browser process. So I'd suggest that we leave it as is and carefully
sync it with the WebCore timings where these two intersect.

> Source/WebCore/platform/network/ResourceLoadTiming.h:96
> +    int responseEnd;

does not belong here as per comment above.