Refactor ChromeGeolocationPermissionContext

Refactor chrome_geolocation_permission_context.* in preperation for adding platform specific functionality. 
Extract GeolocationConfirmInfobarDelegate into its own class to allow platform specific changes to the geolocation infobar. 
Platform specific changes will be added in an upcoming CL (WIP CL http://codereview.chromium.org/11188020/).

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

[John Knottenbelt, 2012-10-17 12:05:24]

chrome/browser/geolocation ltgm.

Please could you update the description and add a bug number.

Marcus, if you get a chance could you also have a look?

[John Knottenbelt, 2012-10-17 12:08:49]

lgtm

[bulach, 2012-10-17 12:59:33]

thanks ramya and john! lgtm, just nits and suggestions below:

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

http://codereview.chromium.org/11183018/diff/6001/chrome/browser/geolocation/...
chrome/browser/geolocation/chrome_geolocation_permission_context.cc:36: Profile
*profile) {
nit: s/Profile *p/Profile* p/

http://codereview.chromium.org/11183018/diff/6001/chrome/browser/geolocation/...
chrome/browser/geolocation/chrome_geolocation_permission_context.cc:151: case
CONTENT_SETTING_BLOCK :
nit: here and the blocks below, no space before ":"

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

http://codereview.chromium.org/11183018/diff/6001/chrome/browser/geolocation/...
chrome/browser/geolocation/chrome_geolocation_permission_context.h:23: static
ChromeGeolocationPermissionContext* Create(Profile *profile);
nit: s/Profile *p/Profile* p/

however, I just chatted with jknotten, and it feels strange to have this factory
method here, since this class can be derived. our suggestion would be:

- add another file, chrome_geolocation_permission_context_factory.h / .cc
- move this method there
- in the implementation, #include both the android and the regular files
- add #ifdefs in the implementation to create the right one..

http://codereview.chromium.org/11183018/diff/6001/chrome/browser/geolocation/...
chrome/browser/geolocation/chrome_geolocation_permission_context.h:86: Profile*
profile() const { return profile_; }
nit: I think this method and QueueController() should go up in 44, i.e., all non
virtuals before the virtual methods..

also, please add a similar header as 27 before the virtuals:
  // GeolocationPermissionContext implementation:

[Ramya, 2012-10-17 17:38:43]

PTAL

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

http://codereview.chromium.org/11183018/diff/6001/chrome/browser/geolocation/...
chrome/browser/geolocation/chrome_geolocation_permission_context.cc:36: Profile
*profile) {
On 2012/10/17 12:59:33, bulach wrote:
> nit: s/Profile *p/Profile* p/

Done.

http://codereview.chromium.org/11183018/diff/6001/chrome/browser/geolocation/...
chrome/browser/geolocation/chrome_geolocation_permission_context.cc:151: case
CONTENT_SETTING_BLOCK :
On 2012/10/17 12:59:33, bulach wrote:
> nit: here and the blocks below, no space before ":"

Done.

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

http://codereview.chromium.org/11183018/diff/6001/chrome/browser/geolocation/...
chrome/browser/geolocation/chrome_geolocation_permission_context.h:23: static
ChromeGeolocationPermissionContext* Create(Profile *profile);
Agreed. Will make the change in the http://codereview.chromium.org/11188020/
which introduces android implementations.

http://codereview.chromium.org/11183018/diff/6001/chrome/browser/geolocation/...
chrome/browser/geolocation/chrome_geolocation_permission_context.h:86: Profile*
profile() const { return profile_; }
On 2012/10/17 12:59:33, bulach wrote:
> nit: I think this method and QueueController() should go up in 44, i.e., all
non
> virtuals before the virtual methods..
> 
> also, please add a similar header as 27 before the virtuals:
>   // GeolocationPermissionContext implementation:
> 

Done.

[willchan no longer on Chromium, 2012-10-17 18:04:05]

chrome/browser/profiles/ LGTM

FWIW, I think you may want to flesh out the changelist description a bit more.
It doesn't really explain why you're making this change. It looks like just a
refactor. What does it mean to "introduce QueueController"? When you write a new
class, is that because you're refactoring code for cleanliness, or to prepare it
for different features, or to fix a bug, or just because you prefer this new
name? Sorry to be one of those anal commit message kinda fellows :P

Also, as a nit, it'd be great not to have to pass in the full Profile* to the
geolocation code, but just whatever dependencies are actually needed. Profile*
has a pretty wide scope, and it'd be nicer to scope things more tightly. I
understand you're not changing anything here, so definitely not a blocker. Just
a for future reference kinda thing.

[Ramya, 2012-10-17 18:29:55]

Sorry about the CL description. I have updated it to explain the change 
I was intending to make.

[willchan no longer on Chromium, 2012-10-17 18:34:06]

Great, thanks!

On Wed, Oct 17, 2012 at 11:29 AM,  <cramya@chromium.org> wrote:
> Sorry about the CL description. I have updated it to explain the change
> I was intending to make.
>
> http://codereview.chromium.org/11183018/

[Jay Civelli, 2012-10-17 20:25:38]

lgtm

[Nico, 2012-10-17 23:08:49]

chrome_browser.gypi lgtm

[commit-bot: I haz the power, 2012-10-17 23:11:30]

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

[commit-bot: I haz the power, 2012-10-18 01:50:24]

Change committed as 162608