Upstream changes for NetworkQualityProvider

chrome/android/java/src/org/chromium/chrome/browser/net/qualityprovider/NetworkQualityProvider.java

Index: chrome/android/java/src/org/chromium/chrome/browser/net/qualityprovider/NetworkQualityProvider.java
diff --git a/chrome/android/java/src/org/chromium/chrome/browser/net/qualityprovider/NetworkQualityProvider.java b/chrome/android/java/src/org/chromium/chrome/browser/net/qualityprovider/NetworkQualityProvider.java
new file mode 100644
index 0000000000000000000000000000000000000000..553182efc5fd999cab7893e4d1d61bdf6cd3a7e0
--- /dev/null
+++ b/chrome/android/java/src/org/chromium/chrome/browser/net/qualityprovider/NetworkQualityProvider.java
@@ -0,0 +1,109 @@
+// Copyright 2015 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+package org.chromium.chrome.browser.net.qualityprovider;
+
+import android.content.Context;
+
+import org.chromium.base.annotations.CalledByNative;
+import org.chromium.chrome.browser.ChromeApplication;
+import org.chromium.chrome.browser.util.NonThreadSafe;
+
+/**
+ * This class provides a base class implementation and may be overridden on operating systems that
+ * provide more useful APIs. This class is not thread safe.
+ */
+public class NetworkQualityProvider {
+    private static NonThreadSafe sThreadCheck = null;
+
+    /**
+     * Value to return if a valid value is unavailable.
+     */
+    protected static final int NO_VALUE = -1;
+    private static NetworkQualityProvider sNetworkQualityProvider;
+    private static final Object sLock = new Object();

[nyquist, 2015/08/06 23:26:28]

Nit: Could you put this final static member above the non-final one? It helps
clarifying what can be changed and not.

In fact, could you reorder the sThreadCheck as well?

so:
... static final ..
... static ...

In practice something like this ordering:
NO_VALUE, sLock, sThreadCheck, sNetworkQualityProvider

And on the super-duper-nitty-and-optional-side (we are not consistent in
chromium codebase), feel free to rename sLock to LOCK, since it's something in
between a singleton and a constant :-p But that's seriously up to you. I feel
stronger about the ordering.

[tbansal1, 2015/08/07 00:52:02]

Done.
Done. Going with the majority:
sVarName is used 31 times.
https://code.google.com/p/chromium/codesearch#search/&q=f:java$%20static%5C%2...
VAR_NAME is used 53 times.
https://code.google.com/p/chromium/codesearch#search/&q=f:java$%20static%5C%2...

+
+    @CalledByNative
+    private static NetworkQualityProvider create(Context context) {
+        synchronized (sLock) {
+            if (sNetworkQualityProvider == null) {
+                assert sThreadCheck == null;
+                assert sNetworkQualityProvider == null;
+                sThreadCheck = new NonThreadSafe();
+                sNetworkQualityProvider =
+                        ((ChromeApplication) context).createNetworkQualityProvider();
+            }
+        }
+        return sNetworkQualityProvider;
+    }
+
+    /**
+     * Creates an instance of |@link #NetworkQualityProvider}.
+     */
+    public NetworkQualityProvider(Context context) {

[nyquist, 2015/08/06 23:26:28]

I'm assuming the downstream code needs the context, which is why it's passed
here, but not used? Or is it just something old?

If it's only necessary downstream, you could in fact just make it part of the
constructor of the subclass instead.

So all-in-all, remove |context| parameter?

[tbansal1, 2015/08/07 00:52:02]

It is needed downstream. Done.

+        assert sThreadCheck.calledOnValidThread();
+    }
+
+    /**
+     * Returns true if the network quality estimate is available.

[nyquist, 2015/08/06 23:26:28]

How do you feel about using "@return ..." form for these comments? By the way,
if you do that, I don't think you also have to have a "method-level comment" in
addition to the @return line.

I don't feel strongly though.

[tbansal1, 2015/08/07 00:52:02]

Done.

+     */
+    @CalledByNative
+    protected boolean isEstimateAvailable() {
+        assert sThreadCheck.calledOnValidThread();
+        return false;
+    }
+
+    /**
+     * Requests the provider to update the network quality.
+     */
+    @CalledByNative
+    protected void requestUpdate() {
+        assert sThreadCheck.calledOnValidThread();
+    }
+
+    /**
+     * Returns the expected RTT duration in milliseconds.
+     * Returns {@link #NO_VALUE} if the estimate is unavailable.
+     */
+    @CalledByNative
+    protected int getRTTMilliseconds() {
+        assert sThreadCheck.calledOnValidThread();
+        return NO_VALUE;
+    }
+
+    /**
+     * Returns the expected downstream throughput in Kbps (Kilobits per second).
+     * Returns {@link #NO_VALUE} if the estimate is unavailable.
+     */
+    @CalledByNative
+    protected long getDownstreamThroughputKbps() {
+        assert sThreadCheck.calledOnValidThread();
+        return NO_VALUE;
+    }
+
+    /**
+     * Returns the expected upstream throughput in Kbps (Kilobits per second).
+     * Returns {@link #NO_VALUE} if the estimate is unavailable.
+     */
+    @CalledByNative
+    protected long getUpstreamThroughputKbps() {
+        assert sThreadCheck.calledOnValidThread();
+        return NO_VALUE;
+    }
+
+    /**
+     * Returns time (in seconds) since the network quality was last updated.
+     */
+    @CalledByNative
+    protected long getTimeSinceLastUpdateSeconds() {
+        assert sThreadCheck.calledOnValidThread();
+        return NO_VALUE;
+    }
+
+    @CalledByNative
+    private static int getNoValue() {
+        assert sThreadCheck.calledOnValidThread();
+        return NetworkQualityProvider.NO_VALUE;

[nyquist, 2015/08/06 23:26:27]

Just return NO_VALUE;

[tbansal1, 2015/08/07 00:52:02]

Done.

+    }
+}