Add default implementation of experimental methods

Add default implementation of experimental methods

Adding default implementation of experimental methods in the API layer
gives us flexibility to remove the methods from the implementation layer
in the future.  This can happen if we decide that the experimental
feature should not be supported anymore.

Also, added mapping of the effective connection and RTT throughput
types, so the API does not reference autogenerated files that may change
in the future and inadvertently break the API backward compatibility.

Committed: https://crrev.com/a943b25840730a07de97d3ae179566e143f368a6
Cr-Commit-Position: refs/heads/master@{#431598}

[kapishnikov, 2016-11-01 20:18:58]

Description was changed from

==========
Add default implementation of experimental methods

Adding default implementation of experimental methods in the API layer
gives us flexibility to remove the methods from the implementation layer
in the future.  This can happen if we decide that the experimental
feature should not be supported anymore.


Also, added mapping of the effective connection and RTT throughput
types, so the API does not reference autogenerated files that may change
in the future and inadvertently break the API backward compatibility.

Add default implementation of experimental methods
==========

to

==========
Add default implementation of experimental methods

Adding default implementation of experimental methods in the API layer
gives us flexibility to remove the methods from the implementation layer
in the future.  This can happen if we decide that the experimental
feature should not be supported anymore.


Also, added mapping of the effective connection and RTT throughput
types, so the API does not reference autogenerated files that may change
in the future and inadvertently break the API backward compatibility.
==========

[kapishnikov, 2016-11-01 20:20:02]

kapishnikov@chromium.org changed reviewers:
+ mef@chromium.org, pauljensen@chromium.org

[kapishnikov, 2016-11-07 17:37:31]

On 2016/11/01 22:18:49, kapishnikov wrote:

Added EFFECTIVE_ prefix to CONNECTION_TYPE constants as Paul recommended.

[pauljensen, 2016-11-08 14:00:32]

I'm happy with the default implementations.  I think we can do more with the NQE
docs, I suggested some improvements.  I think there is still a lot more we could
do, but we can wait until we decide to remove or expose the NQE APIs.

https://codereview.chromium.org/2470903002/diff/20001/components/cronet/andro...
File
components/cronet/android/api/src/org/chromium/net/ExperimentalCronetEngine.java
(right):

https://codereview.chromium.org/2470903002/diff/20001/components/cronet/andro...
components/cronet/android/api/src/org/chromium/net/ExperimentalCronetEngine.java:26:
* Unknown rtt throughput value.
What is "rtt throughput"?
rtt should be capitalized as it's an abreviation.

https://codereview.chromium.org/2470903002/diff/20001/components/cronet/andro...
components/cronet/android/api/src/org/chromium/net/ExperimentalCronetEngine.java:28:
public static final int RTT_THROUGHPUT_UNKNOWN = -1;
We need a better name than "RTT_THROUGHPUT". Perhaps CONNECTION_METRIC_UNKNOWN?

https://codereview.chromium.org/2470903002/diff/20001/components/cronet/andro...
components/cronet/android/api/src/org/chromium/net/ExperimentalCronetEngine.java:31:
* The connection type is unknown.
This needs a lot more description.  It should mention that this is an estimate
and should @link to the function that returns this value.  Ditto for the other
values.

Here's something I wrote up a couple months ago:
https://codereview.chromium.org/2337233003/diff/1/net/nqe/effective_connectio...

https://codereview.chromium.org/2470903002/diff/20001/components/cronet/andro...
components/cronet/android/api/src/org/chromium/net/ExperimentalCronetEngine.java:277:
* estimator.
Add "Call {@link Builder#enableNetworkQualityEstimator} to begin computing this
value."

https://codereview.chromium.org/2470903002/diff/20001/components/cronet/andro...
components/cronet/android/api/src/org/chromium/net/ExperimentalCronetEngine.java:386:
* quality estimator. Set to {@link #RTT_THROUGHPUT_UNKNOWN} if a valid value
"if a valid value is unavailable" made more sense with INVALID, but perhaps we
should say something like "UNKNOWN if no estimate is available".  Ditto for
others.

[kapishnikov, 2016-11-08 15:29:45]

https://codereview.chromium.org/2470903002/diff/20001/components/cronet/andro...
File
components/cronet/android/api/src/org/chromium/net/ExperimentalCronetEngine.java
(right):

https://codereview.chromium.org/2470903002/diff/20001/components/cronet/andro...
components/cronet/android/api/src/org/chromium/net/ExperimentalCronetEngine.java:26:
* Unknown rtt throughput value.
On 2016/11/08 14:00:32, pauljensen wrote:
> What is "rtt throughput"?
> rtt should be capitalized as it's an abreviation.

Renamed to CONNECTION_METRIC_UNKNOWN and changed the description accordingly.

https://codereview.chromium.org/2470903002/diff/20001/components/cronet/andro...
components/cronet/android/api/src/org/chromium/net/ExperimentalCronetEngine.java:28:
public static final int RTT_THROUGHPUT_UNKNOWN = -1;
On 2016/11/08 14:00:32, pauljensen wrote:
> We need a better name than "RTT_THROUGHPUT". Perhaps
CONNECTION_METRIC_UNKNOWN?

Done.

https://codereview.chromium.org/2470903002/diff/20001/components/cronet/andro...
components/cronet/android/api/src/org/chromium/net/ExperimentalCronetEngine.java:31:
* The connection type is unknown.
On 2016/11/08 14:00:32, pauljensen wrote:
> This needs a lot more description.  It should mention that this is an estimate
> and should @link to the function that returns this value.  Ditto for the other
> values.
> 
> Here's something I wrote up a couple months ago:
>
https://codereview.chromium.org/2337233003/diff/1/net/nqe/effective_connectio...

Done.

https://codereview.chromium.org/2470903002/diff/20001/components/cronet/andro...
components/cronet/android/api/src/org/chromium/net/ExperimentalCronetEngine.java:277:
* estimator.
On 2016/11/08 14:00:32, pauljensen wrote:
> Add "Call {@link Builder#enableNetworkQualityEstimator} to begin computing
this
> value."

Done.

https://codereview.chromium.org/2470903002/diff/20001/components/cronet/andro...
components/cronet/android/api/src/org/chromium/net/ExperimentalCronetEngine.java:386:
* quality estimator. Set to {@link #RTT_THROUGHPUT_UNKNOWN} if a valid value
On 2016/11/08 14:00:32, pauljensen wrote:
> "if a valid value is unavailable" made more sense with INVALID, but perhaps we
> should say something like "UNKNOWN if no estimate is available".  Ditto for
> others.

Changed to CONNECTION_METRIC_UNKNOWN + made small changes to the text. PTAL

[pauljensen, 2016-11-09 19:55:16]

https://codereview.chromium.org/2470903002/diff/40001/components/cronet/andro...
File
components/cronet/android/api/src/org/chromium/net/ExperimentalCronetEngine.java
(right):

https://codereview.chromium.org/2470903002/diff/40001/components/cronet/andro...
components/cronet/android/api/src/org/chromium/net/ExperimentalCronetEngine.java:297:
};
please remove all these semicolons after the }'s

https://codereview.chromium.org/2470903002/diff/40001/components/cronet/andro...
File
components/cronet/android/java/src/org/chromium/net/impl/JavaCronetEngine.java
(right):

https://codereview.chromium.org/2470903002/diff/40001/components/cronet/andro...
components/cronet/android/java/src/org/chromium/net/impl/JavaCronetEngine.java:12:
import org.chromium.net.ExperimentalCronetEngine;
can we remove this and instead just import static CONNECTION_METRIC_UNKNOWN?

https://codereview.chromium.org/2470903002/diff/40001/components/cronet/andro...
components/cronet/android/java/src/org/chromium/net/impl/JavaCronetEngine.java:122:
return EFFECTIVE_CONNECTION_TYPE_UNKNOWN;
wait, how can it find this member?  it's not imported...

[kapishnikov, 2016-11-09 20:17:19]

https://codereview.chromium.org/2470903002/diff/40001/components/cronet/andro...
File
components/cronet/android/api/src/org/chromium/net/ExperimentalCronetEngine.java
(right):

https://codereview.chromium.org/2470903002/diff/40001/components/cronet/andro...
components/cronet/android/api/src/org/chromium/net/ExperimentalCronetEngine.java:297:
};
On 2016/11/09 19:55:16, pauljensen wrote:
> please remove all these semicolons after the }'s

Done.

https://codereview.chromium.org/2470903002/diff/40001/components/cronet/andro...
File
components/cronet/android/java/src/org/chromium/net/impl/JavaCronetEngine.java
(right):

https://codereview.chromium.org/2470903002/diff/40001/components/cronet/andro...
components/cronet/android/java/src/org/chromium/net/impl/JavaCronetEngine.java:12:
import org.chromium.net.ExperimentalCronetEngine;
On 2016/11/09 19:55:16, pauljensen wrote:
> can we remove this and instead just import static CONNECTION_METRIC_UNKNOWN?

Removed

https://codereview.chromium.org/2470903002/diff/40001/components/cronet/andro...
components/cronet/android/java/src/org/chromium/net/impl/JavaCronetEngine.java:122:
return EFFECTIVE_CONNECTION_TYPE_UNKNOWN;
On 2016/11/09 19:55:16, pauljensen wrote:
> wait, how can it find this member?  it's not imported...

It can find it because it is declared in the superclass. Removed
"ExperimentalCronetEngine." qualifier for other constants.

[pauljensen, 2016-11-11 15:13:23]

lgtm modulo comments

https://codereview.chromium.org/2470903002/diff/60001/components/cronet/andro...
File
components/cronet/android/api/src/org/chromium/net/ExperimentalUrlRequest.java
(right):

https://codereview.chromium.org/2470903002/diff/60001/components/cronet/andro...
components/cronet/android/api/src/org/chromium/net/ExperimentalUrlRequest.java:29:
};
remove this semicolon

https://codereview.chromium.org/2470903002/diff/60001/components/cronet/andro...
components/cronet/android/api/src/org/chromium/net/ExperimentalUrlRequest.java:42:
};
ditto

https://codereview.chromium.org/2470903002/diff/60001/components/cronet/andro...
File
components/cronet/android/java/src/org/chromium/net/impl/CronetUrlRequestContext.java
(right):

https://codereview.chromium.org/2470903002/diff/60001/components/cronet/andro...
components/cronet/android/java/src/org/chromium/net/impl/CronetUrlRequestContext.java:575:
return EFFECTIVE_CONNECTION_TYPE_UNKNOWN;
can we change this to:
  case EffectiveConnectionType.TYPE_UNKNOWN:
      return EFFECTIVE_CONNECTION_TYPE_UNKNOWN;
  default:
      throw new RuntimeException("Internal Error: Illegal
EffectiveConnectionType value " + type);

[kapishnikov, 2016-11-11 16:55:22]

https://codereview.chromium.org/2470903002/diff/60001/components/cronet/andro...
File
components/cronet/android/api/src/org/chromium/net/ExperimentalUrlRequest.java
(right):

https://codereview.chromium.org/2470903002/diff/60001/components/cronet/andro...
components/cronet/android/api/src/org/chromium/net/ExperimentalUrlRequest.java:29:
};
On 2016/11/11 15:13:23, pauljensen wrote:
> remove this semicolon

Done.

https://codereview.chromium.org/2470903002/diff/60001/components/cronet/andro...
components/cronet/android/api/src/org/chromium/net/ExperimentalUrlRequest.java:42:
};
On 2016/11/11 15:13:23, pauljensen wrote:
> ditto

Done.

https://codereview.chromium.org/2470903002/diff/60001/components/cronet/andro...
File
components/cronet/android/java/src/org/chromium/net/impl/CronetUrlRequestContext.java
(right):

https://codereview.chromium.org/2470903002/diff/60001/components/cronet/andro...
components/cronet/android/java/src/org/chromium/net/impl/CronetUrlRequestContext.java:575:
return EFFECTIVE_CONNECTION_TYPE_UNKNOWN;
On 2016/11/11 15:13:23, pauljensen wrote:
> can we change this to:
>   case EffectiveConnectionType.TYPE_UNKNOWN:
>       return EFFECTIVE_CONNECTION_TYPE_UNKNOWN;
>   default:
>       throw new RuntimeException("Internal Error: Illegal
> EffectiveConnectionType value " + type);

Good idea! Done.

[kapishnikov, 2016-11-11 16:55:43]

The CQ bit was checked by kapishnikov@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-11 16:55:58]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-11 17:04:28]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-11-11 17:04:29]

Dry run: This issue passed the CQ dry run.

[kapishnikov, 2016-11-11 18:11:15]

The CQ bit was checked by kapishnikov@chromium.org

[kapishnikov, 2016-11-11 18:11:15]

The patchset sent to the CQ was uploaded after l-g-t-m from
pauljensen@chromium.org
Link to the patchset: https://codereview.chromium.org/2470903002/#ps80001
(title: "Addressed Paul's comments")

[commit-bot: I haz the power, 2016-11-11 18:11:38]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-11 18:18:22]

Description was changed from

==========
Add default implementation of experimental methods

Adding default implementation of experimental methods in the API layer
gives us flexibility to remove the methods from the implementation layer
in the future.  This can happen if we decide that the experimental
feature should not be supported anymore.


Also, added mapping of the effective connection and RTT throughput
types, so the API does not reference autogenerated files that may change
in the future and inadvertently break the API backward compatibility.
==========

to

==========
Add default implementation of experimental methods

Adding default implementation of experimental methods in the API layer
gives us flexibility to remove the methods from the implementation layer
in the future.  This can happen if we decide that the experimental
feature should not be supported anymore.


Also, added mapping of the effective connection and RTT throughput
types, so the API does not reference autogenerated files that may change
in the future and inadvertently break the API backward compatibility.
==========

[commit-bot: I haz the power, 2016-11-11 18:18:24]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-11-11 19:00:00]

Description was changed from

==========
Add default implementation of experimental methods

Adding default implementation of experimental methods in the API layer
gives us flexibility to remove the methods from the implementation layer
in the future.  This can happen if we decide that the experimental
feature should not be supported anymore.


Also, added mapping of the effective connection and RTT throughput
types, so the API does not reference autogenerated files that may change
in the future and inadvertently break the API backward compatibility.
==========

to

==========
Add default implementation of experimental methods

Adding default implementation of experimental methods in the API layer
gives us flexibility to remove the methods from the implementation layer
in the future.  This can happen if we decide that the experimental
feature should not be supported anymore.

Also, added mapping of the effective connection and RTT throughput
types, so the API does not reference autogenerated files that may change
in the future and inadvertently break the API backward compatibility.

Committed: https://crrev.com/a943b25840730a07de97d3ae179566e143f368a6
Cr-Commit-Position: refs/heads/master@{#431598}
==========

[commit-bot: I haz the power, 2016-11-11 19:00:01]

Patchset 5 (id:??) landed as
https://crrev.com/a943b25840730a07de97d3ae179566e143f368a6
Cr-Commit-Position: refs/heads/master@{#431598}