Simplify Android geolocation permission checks

Simplify Android geolocation permission checks

BUG=400694
TBR=tommi

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=287782

[Miguel Garcia, 2014-08-05 12:31:53]

Bernhard for content settings
Michael for geolocation

[Bernhard Bauer, 2014-08-05 12:43:50]

Nice! LGTM, just some nits, mostly about comments:

https://codereview.chromium.org/441883003/diff/20001/chrome/browser/geolocati...
File chrome/browser/geolocation/geolocation_permission_context_android.cc
(right):

https://codereview.chromium.org/441883003/diff/20001/chrome/browser/geolocati...
chrome/browser/geolocation/geolocation_permission_context_android.cc:48:
google_location_settings_helper_->IsMasterLocationSettingEnabled() &&
Do we need these two separate callbacks now?

(If we can't remove them in this CL because we don't want to break downstream,
we can do it in a followup.)

https://codereview.chromium.org/441883003/diff/20001/chrome/browser/geolocati...
File chrome/browser/geolocation/geolocation_permission_context_android.h
(right):

https://codereview.chromium.org/441883003/diff/20001/chrome/browser/geolocati...
chrome/browser/geolocation/geolocation_permission_context_android.h:8: // The
flow for geolocation permissions on android needs to take into account
Nit: Android is capitalized.

https://codereview.chromium.org/441883003/diff/20001/chrome/browser/geolocati...
chrome/browser/geolocation/geolocation_permission_context_android.h:11: // -
GeolocationPermissionContextAndroid::DecidePermission
Nit: This should be either indented to match the other bullet points below, or
those not indented. It would also be good to indent continued lines so that the
text aligns at the left. Once this is indented, try to fill lines as much as
possible.

https://codereview.chromium.org/441883003/diff/20001/chrome/browser/geolocati...
chrome/browser/geolocation/geolocation_permission_context_android.h:26: // (like
the per site settings) are queried on the UX thread while the
Nit: "UI thread"

[Miguel Garcia, 2014-08-05 16:19:41]

https://codereview.chromium.org/441883003/diff/20001/chrome/browser/geolocati...
File chrome/browser/geolocation/geolocation_permission_context_android.cc
(right):

https://codereview.chromium.org/441883003/diff/20001/chrome/browser/geolocati...
chrome/browser/geolocation/geolocation_permission_context_android.cc:48:
google_location_settings_helper_->IsMasterLocationSettingEnabled() &&
Indeed, let's add the call now, based on those two so it can be part of the same
3 sided patch

On 2014/08/05 12:43:50, Bernhard Bauer wrote:
> Do we need these two separate callbacks now?
> 
> (If we can't remove them in this CL because we don't want to break downstream,
> we can do it in a followup.)

https://codereview.chromium.org/441883003/diff/20001/chrome/browser/geolocati...
File chrome/browser/geolocation/geolocation_permission_context_android.h
(right):

https://codereview.chromium.org/441883003/diff/20001/chrome/browser/geolocati...
chrome/browser/geolocation/geolocation_permission_context_android.h:8: // The
flow for geolocation permissions on android needs to take into account
On 2014/08/05 12:43:50, Bernhard Bauer wrote:
> Nit: Android is capitalized.

Done.

https://codereview.chromium.org/441883003/diff/20001/chrome/browser/geolocati...
chrome/browser/geolocation/geolocation_permission_context_android.h:11: // -
GeolocationPermissionContextAndroid::DecidePermission
On 2014/08/05 12:43:50, Bernhard Bauer wrote:
> Nit: This should be either indented to match the other bullet points below, or
> those not indented. It would also be good to indent continued lines so that
the
> text aligns at the left. Once this is indented, try to fill lines as much as
> possible.

Done.

https://codereview.chromium.org/441883003/diff/20001/chrome/browser/geolocati...
chrome/browser/geolocation/geolocation_permission_context_android.h:26: // (like
the per site settings) are queried on the UX thread while the
On 2014/08/05 12:43:50, Bernhard Bauer wrote:
> Nit: "UI thread"

Done.

[Michael van Ouwerkerk, 2014-08-06 10:47:44]

https://codereview.chromium.org/441883003/diff/40001/chrome/browser/android/m...
File chrome/browser/android/mock_google_location_settings_helper.h (right):

https://codereview.chromium.org/441883003/diff/40001/chrome/browser/android/m...
chrome/browser/android/mock_google_location_settings_helper.h:15: //
GoogleLocationSettingsHelper implementation:
Don't you need virtual bool IsSystemLocationEnabled() OVERRIDE; because
google_location_settings_helper.cc is temporary? Otherwise you'll need to touch
this file again when you remove the temp.

https://codereview.chromium.org/441883003/diff/40001/chrome/browser/android/m...
chrome/browser/android/mock_google_location_settings_helper.h:16: virtual bool
IsMasterLocationSettingEnabled() OVERRIDE;
So this can be deleted as well?

https://codereview.chromium.org/441883003/diff/40001/chrome/browser/android/m...
chrome/browser/android/mock_google_location_settings_helper.h:17: virtual bool
IsGoogleAppsLocationSettingEnabled() OVERRIDE;
And this?

https://codereview.chromium.org/441883003/diff/40001/chrome/browser/android/m...
chrome/browser/android/mock_google_location_settings_helper.h:27: static bool
google_apps_location_enabled;
And these two could be replaced by:
static bool is_system_location_enabled;

https://codereview.chromium.org/441883003/diff/40001/chrome/browser/geolocati...
File chrome/browser/geolocation/geolocation_infobar_delegate.h (right):

https://codereview.chromium.org/441883003/diff/40001/chrome/browser/geolocati...
chrome/browser/geolocation/geolocation_infobar_delegate.h:38: 
Nit: why two lines of ws?

https://codereview.chromium.org/441883003/diff/40001/chrome/browser/geolocati...
File chrome/browser/geolocation/geolocation_permission_context_android.h
(right):

https://codereview.chromium.org/441883003/diff/40001/chrome/browser/geolocati...
chrome/browser/geolocation/geolocation_permission_context_android.h:11: //
GeolocationPermissionContextAndroid::DecidePermissionintercepts the flow in
Nit: add a space between Permission and intercepts.

https://codereview.chromium.org/441883003/diff/40001/chrome/browser/geolocati...
File chrome/browser/geolocation/geolocation_permission_context_unittest.cc
(right):

https://codereview.chromium.org/441883003/diff/40001/chrome/browser/geolocati...
chrome/browser/geolocation/geolocation_permission_context_unittest.cc:325:
TEST_F(GeolocationPermissionContextTests, MasterEnabledGoogleAppsDisabled) {
I'm not sure we still care about the distinction between Master and Google Apps.
It's just 'system' now, right?

[Miguel Garcia, 2014-08-06 12:56:59]

https://codereview.chromium.org/441883003/diff/40001/chrome/browser/android/m...
File chrome/browser/android/mock_google_location_settings_helper.h (right):

https://codereview.chromium.org/441883003/diff/40001/chrome/browser/android/m...
chrome/browser/android/mock_google_location_settings_helper.h:15: //
GoogleLocationSettingsHelper implementation:
On 2014/08/06 10:47:43, Michael van Ouwerkerk wrote:
> Don't you need virtual bool IsSystemLocationEnabled() OVERRIDE; because
> google_location_settings_helper.cc is temporary? Otherwise you'll need to
touch
> this file again when you remove the temp.

Which is fine, once I remove the temp I implement it at once. See
https://chromiumcodereview.appspot.com/440123002/

https://codereview.chromium.org/441883003/diff/40001/chrome/browser/android/m...
chrome/browser/android/mock_google_location_settings_helper.h:16: virtual bool
IsMasterLocationSettingEnabled() OVERRIDE;
On 2014/08/06 10:47:43, Michael van Ouwerkerk wrote:
> So this can be deleted as well?

There are tests that set/check for this condition. I prefer to leave them for
testing purposes.

https://codereview.chromium.org/441883003/diff/40001/chrome/browser/android/m...
chrome/browser/android/mock_google_location_settings_helper.h:17: virtual bool
IsGoogleAppsLocationSettingEnabled() OVERRIDE;
On 2014/08/06 10:47:43, Michael van Ouwerkerk wrote:
> And this?
ditto

https://codereview.chromium.org/441883003/diff/40001/chrome/browser/android/m...
chrome/browser/android/mock_google_location_settings_helper.h:27: static bool
google_apps_location_enabled;
On 2014/08/06 10:47:43, Michael van Ouwerkerk wrote:
> And these two could be replaced by:
> static bool is_system_location_enabled;

ditto. In the tests you want to ensure that changing any of the two conditions
prevent geolocation from showing up. It will make a bit more sense once we
upstream the actual checks that happen in the real code.

https://codereview.chromium.org/441883003/diff/40001/chrome/browser/geolocati...
File chrome/browser/geolocation/geolocation_infobar_delegate.h (right):

https://codereview.chromium.org/441883003/diff/40001/chrome/browser/geolocati...
chrome/browser/geolocation/geolocation_infobar_delegate.h:38: 
On 2014/08/06 10:47:44, Michael van Ouwerkerk wrote:
> Nit: why two lines of ws?

Removed one

https://codereview.chromium.org/441883003/diff/40001/chrome/browser/geolocati...
File chrome/browser/geolocation/geolocation_permission_context_android.h
(right):

https://codereview.chromium.org/441883003/diff/40001/chrome/browser/geolocati...
chrome/browser/geolocation/geolocation_permission_context_android.h:11: //
GeolocationPermissionContextAndroid::DecidePermissionintercepts the flow in
On 2014/08/06 10:47:44, Michael van Ouwerkerk wrote:
> Nit: add a space between Permission and intercepts.

Done.

https://codereview.chromium.org/441883003/diff/40001/chrome/browser/geolocati...
File chrome/browser/geolocation/geolocation_permission_context_unittest.cc
(right):

https://codereview.chromium.org/441883003/diff/40001/chrome/browser/geolocati...
chrome/browser/geolocation/geolocation_permission_context_unittest.cc:325:
TEST_F(GeolocationPermissionContextTests, MasterEnabledGoogleAppsDisabled) {
On 2014/08/06 10:47:44, Michael van Ouwerkerk wrote:
> I'm not sure we still care about the distinction between Master and Google
Apps.
> It's just 'system' now, right?

We do (not at the interface level but internally) I rather keep the test until
we upstream the production check, then move it there.

[Michael van Ouwerkerk, 2014-08-06 13:10:17]

lgtm

[Miguel Garcia, 2014-08-06 13:11:42]

The CQ bit was checked by miguelg@chromium.org

[Miguel Garcia, 2014-08-06 13:11:46]

The CQ bit was unchecked by miguelg@chromium.org

[Miguel Garcia, 2014-08-06 13:16:10]

The CQ bit was checked by miguelg@chromium.org

[commit-bot: I haz the power, 2014-08-06 13:17:19]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/miguelg@chromium.org/441883003/60001

[commit-bot: I haz the power, 2014-08-06 13:25:01]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  linux_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/linux_gpu/builds/...)
  mac_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu/builds/37977)
  win_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu/builds/43176)
  android_aosp on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_aosp/bu...)
  android_chromium_gn_compile_rel on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromiu...)
  android_clang_dbg on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_clang_d...)
  android_dbg on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_dbg/bui...)
  chromium_presubmit on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
  linux_chromium_chromeos_rel_swarming on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_gn_rel on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_rel_swarming on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  ios_dbg_simulator on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)
  ios_rel_device on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device/bu...)
  ios_rel_device_ninja on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)
  mac_chromium_compile_dbg on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel/...)
  win8_chromium_rel on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_rel...)
  win_chromium_compile_dbg on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
  win_chromium_rel on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel/...)
  win_chromium_x64_rel on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[commit-bot: I haz the power, 2014-08-06 13:26:11]

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

[commit-bot: I haz the power, 2014-08-06 13:26:15]

Try jobs failed on following builders:
  mac_chromium_rel on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel/...)

[Miguel Garcia, 2014-08-06 13:34:29]

The CQ bit was checked by miguelg@chromium.org

[commit-bot: I haz the power, 2014-08-06 13:35:25]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/miguelg@chromium.org/441883003/80001

[Miguel Garcia, 2014-08-06 13:44:29]

On 2014/08/06 13:35:25, I haz the power (commit-bot) wrote:
> CQ is trying da patch. Follow status at
>  https://chromium-status.appspot.com/cq/miguelg@chromium.org/441883003/80001

TBR tommi for the change in
browser/media/protected_media_identifier_permission_context.cc

[Miguel Garcia, 2014-08-06 13:44:34]

The CQ bit was unchecked by miguelg@chromium.org

[Miguel Garcia, 2014-08-06 13:44:34]

The CQ bit was checked by miguelg@chromium.org

[commit-bot: I haz the power, 2014-08-06 13:44:56]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/miguelg@chromium.org/441883003/80001

[tommi (sloooow) - chröme, 2014-08-06 13:46:53]

lgtm for protected_media_identifier_permission_context.cc

[commit-bot: I haz the power, 2014-08-06 16:35:54]

Change committed as 287782