Closed Bug 1352274 Opened 3 years ago Closed 3 years ago

Add response header indicator for TCP FastOpen suceeded

Categories

(Core :: Networking: HTTP, enhancement)

55 Branch
enhancement
Not set

Tracking

()

RESOLVED FIXED
mozilla55
Tracking Status
firefox55 --- fixed

People

(Reporter: dragana, Assigned: dragana)

References

Details

(Whiteboard: [necko-active])

Attachments

(1 file, 3 obsolete files)

No description provided.
Assignee: nobody → dd.mozilla
Status: NEW → ASSIGNED
Whiteboard: [necko-active]
Depends on: 1354233
Attached patch bug_1352274_v1.patch (obsolete) — Splinter Review
Attachment #8855436 - Flags: review?(mcmanus)
Attached patch bug_1352274_v1.patch (obsolete) — Splinter Review
Fix tests.
Attachment #8855436 - Attachment is obsolete: true
Attachment #8855436 - Flags: review?(mcmanus)
Attachment #8855738 - Flags: review?(mcmanus)
I need to turn off TFO for 2 test from the patch, because for some platform TFO exists and for other does not, so I do not want to make the test dependent on platform and I will just disable TFO.
Comment on attachment 8855738 [details] [diff] [review]
bug_1352274_v1.patch

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

did you maybe mean to r?francois on this one (for devtools?) rather than the telemetry one? not sure - just asking.

::: netwerk/base/TCPFastOpen.h
@@ +37,5 @@
>    // This will cancel the backup connection and in case of a failure rewind
>    // the transaction.
>    virtual void FastOpenConnected(nsresult error) = 0;
>    virtual void FastOpenNotSupported() = 0;
> +  virtual void FastOpenStatus(uint8_t tfoStatus) = 0;

This should probably be SetFOO - as should FastOpenConnected above if you want to fix that one too..

::: netwerk/base/TCPFastOpenLayer.cpp
@@ +345,5 @@
>  }
>  
>  void
>  TCPFastOpenFinish(PRFileDesc * fd, PRErrorCode *err,
> +                  bool *fastOpenNotSupported, uint8_t *tfoStatus)

all of these result params are better off as c++ style & rather than c style *
Attachment #8855738 - Flags: review?(mcmanus) → review+
I wanted to ping Francois for telemetry, we must do that now, at least we used to be the case.
Comment on attachment 8855738 [details] [diff] [review]
bug_1352274_v1.patch

Do we still need a telemetry review?
Attachment #8855738 - Flags: review?(francois)
I looked through the patch but it wasn't obvious which part of it you wanted me to review from a data collection point of view.

Is this adding more data to an existing telemetry probe?
Flags: needinfo?(dd.mozilla)
Flags: needinfo?(dd.mozilla)
Attachment #8855738 - Flags: review?(francois)
(In reply to François Marier [:francois] from comment #7)
> I looked through the patch but it wasn't obvious which part of it you wanted
> me to review from a data collection point of view.
> 
> Is this adding more data to an existing telemetry probe?

Sorry my mistake. Sorry for a spam.
Attached patch bug_1352274_v1.patch (obsolete) — Splinter Review
Change to SetFastOpenStatus and added check for mSpdySession.
Attachment #8855738 - Attachment is obsolete: true
Attachment #8856963 - Flags: review+
Depends on: 1188435
Rebased
Attachment #8856963 - Attachment is obsolete: true
Attachment #8864054 - Flags: review+
Pushed by dd.mozilla@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/dc09d460ec9d
Add response header indicator for TCP FastOpen suceeded. r=mcmanus
https://hg.mozilla.org/mozilla-central/rev/dc09d460ec9d
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Depends on: 1362961
You need to log in before you can comment on or make changes to this bug.