Bug 50400 - [Web Timing] Add handshakeStart to interface
Summary: [Web Timing] Add handshakeStart to interface
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC OS X 10.5
: P2 Normal
Assignee: Nobody
URL: http://dvcs.w3.org/hg/webperf/raw-fil...
Keywords:
Depends on:
Blocks: 30685
  Show dependency treegraph
 
Reported: 2010-12-02 10:34 PST by James Simonsen
Modified: 2010-12-17 05:12 PST (History)
8 users (show)

See Also:


Attachments
Patch (15.86 KB, patch)
2010-12-02 17:27 PST, James Simonsen
no flags Details | Formatted Diff | Diff
Patch (17.66 KB, patch)
2010-12-03 14:14 PST, James Simonsen
no flags Details | Formatted Diff | Diff
Patch (17.62 KB, patch)
2010-12-06 15:17 PST, James Simonsen
no flags Details | Formatted Diff | Diff
Patch (17.61 KB, patch)
2010-12-16 16:26 PST, James Simonsen
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description James Simonsen 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.
Comment 1 James Simonsen 2010-12-02 16:06:05 PST
Also, connectEnd now includes SSL time.
Comment 2 James Simonsen 2010-12-02 17:27:58 PST
Created attachment 75443 [details]
Patch
Comment 3 James Simonsen 2010-12-03 14:14:45 PST
Created attachment 75541 [details]
Patch
Comment 4 Tony Gentilcore 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.
Comment 5 James Simonsen 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.
Comment 6 James Simonsen 2010-12-06 15:17:16 PST
Created attachment 75739 [details]
Patch
Comment 7 Tony Gentilcore 2010-12-07 20:56:54 PST
LGTM, but you'll still need to find an r+
Comment 8 James Simonsen 2010-12-08 11:25:22 PST
Fishing for an R+. I already have an LGTM.
Comment 9 Darin Fisher (:fishd, Google) 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.
Comment 10 James Simonsen 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.
Comment 11 James Simonsen 2010-12-16 16:26:57 PST
Created attachment 76827 [details]
Patch
Comment 12 WebKit Commit Bot 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>
Comment 13 WebKit Commit Bot 2010-12-17 02:38:19 PST
All reviewed patches have been landed.  Closing bug.
Comment 14 WebKit Review Bot 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
Comment 15 Csaba Osztrogonác 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