Closed Bug 1343600 Opened 7 years ago Closed 7 years ago

Add TLS handshake Start/Stop events

Categories

(Core :: Networking: HTTP, enhancement)

enhancement
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla54
Tracking Status
firefox54 --- fixed

People

(Reporter: dragana, Assigned: dragana)

References

Details

(Whiteboard: [necko-active])

Attachments

(1 file, 3 obsolete files)

We do not have tls handshake events and there for tls time is not counted for navigationTiming
Whiteboard: [necko-active]
Assignee: nobody → dd.mozilla
Status: NEW → ASSIGNED
Attached patch bug_1343600.patch (obsolete) — Splinter Review
Attachment #8842550 - Flags: review?(valentin.gosu)
Comment on attachment 8842550 [details] [diff] [review]
bug_1343600.patch

Review of attachment 8842550 [details] [diff] [review]:
-----------------------------------------------------------------

::: netwerk/protocol/http/nsHttpTransaction.cpp
@@ +2109,5 @@
>  }
>  
>  void
> +nsHttpTransaction::SetConnectEnd(mozilla::TimeStamp timeStamp, bool onlyIfNull,
> +                                 bool tlsEnd)

default argument mismatch! See header.

::: netwerk/protocol/http/nsHttpTransaction.h
@@ +152,5 @@
>      void SetDomainLookupStart(mozilla::TimeStamp timeStamp, bool onlyIfNull = false);
>      void SetDomainLookupEnd(mozilla::TimeStamp timeStamp, bool onlyIfNull = false);
>      void SetConnectStart(mozilla::TimeStamp timeStamp, bool onlyIfNull = false);
> +    void SetConnectEnd(mozilla::TimeStamp timeStamp, bool tlsEnd,
> +                       bool onlyIfNull = false);

Here you have tlsEnd as the second argument, but in the definition of the method you have it as the last argument, so it probably doesn't do what you want it to.
I would leave the signature as it was, and call it with onlyIfNull = true for CONNECTED_TO and onlyIfNull = false for HANDSHAKE_ENDED since we want it to overwrite the connectEnd timestamp.
Attachment #8842550 - Flags: review?(valentin.gosu) → review-
Attached patch bug_1343600.patch (obsolete) — Splinter Review
Attachment #8842550 - Attachment is obsolete: true
Attachment #8842808 - Flags: review?(valentin.gosu)
Comment on attachment 8842808 [details] [diff] [review]
bug_1343600.patch

Review of attachment 8842808 [details] [diff] [review]:
-----------------------------------------------------------------

::: netwerk/protocol/http/nsHttpTransaction.cpp
@@ +605,5 @@
>              SetConnectStart(TimeStamp::Now());
>          } else if (status == NS_NET_STATUS_CONNECTED_TO) {
> +            SetConnectEnd(TimeStamp::Now(), true);
> +        } else if (status == NS_NET_STATUS_TLS_HANDSHAKE_ENDED) {
> +            SetConnectEnd(TimeStamp::Now(), flase);

typo: false
Attachment #8842808 - Flags: review?(valentin.gosu) → review+
Attached patch bug_1343600.patch (obsolete) — Splinter Review
Attachment #8842808 - Attachment is obsolete: true
Attachment #8842819 - Flags: review+
Attachment #8842819 - Attachment is obsolete: true
Attachment #8842820 - Flags: review+
Keywords: checkin-needed
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/08df0e07f0ba
add  TLS handshake Start/Stop events. r=:valentin
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/08df0e07f0ba
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
Depends on: 1344340
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a8564a2c30a3
add text for NS_NET_STATUS_TLS_HANDSHAKE_STARTING and NS_NET_STATUS_TLS_HANDSHAKE_ENDED tansport states. r=valentin
Backout by cbook@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d2013bea9eec
Backed out changeset a8564a2c30a3 for landing with wrong bug number
Depends on: 1350388
Blocks: 1351392
You need to log in before you can comment on or make changes to this bug.