| Summary: | [Web Timing] Add handshakeStart to interface | ||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | James Simonsen <simonjam> | ||||||||||
| Component: | WebCore Misc. | Assignee: | Nobody <webkit-unassigned> | ||||||||||
| Status: | RESOLVED FIXED | ||||||||||||
| Severity: | Normal | CC: | abarth, commit-queue, eric, fishd, ossy, simonjam, tonyg, webkit.review.bot | ||||||||||
| Priority: | P2 | ||||||||||||
| Version: | 528+ (Nightly build) | ||||||||||||
| Hardware: | PC | ||||||||||||
| OS: | OS X 10.5 | ||||||||||||
| URL: | http://dvcs.w3.org/hg/webperf/raw-file/tip/specs/NavigationTiming/Overview.html#nt-handshake-start | ||||||||||||
| Bug Depends on: | |||||||||||||
| Bug Blocks: | 30685 | ||||||||||||
| Attachments: |
|
||||||||||||
|
Description
James Simonsen
2010-12-02 10:34:32 PST
Also, connectEnd now includes SSL time. Created attachment 75443 [details]
Patch
Created attachment 75541 [details]
Patch
Comment on attachment 75541 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=75541&action=review > LayoutTests/http/tests/misc/webtiming-ssl-expected.txt:1 > +This test checks that Web Timing reports handshakeStart correctly and has reasonable values for connectStart and connectEnd. Note that DumpRenderTree doesn't set handshakeStart. Thanks for adding the test even though it fails. We should probably open a bug to track this. > WebCore/page/PerformanceTiming.cpp:-190 > - connectStart = timing->dnsEnd; It isn't clear to me why this is removed. Trimming DNS seems unrelated to trimming SSL. If it is correct, a better explanation in the ChangeLog is probably warranted. (In reply to comment #4) > (From update of attachment 75541 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=75541&action=review > > > LayoutTests/http/tests/misc/webtiming-ssl-expected.txt:1 > > +This test checks that Web Timing reports handshakeStart correctly and has reasonable values for connectStart and connectEnd. Note that DumpRenderTree doesn't set handshakeStart. > > Thanks for adding the test even though it fails. We should probably open a bug to track this. Done. https://bugs.webkit.org/show_bug.cgi?id=50592 > > WebCore/page/PerformanceTiming.cpp:-190 > > - connectStart = timing->dnsEnd; > > It isn't clear to me why this is removed. Trimming DNS seems unrelated to trimming SSL. If it is correct, a better explanation in the ChangeLog is probably warranted. Reverted. I must've misunderstood the comment or something. Clearly the original code is correct. Created attachment 75739 [details]
Patch
LGTM, but you'll still need to find an r+ Fishing for an R+. I already have an LGTM. This is more of a question for the spec, but why is it "handshakeStart" and not "sslHandshakeStart" or something like that? It seems like there could be other handshakes involved in fetching a document. (In reply to comment #9) > This is more of a question for the spec, but why is it "handshakeStart" and not "sslHandshakeStart" or something like that? It seems like there could be other handshakes involved in fetching a document. Good point. They seem to agree on the list, so that's what we'll go with. Created attachment 76827 [details]
Patch
Comment on attachment 76827 [details] Patch Clearing flags on attachment: 76827 Committed r74242: <http://trac.webkit.org/changeset/74242> All reviewed patches have been landed. Closing bug. http://trac.webkit.org/changeset/74242 might have broken Qt Linux Release The following tests are not passing: http/tests/misc/webtiming-ssl.php (In reply to comment #14) > http://trac.webkit.org/changeset/74242 might have broken Qt Linux Release > The following tests are not passing: > http/tests/misc/webtiming-ssl.php I filed a new bug on it: https://bugs.webkit.org/show_bug.cgi?id=51245 |