Introduce Visible Networks Tracker.

Introduce Visible Networks Tracker.
This is used to keep a cache of visible networks and
pre-compute it in a background thread.

It can also compute visible networks synchronously if
the cache is empty or old.

BUG=718475
Review-Url: https://codereview.chromium.org/2883063003
Cr-Commit-Position: refs/heads/master@{#473544}
Committed: https://chromium.googlesource.com/chromium/src/+/e630002e04d5b0e9436a325f224df82fa694bc26

[lbargu, 2017-05-16 12:43:34]

Description was changed from

==========
Introduce Visible Networks Tracker.
This is used to keep a cache of visible networks and
pre-compute it in a background thread.

It can also compute visible networks synchronously if
the cache is empty or old.

BUG=718475
==========

to

==========
Introduce Visible Networks Tracker.
This is used to keep a cache of visible networks and
pre-compute it in a background thread.

It can also compute visible networks synchronously if
the cache is empty or old.

BUG=718475
==========

[lbargu, 2017-05-16 12:43:35]

lbargu@google.com changed reviewers:
+ tedchoc@chromium.org

[lbargu, 2017-05-17 15:45:10]

The CQ bit was checked by lbargu@google.com to run a CQ dry run

[commit-bot: I haz the power, 2017-05-17 15:45:44]

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, 2017-05-17 17:17:07]

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

[commit-bot: I haz the power, 2017-05-17 17:17:08]

Dry run: Try jobs failed on following builders:
  android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)

[lbargu, 2017-05-17 17:23:36]

lbargu@google.com changed reviewers:
+ dougt@chromium.org

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

https://codereview.chromium.org/2883063003/diff/40001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/omnibox/geo/VisibleNetworksTracker.java
(right):

https://codereview.chromium.org/2883063003/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/omnibox/geo/VisibleNetworksTracker.java:25:
@GuardedBy("this")
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.

https://codereview.chromium.org/2883063003/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/omnibox/geo/VisibleNetworksTracker.java:38:
public static synchronized VisibleNetworks getLastKnownVisibleNetworks(Context
context) {
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

https://codereview.chromium.org/2883063003/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/omnibox/geo/VisibleNetworksTracker.java:57:
* Refreshes the visible networks cache if the cache is not valid by starting a
background task
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".

https://codereview.chromium.org/2883063003/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/omnibox/geo/VisibleNetworksTracker.java:65:
new AsyncTask<Void, Void, VisibleNetworks>() {
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.

https://codereview.chromium.org/2883063003/diff/40001/chrome/android/junit/sr...
File
chrome/android/junit/src/org/chromium/chrome/browser/omnibox/geo/VisibleNetworksTrackerTest.java
(right):

https://codereview.chromium.org/2883063003/diff/40001/chrome/android/junit/sr...
chrome/android/junit/src/org/chromium/chrome/browser/omnibox/geo/VisibleNetworksTrackerTest.java:73:
private static final int AGE_THRESHOLD = 5 * 60 * 1000; // 5 min
i would just make the other constant protected

https://codereview.chromium.org/2883063003/diff/40001/chrome/android/junit/sr...
chrome/android/junit/src/org/chromium/chrome/browser/omnibox/geo/VisibleNetworksTrackerTest.java:84:
VisibleNetworksTracker.sTimeProvider = new VisibleNetworksTracker.TimeProvider()
{
instead of introducing the TimeProvider, can you instead use ShadowSystemClock
for your use case?

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

https://codereview.chromium.org/2883063003/diff/40001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/omnibox/geo/VisibleNetworksTracker.java
(right):

https://codereview.chromium.org/2883063003/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/omnibox/geo/VisibleNetworksTracker.java:27:
private static VisibleNetworks sVisibleNetworks;
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?

https://codereview.chromium.org/2883063003/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/omnibox/geo/VisibleNetworksTracker.java:38:
public static synchronized VisibleNetworks getLastKnownVisibleNetworks(Context
context) {
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]

https://codereview.chromium.org/2883063003/diff/40001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/omnibox/geo/VisibleNetworksTracker.java
(right):

https://codereview.chromium.org/2883063003/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/omnibox/geo/VisibleNetworksTracker.java:25:
@GuardedBy("this")
On 2017/05/18 02:13:33, Ted C wrote:
> 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.

Sorry, fixed throughout.

https://codereview.chromium.org/2883063003/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/omnibox/geo/VisibleNetworksTracker.java:27:
private static VisibleNetworks sVisibleNetworks;
On 2017/05/18 02:59:03, dougt wrote:
> 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?

Removed locking. Using UI thread.

https://codereview.chromium.org/2883063003/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/omnibox/geo/VisibleNetworksTracker.java:38:
public static synchronized VisibleNetworks getLastKnownVisibleNetworks(Context
context) {
On 2017/05/18 02:13:33, Ted C wrote:
> 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

Done.

https://codereview.chromium.org/2883063003/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/omnibox/geo/VisibleNetworksTracker.java:38:
public static synchronized VisibleNetworks getLastKnownVisibleNetworks(Context
context) {
On 2017/05/18 02:59:03, dougt wrote:
> 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?
> 

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.

https://codereview.chromium.org/2883063003/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/omnibox/geo/VisibleNetworksTracker.java:57:
* Refreshes the visible networks cache if the cache is not valid by starting a
background task
On 2017/05/18 02:13:33, Ted C wrote:
> 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".

Done.

https://codereview.chromium.org/2883063003/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/omnibox/geo/VisibleNetworksTracker.java:65:
new AsyncTask<Void, Void, VisibleNetworks>() {
On 2017/05/18 02:13:33, Ted C wrote:
> 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.

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

https://codereview.chromium.org/2883063003/diff/40001/chrome/android/junit/sr...
File
chrome/android/junit/src/org/chromium/chrome/browser/omnibox/geo/VisibleNetworksTrackerTest.java
(right):

https://codereview.chromium.org/2883063003/diff/40001/chrome/android/junit/sr...
chrome/android/junit/src/org/chromium/chrome/browser/omnibox/geo/VisibleNetworksTrackerTest.java:73:
private static final int AGE_THRESHOLD = 5 * 60 * 1000; // 5 min
On 2017/05/18 02:13:33, Ted C wrote:
> i would just make the other constant protected

Done.

https://codereview.chromium.org/2883063003/diff/40001/chrome/android/junit/sr...
chrome/android/junit/src/org/chromium/chrome/browser/omnibox/geo/VisibleNetworksTrackerTest.java:84:
VisibleNetworksTracker.sTimeProvider = new VisibleNetworksTracker.TimeProvider()
{
On 2017/05/18 02:13:33, Ted C wrote:
> instead of introducing the TimeProvider, can you instead use ShadowSystemClock
> for your use case?

Done.

[lbargu, 2017-05-18 14:15:36]

The CQ bit was checked by lbargu@google.com to run a CQ dry run

[commit-bot: I haz the power, 2017-05-18 14:15:56]

Dry run: CQ is trying da patch.

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

[Ted C, 2017-05-18 14:56:30]

https://codereview.chromium.org/2883063003/diff/60001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/omnibox/geo/VisibleNetworksTracker.java
(right):

https://codereview.chromium.org/2883063003/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/omnibox/geo/VisibleNetworksTracker.java:57:
ThreadUtils.runOnUiThread(new Runnable() {
When is getLastKnownVisibleNetworks not run on the UI thread? 
GeolocationHeader#getGeoHeader looks to only be called on the UI thread from
prod code?  Is this for tests?  If so, I think we should fix the test instead of
introduce this hop.

https://codereview.chromium.org/2883063003/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/omnibox/geo/VisibleNetworksTracker.java:112:
ThreadUtils.runOnUiThread(new Runnable() {
same thing, if the thread restriction is a test thing, then let's do the thread
wrapping in the test code (and in tests, you might want to do
runOnUiThreadBlocking to make sure the ordering is predictable)

[commit-bot: I haz the power, 2017-05-18 15:05:32]

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

[commit-bot: I haz the power, 2017-05-18 15:05:33]

Dry run: Try jobs failed on following builders:
  android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)

[lbargu, 2017-05-18 15:40:41]

https://codereview.chromium.org/2883063003/diff/60001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/omnibox/geo/VisibleNetworksTracker.java
(right):

https://codereview.chromium.org/2883063003/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/omnibox/geo/VisibleNetworksTracker.java:57:
ThreadUtils.runOnUiThread(new Runnable() {
On 2017/05/18 14:56:30, Ted C wrote:
> When is getLastKnownVisibleNetworks not run on the UI thread? 
> GeolocationHeader#getGeoHeader looks to only be called on the UI thread from
> prod code?  Is this for tests?  If so, I think we should fix the test instead
of
> introduce this hop.

Updated to post on UI Thread instead of run.
I wanted to return visible networks as soon as possible, since this is in the
critical path.

Also I don't know enough about where getGeoHeader is called. But if you can
confirm it will always called from UI thread and think we are fine doing this in
the critical path, then I'm happy to remove it.

https://codereview.chromium.org/2883063003/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/omnibox/geo/VisibleNetworksTracker.java:112:
ThreadUtils.runOnUiThread(new Runnable() {
On 2017/05/18 14:56:30, Ted C wrote:
> same thing, if the thread restriction is a test thing, then let's do the
thread
> wrapping in the test code (and in tests, you might want to do
> runOnUiThreadBlocking to make sure the ordering is predictable)

Done.

[Ted C, 2017-05-18 15:45:40]

https://codereview.chromium.org/2883063003/diff/60001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/omnibox/geo/VisibleNetworksTracker.java
(right):

https://codereview.chromium.org/2883063003/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/omnibox/geo/VisibleNetworksTracker.java:57:
ThreadUtils.runOnUiThread(new Runnable() {
On 2017/05/18 15:40:41, lbargu wrote:
> On 2017/05/18 14:56:30, Ted C wrote:
> > When is getLastKnownVisibleNetworks not run on the UI thread? 
> > GeolocationHeader#getGeoHeader looks to only be called on the UI thread from
> > prod code?  Is this for tests?  If so, I think we should fix the test
instead
> of
> > introduce this hop.
> 
> Updated to post on UI Thread instead of run.
> I wanted to return visible networks as soon as possible, since this is in the
> critical path.
> 
> Also I don't know enough about where getGeoHeader is called. But if you can
> confirm it will always called from UI thread and think we are fine doing this
in
> the critical path, then I'm happy to remove it.

The two callers are LocationBarLayout#loadUrl and
ChromeActionModeCallback#search, which are both definitely on the UI thread. 
Even the existing GeolocationHeaderTest always calls getGeoHeader on the UI
thread.

So, I would just remove this block entirely.  refreshVisibleNetworks kicks off
an AsyncTask so that is fine in terms of not doing a lot of work on the main
thread.

[lbargu, 2017-05-18 16:02:34]

https://codereview.chromium.org/2883063003/diff/60001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/omnibox/geo/VisibleNetworksTracker.java
(right):

https://codereview.chromium.org/2883063003/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/omnibox/geo/VisibleNetworksTracker.java:57:
ThreadUtils.runOnUiThread(new Runnable() {
On 2017/05/18 15:45:40, Ted C wrote:
> On 2017/05/18 15:40:41, lbargu wrote:
> > On 2017/05/18 14:56:30, Ted C wrote:
> > > When is getLastKnownVisibleNetworks not run on the UI thread? 
> > > GeolocationHeader#getGeoHeader looks to only be called on the UI thread
from
> > > prod code?  Is this for tests?  If so, I think we should fix the test
> instead
> > of
> > > introduce this hop.
> > 
> > Updated to post on UI Thread instead of run.
> > I wanted to return visible networks as soon as possible, since this is in
the
> > critical path.
> > 
> > Also I don't know enough about where getGeoHeader is called. But if you can
> > confirm it will always called from UI thread and think we are fine doing
this
> in
> > the critical path, then I'm happy to remove it.
> 
> The two callers are LocationBarLayout#loadUrl and
> ChromeActionModeCallback#search, which are both definitely on the UI thread. 
> Even the existing GeolocationHeaderTest always calls getGeoHeader on the UI
> thread.
> 
> So, I would just remove this block entirely.  refreshVisibleNetworks kicks off
> an AsyncTask so that is fine in terms of not doing a lot of work on the main
> thread.

Done.

[Ted C, 2017-05-18 16:04:44]

lgtm

[lbargu, 2017-05-18 16:04:50]

The CQ bit was checked by lbargu@google.com to run a CQ dry run

[commit-bot: I haz the power, 2017-05-18 16:05:47]

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, 2017-05-18 17:00:01]

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

[commit-bot: I haz the power, 2017-05-18 17:00:02]

Dry run: Try jobs failed on following builders:
  android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)

[lbargu, 2017-05-19 15:11:41]

The CQ bit was checked by lbargu@google.com

[lbargu, 2017-05-19 15:11:41]

The patchset sent to the CQ was uploaded after l-g-t-m from tedchoc@chromium.org
Link to the patchset: https://codereview.chromium.org/2883063003/#ps120001
(title: "Introduce Visible Networks Tracker. This is used to keep a cache of
visible networks and pre-comput…")

[commit-bot: I haz the power, 2017-05-19 15:12:07]

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, 2017-05-19 16:55:08]

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

[commit-bot: I haz the power, 2017-05-19 16:55:09]

Try jobs failed on following builders:
  android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)

[lbargu, 2017-05-22 08:31:16]

The CQ bit was checked by lbargu@google.com

[lbargu, 2017-05-22 08:31:16]

The patchset sent to the CQ was uploaded after l-g-t-m from tedchoc@chromium.org
Link to the patchset: https://codereview.chromium.org/2883063003/#ps140001
(title: "Introduce Visible Networks Tracker. This is used to keep a cache of
visible networks and pre-comput…")

[commit-bot: I haz the power, 2017-05-22 08:31:32]

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, 2017-05-22 08:56:08]

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

[commit-bot: I haz the power, 2017-05-22 08:56:09]

Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)

[lbargu, 2017-05-22 09:06:01]

The CQ bit was checked by lbargu@google.com

[lbargu, 2017-05-22 09:06:01]

The patchset sent to the CQ was uploaded after l-g-t-m from tedchoc@chromium.org
Link to the patchset: https://codereview.chromium.org/2883063003/#ps160001
(title: "Introduce Visible Networks Tracker. This is used to keep a cache of
visible networks and pre-comput…")

[commit-bot: I haz the power, 2017-05-22 09:06:25]

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, 2017-05-22 10:39:49]

CQ is committing da patch.

Bot data: {"patchset_id": 160001, "attempt_start_ts": 1495443961268580,
"parent_rev": "f8c5824267cdbcb81a45de00e4a8a741cc5f9247", "commit_rev":
"e630002e04d5b0e9436a325f224df82fa694bc26"}

[commit-bot: I haz the power, 2017-05-22 10:39:57]

Description was changed from

==========
Introduce Visible Networks Tracker.
This is used to keep a cache of visible networks and
pre-compute it in a background thread.

It can also compute visible networks synchronously if
the cache is empty or old.

BUG=718475
==========

to

==========
Introduce Visible Networks Tracker.
This is used to keep a cache of visible networks and
pre-compute it in a background thread.

It can also compute visible networks synchronously if
the cache is empty or old.

BUG=718475

Review-Url: https://codereview.chromium.org/2883063003
Cr-Commit-Position: refs/heads/master@{#473544}
Committed:
https://chromium.googlesource.com/chromium/src/+/e630002e04d5b0e9436a325f224d...
==========

[commit-bot: I haz the power, 2017-05-22 10:39:59]

Committed patchset #9 (id:160001) as
https://chromium.googlesource.com/chromium/src/+/e630002e04d5b0e9436a325f224d...