Introduce Android variants of ChromeGeolocationPermissionContext, GeolocationConfirmInfoBarDelegate

Introduce Android variants of ChromeGeolocationPermissionContext, GeolocationInfobarQueueController, GeolocationConfirmInfoBarDelegate to handle Android-specific flow.
Introduce factory create methods for ChromeGeolocationPermissionContext to select the appropriate platform constructor.

This CL is part 2/3.
Part1 at http://codereview.chromium.org/11183018/
Part3 at http://codereview.chromium.org/11186010/ (WIP)

BUG=152236
CQ-DEPEND=8fa4bd0cd62974e61632e7c0420fa2f47ab62554

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

[Ramya, 2012-10-17 06:16:52]

This CL is dependent on http://codereview.chromium.org/11183018/
Still need to find a way to run git try on dependent CL's.

[John Knottenbelt, 2012-10-17 12:19:34]

Thanks Ramya! lgtm. We should add tests to check that:
 - If the Google Location Settings is available, that the global enable/disable
switch is ignored.
 - If the Google Location Settings is unavailable, that the global
enable/disable switch is observed.
 - That the Google Location Settings activity is requested to come up under the
correct circumstances.
 - That the (persistent) content settings are not updated in the case that the
Google Location Settings is shown. It's expected that a page reload is required.
 - That the permission context denies permission if the Google Location
Setttings disallows.

These might be best added as unit tests on
ChromeGeolocationPermissionContextAndroid.

[bulach, 2012-10-17 13:17:38]

thanks all! nits, and one suggestion for a factory function for the
infobardelegate, please let me know your thoughts..

https://chromiumcodereview.appspot.com/11188020/diff/10003/chrome/browser/geo...
File chrome/browser/geolocation/chrome_geolocation_permission_context.cc
(right):

https://chromiumcodereview.appspot.com/11188020/diff/10003/chrome/browser/geo...
chrome/browser/geolocation/chrome_geolocation_permission_context.cc:33: #if
!defined(OS_ANDROID)
if you agree with the comment in the other patch, this could be moved to real a
factory class, so that this file doesn't know anything about android..

https://chromiumcodereview.appspot.com/11188020/diff/10003/chrome/browser/geo...
File chrome/browser/geolocation/chrome_geolocation_permission_context_android.cc
(right):

https://chromiumcodereview.appspot.com/11188020/diff/10003/chrome/browser/geo...
chrome/browser/geolocation/chrome_geolocation_permission_context_android.cc:14:
Profile *profile) {
nit: s/Profile *p/Profile* p/

https://chromiumcodereview.appspot.com/11188020/diff/10003/chrome/browser/geo...
chrome/browser/geolocation/chrome_geolocation_permission_context_android.cc:19:
PrefService *user_prefs) {
nit: s/PrefService *u/PrefService* u/

https://chromiumcodereview.appspot.com/11188020/diff/10003/chrome/browser/geo...
chrome/browser/geolocation/chrome_geolocation_permission_context_android.cc:62:
ChromeGeolocationPermissionContextAndroid::PermissionDecided(
this seems recursive :) probably remove 53-65 altogether?

https://chromiumcodereview.appspot.com/11188020/diff/10003/chrome/browser/geo...
File chrome/browser/geolocation/chrome_geolocation_permission_context_android.h
(right):

https://chromiumcodereview.appspot.com/11188020/diff/10003/chrome/browser/geo...
chrome/browser/geolocation/chrome_geolocation_permission_context_android.h:19: 
nit: 
 // ChromeGeolocationPermissionContext implementation:

https://chromiumcodereview.appspot.com/11188020/diff/10003/chrome/browser/geo...
chrome/browser/geolocation/chrome_geolocation_permission_context_android.h:36:
virtual GeolocationInfoBarQueueController* CreateQueueController() OVERRIDE;
nit:

DISALLOW_COPY_AND_ASSIGN(ChromeGeolocationPermissionContextAndroid);

https://chromiumcodereview.appspot.com/11188020/diff/10003/chrome/browser/geo...
File chrome/browser/geolocation/geolocation_confirm_infobar_delegate_android.cc
(right):

https://chromiumcodereview.appspot.com/11188020/diff/10003/chrome/browser/geo...
chrome/browser/geolocation/geolocation_confirm_infobar_delegate_android.cc:32:
return GeolocationConfirmInfoBarDelegate::Accept();
see above, I think we can remove 31-33..

https://chromiumcodereview.appspot.com/11188020/diff/10003/chrome/browser/geo...
File chrome/browser/geolocation/geolocation_confirm_infobar_delegate_android.h
(right):

https://chromiumcodereview.appspot.com/11188020/diff/10003/chrome/browser/geo...
chrome/browser/geolocation/geolocation_confirm_infobar_delegate_android.h:25:
virtual bool Accept() OVERRIDE;
Accept() seems unnecessary?

https://chromiumcodereview.appspot.com/11188020/diff/10003/chrome/browser/geo...
File chrome/browser/geolocation/geolocation_infobar_queue_controller_android.cc
(right):

https://chromiumcodereview.appspot.com/11188020/diff/10003/chrome/browser/geo...
chrome/browser/geolocation/geolocation_infobar_queue_controller_android.cc:15:
GeolocationInfoBarQueueControllerAndroid::CreateInfoBarDelegate(
see above, I think this can be made into a simpler factory function for
geolocationinfobardelegate rather than deriving the entire queuecontroller..

https://chromiumcodereview.appspot.com/11188020/diff/10003/chrome/browser/geo...
File chrome/browser/geolocation/geolocation_infobar_queue_controller_android.h
(right):

https://chromiumcodereview.appspot.com/11188020/diff/10003/chrome/browser/geo...
chrome/browser/geolocation/geolocation_infobar_queue_controller_android.h:14: :
public GeolocationInfoBarQueueController {
the queue controller class is pretty heavy weight and does a lot of stuff..
in here, it seems we only need to CreateInfoBarDelegate, but nothing else from
the base class, that is, the method itself only requires the params that are
passed to it.

how about this:
class GeolocationConfirmInfoBarDelegateFactory {
  // Creates a platform-specific geolocation ConfirmInfoBarDelegate.
  static GeolocationConfirmInfoBarDelegate* create(...);
};

the in the .cc file, we just have a simple ifdef to create an instance of the
specific class... how does it sound?

reason: when there's real platform-specific behavior change, such as in the
permission context, it makes sense to to split into a base class and have
different implementations.
but in this case, it's just a matter of creating the right class, so the
overhead is probably not worth it.. wdyt?

https://chromiumcodereview.appspot.com/11188020/diff/10003/chrome/browser/geo...
chrome/browser/geolocation/geolocation_infobar_queue_controller_android.h:20:
private:
nit:
  // GeolocationInfoBarQueueController

https://chromiumcodereview.appspot.com/11188020/diff/10003/chrome/browser/geo...
chrome/browser/geolocation/geolocation_infobar_queue_controller_android.h:28:
const std::string& display_languages) OVERRIDE;
nit:

\n
DISALLOW_COPY_AND_ASSIGN(GeolocationInfoBarQueueControllerAndroid);

[John Knottenbelt, 2012-10-17 13:39:40]

Thanks for review, Marcus!

https://codereview.chromium.org/11188020/diff/10003/chrome/browser/geolocatio...
File chrome/browser/geolocation/geolocation_infobar_queue_controller_android.h
(right):

https://codereview.chromium.org/11188020/diff/10003/chrome/browser/geolocatio...
chrome/browser/geolocation/geolocation_infobar_queue_controller_android.h:14: :
public GeolocationInfoBarQueueController {
Thanks, Marcus - yes I agree, this sounds simpler indeed.

On 2012/10/17 13:17:38, bulach wrote:
> the queue controller class is pretty heavy weight and does a lot of stuff..
> in here, it seems we only need to CreateInfoBarDelegate, but nothing else from
> the base class, that is, the method itself only requires the params that are
> passed to it.
> 
> how about this:
> class GeolocationConfirmInfoBarDelegateFactory {
>   // Creates a platform-specific geolocation ConfirmInfoBarDelegate.
>   static GeolocationConfirmInfoBarDelegate* create(...);
> };
> 
> the in the .cc file, we just have a simple ifdef to create an instance of the
> specific class... how does it sound?
> 
> reason: when there's real platform-specific behavior change, such as in the
> permission context, it makes sense to to split into a base class and have
> different implementations.
> but in this case, it's just a matter of creating the right class, so the
> overhead is probably not worth it.. wdyt?

[Ramya, 2012-10-17 21:43:31]

PTAL

Tests will be added as part of http://codereview.chromium.org/11186010/ once the
android functionality has been fully implemented.

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

http://codereview.chromium.org/11188020/diff/10003/chrome/browser/geolocation...
chrome/browser/geolocation/chrome_geolocation_permission_context.cc:33: #if
!defined(OS_ANDROID)
On 2012/10/17 13:17:38, bulach wrote:
> if you agree with the comment in the other patch, this could be moved to real
a
> factory class, so that this file doesn't know anything about android..

Done.

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

http://codereview.chromium.org/11188020/diff/10003/chrome/browser/geolocation...
chrome/browser/geolocation/chrome_geolocation_permission_context_android.cc:14:
Profile *profile) {
On 2012/10/17 13:17:38, bulach wrote:
> nit: s/Profile *p/Profile* p/

Done.

http://codereview.chromium.org/11188020/diff/10003/chrome/browser/geolocation...
chrome/browser/geolocation/chrome_geolocation_permission_context_android.cc:19:
PrefService *user_prefs) {
On 2012/10/17 13:17:38, bulach wrote:
> nit: s/PrefService *u/PrefService* u/

Done.

http://codereview.chromium.org/11188020/diff/10003/chrome/browser/geolocation...
chrome/browser/geolocation/chrome_geolocation_permission_context_android.cc:62:
ChromeGeolocationPermissionContextAndroid::PermissionDecided(
On 2012/10/17 13:17:38, bulach wrote:
> this seems recursive :) probably remove 53-65 altogether?

Done.

http://codereview.chromium.org/11188020/diff/10003/chrome/browser/geolocation...
File chrome/browser/geolocation/chrome_geolocation_permission_context_android.h
(right):

http://codereview.chromium.org/11188020/diff/10003/chrome/browser/geolocation...
chrome/browser/geolocation/chrome_geolocation_permission_context_android.h:19: 
On 2012/10/17 13:17:38, bulach wrote:
> nit: 
>  // ChromeGeolocationPermissionContext implementation:

Done.

http://codereview.chromium.org/11188020/diff/10003/chrome/browser/geolocation...
chrome/browser/geolocation/chrome_geolocation_permission_context_android.h:36:
virtual GeolocationInfoBarQueueController* CreateQueueController() OVERRIDE;
On 2012/10/17 13:17:38, bulach wrote:
> nit:
> 
> DISALLOW_COPY_AND_ASSIGN(ChromeGeolocationPermissionContextAndroid);

Done.

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

http://codereview.chromium.org/11188020/diff/10003/chrome/browser/geolocation...
chrome/browser/geolocation/geolocation_confirm_infobar_delegate_android.cc:32:
return GeolocationConfirmInfoBarDelegate::Accept();
On 2012/10/17 13:17:38, bulach wrote:
> see above, I think we can remove 31-33..

Done.

http://codereview.chromium.org/11188020/diff/10003/chrome/browser/geolocation...
File chrome/browser/geolocation/geolocation_confirm_infobar_delegate_android.h
(right):

http://codereview.chromium.org/11188020/diff/10003/chrome/browser/geolocation...
chrome/browser/geolocation/geolocation_confirm_infobar_delegate_android.h:25:
virtual bool Accept() OVERRIDE;
On 2012/10/17 13:17:38, bulach wrote:
> Accept() seems unnecessary?

Done.

http://codereview.chromium.org/11188020/diff/10003/chrome/browser/geolocation...
File chrome/browser/geolocation/geolocation_infobar_queue_controller_android.cc
(right):

http://codereview.chromium.org/11188020/diff/10003/chrome/browser/geolocation...
chrome/browser/geolocation/geolocation_infobar_queue_controller_android.cc:15:
GeolocationInfoBarQueueControllerAndroid::CreateInfoBarDelegate(
On 2012/10/17 13:17:38, bulach wrote:
> see above, I think this can be made into a simpler factory function for
> geolocationinfobardelegate rather than deriving the entire queuecontroller..

Done.

http://codereview.chromium.org/11188020/diff/10003/chrome/browser/geolocation...
File chrome/browser/geolocation/geolocation_infobar_queue_controller_android.h
(right):

http://codereview.chromium.org/11188020/diff/10003/chrome/browser/geolocation...
chrome/browser/geolocation/geolocation_infobar_queue_controller_android.h:14: :
public GeolocationInfoBarQueueController {
Done.

[Ramya, 2012-10-18 00:22:31]

bulach, jknotten:
I had to add back GeolocationInfobarQueueControllerAndroid since it is needed
for the next CL http://codereview.chromium.org/11186010/ (WIP) where I pass a
pointer to GoogleLocationSettingsHelper from ChromeGeolocationPermissionContext
since it is needed for GeolocationConfirmInfobarAndroid.

thestig,
Can you please LGTM the chrome/chrome_browser.gypi change

willchan,
Can you please LGTM the chrome/browser/profiles change

jcivelli,
Can you please LGTM the chrome/test change

[willchan no longer on Chromium, 2012-10-18 00:27:35]

lgtm

[Lei Zhang, 2012-10-18 00:32:41]

gypi file lgtm

[John Knottenbelt, 2012-10-18 08:49:11]

Hi Ramya, in order to avoid subclassing the queue controller, the
GeolocationInfobarDelegateAndroid will have to own it's a separate instance of
the GoogleLocationSettingsHelper, which is quite similar to what you had at the
beginning. Sorry about that!

On 2012/10/18 00:22:31, Ramya wrote:
> bulach, jknotten:
> I had to add back GeolocationInfobarQueueControllerAndroid since it is needed
> for the next CL http://codereview.chromium.org/11186010/ (WIP) where I pass a
> pointer to GoogleLocationSettingsHelper from
ChromeGeolocationPermissionContext
> since it is needed for GeolocationConfirmInfobarAndroid.
> 
> thestig,
> Can you please LGTM the chrome/chrome_browser.gypi change
> 
> willchan,
> Can you please LGTM the chrome/browser/profiles change
> 
> jcivelli,
> Can you please LGTM the chrome/test change

[Ramya, 2012-10-19 18:24:58]

jknotten, bulach
PTAL. Sorry forgot to send this out yesterday.

[John Knottenbelt, 2012-10-22 10:03:49]

lgtm. Thanks, Ramya!

[bulach, 2012-10-22 13:01:03]

lgtm, thanks ramya! just nits below, feel free to go ahead once they're
addressed:

http://codereview.chromium.org/11188020/diff/7025/chrome/browser/geolocation/...
File chrome/browser/geolocation/chrome_geolocation_permission_context.h (right):

http://codereview.chromium.org/11188020/diff/7025/chrome/browser/geolocation/...
chrome/browser/geolocation/chrome_geolocation_permission_context.h:21: 
nit: here and in a few classes below, please remove this extra \n before public

http://codereview.chromium.org/11188020/diff/7025/chrome/browser/geolocation/...
File chrome/browser/geolocation/chrome_geolocation_permission_context_factory.h
(right):

http://codereview.chromium.org/11188020/diff/7025/chrome/browser/geolocation/...
chrome/browser/geolocation/chrome_geolocation_permission_context_factory.h:11:
#include
"chrome/browser/geolocation/chrome_geolocation_permission_context_android.h"
nit: remove this from here and add to the .cc file

http://codereview.chromium.org/11188020/diff/7025/chrome/browser/geolocation/...
File chrome/browser/geolocation/geolocation_confirm_infobar_delegate_factory.h
(right):

http://codereview.chromium.org/11188020/diff/7025/chrome/browser/geolocation/...
chrome/browser/geolocation/geolocation_confirm_infobar_delegate_factory.h:10:
#include
"chrome/browser/geolocation/geolocation_confirm_infobar_delegate_android.h"
nit: remove from here and keep it in the .cc file

http://codereview.chromium.org/11188020/diff/7025/chrome/browser/geolocation/...
File chrome/browser/geolocation/geolocation_infobar_queue_controller.cc (right):

http://codereview.chromium.org/11188020/diff/7025/chrome/browser/geolocation/...
chrome/browser/geolocation/geolocation_infobar_queue_controller.cc:284: return
GeolocationConfirmInfoBarDelegateFactory::Create(infobar_helper,
nit: calling code can be more compact, like:
return GeolocationConfirmInfoBarDelegateFactory::Create(
    param_0, param_1, param_2, ...
    param_x, param_y);

[Jay Civelli, 2012-10-22 16:30:58]

LGTM for chrome/tests

[Ramya, 2012-10-22 20:58:03]

http://codereview.chromium.org/11188020/diff/7025/chrome/browser/geolocation/...
File chrome/browser/geolocation/chrome_geolocation_permission_context.h (right):

http://codereview.chromium.org/11188020/diff/7025/chrome/browser/geolocation/...
chrome/browser/geolocation/chrome_geolocation_permission_context.h:21: 
On 2012/10/22 13:01:03, bulach wrote:
> nit: here and in a few classes below, please remove this extra \n before
public

Done.

http://codereview.chromium.org/11188020/diff/7025/chrome/browser/geolocation/...
File chrome/browser/geolocation/chrome_geolocation_permission_context_factory.h
(right):

http://codereview.chromium.org/11188020/diff/7025/chrome/browser/geolocation/...
chrome/browser/geolocation/chrome_geolocation_permission_context_factory.h:11:
#include
"chrome/browser/geolocation/chrome_geolocation_permission_context_android.h"
On 2012/10/22 13:01:03, bulach wrote:
> nit: remove this from here and add to the .cc file

Done.

http://codereview.chromium.org/11188020/diff/7025/chrome/browser/geolocation/...
File chrome/browser/geolocation/geolocation_confirm_infobar_delegate_factory.h
(right):

http://codereview.chromium.org/11188020/diff/7025/chrome/browser/geolocation/...
chrome/browser/geolocation/geolocation_confirm_infobar_delegate_factory.h:10:
#include
"chrome/browser/geolocation/geolocation_confirm_infobar_delegate_android.h"
On 2012/10/22 13:01:03, bulach wrote:
> nit: remove from here and keep it in the .cc file

Done.

http://codereview.chromium.org/11188020/diff/7025/chrome/browser/geolocation/...
File chrome/browser/geolocation/geolocation_infobar_queue_controller.cc (right):

http://codereview.chromium.org/11188020/diff/7025/chrome/browser/geolocation/...
chrome/browser/geolocation/geolocation_infobar_queue_controller.cc:284: return
GeolocationConfirmInfoBarDelegateFactory::Create(infobar_helper,
On 2012/10/22 13:01:03, bulach wrote:
> nit: calling code can be more compact, like:
> return GeolocationConfirmInfoBarDelegateFactory::Create(
>     param_0, param_1, param_2, ...
>     param_x, param_y);

Done.

[commit-bot: I haz the power, 2012-10-22 21:11:27]

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

[commit-bot: I haz the power, 2012-10-22 21:28:05]

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

[commit-bot: I haz the power, 2012-10-23 00:12:01]

Change committed as 163449