Bug 42091

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

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.

> 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 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?

> 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.

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.

(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".

(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) :).

(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.

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

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).

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

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.

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>

All reviewed patches have been landed. Closing bug.