Introduce Visible Networks Tracker.

chrome/android/java/src/org/chromium/chrome/browser/omnibox/geo/VisibleNetworksTracker.java

+// Copyright 2017 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.omnibox.geo;
+
+import android.content.Context;
+import android.os.AsyncTask;
+
+import org.chromium.base.Log;
+import org.chromium.base.VisibleForTesting;
+
+import javax.annotation.Nullable;
+import javax.annotation.concurrent.GuardedBy;
+
+/**
+ * VisibleNetworksTracker keeps track of the visible networks.
+ */
+public class VisibleNetworksTracker {
+    private static final String TAG = "VNTracker";
+
+    static TimeProvider sTimeProvider = new TimeProvider();
+    private static final int AGE_THRESHOLD = 5 * 60 * 1000; // 5 min
+
+    @GuardedBy("this")

[Ted C, 2017/05/18 02:13:33]

As these are statics, it isn't guarded by this because there is no "this" for
statics.  Actually, looks like proguard is complaining about this as well.

[lbargu, 2017/05/18 14:15:29]

Sorry, fixed throughout.

+    @Nullable
+    private static VisibleNetworks sVisibleNetworks;

[dougt, 2017/05/18 02:59:03]

Sorry, I am not sure what synchronized on |this| means for static in Java.  I
think what you want is to have a getter/setter.  Something like:

static synchronized VisibleNetworks getVisibleNetworks() {...}
static synchronized void setVisibleNetworks(VisibleNetworks vn) {...}

i am not sure if this is the best approach.  I'd love it so that you didn't need
any locking.  Maybe post the result from refreshVisibleNetworks back to the main
thread?

[lbargu, 2017/05/18 14:15:29]

Removed locking. Using UI thread.

+    @GuardedBy("this")
+    private static long sVisibleNetworksTime = Long.MAX_VALUE;
+
+    private static VisibleNetworks sVisibleNetworksForTesting;
+    private static boolean sUseVisibleNetworksForTesting;
+
+    /**
+     * Returns last known visible networks. It returns the cached value if the cache is valid or it
+     * computes the visibleNetworks, caches the value and then returns it.
+     */
+    public static synchronized VisibleNetworks getLastKnownVisibleNetworks(Context context) {

[Ted C, 2017/05/18 02:13:33]

What threads do we expect this to run on?

We rarely use synchronized in our codebase, so I'm wondering why we are here. 
Any reason it needs to support multiple threads?

More often than not we use ThreadUtils.assertOnXXX to verify threading than
allowing multiple threads to access

[dougt, 2017/05/18 02:59:03]

This method is pretty odd.  It sometimes returns all access points and it
sometimes doesn't -- based on if the data is stale or not.  In some ways, I'd
rather have the interface be more explict:

Have getLastKnownVisibleNetworks always returns the last
getCachedVisibleNetworks() and have the consumer make the decision if they want
something cheaper/faster/less accurate.

So, the consumer will do something like:

1) Get getLastKnownVisibleNetworks()
2) Check to time of it to determine if it's good enough
3) Then either use it, or call getFastVisibileNetworks (or whatever you want to
name it)

This way it's very clear to the consumer what's going on, and you don't have a
method that returns very different objects because of staleness.  WDYT?


Also, wondering out loud a bit -- I wonder if we can somehow ensure that we
always have a good cached location so that we don't end um in the 'false' case. 
Do we have any idea how often this case will happen?

[lbargu, 2017/05/18 14:15:29]

Decided to keep the logic here but simpler.
Redoing the logic a bit and documenting in JavaDoc.
This also guarantees we don't end up caching an incomplete set of networks.
Updated tests too.

[lbargu, 2017/05/18 14:15:29]

Done.

+        if (sUseVisibleNetworksForTesting) return sVisibleNetworksForTesting;
+
+        if (isValidCachedVisibleNetworks()) return getCachedVisibleNetworks();
+
+        VisibleNetworks visibleNetworks = null;
+        try {
+            // Include only the connected cell/wifi to minimize latency and compute the simplest
+            // visible networks possible.
+            visibleNetworks = PlatformNetworksManager.computeVisibleNetworks(
+                    context, false /* includeAllVisibleNotConnectedNetworks */);
+        } catch (Exception e) {
+            Log.e(TAG, "Failed to get the visible networks. Error: ", e.toString());
+        }
+        setCachedVisibleNetworks(visibleNetworks);
+        return sVisibleNetworks;
+    }
+
+    /**
+     * Refreshes the visible networks cache if the cache is not valid by starting a background task

[Ted C, 2017/05/18 02:13:33]

this describes a bit too much about the implementation IMO.  I think it would be
fine to just say "Determines if the visible networks need to be refreshed and
asynchronously updates them if needed".

[lbargu, 2017/05/18 14:15:29]

Done.

+     * to compute the visible networks including all visible wifis/cells. Caches the returned value
+     * when the background task completes.
+     */
+    public static void refreshVisibleNetworks(final Context context) {
+        if (isValidCachedVisibleNetworks()) {
+            return;
+        }
+        new AsyncTask<Void, Void, VisibleNetworks>() {

[Ted C, 2017/05/18 02:13:33]

How expensive is this calculation?  Looks like it would be possible for multiple
calls to refreshVisibleNetworks to happen and kick off multiple AsyncTasks (if
you were to call this method back, to back, to back, you'd kick off AsyncTasks
until the first one completed and updated isValidCachedVisibleNetworks).

If this is fast and cheap, probably not worth really worrying too much about it.
 If it isn't, we should keep a reference to the async task and set it to null in
onPostExecute and ensure it is non-null here before starting a new one.

[lbargu, 2017/05/18 14:15:29]

Done. Good point, it might be not that cheap on some devices.

+            @Override
+            protected VisibleNetworks doInBackground(Void... params) {
+                VisibleNetworks visibleNetworks = null;
+                try {
+                    // Include all visible wifis and cells.
+                    visibleNetworks = PlatformNetworksManager.computeVisibleNetworks(
+                            context, true /* includeAllVisibleNotConnectedNetworks */);
+                } catch (Exception e) {
+                    Log.e(TAG, "Failed to get the visible networks. Error: ", e.toString());
+                }
+                return visibleNetworks;
+            }
+
+            @Override
+            protected void onPostExecute(VisibleNetworks visibleNetworks) {
+                setCachedVisibleNetworks(visibleNetworks);
+            }
+        }
+                .executeOnExecutor(AsyncTask.THREAD_POOL_EXECUTOR);
+    }
+
+    @Nullable
+    @VisibleForTesting
+    static synchronized VisibleNetworks getCachedVisibleNetworks() {
+        return sVisibleNetworks;
+    }
+
+    @VisibleForTesting
+    static synchronized long getCachedVisibleNetworksTime() {
+        return sVisibleNetworksTime;
+    }
+
+    @VisibleForTesting
+    static synchronized void clearCache() {
+        setCachedVisibleNetworks(null);
+        sVisibleNetworksTime = Long.MAX_VALUE;
+    }
+
+    @VisibleForTesting
+    static void setVisibleNetworksForTesting(VisibleNetworks visibleNetworksForTesting) {
+        sVisibleNetworksForTesting = visibleNetworksForTesting;
+        sUseVisibleNetworksForTesting = true;
+    }
+
+    private static synchronized void setCachedVisibleNetworks(VisibleNetworks visibleNetworks) {
+        sVisibleNetworks = visibleNetworks;
+        sVisibleNetworksTime = sTimeProvider.getCurrentTime();
+    }
+
+    private static synchronized boolean isValidCachedVisibleNetworks() {
+        return sVisibleNetworks != null && sVisibleNetworksTime != Long.MAX_VALUE
+                && !sVisibleNetworks.isEmpty()
+                && sTimeProvider.getCurrentTime() - sVisibleNetworksTime < AGE_THRESHOLD;
+    }
+
+    /**
+     * Wrapper around static time providers that allows us to mock the implementation in
+     * tests.
+     */
+    public static class TimeProvider {
+        /**
+         * Get current time in milliseconds.
+         */
+        public long getCurrentTime() {
+            return System.currentTimeMillis();
+        }
+    }
+}