Prevent leaking geolocation permission from Incognito

Prevents leaking geolocation permission from Incognito mode to regular profile.


BUG=51204
TEST=GeolocationBrowserTest.NoLeakFromOffTheRecord

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

[vasilii, 2013-04-03 16:41:09]

Hello,
Please review the changes.

Jonathan: geolocation
Eliot: profiles

[battre, 2013-04-03 16:53:40]

+markusheintz

I am not sure whether we want this way of changing it. You are basically
introducing a second GeolocationPermissionContext that is always empty if you
start a new profile.

For extensions we handle this differently. If an extension wants to query the
value of a preference in incognito mode, the read operation is passed on to the
regular mode. If the extension then writes a value, this write operation is
stored in temporary storage. A subsequent read is answered from that temporary
storage.

+Markus so that we can get a common implementation for all content setting
types.

[markusheintz_, 2013-04-04 11:27:02]

https://codereview.chromium.org/13548003/diff/1/chrome/browser/geolocation/ge...
File chrome/browser/geolocation/geolocation_browsertest.cc (right):

https://codereview.chromium.org/13548003/diff/1/chrome/browser/geolocation/ge...
chrome/browser/geolocation/geolocation_browsertest.cc:476:
SetInfobarResponse(current_url_, false);
Should we also check here that the location can't be accessed?

[markusheintz_, 2013-04-04 11:28:03]

Please add a TEST= line to the description.
Also could you improve the description a bit.

Thanks :)

On 2013/04/04 11:27:02, markusheintz_ wrote:
>
https://codereview.chromium.org/13548003/diff/1/chrome/browser/geolocation/ge...
> File chrome/browser/geolocation/geolocation_browsertest.cc (right):
> 
>
https://codereview.chromium.org/13548003/diff/1/chrome/browser/geolocation/ge...
> chrome/browser/geolocation/geolocation_browsertest.cc:476:
> SetInfobarResponse(current_url_, false);
> Should we also check here that the location can't be accessed?

[markusheintz_, 2013-04-04 11:39:16]

In general this LG2M.

On 2013/04/04 11:28:03, markusheintz_ wrote:
> Please add a TEST= line to the description.
> Also could you improve the description a bit.
> 
> Thanks :)
> 
> On 2013/04/04 11:27:02, markusheintz_ wrote:
> >
>
https://codereview.chromium.org/13548003/diff/1/chrome/browser/geolocation/ge...
> > File chrome/browser/geolocation/geolocation_browsertest.cc (right):
> > 
> >
>
https://codereview.chromium.org/13548003/diff/1/chrome/browser/geolocation/ge...
> > chrome/browser/geolocation/geolocation_browsertest.cc:476:
> > SetInfobarResponse(current_url_, false);
> > Should we also check here that the location can't be accessed?

[vasilii, 2013-04-04 12:11:26]

Dominic, OffTheRecord profile inherits the parent's geo permissions. That
functionality is secured by GeolocationBrowserTest.NoInfobarForOffTheRecord.
ChromeGeolocationPermissionContext is just a thin wrapper around Profile*.

https://codereview.chromium.org/13548003/diff/1/chrome/browser/geolocation/ge...
File chrome/browser/geolocation/geolocation_browsertest.cc (right):

https://codereview.chromium.org/13548003/diff/1/chrome/browser/geolocation/ge...
chrome/browser/geolocation/geolocation_browsertest.cc:476:
SetInfobarResponse(current_url_, false);
On 2013/04/04 11:27:02, markusheintz_ wrote:
> Should we also check here that the location can't be accessed?

Done.

[markusheintz_, 2013-04-04 12:14:46]

On 2013/04/04 12:11:26, vasilii wrote:
> Dominic, OffTheRecord profile inherits the parent's geo permissions. That
> functionality is secured by GeolocationBrowserTest.NoInfobarForOffTheRecord.
> ChromeGeolocationPermissionContext is just a thin wrapper around Profile*.
> 
>
https://codereview.chromium.org/13548003/diff/1/chrome/browser/geolocation/ge...
> File chrome/browser/geolocation/geolocation_browsertest.cc (right):
> 
>
https://codereview.chromium.org/13548003/diff/1/chrome/browser/geolocation/ge...
> chrome/browser/geolocation/geolocation_browsertest.cc:476:
> SetInfobarResponse(current_url_, false);
> On 2013/04/04 11:27:02, markusheintz_ wrote:
> > Should we also check here that the location can't be accessed?
> 
> Done.

LGTM

[Elliot Glaysher, 2013-04-04 16:38:35]

On 2013/04/04 12:11:26, vasilii wrote:
> Dominic, OffTheRecord profile inherits the parent's geo permissions. That
> functionality is secured by GeolocationBrowserTest.NoInfobarForOffTheRecord.
> ChromeGeolocationPermissionContext is just a thin wrapper around Profile*.

lgtm then.

[joth, 2013-04-04 17:16:41]

geolocation/ lgtm rubberstamp

[commit-bot: I haz the power, 2013-04-05 07:26:19]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/vasilii@chromium.org/13548003/9002

[commit-bot: I haz the power, 2013-04-05 10:09:41]

Change committed as 192540