Uses GeolocationContentSettingsMap on GeolocationPermissionContext to persist settings.

Uses GeolocationContentSettingsMap on GeolocationPermissionContext to persist settings.

TEST=geolocation_browsertest.cc

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

[joth, 2010-03-22 13:21:19]

Generally LGTM, just my stock grumble about GetOrigin and a couple nits.

http://codereview.chromium.org/1141004/diff/2001/3001
File chrome/browser/geolocation/geolocation_browsertest.cc (right):

http://codereview.chromium.org/1141004/diff/2001/3001#newcode399
chrome/browser/geolocation/geolocation_browsertest.cc:399: // Checks infobar
will be created for regular profile.
incorrect comment?

http://codereview.chromium.org/1141004/diff/2001/3002
File chrome/browser/geolocation/geolocation_permission_context.cc (right):

http://codereview.chromium.org/1141004/diff/2001/3002#newcode135
chrome/browser/geolocation/geolocation_permission_context.cc:135:
requesting_frame.GetOrigin(), embedder.GetOrigin());
the setting map already does GetOrigin. Really need to do it at call site too?
I'd much prefer we clearly define the points that are responsible for it than
sprinkle it everywhere 'just in case'

http://codereview.chromium.org/1141004/diff/2001/3002#newcode142
chrome/browser/geolocation/geolocation_permission_context.cc:142: }
either put 'else' here (and remove returns), or no 'else' on line 139

I'd put:
 } else {  // setting == ask. Prompt the user.

http://codereview.chromium.org/1141004/diff/2001/3002#newcode157
chrome/browser/geolocation/geolocation_permission_context.cc:157:
requesting_frame.GetOrigin(), embedder.GetOrigin(), content_setting);
ditto re. GetOrigin()

[bulach, 2010-03-22 14:13:17]

thanks joth! merged/resolved with the other change, another look please?

http://codereview.chromium.org/1141004/diff/2001/3001
File chrome/browser/geolocation/geolocation_browsertest.cc (right):

http://codereview.chromium.org/1141004/diff/2001/3001#newcode399
chrome/browser/geolocation/geolocation_browsertest.cc:399: // Checks infobar
will be created for regular profile.
On 2010/03/22 13:21:20, joth wrote:
> incorrect comment?

not sure I understood, line 403 checks that infobar is created..

http://codereview.chromium.org/1141004/diff/2001/3002
File chrome/browser/geolocation/geolocation_permission_context.cc (right):

http://codereview.chromium.org/1141004/diff/2001/3002#newcode135
chrome/browser/geolocation/geolocation_permission_context.cc:135:
requesting_frame.GetOrigin(), embedder.GetOrigin());
On 2010/03/22 13:21:20, joth wrote:
> the setting map already does GetOrigin. Really need to do it at call site too?
> I'd much prefer we clearly define the points that are responsible for it than
> sprinkle it everywhere 'just in case'
> 

yeah, good point. removed..

http://codereview.chromium.org/1141004/diff/2001/3002#newcode142
chrome/browser/geolocation/geolocation_permission_context.cc:142: }
On 2010/03/22 13:21:20, joth wrote:
> either put 'else' here (and remove returns), or no 'else' on line 139
> 
> I'd put:
>  } else {  // setting == ask. Prompt the user.

Done.

http://codereview.chromium.org/1141004/diff/2001/3002#newcode157
chrome/browser/geolocation/geolocation_permission_context.cc:157:
requesting_frame.GetOrigin(), embedder.GetOrigin(), content_setting);
On 2010/03/22 13:21:20, joth wrote:
> ditto re. GetOrigin()

Done.

[joth, 2010-03-23 11:20:38]

LGTM - couple remaining nits

http://codereview.chromium.org/1141004/diff/2001/3001
File chrome/browser/geolocation/geolocation_browsertest.cc (right):

http://codereview.chromium.org/1141004/diff/2001/3001#newcode399
chrome/browser/geolocation/geolocation_browsertest.cc:399: // Checks infobar
will be created for regular profile.
On 2010/03/22 14:13:18, bulach wrote:
> On 2010/03/22 13:21:20, joth wrote:
> > incorrect comment?
> 
> not sure I understood, line 403 checks that infobar is created..

Sorry I thought the comment applied to the whole fn.

Maybe
// First, check infobar will be created for regular profile

http://codereview.chromium.org/1141004/diff/2001/3001#newcode403
chrome/browser/geolocation/geolocation_browsertest.cc:403:
SetInfobarResponse(true);
// Response will be persisted

http://codereview.chromium.org/1141004/diff/9001/10002
File chrome/browser/geolocation/geolocation_permission_context.cc (right):

http://codereview.chromium.org/1141004/diff/9001/10002#newcode154
chrome/browser/geolocation/geolocation_permission_context.cc:154: }
ah just remembered it's chrome style not to use the { }
perhaps best to remove the 2 x return then the whole lot can be done without {}

[bulach, 2010-03-23 11:54:22]

thanks joth!
comments addressed, will submit shortly.

http://codereview.chromium.org/1141004/diff/2001/3001
File chrome/browser/geolocation/geolocation_browsertest.cc (right):

http://codereview.chromium.org/1141004/diff/2001/3001#newcode399
chrome/browser/geolocation/geolocation_browsertest.cc:399: // Checks infobar
will be created for regular profile.
On 2010/03/23 11:20:38, joth wrote:
> On 2010/03/22 14:13:18, bulach wrote:
> > On 2010/03/22 13:21:20, joth wrote:
> > > incorrect comment?
> > 
> > not sure I understood, line 403 checks that infobar is created..
> 
> Sorry I thought the comment applied to the whole fn.
> 
> Maybe
> // First, check infobar will be created for regular profile

Done.

http://codereview.chromium.org/1141004/diff/2001/3001#newcode403
chrome/browser/geolocation/geolocation_browsertest.cc:403:
SetInfobarResponse(true);
On 2010/03/23 11:20:38, joth wrote:
> // Response will be persisted

Done.

http://codereview.chromium.org/1141004/diff/2001/3001#newcode403
chrome/browser/geolocation/geolocation_browsertest.cc:403:
SetInfobarResponse(true);
On 2010/03/23 11:20:38, joth wrote:
> // Response will be persisted

Done.

http://codereview.chromium.org/1141004/diff/9001/10002
File chrome/browser/geolocation/geolocation_permission_context.cc (right):

http://codereview.chromium.org/1141004/diff/9001/10002#newcode154
chrome/browser/geolocation/geolocation_permission_context.cc:154: }
On 2010/03/23 11:20:38, joth wrote:
> ah just remembered it's chrome style not to use the { }
> perhaps best to remove the 2 x return then the whole lot can be done without
{}

good catch! done.

[Peter Kasting, 2010-03-23 17:31:06]

This is rather drive-by, but did you guys ever run the general design of the
GeolocationPermissionContext and related bits by darin and/or eroman?

I ask because there's a lot of subtleties in how we use the existing
HostContentSettingsMap and HostZoomMap that I think they're the only qualified
folks to understand, and I want to make sure we don't have any weird bits coming
to bite us later.

If you haven't, please do run these by at least Darin.  I mentioned to him a
couple weeks ago that I'd try to get these into his hands once the functionality
wasn't quite so in flux.

[bulach, 2010-03-23 18:16:19]

Thanks Peter!

Darin: we (myself, jorlow and you) chatted quite a while ago when I was in MTV
about the GeolocationPermissionContext. Back then, we didn't have
"HostContentSettingsMap" and we talked about a more complex design with a memory
cache and a file-based backend for storage.

We're now finally implementing it, however we split the persistence backend into
GeolocationContentSettingsMap, which is based on HostContentSettingsMap, with
the added functionality of mapping [requesting frame:[top level: permission]].

I'm not sure if this is enough background :) so please let me know if you need
any more info. We'd greatly appreciate your help reviewing this change!

On 2010/03/23 17:31:06, Peter Kasting wrote:
> This is rather drive-by, but did you guys ever run the general design of the
> GeolocationPermissionContext and related bits by darin and/or eroman?
> 
> I ask because there's a lot of subtleties in how we use the existing
> HostContentSettingsMap and HostZoomMap that I think they're the only qualified
> folks to understand, and I want to make sure we don't have any weird bits
coming
> to bite us later.
> 
> If you haven't, please do run these by at least Darin.  I mentioned to him a
> couple weeks ago that I'd try to get these into his hands once the
functionality
> wasn't quite so in flux.

[Peter Kasting, 2010-03-23 18:23:11]

On Tue, Mar 23, 2010 at 11:16 AM, <bulach@chromium.org> wrote:

> Darin: we (myself, jorlow and you) chatted quite a while ago when I was in
> MTV
> about the GeolocationPermissionContext. Back then, we didn't have
> "HostContentSettingsMap" and we talked about a more complex design with a
> memory
> cache and a file-based backend for storage.
>
> We're now finally implementing it, however we split the persistence backend
> into
> GeolocationContentSettingsMap, which is based on HostContentSettingsMap,
> with
> the added functionality of mapping [requesting frame:[top level:
> permission]].


For the record: My main concern is that I don't recall us needing a
"permission context" object with the other content settings -- I think we
just stored scoped_refptr<HostContentSettingsMap> objects on the relevant
classes -- so I want to make sure this isn't overdesigned.


> http://codereview.chromium.org/1141004
>

[joth, 2010-03-24 11:48:17]

On 23 March 2010 18:23, Peter Kasting <pkasting@chromium.org> wrote:

> On Tue, Mar 23, 2010 at 11:16 AM, <bulach@chromium.org> wrote:
>
>> Darin: we (myself, jorlow and you) chatted quite a while ago when I was in
>> MTV
>> about the GeolocationPermissionContext. Back then, we didn't have
>> "HostContentSettingsMap" and we talked about a more complex design with a
>> memory
>> cache and a file-based backend for storage.
>>
>> We're now finally implementing it, however we split the persistence
>> backend into
>> GeolocationContentSettingsMap, which is based on HostContentSettingsMap,
>> with
>> the added functionality of mapping [requesting frame:[top level:
>> permission]].
>
>
> For the record: My main concern is that I don't recall us needing a
> "permission context" object with the other content settings -- I think we
> just stored scoped_refptr<HostContentSettingsMap> objects on the relevant
> classes -- so I want to make sure this isn't overdesigned.
>
>
Permissions flow is more involved for geolocation, as we have explicit host
IPC from java script to 'request' permission, and an explicit response IPC
back to the render with the response that needs to be tied to the original
request. In addition, we have some privacy requirements around
non-disclosure of user's location to google location service (GLS) until the
user has agreed to use location services; this will also be handled in the
permissions context. So this class has a solid role (or 2) to justify its
existence.









>  http://codereview.chromium.org/1141004
>>
>
>

[Peter Kasting, 2010-03-24 18:32:05]

On Wed, Mar 24, 2010 at 4:47 AM, Jonathan Dixon <joth@chromium.org> wrote:

>  this class has a solid role (or 2) to justify its existence.
>

SGTM

http://codereview.chromium.org/1141004
>>>
>>

[joth, 2010-03-26 13:53:06]

LGTM.

Lets ship it!

http://codereview.chromium.org/1141004/diff/27001/28002
File chrome/browser/geolocation/geolocation_permission_context.cc (right):

http://codereview.chromium.org/1141004/diff/27001/28002#newcode163
chrome/browser/geolocation/geolocation_permission_context.cc:163:
tab_util::GetTabContentsByID(render_process_id, render_view_id);
should we have a check / if on tab_contents here?

[bulach, 2010-03-26 14:22:04]

thanks joth! reply inline:

http://codereview.chromium.org/1141004/diff/27001/28002
File chrome/browser/geolocation/geolocation_permission_context.cc (right):

http://codereview.chromium.org/1141004/diff/27001/28002#newcode163
chrome/browser/geolocation/geolocation_permission_context.cc:163:
tab_util::GetTabContentsByID(render_process_id, render_view_id);
On 2010/03/26 13:53:06, joth wrote:
> should we have a check / if on tab_contents here?

I don't think it's needed: this is called by the infobar whose live is shorter
than the tabcontents (afaict).

I'm landing this change now, happy to follow up any other comments from anyone
else.

[TVL, 2010-03-26 15:24:16]

drive by

http://codereview.chromium.org/1141004/diff/27001/28001
File chrome/browser/geolocation/geolocation_browsertest.cc (right):

http://codereview.chromium.org/1141004/diff/27001/28001#newcode443
chrome/browser/geolocation/geolocation_browsertest.cc:443: #define
MAYBE_InfobarForOffTheRecord InfobarForOffTheRecord
I think these should all be "NoInfo...", they don't actually map to the test
below right now.