Make geolocation API and X-Geo header access consistent

Make geolocation API and X-Geo header access consistent

This change allows geolocation access for the default search engine if
it would have access to geolocation information via the X-Geo header.

To effect this, a new setting is enabled controlling whether the default
search engine has geolocation access. This is only used if the search
engine supports the X-Geo header, i.e. it is Google.

The setting is consulted for the DSE both when deciding if the X-Geo
header should be added, and when deciding if geolocation API access
should be granted.

To allow the UI to make sense and provide the greatest clarity, the
setting should always be consistent with the underlying content setting
for the DSE's origin. In this end it is initialized accordingly and
various checks are in place to keep it consistent.

BUG=674389
Review-Url: https://codereview.chromium.org/2612993002
Cr-Commit-Position: refs/heads/master@{#443161}
Committed: https://chromium.googlesource.com/chromium/src/+/fd39adcc3b11e8ebe81754de74fc3cd191e44c29

[benwells, 2017-01-05 05:47:42]

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

[commit-bot: I haz the power, 2017-01-05 05:47:54]

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-01-05 07:28:09]

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

[commit-bot: I haz the power, 2017-01-05 07:28:10]

Dry run: 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_...)

[benwells, 2017-01-06 05:38:41]

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

[commit-bot: I haz the power, 2017-01-06 05:38:56]

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-01-06 08:21:08]

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

[commit-bot: I haz the power, 2017-01-06 08:21:08]

Dry run: 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_...)

[benwells, 2017-01-08 21:30:59]

benwells@chromium.org changed reviewers:
+ raymes@chromium.org

[benwells, 2017-01-08 21:30:59]

Welcome back Raymes ;)

Sorry to dump this on you first thing back, I thought you'd be the best reviewer
as you know the background.

[benwells, 2017-01-08 21:33:43]

BTW this change affects the logic, and a little of the UI. The UI stuff still be
done completed:
- page info
- search engine settings
- show the disclosure when geolocation api used for the first time

The site settings page should be good.

Hopefully this patch gets the logic all in place and the rest will be UI.

And tests - I haven't added an automated test but will probably add a unit test
soon.

[raymes, 2017-01-09 05:47:27]

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

https://codereview.chromium.org/2612993002/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/omnibox/geo/GeolocationHeader.java:363:
return disabled;
Hmm, the intended logic here is a little unclear. Is it intended to be "if
should use DSE geolocation AND DSE location is true" then it's enabled,
otherwise disabled? I think that unwinds to what you have here, but it's a bit
hard to parse. It might be a little clearer to have 

return !(nativeShouldUseDSEGeolocationSetting() &&
nativeGetDSEGeolocationSetting()); 

or

boolean enabled = nativeShouldUseDSEGeolocationSetting() &&
nativeGetDSEGeolocationSetting();
return !enabled

https://codereview.chromium.org/2612993002/diff/60001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/preferences/website/SingleWebsitePreferences.java
(right):

https://codereview.chromium.org/2612993002/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/preferences/website/SingleWebsitePreferences.java:469:
if (shouldUseDSEGeolocationSetting()) {
I haven't reviewed the code here closely since I'm not too familiar with it.

https://codereview.chromium.org/2612993002/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/preferences/website/SingleWebsitePreferences.java:510:
mSite.getAddress().getOrigin(), false);
It looks like you're always passing false here for incognito? Is that intended?

https://codereview.chromium.org/2612993002/diff/60001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/preferences/website/Website.java
(right):

https://codereview.chromium.org/2612993002/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/preferences/website/Website.java:211:
WebsitePreferenceBridge.nativeSetDSEGeolocationSetting(value !=
ContentSetting.BLOCK);
hmm could the value passed in here ever be ASK? I guess not but it might be
worth having value == ContentSetting.ALLOW instead to be safe?

https://codereview.chromium.org/2612993002/diff/60001/chrome/browser/android/...
File chrome/browser/android/preferences/website_preference_bridge.cc (right):

https://codereview.chromium.org/2612993002/diff/60001/chrome/browser/android/...
chrome/browser/android/preferences/website_preference_bridge.cc:774:
GetActiveUserProfile(false));
nit: document the boolean /* is_incognito */

https://codereview.chromium.org/2612993002/diff/60001/chrome/browser/android/...
chrome/browser/android/preferences/website_preference_bridge.cc:783:
GetActiveUserProfile(false));
nit: same here.

https://codereview.chromium.org/2612993002/diff/60001/chrome/browser/android/...
File chrome/browser/android/search_geolocation/search_geolocation_service.cc
(right):

https://codereview.chromium.org/2612993002/diff/60001/chrome/browser/android/...
chrome/browser/android/search_geolocation/search_geolocation_service.cc:59:
EnsureDSEGeolocationSettingIsValid();
It might be worth adding a comment about why this is needed at startup?

https://codereview.chromium.org/2612993002/diff/60001/chrome/browser/android/...
chrome/browser/android/search_geolocation/search_geolocation_service.cc:68:
return false;
nit: should this just be the first check?

https://codereview.chromium.org/2612993002/diff/60001/chrome/browser/android/...
chrome/browser/android/search_geolocation/search_geolocation_service.cc:72: 
I think we could just add an "IsUserSettable()" check here too which would guard
against enterprise policy, though it's not strictly needed.

https://codereview.chromium.org/2612993002/diff/60001/chrome/browser/android/...
chrome/browser/android/search_geolocation/search_geolocation_service.cc:76: bool
SearchGeolocationService::GetDSEGeolocationSetting() {
I vaguely remember talking about ensuring validity at this point, to guard
against the content setting changing for weird reasons, like enterprise policy
being enforced, etc. I wonder if that's still a good idea?

https://codereview.chromium.org/2612993002/diff/60001/chrome/browser/android/...
chrome/browser/android/search_geolocation/search_geolocation_service.cc:84: 
As mentioned, I think I'd prefer having 
if (!IsUserSettable()) return; here and not the call to
EnsureDSEGeolocatioNSettingIsValid below. I think it makes it clearer what's
going on.

https://codereview.chromium.org/2612993002/diff/60001/chrome/browser/android/...
chrome/browser/android/search_geolocation/search_geolocation_service.cc:159: //
Initialize the pref if it hasn't been initialize yet.
nit: initialized

https://codereview.chromium.org/2612993002/diff/60001/chrome/browser/android/...
chrome/browser/android/search_geolocation/search_geolocation_service.cc:222:
return pref;
I think the struct will be uninitialized? We could give it a constructor to make
sure it's initialized as expected.

https://codereview.chromium.org/2612993002/diff/60001/chrome/browser/android/...
chrome/browser/android/search_geolocation/search_geolocation_service.cc:238:
host_content_settings_map_->GetWebsiteSetting(
Since you're not using "info" here, you can just call GetContentSetting and
avoid the conversion below.

https://codereview.chromium.org/2612993002/diff/60001/chrome/browser/android/...
File chrome/browser/android/search_geolocation/search_geolocation_service.h
(right):

https://codereview.chromium.org/2612993002/diff/60001/chrome/browser/android/...
chrome/browser/android/search_geolocation/search_geolocation_service.h:13:
#include "url/gurl.h"
nit: I don't think this is needed anymore

https://codereview.chromium.org/2612993002/diff/60001/chrome/browser/android/...
chrome/browser/android/search_geolocation/search_geolocation_service.h:31: //
whether the setting should be used and it's current value.
nit: not sure if it's important to explain what CCTLD and DSE terms mean here.
Other readers might be confused.

https://codereview.chromium.org/2612993002/diff/60001/chrome/browser/android/...
chrome/browser/android/search_geolocation/search_geolocation_service.h:41: //
requests for the given origin.
nit: might want to clarify that this is the top level origin

https://codereview.chromium.org/2612993002/diff/60001/chrome/browser/android/...
chrome/browser/android/search_geolocation/search_geolocation_service.h:55: class
Factory : public BrowserContextKeyedServiceFactory {
nit: I think inner classes tend to go before the member functions (in order).
The same in the .cc file

https://codereview.chromium.org/2612993002/diff/60001/chrome/browser/android/...
chrome/browser/android/search_geolocation/search_geolocation_service.h:107: //
Another way is if the setting was changed while enterprise policy is in
nit: fill the previous line to 80chars

https://codereview.chromium.org/2612993002/diff/60001/chrome/browser/android/...
chrome/browser/android/search_geolocation/search_geolocation_service.h:111: //
Returns true if the current DSE is Google.
nit: might want to mention that Google is currently the only search engine that
supports DSE geolocation

https://codereview.chromium.org/2612993002/diff/60001/chrome/browser/android/...
chrome/browser/android/search_geolocation/search_geolocation_service.h:121:
ContentSetting GetCurrentContentSetting();
nit: might want to clairfy that these apply to the current DSE CCTLD origin

https://codereview.chromium.org/2612993002/diff/60001/chrome/browser/geolocat...
File chrome/browser/geolocation/geolocation_permission_context.cc (right):

https://codereview.chromium.org/2612993002/diff/60001/chrome/browser/geolocat...
chrome/browser/geolocation/geolocation_permission_context.cc:70: :
CONTENT_SETTING_BLOCK;
As discussed I think we want to move this to GetPermissionStatus. I think we
also only want to apply the setting if the user content setting is ASK.

https://codereview.chromium.org/2612993002/diff/60001/chrome/browser/permissi...
File chrome/browser/permissions/permission_manager.h (right):

https://codereview.chromium.org/2612993002/diff/60001/chrome/browser/permissi...
chrome/browser/permissions/permission_manager.h:39: static const char
kPermissionsKillSwitchBlockedValue[];
Hmm, are these meant to be in this CL?

[benwells, 2017-01-09 06:58:21]

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

[commit-bot: I haz the power, 2017-01-09 06:58:26]

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-01-09 07:19:14]

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

[commit-bot: I haz the power, 2017-01-09 07:19:14]

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

[benwells, 2017-01-09 07:26:00]

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

[commit-bot: I haz the power, 2017-01-09 07:26:14]

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-01-09 10:12:43]

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

[commit-bot: I haz the power, 2017-01-09 10:12:43]

Dry run: 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_...)

[benwells, 2017-01-09 21:02:22]

Now with some tests

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

https://codereview.chromium.org/2612993002/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/omnibox/geo/GeolocationHeader.java:363:
return disabled;
On 2017/01/09 05:47:26, raymes (a bit slow) wrote:
> Hmm, the intended logic here is a little unclear. Is it intended to be "if
> should use DSE geolocation AND DSE location is true" then it's enabled,
> otherwise disabled? I think that unwinds to what you have here, but it's a bit
> hard to parse. It might be a little clearer to have 
> 
> return !(nativeShouldUseDSEGeolocationSetting() &&
> nativeGetDSEGeolocationSetting()); 
> 
> or
> 
> boolean enabled = nativeShouldUseDSEGeolocationSetting() &&
> nativeGetDSEGeolocationSetting();
> return !enabled

Done.

https://codereview.chromium.org/2612993002/diff/60001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/preferences/website/SingleWebsitePreferences.java
(right):

https://codereview.chromium.org/2612993002/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/preferences/website/SingleWebsitePreferences.java:469:
if (shouldUseDSEGeolocationSetting()) {
On 2017/01/09 05:47:26, raymes (a bit slow) wrote:
> I haven't reviewed the code here closely since I'm not too familiar with it. 

Acknowledged.

https://codereview.chromium.org/2612993002/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/preferences/website/SingleWebsitePreferences.java:510:
mSite.getAddress().getOrigin(), false);
On 2017/01/09 05:47:26, raymes (a bit slow) wrote:
> It looks like you're always passing false here for incognito? Is that
intended?

Yes, the settings are always shown for the non-incognito profile. 'false' for is
incognito is also passed into the isGeoHeaderEnabledForUrl call on line 501
above.

https://codereview.chromium.org/2612993002/diff/60001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/preferences/website/Website.java
(right):

https://codereview.chromium.org/2612993002/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/preferences/website/Website.java:211:
WebsitePreferenceBridge.nativeSetDSEGeolocationSetting(value !=
ContentSetting.BLOCK);
On 2017/01/09 05:47:26, raymes (a bit slow) wrote:
> hmm could the value passed in here ever be ASK? I guess not but it might be
> worth having value == ContentSetting.ALLOW instead to be safe?

Yes, it can be ASK - when the user presses the 'reset site' button. My thinking
was that in that case the site should be set back to it's default value, and the
default for the DSE CCTLD is on, hence the != BLOCK.

We should check with the privacy team that the reset button should do this -
I'll email them about it. Maybe it should reset to enable but show the banner
again...

https://codereview.chromium.org/2612993002/diff/60001/chrome/browser/android/...
File chrome/browser/android/preferences/website_preference_bridge.cc (right):

https://codereview.chromium.org/2612993002/diff/60001/chrome/browser/android/...
chrome/browser/android/preferences/website_preference_bridge.cc:774:
GetActiveUserProfile(false));
On 2017/01/09 05:47:26, raymes (a bit slow) wrote:
> nit: document the boolean /* is_incognito */

Done.

https://codereview.chromium.org/2612993002/diff/60001/chrome/browser/android/...
chrome/browser/android/preferences/website_preference_bridge.cc:783:
GetActiveUserProfile(false));
On 2017/01/09 05:47:26, raymes (a bit slow) wrote:
> nit: same here.

Done.

https://codereview.chromium.org/2612993002/diff/60001/chrome/browser/android/...
File chrome/browser/android/search_geolocation/search_geolocation_service.cc
(right):

https://codereview.chromium.org/2612993002/diff/60001/chrome/browser/android/...
chrome/browser/android/search_geolocation/search_geolocation_service.cc:59:
EnsureDSEGeolocationSettingIsValid();
On 2017/01/09 05:47:26, raymes (a bit slow) wrote:
> It might be worth adding a comment about why this is needed at startup?

Done.

https://codereview.chromium.org/2612993002/diff/60001/chrome/browser/android/...
chrome/browser/android/search_geolocation/search_geolocation_service.cc:68:
return false;
On 2017/01/09 05:47:26, raymes (a bit slow) wrote:
> nit: should this just be the first check?

Done.

https://codereview.chromium.org/2612993002/diff/60001/chrome/browser/android/...
chrome/browser/android/search_geolocation/search_geolocation_service.cc:72: 
On 2017/01/09 05:47:26, raymes (a bit slow) wrote:
> I think we could just add an "IsUserSettable()" check here too which would
guard
> against enterprise policy, though it's not strictly needed.

The enterprise policy could force geolocation on, so I think a check here could
introduce problems.

https://codereview.chromium.org/2612993002/diff/60001/chrome/browser/android/...
chrome/browser/android/search_geolocation/search_geolocation_service.cc:76: bool
SearchGeolocationService::GetDSEGeolocationSetting() {
On 2017/01/09 05:47:26, raymes (a bit slow) wrote:
> I vaguely remember talking about ensuring validity at this point, to guard
> against the content setting changing for weird reasons, like enterprise policy
> being enforced, etc. I wonder if that's still a good idea? 

I'm not sure if it's needed or not. I've added in the call with a TODO to check.

https://codereview.chromium.org/2612993002/diff/60001/chrome/browser/android/...
chrome/browser/android/search_geolocation/search_geolocation_service.cc:84: 
On 2017/01/09 05:47:26, raymes (a bit slow) wrote:
> As mentioned, I think I'd prefer having 
> if (!IsUserSettable()) return; here and not the call to
> EnsureDSEGeolocatioNSettingIsValid below. I think it makes it clearer what's
> going on.

Done.

https://codereview.chromium.org/2612993002/diff/60001/chrome/browser/android/...
chrome/browser/android/search_geolocation/search_geolocation_service.cc:159: //
Initialize the pref if it hasn't been initialize yet.
On 2017/01/09 05:47:26, raymes (a bit slow) wrote:
> nit: initialized

Done.

https://codereview.chromium.org/2612993002/diff/60001/chrome/browser/android/...
chrome/browser/android/search_geolocation/search_geolocation_service.cc:222:
return pref;
On 2017/01/09 05:47:26, raymes (a bit slow) wrote:
> I think the struct will be uninitialized? We could give it a constructor to
make
> sure it's initialized as expected.

Done (via default values).

https://codereview.chromium.org/2612993002/diff/60001/chrome/browser/android/...
chrome/browser/android/search_geolocation/search_geolocation_service.cc:238:
host_content_settings_map_->GetWebsiteSetting(
On 2017/01/09 05:47:26, raymes (a bit slow) wrote:
> Since you're not using "info" here, you can just call GetContentSetting and
> avoid the conversion below.

Done.

https://codereview.chromium.org/2612993002/diff/60001/chrome/browser/android/...
File chrome/browser/android/search_geolocation/search_geolocation_service.h
(right):

https://codereview.chromium.org/2612993002/diff/60001/chrome/browser/android/...
chrome/browser/android/search_geolocation/search_geolocation_service.h:13:
#include "url/gurl.h"
On 2017/01/09 05:47:26, raymes (a bit slow) wrote:
> nit: I don't think this is needed anymore

Done.

https://codereview.chromium.org/2612993002/diff/60001/chrome/browser/android/...
chrome/browser/android/search_geolocation/search_geolocation_service.h:31: //
whether the setting should be used and it's current value.
On 2017/01/09 05:47:26, raymes (a bit slow) wrote:
> nit: not sure if it's important to explain what CCTLD and DSE terms mean here.
> Other readers might be confused.

Done.

https://codereview.chromium.org/2612993002/diff/60001/chrome/browser/android/...
chrome/browser/android/search_geolocation/search_geolocation_service.h:41: //
requests for the given origin.
On 2017/01/09 05:47:26, raymes (a bit slow) wrote:
> nit: might want to clarify that this is the top level origin

Done.

https://codereview.chromium.org/2612993002/diff/60001/chrome/browser/android/...
chrome/browser/android/search_geolocation/search_geolocation_service.h:55: class
Factory : public BrowserContextKeyedServiceFactory {
On 2017/01/09 05:47:26, raymes (a bit slow) wrote:
> nit: I think inner classes tend to go before the member functions (in order).
> The same in the .cc file

Done.

https://codereview.chromium.org/2612993002/diff/60001/chrome/browser/android/...
chrome/browser/android/search_geolocation/search_geolocation_service.h:107: //
Another way is if the setting was changed while enterprise policy is in
On 2017/01/09 05:47:27, raymes (a bit slow) wrote:
> nit: fill the previous line to 80chars

Done. I also updated the text a bit.

https://codereview.chromium.org/2612993002/diff/60001/chrome/browser/android/...
chrome/browser/android/search_geolocation/search_geolocation_service.h:111: //
Returns true if the current DSE is Google.
On 2017/01/09 05:47:26, raymes (a bit slow) wrote:
> nit: might want to mention that Google is currently the only search engine
that
> supports DSE geolocation

Done.

https://codereview.chromium.org/2612993002/diff/60001/chrome/browser/android/...
chrome/browser/android/search_geolocation/search_geolocation_service.h:121:
ContentSetting GetCurrentContentSetting();
On 2017/01/09 05:47:27, raymes (a bit slow) wrote:
> nit: might want to clairfy that these apply to the current DSE CCTLD origin

Done.

https://codereview.chromium.org/2612993002/diff/60001/chrome/browser/geolocat...
File chrome/browser/geolocation/geolocation_permission_context.cc (right):

https://codereview.chromium.org/2612993002/diff/60001/chrome/browser/geolocat...
chrome/browser/geolocation/geolocation_permission_context.cc:70: :
CONTENT_SETTING_BLOCK;
On 2017/01/09 05:47:27, raymes (a bit slow) wrote:
> As discussed I think we want to move this to GetPermissionStatus. I think we
> also only want to apply the setting if the user content setting is ASK.

Done. That's much much nicer!

https://codereview.chromium.org/2612993002/diff/60001/chrome/browser/permissi...
File chrome/browser/permissions/permission_manager.h (right):

https://codereview.chromium.org/2612993002/diff/60001/chrome/browser/permissi...
chrome/browser/permissions/permission_manager.h:39: static const char
kPermissionsKillSwitchBlockedValue[];
On 2017/01/09 05:47:27, raymes (a bit slow) wrote:
> Hmm, are these meant to be in this CL?

Ummm ... no ....

I have no idea how that got here. Removed!

[raymes, 2017-01-10 03:19:33]

lgtm

https://codereview.chromium.org/2612993002/diff/60001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/preferences/website/SingleWebsitePreferences.java
(right):

https://codereview.chromium.org/2612993002/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/preferences/website/SingleWebsitePreferences.java:510:
mSite.getAddress().getOrigin(), false);
On 2017/01/09 21:02:21, benwells wrote:
> On 2017/01/09 05:47:26, raymes (a bit slow) wrote:
> > It looks like you're always passing false here for incognito? Is that
> intended?
> 
> Yes, the settings are always shown for the non-incognito profile. 'false' for
is
> incognito is also passed into the isGeoHeaderEnabledForUrl call on line 501
> above.

I see so this function is only called for non-incognito then?

https://codereview.chromium.org/2612993002/diff/60001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/preferences/website/Website.java
(right):

https://codereview.chromium.org/2612993002/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/preferences/website/Website.java:211:
WebsitePreferenceBridge.nativeSetDSEGeolocationSetting(value !=
ContentSetting.BLOCK);
On 2017/01/09 21:02:21, benwells wrote:
> On 2017/01/09 05:47:26, raymes (a bit slow) wrote:
> > hmm could the value passed in here ever be ASK? I guess not but it might be
> > worth having value == ContentSetting.ALLOW instead to be safe?
> 
> Yes, it can be ASK - when the user presses the 'reset site' button. My
thinking
> was that in that case the site should be set back to it's default value, and
the
> default for the DSE CCTLD is on, hence the != BLOCK.
> 
> We should check with the privacy team that the reset button should do this -
> I'll email them about it. Maybe it should reset to enable but show the banner
> again...

Sounds good.

https://codereview.chromium.org/2612993002/diff/60001/chrome/browser/android/...
File chrome/browser/android/search_geolocation/search_geolocation_service.cc
(right):

https://codereview.chromium.org/2612993002/diff/60001/chrome/browser/android/...
chrome/browser/android/search_geolocation/search_geolocation_service.cc:72: 
On 2017/01/09 21:02:22, benwells wrote:
> On 2017/01/09 05:47:26, raymes (a bit slow) wrote:
> > I think we could just add an "IsUserSettable()" check here too which would
> guard
> > against enterprise policy, though it's not strictly needed.
> 
> The enterprise policy could force geolocation on, so I think a check here
could
> introduce problems.

Hmm, but wouldn't we fall back to using the enterprise policy in that case?
Either way it's not a problem as it shouldn't affect most cases..

https://codereview.chromium.org/2612993002/diff/120001/chrome/android/javates...
File
chrome/android/javatests/src/org/chromium/chrome/browser/omnibox/geo/GeolocationHeaderTest.java
(right):

https://codereview.chromium.org/2612993002/diff/120001/chrome/android/javates...
chrome/android/javatests/src/org/chromium/chrome/browser/omnibox/geo/GeolocationHeaderTest.java:31:
private static final String ENABLE_CONSISTENT_GEOLOCATION_GEATURE =
nit: GESTURE?

https://codereview.chromium.org/2612993002/diff/120001/chrome/android/javates...
chrome/android/javatests/src/org/chromium/chrome/browser/omnibox/geo/GeolocationHeaderTest.java:33:
private static final String DISABLE_CONSISTENT_GEOLOCATION_GEATURE =
nit: GESTURE?

https://codereview.chromium.org/2612993002/diff/120001/chrome/browser/geoloca...
File chrome/browser/geolocation/geolocation_permission_context_android.cc
(right):

https://codereview.chromium.org/2612993002/diff/120001/chrome/browser/geoloca...
chrome/browser/geolocation/geolocation_permission_context_android.cc:49:
url::Origin(embedding_origin))) {
I think we only want to do this if requesting_origin == embedding_origin? Since
geolocation uses the combination of origins, we should probably just treat
embedded cases as usual and prompt the user for a separate decision.

Eventually once we sort out scoping/permission delegation/etc this wouldn't be a
problem.

[raymes, 2017-01-10 03:19:37]

lgtm

[benwells, 2017-01-10 09:32:07]

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

[commit-bot: I haz the power, 2017-01-10 09:32:19]

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-01-10 11:13:24]

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

[commit-bot: I haz the power, 2017-01-10 11:13:25]

Dry run: 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_...)

[benwells, 2017-01-11 03:16:48]

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

[commit-bot: I haz the power, 2017-01-11 03:17:26]

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-01-11 04:01:07]

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

[commit-bot: I haz the power, 2017-01-11 04:01:08]

Dry run: Try jobs failed on following builders:
  chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...)
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[benwells, 2017-01-11 05:35:32]

https://codereview.chromium.org/2612993002/diff/60001/chrome/browser/android/...
File chrome/browser/android/search_geolocation/search_geolocation_service.cc
(right):

https://codereview.chromium.org/2612993002/diff/60001/chrome/browser/android/...
chrome/browser/android/search_geolocation/search_geolocation_service.cc:72: 
On 2017/01/10 03:19:32, raymes (a bit slow) wrote:
> On 2017/01/09 21:02:22, benwells wrote:
> > On 2017/01/09 05:47:26, raymes (a bit slow) wrote:
> > > I think we could just add an "IsUserSettable()" check here too which would
> > guard
> > > against enterprise policy, though it's not strictly needed.
> > 
> > The enterprise policy could force geolocation on, so I think a check here
> could
> > introduce problems.
> 
> Hmm, but wouldn't we fall back to using the enterprise policy in that case?
> Either way it's not a problem as it shouldn't affect most cases..

After our discussion offline, I've made it so if !IsUseSettable and content
setting is not ALLOW we return false. That will let the geolocation header
respect all enterprise policy settings - allow, ask and block.

https://codereview.chromium.org/2612993002/diff/120001/chrome/android/javates...
File
chrome/android/javatests/src/org/chromium/chrome/browser/omnibox/geo/GeolocationHeaderTest.java
(right):

https://codereview.chromium.org/2612993002/diff/120001/chrome/android/javates...
chrome/android/javatests/src/org/chromium/chrome/browser/omnibox/geo/GeolocationHeaderTest.java:31:
private static final String ENABLE_CONSISTENT_GEOLOCATION_GEATURE =
On 2017/01/10 03:19:32, raymes (a bit slow) wrote:
> nit: GESTURE?

Oops, FEATURE actually. Fixed.

https://codereview.chromium.org/2612993002/diff/120001/chrome/android/javates...
chrome/android/javatests/src/org/chromium/chrome/browser/omnibox/geo/GeolocationHeaderTest.java:33:
private static final String DISABLE_CONSISTENT_GEOLOCATION_GEATURE =
On 2017/01/10 03:19:32, raymes (a bit slow) wrote:
> nit: GESTURE?

Done.

https://codereview.chromium.org/2612993002/diff/120001/chrome/browser/geoloca...
File chrome/browser/geolocation/geolocation_permission_context_android.cc
(right):

https://codereview.chromium.org/2612993002/diff/120001/chrome/browser/geoloca...
chrome/browser/geolocation/geolocation_permission_context_android.cc:49:
url::Origin(embedding_origin))) {
On 2017/01/10 03:19:32, raymes (a bit slow) wrote:
> I think we only want to do this if requesting_origin == embedding_origin?
Since
> geolocation uses the combination of origins, we should probably just treat
> embedded cases as usual and prompt the user for a separate decision.
> 
> Eventually once we sort out scoping/permission delegation/etc this wouldn't be
a
> problem.

Done.

[benwells, 2017-01-11 05:37:26]

benwells@chromium.org changed reviewers:
+ msramek@chromium.org

[benwells, 2017-01-11 05:37:27]

+msramek for advice on the failing HostContentSettingsMap test. The test is
GuestProfileMigration. This test relies on the HostContentSettingsMap not being
created by the profile, but it is now.

I don't really know how to fix this test - can you give any advice? As this is
only a problem on Android I could just disable it there. I'm reluctant to do
that but don't have any ideas...

[benwells, 2017-01-11 05:37:52]

benwells@chromium.org changed reviewers:
+ dfalcantara@chromium.org, sky@chromium.org

[benwells, 2017-01-11 05:37:53]

+dfalcantara for Java stuff
+sky for chrome/browser/geolocation and chrome/browser/profiles

Note chrome/browser/geolocation changes are permissions related and have been
reviewed by a permissions owner.

[raymes, 2017-01-11 08:30:12]

https://codereview.chromium.org/2612993002/diff/200001/chrome/browser/android...
File chrome/browser/android/search_geolocation/search_geolocation_service.cc
(right):

https://codereview.chromium.org/2612993002/diff/200001/chrome/browser/android...
chrome/browser/android/search_geolocation/search_geolocation_service.cc:118:
GetCurrentContentSetting() == CONTENT_SETTING_ASK) {
I'm not sure if the check for ASK is actually needed?

https://codereview.chromium.org/2612993002/diff/200001/chrome/browser/content...
File chrome/browser/content_settings/host_content_settings_map_unittest.cc
(right):

https://codereview.chromium.org/2612993002/diff/200001/chrome/browser/content...
chrome/browser/content_settings/host_content_settings_map_unittest.cc:38: #endif
Are these 2 includes above needed?

https://codereview.chromium.org/2612993002/diff/200001/chrome/browser/content...
chrome/browser/content_settings/host_content_settings_map_unittest.cc:1264: "",
nullptr, TestingProfile::TestingFactories(),
nit: std::string() for empty strings

https://codereview.chromium.org/2612993002/diff/200001/chrome/browser/content...
chrome/browser/content_settings/host_content_settings_map_unittest.cc:1523: //
platform it is created lazily. Note, migration should be run here.
nit: platforms

https://codereview.chromium.org/2612993002/diff/200001/chrome/browser/content...
chrome/browser/content_settings/host_content_settings_map_unittest.cc:1552: //
Migration is done on construction of HostContentSettingsMap. True to run it
nit: True->Try

[benwells, 2017-01-11 09:26:11]

https://codereview.chromium.org/2612993002/diff/200001/chrome/browser/android...
File chrome/browser/android/search_geolocation/search_geolocation_service.cc
(right):

https://codereview.chromium.org/2612993002/diff/200001/chrome/browser/android...
chrome/browser/android/search_geolocation/search_geolocation_service.cc:118:
GetCurrentContentSetting() == CONTENT_SETTING_ASK) {
On 2017/01/11 08:30:12, raymes (a bit slow) wrote:
> I'm not sure if the check for ASK is actually needed? 

We can chat about it more tomorrow. I think it is necessary, in particular to
correctly send (or not) the X-Geo header.

If there is enterprise policy, and:
* the content setting is BLOCK, we can return true, but it doesn't matter. In
this case the DSE setting will also be false.
* the content setting is ALLOW, we should return true. In this case the DSE
setting will also be true, and the header should be sent. Note the header is
only sent if this function returns true.
* the content setting is ASK, we should return false as we don't want to send
the header.

https://codereview.chromium.org/2612993002/diff/200001/chrome/browser/content...
File chrome/browser/content_settings/host_content_settings_map_unittest.cc
(right):

https://codereview.chromium.org/2612993002/diff/200001/chrome/browser/content...
chrome/browser/content_settings/host_content_settings_map_unittest.cc:38: #endif
On 2017/01/11 08:30:12, raymes (a bit slow) wrote:
> Are these 2 includes above needed?

Yes, they are needed because two of the nullptrs passed into TestingProfile
constructor are smart pointers. These can't be constructed unless they know the
size of the type etc.

[msramek, 2017-01-11 09:51:10]

https://codereview.chromium.org/2612993002/diff/200001/chrome/browser/content...
File chrome/browser/content_settings/host_content_settings_map_unittest.cc
(right):

https://codereview.chromium.org/2612993002/diff/200001/chrome/browser/content...
chrome/browser/content_settings/host_content_settings_map_unittest.cc:1258:
TEST_F(HostContentSettingsMapTest, GuestProfile) {
The guest profile is Desktop only (unless I'm really missing something).

I haven't looked deeper into Profile and TestingProfile, but my hunch is that
the previous code SetGuestSession(true) was somehow interpreted as incognito on
Android and so nothing was written to the disk, while your new code is
interpreted as regular mode and thus we do write to the disk.

In that sense, disabling this test on Android would actually be the right thing
to do.

[sky, 2017-01-11 15:36:45]

LGTM

https://codereview.chromium.org/2612993002/diff/200001/chrome/browser/geoloca...
File chrome/browser/geolocation/geolocation_permission_context_android.h
(right):

https://codereview.chromium.org/2612993002/diff/200001/chrome/browser/geoloca...
chrome/browser/geolocation/geolocation_permission_context_android.h:48:
ContentSetting GetPermissionStatusInternal(
Add description with where this override comes from.

[gone, 2017-01-11 18:39:35]

https://codereview.chromium.org/2612993002/diff/200001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/preferences/website/SingleWebsitePreferences.java
(right):

https://codereview.chromium.org/2612993002/diff/200001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/preferences/website/SingleWebsitePreferences.java:505:
* Returns true if the DSE geolocation setting should be used for the current
host. This will be
Is DSE a commonly understood acronym?  It's used three times here and I'm still
not sure what it is.

https://codereview.chromium.org/2612993002/diff/200001/chrome/android/java/st...
File chrome/android/java/strings/android_chrome_strings.grd (right):

https://codereview.chromium.org/2612993002/diff/200001/chrome/android/java/st...
chrome/android/java/strings/android_chrome_strings.grd:726: Allow for Google
searches
These strings need work.  It's not clear that "allow for google searches" means
that this happens on address bar searches, let alone for Google search result
pages.  It also doesn't say anything about a "default search engine".  Can you
poke srahim@google.com for some ideas?  She's the new UX string person.

[benwells, 2017-01-11 22:29:26]

https://codereview.chromium.org/2612993002/diff/200001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/preferences/website/SingleWebsitePreferences.java
(right):

https://codereview.chromium.org/2612993002/diff/200001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/preferences/website/SingleWebsitePreferences.java:505:
* Returns true if the DSE geolocation setting should be used for the current
host. This will be
On 2017/01/11 18:39:35, dfalcantara (check queue) wrote:
> Is DSE a commonly understood acronym?  It's used three times here and I'm
still
> not sure what it is.

Fair point. I've added comments here and in the WebsitePreferencesBridge file
(and added wrapper functions there as discussed).

https://codereview.chromium.org/2612993002/diff/200001/chrome/android/java/st...
File chrome/android/java/strings/android_chrome_strings.grd (right):

https://codereview.chromium.org/2612993002/diff/200001/chrome/android/java/st...
chrome/android/java/strings/android_chrome_strings.grd:726: Allow for Google
searches
On 2017/01/11 18:39:35, dfalcantara (check queue) wrote:
> These strings need work.  It's not clear that "allow for google searches"
means
> that this happens on address bar searches, let alone for Google search result
> pages.  It also doesn't say anything about a "default search engine".  Can you
> poke mailto:srahim@google.com for some ideas?  She's the new UX string person.

As discussed we've been working with srahim on this. I've put in the current
contender for the string, hopefully it is the final string.

https://codereview.chromium.org/2612993002/diff/200001/chrome/browser/geoloca...
File chrome/browser/geolocation/geolocation_permission_context_android.h
(right):

https://codereview.chromium.org/2612993002/diff/200001/chrome/browser/geoloca...
chrome/browser/geolocation/geolocation_permission_context_android.h:48:
ContentSetting GetPermissionStatusInternal(
On 2017/01/11 15:36:44, sky wrote:
> Add description with where this override comes from.

Done.

[gone, 2017-01-11 22:31:53]

lgtm for the java stuff

[benwells, 2017-01-12 02:50:23]

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

[commit-bot: I haz the power, 2017-01-12 02:50:52]

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-01-12 02:53:54]

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

[commit-bot: I haz the power, 2017-01-12 02:53:55]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)

[benwells, 2017-01-12 03:10:43]

https://codereview.chromium.org/2612993002/diff/200001/chrome/browser/content...
File chrome/browser/content_settings/host_content_settings_map_unittest.cc
(right):

https://codereview.chromium.org/2612993002/diff/200001/chrome/browser/content...
chrome/browser/content_settings/host_content_settings_map_unittest.cc:1523: //
platform it is created lazily. Note, migration should be run here.
On 2017/01/11 08:30:12, raymes wrote:
> nit: platforms

Done.

https://codereview.chromium.org/2612993002/diff/200001/chrome/browser/content...
chrome/browser/content_settings/host_content_settings_map_unittest.cc:1552: //
Migration is done on construction of HostContentSettingsMap. True to run it
On 2017/01/11 08:30:12, raymes wrote:
> nit: True->Try

Done.

[benwells, 2017-01-12 03:19:54]

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

[commit-bot: I haz the power, 2017-01-12 03:20:10]

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-01-12 03:28:59]

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

[commit-bot: I haz the power, 2017-01-12 03:29:00]

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

[benwells, 2017-01-12 03:59:24]

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

[commit-bot: I haz the power, 2017-01-12 03:59:40]

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-01-12 05:18:05]

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

[commit-bot: I haz the power, 2017-01-12 05:18:06]

Dry run: This issue passed the CQ dry run.

[benwells, 2017-01-12 05:21:37]

The CQ bit was checked by benwells@chromium.org

[benwells, 2017-01-12 05:21:38]

The patchset sent to the CQ was uploaded after l-g-t-m from raymes@chromium.org,
sky@chromium.org, dfalcantara@chromium.org
Link to the patchset: https://codereview.chromium.org/2612993002/#ps270001
(title: "Fix test / merge error")

[commit-bot: I haz the power, 2017-01-12 05:22:01]

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-01-12 05:26:25]

CQ is committing da patch.

Bot data: {"patchset_id": 270001, "attempt_start_ts": 1484198497617820,
"parent_rev": "67e10e8e011f062b45fdcb83bc0296d47bcaa859", "commit_rev":
"fd39adcc3b11e8ebe81754de74fc3cd191e44c29"}

[commit-bot: I haz the power, 2017-01-12 05:27:46]

Description was changed from

==========
Make geolocation API and X-Geo header access consistent

This change allows geolocation access for the default search engine if
it would have access to geolocation information via the X-Geo header.

To effect this, a new setting is enabled controlling whether the default
search engine has geolocation access. This is only used if the search
engine supports the X-Geo header, i.e. it is Google.

The setting is consulted for the DSE both when deciding if the X-Geo
header should be added, and when deciding if geolocation API access
should be granted.

To allow the UI to make sense and provide the greatest clarity, the
setting should always be consistent with the underlying content setting
for the DSE's origin. In this end it is initialized accordingly and
various checks are in place to keep it consistent.

BUG=674389
==========

to

==========
Make geolocation API and X-Geo header access consistent

This change allows geolocation access for the default search engine if
it would have access to geolocation information via the X-Geo header.

To effect this, a new setting is enabled controlling whether the default
search engine has geolocation access. This is only used if the search
engine supports the X-Geo header, i.e. it is Google.

The setting is consulted for the DSE both when deciding if the X-Geo
header should be added, and when deciding if geolocation API access
should be granted.

To allow the UI to make sense and provide the greatest clarity, the
setting should always be consistent with the underlying content setting
for the DSE's origin. In this end it is initialized accordingly and
various checks are in place to keep it consistent.

BUG=674389

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

[commit-bot: I haz the power, 2017-01-12 05:27:48]

Committed patchset #15 (id:270001) as
https://chromium.googlesource.com/chromium/src/+/fd39adcc3b11e8ebe81754de74fc...