Bug 50400 – [Web Timing] Add handshakeStart to interface

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[James Simonsen]

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.

[James Simonsen]

Also, connectEnd now includes SSL time.

[James Simonsen]

Created attachment 75443 [details]
Patch

[James Simonsen]

Created attachment 75541 [details]
Patch

[Tony Gentilcore]

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]

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

Created attachment 75739 [details]
Patch

[Tony Gentilcore]

LGTM, but you'll still need to find an r+

[James Simonsen]

Fishing for an R+. I already have an LGTM.

[Darin Fisher (:fishd, Google)]

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]

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

Created attachment 76827 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 76827 [details]
Patch

Clearing flags on attachment: 76827

Committed r74242: <http://trac.webkit.org/changeset/74242>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[WebKit Review Bot]

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]

(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