Update geolocation infobar to handle Android system settings

Add Google Location Settings geolocation permission flow

Pre MR0 version in Android, we had an explicit "Enable Location"
setting in Chrome that will show the infobar based on the
settings. Post MR0, we use the combination of system master
location setting and system google apps location setting to
decide between

1. Showing the infobar with Allow button (if both master and
google apps are turned on)

2. Showing the infobar with Settings button that will take user
to Google Apps Location Setting activity (if master is on but
google apps location setting is turned off)

3. Do not show infobar (if master is off)

Above functionality has been added in this CL for both pre and
post MR0 cases.

This CL also removes the android unit_test for GeolocationEnabledDisabled. Since the android specific geolocation code is going to be refactored, this test along with few more android specific unit_tests will be added in an upcoming CL.

CQ-DEPEND=0f28fce084f6010242360fb7b6633b8fd7296be4


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=164466

[Ramya, 2012-10-17 06:17:44]

This CL is dependent on http://codereview.chromium.org/11188020/

[John Knottenbelt, 2012-10-17 10:28:29]

Thanks, Ramya. 

Do you have any ideas on what sort of tests (unit tests, preferably) we could
add to test the new behaviour?

http://codereview.chromium.org/11186010/diff/5001/chrome/browser/android/goog...
File chrome/browser/android/google_location_settings_helper.h (right):

http://codereview.chromium.org/11186010/diff/5001/chrome/browser/android/goog...
chrome/browser/android/google_location_settings_helper.h:16: * This class is
needed mainly for Android to fetch the current system location
delete "mainly for Android" - this file is already Android-specific.

http://codereview.chromium.org/11186010/diff/5001/chrome/browser/geolocation/...
File chrome/browser/geolocation/geolocation_confirm_infobar_delegate_android.cc
(right):

http://codereview.chromium.org/11186010/diff/5001/chrome/browser/geolocation/...
chrome/browser/geolocation/geolocation_confirm_infobar_delegate_android.cc:35:
// Accept button text could be either 'Allow' or 'Google Location Settings'.
Please update this comment - have you got it that IsPlatformSettingEnabled() now
returns true in the legacy case (if there is no Google Location Settings)?

[John Knottenbelt, 2012-10-17 12:35:47]

There seems to be some overlap with this and
http://codereview.chromium.org/11188020/

[John Knottenbelt, 2012-10-22 12:58:34]

http://codereview.chromium.org/11186010/diff/12001/chrome/browser/android/goo...
File chrome/browser/android/google_location_settings_helper.h (right):

http://codereview.chromium.org/11186010/diff/12001/chrome/browser/android/goo...
chrome/browser/android/google_location_settings_helper.h:22: 
nit: remove blank line and #37.

http://codereview.chromium.org/11186010/diff/12001/chrome/browser/geolocation...
File chrome/browser/geolocation/chrome_geolocation_permission_context_android.cc
(right):

http://codereview.chromium.org/11186010/diff/12001/chrome/browser/geolocation...
chrome/browser/geolocation/chrome_geolocation_permission_context_android.cc:31:
if (!profile()->GetPrefs()->GetBoolean(prefs::kGeolocationEnabled)) {
If the Google Location Settings are available, will there be any UI to adjust
this preference? If not, then we shouldn't check this preference as it may be
stuck in the disabled setting with no way to change it back.

[Ramya, 2012-10-22 23:57:11]

PTAL

thestig,
Can you take a look at chrome/chrome_browser.gypi?

jcivelli,
Can you take a look at chrome/test changes?

bulach,jknotten
Can you take a look at CL?

http://codereview.chromium.org/11186010/diff/5001/chrome/browser/android/goog...
File chrome/browser/android/google_location_settings_helper.h (right):

http://codereview.chromium.org/11186010/diff/5001/chrome/browser/android/goog...
chrome/browser/android/google_location_settings_helper.h:16: * This class is
needed mainly for Android to fetch the current system location
On 2012/10/17 10:28:29, John Knottenbelt wrote:
> delete "mainly for Android" - this file is already Android-specific.

Done.

http://codereview.chromium.org/11186010/diff/5001/chrome/browser/geolocation/...
File chrome/browser/geolocation/geolocation_confirm_infobar_delegate_android.cc
(right):

http://codereview.chromium.org/11186010/diff/5001/chrome/browser/geolocation/...
chrome/browser/geolocation/geolocation_confirm_infobar_delegate_android.cc:35:
// Accept button text could be either 'Allow' or 'Google Location Settings'.
Yes, this works in the legacy case as well.

http://codereview.chromium.org/11186010/diff/12001/chrome/browser/android/goo...
File chrome/browser/android/google_location_settings_helper.h (right):

http://codereview.chromium.org/11186010/diff/12001/chrome/browser/android/goo...
chrome/browser/android/google_location_settings_helper.h:22: 
On 2012/10/22 12:58:35, John Knottenbelt wrote:
> nit: remove blank line and #37.

Done.

http://codereview.chromium.org/11186010/diff/12001/chrome/browser/geolocation...
File chrome/browser/geolocation/chrome_geolocation_permission_context_android.cc
(right):

http://codereview.chromium.org/11186010/diff/12001/chrome/browser/geolocation...
chrome/browser/geolocation/chrome_geolocation_permission_context_android.cc:31:
if (!profile()->GetPrefs()->GetBoolean(prefs::kGeolocationEnabled)) {
This is set to the same boolean value as master location setting and gets reset
everytime the user changes this. So the user can always enable the master
location setting in Android to change it back.

[Jay Civelli, 2012-10-23 00:18:19]

chrome/tests LGTM

[Lei Zhang, 2012-10-23 00:34:05]

gyp files lgtm

[bulach, 2012-10-23 13:06:26]

Hi Ramya,

I have some suggestions below, hopefully that will simplify the design of this
patch..

please let me know your thoughts, specially on the
google_location_settings_helper.h dependency suggestion below:

http://codereview.chromium.org/11186010/diff/11009/chrome/android/java/src/or...
File
chrome/android/java/src/org/chromium/chrome/browser/GoogleLocationSettingsHelper.java
(right):

http://codereview.chromium.org/11186010/diff/11009/chrome/android/java/src/or...
chrome/android/java/src/org/chromium/chrome/browser/GoogleLocationSettingsHelper.java:13:

nit: remove \n

http://codereview.chromium.org/11186010/diff/11009/chrome/android/java/src/or...
File
chrome/android/java/src/org/chromium/chrome/browser/GoogleLocationSettingsHelperImpl.java
(right):

http://codereview.chromium.org/11186010/diff/11009/chrome/android/java/src/or...
chrome/android/java/src/org/chromium/chrome/browser/GoogleLocationSettingsHelperImpl.java:15:
public class GoogleLocationSettingsHelperImpl implements
GoogleLocationSettingsHelper {
nit: maybe "Stub" rather than Impl, since the real Impl will live in the other
side?

http://codereview.chromium.org/11186010/diff/11009/chrome/android/java/src/or...
chrome/android/java/src/org/chromium/chrome/browser/GoogleLocationSettingsHelperImpl.java:18:
private static final String sDefaultInfoBarString = "Allow";
since this is just a stub, maybe inline this in 26?

http://codereview.chromium.org/11186010/diff/11009/chrome/browser/android/goo...
File chrome/browser/android/google_location_settings_helper.h (right):

http://codereview.chromium.org/11186010/diff/11009/chrome/browser/android/goo...
chrome/browser/android/google_location_settings_helper.h:20: */
nit: C++ comments are // instead of /* */

http://codereview.chromium.org/11186010/diff/11009/chrome/browser/android/goo...
chrome/browser/android/google_location_settings_helper.h:24:
~GoogleLocationSettingsHelper() {}
nit: add \n after this.
also, the destructor shouldn't be inline, as this class contains non-POD member
(clang bot will complain..)

http://codereview.chromium.org/11186010/diff/11009/chrome/browser/android/goo...
chrome/browser/android/google_location_settings_helper.h:35: static
GoogleLocationSettingsHelperFactory* helper_factory_;
this will leak.. :(

how about we invert the dependency?
rather than this class requiring the embedder to call "Set...Factory", we could:

- make this class a pure c++ interface.
- add "static GoogleLocationSettingsHelper* Create()"
|__ this method will be implemented by content shell, or the android app. in
tests, we could then implement a mock

- if you agree with the above, we could completely remove all the java files
from this patch... that is, chromium requires a very thin C++ interface, and in
the app side in android it's an implementation detail that it happens to be
implemented in java. both content shell and tests can then provide their own
implementations..

[John Knottenbelt, 2012-10-23 13:10:05]

Thanks, Ramya - one comment.

https://codereview.chromium.org/11186010/diff/11009/chrome/browser/geolocatio...
File chrome/browser/geolocation/chrome_geolocation_permission_context.cc
(right):

https://codereview.chromium.org/11186010/diff/11009/chrome/browser/geolocatio...
chrome/browser/geolocation/chrome_geolocation_permission_context.cc:98: #if
defined(OS_ANDROID)
Remove this diff as the code is already in
ChromeGeolocationPermissionContextAndroid::DecidePermission

[Jay Civelli, 2012-10-23 17:37:52]

http://codereview.chromium.org/11186010/diff/11009/chrome/browser/android/goo...
File chrome/browser/android/google_location_settings_helper.h (right):

http://codereview.chromium.org/11186010/diff/11009/chrome/browser/android/goo...
chrome/browser/android/google_location_settings_helper.h:35: static
GoogleLocationSettingsHelperFactory* helper_factory_;
On 2012/10/23 13:06:26, bulach wrote:
> this will leak.. :(
It should not be too bad a leak though, we only have one
GoogleLocationSettingsHelperFactory per run of the application.

> 
> how about we invert the dependency?
> rather than this class requiring the embedder to call "Set...Factory", we
could:
> 
> - make this class a pure c++ interface.
> - add "static GoogleLocationSettingsHelper* Create()"
> |__ this method will be implemented by content shell, or the android app. in
> tests, we could then implement a mock
> 
> - if you agree with the above, we could completely remove all the java files
> from this patch... that is, chromium requires a very thin C++ interface, and
in
> the app side in android it's an implementation detail that it happens to be
> implemented in java. both content shell and tests can then provide their own
> implementations..
I believe this is the approach that Ramya had originally taken but then review
comments suggested her to go the factory route, so she reimplemented it that
way.
Do you think she could revisit that at a later point?

[Ramya, 2012-10-23 21:57:13]

http://codereview.chromium.org/11186010/diff/11009/chrome/android/java/src/or...
File
chrome/android/java/src/org/chromium/chrome/browser/GoogleLocationSettingsHelper.java
(right):

http://codereview.chromium.org/11186010/diff/11009/chrome/android/java/src/or...
chrome/android/java/src/org/chromium/chrome/browser/GoogleLocationSettingsHelper.java:13:

On 2012/10/23 13:06:26, bulach wrote:
> nit: remove \n

Done.

http://codereview.chromium.org/11186010/diff/11009/chrome/android/java/src/or...
File
chrome/android/java/src/org/chromium/chrome/browser/GoogleLocationSettingsHelperImpl.java
(right):

http://codereview.chromium.org/11186010/diff/11009/chrome/android/java/src/or...
chrome/android/java/src/org/chromium/chrome/browser/GoogleLocationSettingsHelperImpl.java:15:
public class GoogleLocationSettingsHelperImpl implements
GoogleLocationSettingsHelper {
On 2012/10/23 13:06:26, bulach wrote:
> nit: maybe "Stub" rather than Impl, since the real Impl will live in the other
> side?

Done.

http://codereview.chromium.org/11186010/diff/11009/chrome/android/java/src/or...
chrome/android/java/src/org/chromium/chrome/browser/GoogleLocationSettingsHelperImpl.java:18:
private static final String sDefaultInfoBarString = "Allow";
On 2012/10/23 13:06:26, bulach wrote:
> since this is just a stub, maybe inline this in 26?

Done.

http://codereview.chromium.org/11186010/diff/11009/chrome/browser/android/goo...
File chrome/browser/android/google_location_settings_helper.h (right):

http://codereview.chromium.org/11186010/diff/11009/chrome/browser/android/goo...
chrome/browser/android/google_location_settings_helper.h:20: */
On 2012/10/23 13:06:26, bulach wrote:
> nit: C++ comments are // instead of /* */

Done.

http://codereview.chromium.org/11186010/diff/11009/chrome/browser/android/goo...
chrome/browser/android/google_location_settings_helper.h:24:
~GoogleLocationSettingsHelper() {}
On 2012/10/23 13:06:26, bulach wrote:
> nit: add \n after this.
> also, the destructor shouldn't be inline, as this class contains non-POD
member
> (clang bot will complain..)

Done.

http://codereview.chromium.org/11186010/diff/11009/chrome/browser/geolocation...
File chrome/browser/geolocation/chrome_geolocation_permission_context.cc
(right):

http://codereview.chromium.org/11186010/diff/11009/chrome/browser/geolocation...
chrome/browser/geolocation/chrome_geolocation_permission_context.cc:98: #if
defined(OS_ANDROID)
On 2012/10/23 13:10:05, John Knottenbelt wrote:
> Remove this diff as the code is already in
> ChromeGeolocationPermissionContextAndroid::DecidePermission

Done.

[bulach, 2012-10-24 10:02:54]

hi Jay, Ramya,

I understand and apologize if we caused any confusion... :)
as any large patch that gets split and moved across different trees, it's hard
for both the author and the reviewers to get it right the first time, so
apologies for the back and forth... ;)

I think that removing the java code from here and keeping the factory /
interface as pure c++ will simplify not only this patch, but also, make it
possible to write tests for the new flow...

if you think it's better to do this in two stages, that works for me too..

I'll send remamining comments shortly..

[John Knottenbelt, 2012-10-24 12:49:02]

Thanks for your perseverence, Ramya, we're getting there!

lgtm with a couple of nits.

I'm also ok with refactoring GeolocationLocationSettingsHelper to drop the Java
dependency (Chromium side) in a later patch. This will enable native c++ unit
testing of the ChromeGeolocationPermissionContextAndroid class.

http://codereview.chromium.org/11186010/diff/16023/chrome/android/testshell/t...
File chrome/android/testshell/testshell_stubs.cc (right):

http://codereview.chromium.org/11186010/diff/16023/chrome/android/testshell/t...
chrome/android/testshell/testshell_stubs.cc:8: #include
"chrome/browser/autofill/autofill_external_delegate.h"
nit: can this include be removed?

http://codereview.chromium.org/11186010/diff/16023/chrome/browser/android/goo...
File chrome/browser/android/google_location_settings_helper.cc (right):

http://codereview.chromium.org/11186010/diff/16023/chrome/browser/android/goo...
chrome/browser/android/google_location_settings_helper.cc:38:
ScopedJavaLocalRef<jstring> inforbar_allow_text =
nit: infobar_allow_text

[Ramya, 2012-10-24 23:16:06]

http://codereview.chromium.org/11186010/diff/16023/chrome/android/testshell/t...
File chrome/android/testshell/testshell_stubs.cc (right):

http://codereview.chromium.org/11186010/diff/16023/chrome/android/testshell/t...
chrome/android/testshell/testshell_stubs.cc:8: #include
"chrome/browser/autofill/autofill_external_delegate.h"
On 2012/10/24 12:49:02, John Knottenbelt wrote:
> nit: can this include be removed?

Done.

http://codereview.chromium.org/11186010/diff/16023/chrome/browser/android/goo...
File chrome/browser/android/google_location_settings_helper.cc (right):

http://codereview.chromium.org/11186010/diff/16023/chrome/browser/android/goo...
chrome/browser/android/google_location_settings_helper.cc:38:
ScopedJavaLocalRef<jstring> inforbar_allow_text =
On 2012/10/24 12:49:02, John Knottenbelt wrote:
> nit: infobar_allow_text

Done.

[John Knottenbelt, 2012-10-25 15:34:06]

lgtm

[commit-bot: I haz the power, 2012-10-26 21:40:38]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/cramya@chromium.org/11186010/31011

[commit-bot: I haz the power, 2012-10-27 00:49:39]

Change committed as 164466