Bug 50400

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 Flags
Patch
none
Patch
none
Patch
none
Patch none

James Simonsen
Reported 2010-12-02 10:34:32 PST
Add a marker when the HTTPS handshake begins. It should return 0 if HTTPS is not used or there is no way for the UA to know when the handshake begins.
Attachments
Patch (15.86 KB, patch)
2010-12-02 17:27 PST, James Simonsen 		no flags
DetailsFormatted DiffDiff
Patch (17.66 KB, patch)
2010-12-03 14:14 PST, James Simonsen 		no flags
DetailsFormatted DiffDiff
Patch (17.62 KB, patch)
2010-12-06 15:17 PST, James Simonsen 		no flags
DetailsFormatted DiffDiff
Patch (17.61 KB, patch)
2010-12-16 16:26 PST, James Simonsen 		no flags
DetailsFormatted DiffDiff
Show Obsolete (3) View All Add attachment proposed patch, testcase, etc.
James Simonsen
Comment 1 2010-12-02 16:06:05 PST
Also, connectEnd now includes SSL time.
James Simonsen
Comment 2 2010-12-02 17:27:58 PST
Created attachment 75443 [details] Patch
James Simonsen
Comment 3 2010-12-03 14:14:45 PST
Created attachment 75541 [details] Patch
Tony Gentilcore
Comment 4 2010-12-04 09:52:40 PST
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.
James Simonsen
Comment 5 2010-12-06 15:16:56 PST
(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.
James Simonsen
Comment 6 2010-12-06 15:17:16 PST
Created attachment 75739 [details] Patch
Tony Gentilcore
Comment 7 2010-12-07 20:56:54 PST
LGTM, but you'll still need to find an r+
James Simonsen
Comment 8 2010-12-08 11:25:22 PST
Fishing for an R+. I already have an LGTM.
Darin Fisher (:fishd, Google)
Comment 9 2010-12-13 00:13:09 PST
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.
James Simonsen
Comment 10 2010-12-16 16:25:10 PST
(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.
James Simonsen
Comment 11 2010-12-16 16:26:57 PST
Created attachment 76827 [details] Patch
WebKit Commit Bot
Comment 12 2010-12-17 02:38:13 PST
Comment on attachment 76827 [details] Patch Clearing flags on attachment: 76827 Committed r74242: <http://trac.webkit.org/changeset/74242>
WebKit Commit Bot
Comment 13 2010-12-17 02:38:19 PST
All reviewed patches have been landed. Closing bug.
WebKit Review Bot
Comment 14 2010-12-17 04:58:35 PST
http://trac.webkit.org/changeset/74242 might have broken Qt Linux Release The following tests are not passing: http/tests/misc/webtiming-ssl.php
Csaba Osztrogonác
Comment 15 2010-12-17 05:12:08 PST
(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
Note You need to log in before you can comment on or make changes to this bug.