Bug 42091 – Web Inspector: provide starts and ends for network phases instead of duration.

• Component: Web Inspector (Deprecated)
• Status: RESOLVED FIXED

[Pavel Feldman]

Patch to follow.

[Pavel Feldman]

Created attachment 61242 [details]
[PATCH] Proposed change.

[Tony Gentilcore]

Thanks for the update, Pavel. This looks like it will work perfectly for Web Timing.

FYI, Here's how Web Timing will make use of this API:
domainLookupStart = dnsStart
domainLookupEnd = dnsEnd
connectStart = connectStart
connectEnd = connectEnd
requestStart = sendStart
requestEnd = sendEnd
responseStart = receiveHeadersStart

Then it will convert -1s to 0s.

[Pavel Feldman]

> responseStart == receiveHeadersStart
> 
From what I see,

responseStart != receiveHeadersStart.

In most of the cases,

responseStart = requestEnd instead.

So you need to use receiveHeadersEnd as responseStart for more realistic picture. That's what I do in inspector.

[Tony Gentilcore]

(In reply to comment #3)
> > responseStart == receiveHeadersStart
> > 
> From what I see,
> 
> responseStart != receiveHeadersStart.
> 
> In most of the cases,
> 
> responseStart = requestEnd instead.
> 
> So you need to use receiveHeadersEnd as responseStart for more realistic picture. That's what I do in inspector.

In the Web Timing definition responseStart marks the time when the first byte of data is received from the network. From the names alone, I'd expect that requestEnd marks the time when the last byte of the request is sent. Then there should be a non-trivial amount network + server latency. After that, the first packet of the response should contain all or some of the HTTP headers and possibly some of the response body. I expect the time this first packet is received corresponds to receiveHeadersStart, no?

[Pavel Feldman]

> In the Web Timing definition responseStart marks the time when the first byte of data is received from the network. From the names alone, I'd expect that requestEnd marks the time when the last byte of the request is sent. Then there should be a non-trivial amount network + server latency. After that, the first packet of the response should contain all or some of the HTTP headers and possibly some of the response body. I expect the time this first packet is received corresponds to receiveHeadersStart, no?

Couple of things here:
- receiveHeadersStart and receiveHeadersEnd would be equal in majority of cases (all headers would fit into a packet).
- chromium's stack considers receiving headers to start once it issued the request.

These two reasons made me mimic chromium's understanding of things here. So you are right, we could fix it and remove receiveHeadersStart.

[Joseph Pecoraro]

Comment on attachment 61242 [details]
[PATCH] Proposed change.

This looks good. Thanks for the IRC explanation of the m_cached in updateResponse.

>  private:
> +        , receiveHeadersStart(0)
> +        , receiveHeadersEnd(0)
> +        , sslStart(-1)
> +        , sslEnd(-1)

I know the old code had -1.0 and 0.0. It might
be clearer if -1 was given a constant. "NoValue"
or something. But I'm perfectly fine with -1, as
that is what the old code had.

[Tony Gentilcore]

(In reply to comment #5)
> > In the Web Timing definition responseStart marks the time when the first byte of data is received from the network. From the names alone, I'd expect that requestEnd marks the time when the last byte of the request is sent. Then there should be a non-trivial amount network + server latency. After that, the first packet of the response should contain all or some of the HTTP headers and possibly some of the response body. I expect the time this first packet is received corresponds to receiveHeadersStart, no?
> 
> Couple of things here:
> - receiveHeadersStart and receiveHeadersEnd would be equal in majority of cases (all headers would fit into a packet).
> - chromium's stack considers receiving headers to start once it issued the request.
> 
> These two reasons made me mimic chromium's understanding of things here. So you are right, we could fix it and remove receiveHeadersStart.

Thank you for the explanation. If that is the case, then based on the chromium implementation, I do want receiveHeadersEnd for now.

However, we need to make sure ResourceLoadTiming.h is a clear and generic API that can be implemented by any of the network platforms (and not tied to chromium quirks). I'd argue that receiveHeadersStart is a misleading name for "finished sending request".

[Pavel Feldman]

(In reply to comment #7)
> (In reply to comment #5)
> > > In the Web Timing definition responseStart marks the time when the first byte of data is received from the network. From the names alone, I'd expect that requestEnd marks the time when the last byte of the request is sent. Then there should be a non-trivial amount network + server latency. After that, the first packet of the response should contain all or some of the HTTP headers and possibly some of the response body. I expect the time this first packet is received corresponds to receiveHeadersStart, no?
> > 
> > Couple of things here:
> > - receiveHeadersStart and receiveHeadersEnd would be equal in majority of cases (all headers would fit into a packet).
> > - chromium's stack considers receiving headers to start once it issued the request.
> > 
> > These two reasons made me mimic chromium's understanding of things here. So you are right, we could fix it and remove receiveHeadersStart.
> 
> Thank you for the explanation. If that is the case, then based on the chromium implementation, I do want receiveHeadersEnd for now.
> 
> However, we need to make sure ResourceLoadTiming.h is a clear and generic API that can be implemented by any of the network platforms (and not tied to chromium quirks). I'd argue that receiveHeadersStart is a misleading name for "finished sending request".

I agree. I am not saying that receiveHeadersStart is a name for "finished sending request". There is a requestEnd that is a name for that. So our options are:
1) remove receiveHeadersStart as a whole
2) leave it and allow embedders filling it properly (chromium will most likely fake it using same value as receiveHeadersEnd for now).

(2) Is the right thing to do according to the spec - it requires the real first byte delivery time. So if you go with the spec in other places, you should be consistent. It is just me who thinks the spec is wrong, so I am suggesting (1) :).

[Tony Gentilcore]

(In reply to comment #8)
> (In reply to comment #7)
> > (In reply to comment #5)
> > > > In the Web Timing definition responseStart marks the time when the first byte of data is received from the network. From the names alone, I'd expect that requestEnd marks the time when the last byte of the request is sent. Then there should be a non-trivial amount network + server latency. After that, the first packet of the response should contain all or some of the HTTP headers and possibly some of the response body. I expect the time this first packet is received corresponds to receiveHeadersStart, no?
> > > 
> > > Couple of things here:
> > > - receiveHeadersStart and receiveHeadersEnd would be equal in majority of cases (all headers would fit into a packet).
> > > - chromium's stack considers receiving headers to start once it issued the request.
> > > 
> > > These two reasons made me mimic chromium's understanding of things here. So you are right, we could fix it and remove receiveHeadersStart.
> > 
> > Thank you for the explanation. If that is the case, then based on the chromium implementation, I do want receiveHeadersEnd for now.
> > 
> > However, we need to make sure ResourceLoadTiming.h is a clear and generic API that can be implemented by any of the network platforms (and not tied to chromium quirks). I'd argue that receiveHeadersStart is a misleading name for "finished sending request".
> 
> I agree. I am not saying that receiveHeadersStart is a name for "finished sending request". There is a requestEnd that is a name for that. So our options are:
> 1) remove receiveHeadersStart as a whole
> 2) leave it and allow embedders filling it properly (chromium will most likely fake it using same value as receiveHeadersEnd for now).
> 
> (2) Is the right thing to do according to the spec - it requires the real first byte delivery time. So if you go with the spec in other places, you should be consistent. It is just me who thinks the spec is wrong, so I am suggesting (1) :).

#1 sounds good to me too.

[Pavel Feldman]

Created attachment 61269 [details]
[PATCH] For landing.

[WebKit Commit Bot]

Comment on attachment 61269 [details]
[PATCH] For landing.

Rejecting patch 61269 from commit-queue.

Unexpected failure when processing patch!  Please file a bug against webkit-patch.
Failed to run "['./WebKitTools/Scripts/webkit-patch', '--status-host=webkit-commit-queue.appspot.com', 'land-attachment', '--force-clean', '--build', '--non-interactive', '--ignore-builders', '--build-style=both', '--quiet', 61269, '--test', '--parent-command=commit-queue', '--no-update']" exit_code: 1
Last 500 characters of output:
ing: https://bugs.webkit.org/show_bug.cgi?id=42091&ctype=xml
Processing 1 patch from 1 bug.
Cleaning working directory
Processing patch 61269 from bug 42091.
 Reviewed by Joseph Pecoraro found in /Users/eseidel/Projects/CommitQueue/WebKit/chromium/ChangeLog does not appear to be a valid reviewer according to committers.py.
ERROR: /Users/eseidel/Projects/CommitQueue/WebKit/chromium/ChangeLog neither lists a valid reviewer nor contains the string "Unreviewed" or "Rubber stamp" (case insensitive).

[Tony Gentilcore]

Created attachment 61303 [details]
Fix Reviewed by line in WebKit/chromium/ChangeLog

[Tony Gentilcore]

This failed the cq because of a mangled "Reviewed by" line. So I took the liberty of fixing that bit and putting it back in the queue.

[WebKit Commit Bot]

Comment on attachment 61303 [details]
Fix Reviewed by line in WebKit/chromium/ChangeLog

Clearing flags on attachment: 61303

Committed r63166: <http://trac.webkit.org/changeset/63166>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.