1352274 - Add response header indicator for TCP FastOpen suceeded

Status: RESOLVED FIXED

(Core :: Networking: HTTP, enhancement)

Comment 1

[Dragana Damjanovic [:dragana]]

Attachment: bug_1352274_v1.patch

Comment 2

[Dragana Damjanovic [:dragana]]

Attachment: bug_1352274_v1.patch

Fix tests.

Comment 3

[Dragana Damjanovic [:dragana]]

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 4

[Patrick McManus [:mcmanus]]

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 *

Comment 5

[Dragana Damjanovic [:dragana]]

I wanted to ping Francois for telemetry, we must do that now, at least we used to be the case.

Comment 6

[Dragana Damjanovic [:dragana]]

Comment on attachment 8855738 [details] [diff] [review]
bug_1352274_v1.patch

Do we still need a telemetry review?

Comment 7

[François Marier [: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?

Comment 8

[Dragana Damjanovic [:dragana]]

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

Comment 9

[Dragana Damjanovic [:dragana]]

Attachment: bug_1352274_v1.patch

Change to SetFastOpenStatus and added check for mSpdySession.

Comment 10

[Dragana Damjanovic [:dragana]]

Attachment: bug_1352274_v1.patch

Rebased

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/dc09d460ec9d
Add response header indicator for TCP FastOpen suceeded. r=mcmanus

Comment 12

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/dc09d460ec9d