Prevent NetworkLocationProvider from sending wifi data if started low accuracy

Prevent NetworkLocationProvider from sending wifi data if started low accuracy

If NetworkLocationProvider is started with high accuracy disabled, then it 
shouldn't send wifi access points data and default IP-based geolocation only.

BUG=726949
TEST=device_unittests --gtest_filter=GeolocationNetworkProviderTest.*

[afakhry, 2017-05-26 19:10:32]

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

[commit-bot: I haz the power, 2017-05-26 19:11:31]

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-26 19:19:31]

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

[commit-bot: I haz the power, 2017-05-26 19:19:31]

Dry run: Try jobs failed on following builders:
  android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)
  android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)
  android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)
  android_cronet on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_cron...)
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
  cast_shell_android on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[afakhry, 2017-05-26 19:52:34]

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

[commit-bot: I haz the power, 2017-05-26 19:53:14]

Dry run: CQ is trying da patch.

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

[afakhry, 2017-05-26 19:53:57]

afakhry@chromium.org changed reviewers:
+ mcasas@chromium.org

[afakhry, 2017-05-26 19:53:57]

mcasas@ That's the basic idea. But I'm not sure if I missed things here and
there. Please take a look and let me know if this is as viable solution.

[commit-bot: I haz the power, 2017-05-26 21:03:34]

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

[commit-bot: I haz the power, 2017-05-26 21:03:35]

Dry run: This issue passed the CQ dry run.

[mcasas, 2017-05-26 21:35:19]

mcasas@chromium.org changed reviewers:
+ mvanouwerkerk@chromium.org

[mcasas, 2017-05-26 21:35:20]

This looks good to me. Would like to hear mvanouwerkerk@s 
opinion:

Michael, is there any reason not to wire low-accuracy in CrOs,
which is now simply ignored, to don't-use-WiFi-networks?

[afakhry, 2017-05-27 00:08:14]

Description was changed from

==========
Prevent NetworkLocationProvider from sending wifi data if started low accuracy

BUG=
==========

to

==========
Prevent NetworkLocationProvider from sending wifi data if started low accuracy

If NetworkLocationProvider is started with high accuracy disabled, then it 
shouldn't send wifi access points data and default IP-based geolocation only.

BUG=726949
TEST=device_unittests --gtest_filter=GeolocationNetworkProviderTest.*
==========

[afakhry, 2017-05-27 00:08:59]

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

[commit-bot: I haz the power, 2017-05-27 00:09:13]

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-27 00:18:44]

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

[commit-bot: I haz the power, 2017-05-27 00:18:45]

Dry run: Try jobs failed on following builders:
  chromeos_amd64-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...)
  chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...)

[afakhry, 2017-05-27 01:19:27]

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

[commit-bot: I haz the power, 2017-05-27 01:19:45]

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-27 02:20:35]

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

[commit-bot: I haz the power, 2017-05-27 02:20:36]

Dry run: This issue passed the CQ dry run.

[blundell, 2017-05-29 07:22:48]

blundell@chromium.org changed reviewers:
+ blundell@chromium.org

[blundell, 2017-05-29 07:22:49]

I think that there's already a canonical way to do this:

https://cs.chromium.org/chromium/src/device/geolocation/geolocation_delegate....

Why would we need a second way?

[blundell, 2017-05-29 13:13:07]

On 2017/05/29 07:22:49, blundell wrote:
> I think that there's already a canonical way to do this:
> 
>
https://cs.chromium.org/chromium/src/device/geolocation/geolocation_delegate....
> 
> Why would we need a second way?

OK, I understand now after looking this over more carefully. I don't think it
makes sense to reuse |enable_high_accuracy|, which is a property set by the
client of geolocation. You want to set this policy as the *embedder* of
geolocation. I think that you should add a method to geolocation_delegate.h,
similar to UseNetworkLocation().

Implementation-wise, I think it would be cleaner to define an empty
WifiDataProvider and set WifiDataProviderManager's factory constructor to use
the empty WifiDataProvider (i.e., so when WifiDataProviderManager::Register() is
called, it creates an instance of the empty wifi data provider). That way you
won't need to make any invasive changes to NetworkLocationProvider and things
will basically just work within the existing architecture.

[afakhry, 2017-05-30 18:01:05]

On 2017/05/29 13:13:07, blundell wrote:
> On 2017/05/29 07:22:49, blundell wrote:
> > I think that there's already a canonical way to do this:
> > 
> >
>
https://cs.chromium.org/chromium/src/device/geolocation/geolocation_delegate....
> > 
> > Why would we need a second way?
> 
> OK, I understand now after looking this over more carefully. I don't think it
> makes sense to reuse |enable_high_accuracy|, which is a property set by the
> client of geolocation. You want to set this policy as the *embedder* of
> geolocation. I think that you should add a method to geolocation_delegate.h,
> similar to UseNetworkLocation().
> 

A few points here:
1- Why is |enable_high_accuracy| is ignored entirely here?
2- The problem, I think, with setting this policy in the embedder is that I'm
not supposed to
   modify the embedder. Changing it would fail this DCHECK()
https://cs.chromium.org/chromium/src/device/geolocation/geolocation_provider_....
I think of it as a client parameter, just like |enable_high_accuracy|,
   similar to how SimpleGeolocationProvider::RequestGeolocation() is implemented
(see |send_wifi_access_points| and |send_cell_towers|)
https://cs.chromium.org/chromium/src/chromeos/geolocation/simple_geolocation_....
   Unless it's OK to set it in the delegate, which will affect all Requests.

> Implementation-wise, I think it would be cleaner to define an empty
> WifiDataProvider and set WifiDataProviderManager's factory constructor to use
> the empty WifiDataProvider (i.e., so when WifiDataProviderManager::Register()
is
> called, it creates an instance of the empty wifi data provider). That way you
> won't need to make any invasive changes to NetworkLocationProvider and things
> will basically just work within the existing architecture.

Let me give that a try. Thanks for suggesting.

[afakhry, 2017-05-30 18:25:39]

On 2017/05/30 18:01:05, afakhry wrote:
> On 2017/05/29 13:13:07, blundell wrote:
> > On 2017/05/29 07:22:49, blundell wrote:
> > > I think that there's already a canonical way to do this:
> > > 
> > >
> >
>
https://cs.chromium.org/chromium/src/device/geolocation/geolocation_delegate....
> > > 
> > > Why would we need a second way?
> > 
> > OK, I understand now after looking this over more carefully. I don't think
it
> > makes sense to reuse |enable_high_accuracy|, which is a property set by the
> > client of geolocation. You want to set this policy as the *embedder* of
> > geolocation. I think that you should add a method to geolocation_delegate.h,
> > similar to UseNetworkLocation().
> > 
> 
> A few points here:
> 1- Why is |enable_high_accuracy| is ignored entirely here?
> 2- The problem, I think, with setting this policy in the embedder is that I'm
> not supposed to
>    modify the embedder. Changing it would fail this DCHECK()
>
https://cs.chromium.org/chromium/src/device/geolocation/geolocation_provider_....
> I think of it as a client parameter, just like |enable_high_accuracy|,
>    similar to how SimpleGeolocationProvider::RequestGeolocation() is
implemented
> (see |send_wifi_access_points| and |send_cell_towers|)
>
https://cs.chromium.org/chromium/src/chromeos/geolocation/simple_geolocation_....
>    Unless it's OK to set it in the delegate, which will affect all Requests.
> 
> > Implementation-wise, I think it would be cleaner to define an empty
> > WifiDataProvider and set WifiDataProviderManager's factory constructor to
use
> > the empty WifiDataProvider (i.e., so when
WifiDataProviderManager::Register()
> is
> > called, it creates an instance of the empty wifi data provider). That way
you
> > won't need to make any invasive changes to NetworkLocationProvider and
things
> > will basically just work within the existing architecture.
> 
> Let me give that a try. Thanks for suggesting.

What if the WifiDataProviderManager instance has already been created? Then we
won't have a chance to create it with an empty WifiDataProvider impl_.

[blundell, 2017-05-31 14:48:59]

FWIW, I think it might be easier to start from a design doc here, as the issues
seem pretty subtle to me.

On 2017/05/30 18:25:39, afakhry wrote:
> On 2017/05/30 18:01:05, afakhry wrote:
> > On 2017/05/29 13:13:07, blundell wrote:
> > > On 2017/05/29 07:22:49, blundell wrote:
> > > > I think that there's already a canonical way to do this:
> > > > 
> > > >
> > >
> >
>
https://cs.chromium.org/chromium/src/device/geolocation/geolocation_delegate....
> > > > 
> > > > Why would we need a second way?
> > > 
> > > OK, I understand now after looking this over more carefully. I don't think
> it
> > > makes sense to reuse |enable_high_accuracy|, which is a property set by
the
> > > client of geolocation. You want to set this policy as the *embedder* of
> > > geolocation. I think that you should add a method to
geolocation_delegate.h,
> > > similar to UseNetworkLocation().
> > > 
> > 
> > A few points here:
> > 1- Why is |enable_high_accuracy| is ignored entirely here?

The problem is that you're reusing something that wasn't necessarily intended to
have the semantics you're giving it. Are there existing platforms/use cases
other than this one where WiFi data is sent? If so, you're silently changing
behavior in those cases so that it's sent only when the client requests high
accuracy. It's not clear offhand whether that's an OK change to make.

> > 2- The problem, I think, with setting this policy in the embedder is that
I'm
> > not supposed to
> >    modify the embedder. Changing it would fail this DCHECK()
> >
>
https://cs.chromium.org/chromium/src/device/geolocation/geolocation_provider_....
> > I think of it as a client parameter, just like |enable_high_accuracy|,
> >    similar to how SimpleGeolocationProvider::RequestGeolocation() is
> implemented
> > (see |send_wifi_access_points| and |send_cell_towers|)

This makes sense, thanks.

> >
>
https://cs.chromium.org/chromium/src/chromeos/geolocation/simple_geolocation_....
> >    Unless it's OK to set it in the delegate, which will affect all Requests.
> > 
> > > Implementation-wise, I think it would be cleaner to define an empty
> > > WifiDataProvider and set WifiDataProviderManager's factory constructor to
> use
> > > the empty WifiDataProvider (i.e., so when
> WifiDataProviderManager::Register()
> > is
> > > called, it creates an instance of the empty wifi data provider). That way
> you
> > > won't need to make any invasive changes to NetworkLocationProvider and
> things
> > > will basically just work within the existing architecture.
> > 
> > Let me give that a try. Thanks for suggesting.
> 
> What if the WifiDataProviderManager instance has already been created? Then we
> won't have a chance to create it with an empty WifiDataProvider impl_.

If it was already created that would mean that a client had already requested
high accuracy, which would mean that they'd already obtained user consent,
right?

[blundell, 2017-05-31 14:49:35]

https://codereview.chromium.org/2901413006/diff/40001/device/geolocation/netw...
File device/geolocation/network_location_provider.cc (right):

https://codereview.chromium.org/2901413006/diff/40001/device/geolocation/netw...
device/geolocation/network_location_provider.cc:195: if (IsStarted())
Here you're never changing the accuracy once the NetworkLocationProvider is
first started. So if a second client came along and wanted Wifi data they
wouldn't get it.

[Michael van Ouwerkerk, 2017-06-02 09:16:00]

On 2017/05/31 14:49:35, blundell wrote:
>
https://codereview.chromium.org/2901413006/diff/40001/device/geolocation/netw...
> File device/geolocation/network_location_provider.cc (right):
> 
>
https://codereview.chromium.org/2901413006/diff/40001/device/geolocation/netw...
> device/geolocation/network_location_provider.cc:195: if (IsStarted())
> Here you're never changing the accuracy once the NetworkLocationProvider is
> first started. So if a second client came along and wanted Wifi data they
> wouldn't get it.

I have some concerns about this CL, I added some comments to the bug:
https://bugs.chromium.org/p/chromium/issues/detail?id=726949#c2

[Michael van Ouwerkerk, 2017-07-31 12:11:58]

mvanouwerkerk@chromium.org changed reviewers:
- mvanouwerkerk@chromium.org