Adds GeolocationContentSettings on TabContents.

Adds GeolocationContentSettings on TabContents.

This data structure and the notification flow will be used to populate the location bar icon and its bubble.
(this was originally part of http://codereview.chromium.org/650180)

(landed by joth on http://codereview.chromium.org/1320005)

TEST=geolocation_browsertest.cc

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

LGTM

http://codereview.chromium.org/1143002/diff/1/6
File chrome/browser/tab_contents/tab_contents.cc (right):

http://codereview.chromium.org/1143002/diff/1/6#newcode2039
chrome/browser/tab_contents/tab_contents.cc:2039: // TODO(bulach): rename
OnBlockedContentChange to OnContentSettingsChange.
The comment for OnBlockedContentChange explicitly ties it to
TabContents::IsContentBlocked
would it be better to make a new callback for OnGeolocationSettingsChange tied
to the new geolocation setting map?
Or else we just need can broaden the comment for the delegate callback.

Either way, agree it's best done as a followup.

http://codereview.chromium.org/1143002/diff/1/7
File chrome/browser/tab_contents/tab_contents.h (right):

http://codereview.chromium.org/1143002/diff/1/7#newcode251
chrome/browser/tab_contents/tab_contents.h:251: // Returns the current settings
for geolocation services in this page.
Maybe -
Returns the map of settings per origin that has used the geolocation API on this
page.

[bulach, 2010-03-23 12:25:43]

thanks Joth!

peter, one question for you below, let me know your thoughts:

http://codereview.chromium.org/1143002/diff/1/6
File chrome/browser/tab_contents/tab_contents.cc (right):

http://codereview.chromium.org/1143002/diff/1/6#newcode2039
chrome/browser/tab_contents/tab_contents.cc:2039: // TODO(bulach): rename
OnBlockedContentChange to OnContentSettingsChange.
On 2010/03/23 11:11:11, joth wrote:
> The comment for OnBlockedContentChange explicitly ties it to
> TabContents::IsContentBlocked
> would it be better to make a new callback for OnGeolocationSettingsChange tied
> to the new geolocation setting map?
> Or else we just need can broaden the comment for the delegate callback.
> 
> Either way, agree it's best done as a followup.
> 

hmm, the call to TabContents::IsContentBlocked is now pushed down on to the
ContentSettingBlockedImageModel, so I think we need to rename (and update the
comment).

pkasting, WDYT?

http://codereview.chromium.org/1143002/diff/1/7
File chrome/browser/tab_contents/tab_contents.h (right):

http://codereview.chromium.org/1143002/diff/1/7#newcode251
chrome/browser/tab_contents/tab_contents.h:251: // Returns the current settings
for geolocation services in this page.
On 2010/03/23 11:11:11, joth wrote:
> Maybe -
> Returns the map of settings per origin that has used the geolocation API on
this
> page.

Done.

[brettw, 2010-03-23 16:03:38]

Drive by style nit.

http://codereview.chromium.org/1143002/diff/5001/6005
File chrome/browser/tab_contents/tab_contents.cc (right):

http://codereview.chromium.org/1143002/diff/5001/6005#newcode2037
chrome/browser/tab_contents/tab_contents.cc:2037: const GURL& requesting_origin,
bool allowed) {
Since the args fit, they should be put after the "(", so:

void TabContents::OnGeolocationPermissionSet(const GURL& requesting_origin,
                                             bool allowed) {

The 4-space indent is really only for when they don't fit like this.

[Peter Kasting, 2010-03-23 17:46:20]

As with the other patch I commented on, I really think you should get darin to
look at how all your geolocation stuff works if you haven't already.

http://codereview.chromium.org/1143002/diff/1/6
File chrome/browser/tab_contents/tab_contents.cc (right):

http://codereview.chromium.org/1143002/diff/1/6#newcode2039
chrome/browser/tab_contents/tab_contents.cc:2039: // TODO(bulach): rename
OnBlockedContentChange to OnContentSettingsChange.
On 2010/03/23 12:25:43, bulach wrote:
> pkasting, WDYT?

I'm sorry, I can't understand a thing you guys are saying.  Maybe I'd have to go
review this entire patch to figure out what's going on.

[bulach, 2010-03-23 18:45:18]

Peter: the question is if we should reuse the existing mechanism via
"OnBlockedContentChange" to notify the location bar to refresh its icons, or if
we should introduce another one specifically for geolocation. I'm tempted to
reuse (and rename) rather than introduce yet another coupling between the two,
but I'd appreciate your help looking through this code.

Darin, as peter suggested, would you mind taking a look at this one as well?
Again, I think I may need to give you some background here:
This change was originally made as a whole, and we're in the process of
splitting it up in smaller chunks:
http://codereview.chromium.org/650180/show
(this change is a bit out dated and will not be submitted, but may give you an
overview on where we're heading to..)

Here specifically, we'll store in the tabcontents a mapping between all frames
using geolocation and its permission. This will then be used to provide
information to create the icon and associated bubble in the location bar.

Peter and Glen are working on finalizing the mocks, you may also want to check
with them to get the 'big picture'.

Thanks,
Marcus

On 2010/03/23 17:46:20, Peter Kasting wrote:
> As with the other patch I commented on, I really think you should get darin to
> look at how all your geolocation stuff works if you haven't already.
> 
> http://codereview.chromium.org/1143002/diff/1/6
> File chrome/browser/tab_contents/tab_contents.cc (right):
> 
> http://codereview.chromium.org/1143002/diff/1/6#newcode2039
> chrome/browser/tab_contents/tab_contents.cc:2039: // TODO(bulach): rename
> OnBlockedContentChange to OnContentSettingsChange.
> On 2010/03/23 12:25:43, bulach wrote:
> > pkasting, WDYT?
> 
> I'm sorry, I can't understand a thing you guys are saying.  Maybe I'd have to
go
> review this entire patch to figure out what's going on.

[Use pkasting(at)chromium.org, 2010-03-23 19:19:21]

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

> Peter: the question is if we should reuse the existing mechanism via
> "OnBlockedContentChange" to notify the location bar to refresh its icons,
> or if
> we should introduce another one specifically for geolocation.


In the abstract I don't see what's wrong with using the existing mechanism.


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

[darin (slow to review), 2010-03-23 20:33:00]

This looks to be about implementing support for mapping a (top-frame,
requesting-frame) pair to a ContentSetting value.  I see that you are doing
that specifically for Geolocation.  Wouldn't it be worth it to generalize
HostContentSettingsMap, or to create something similar to it that is
likewise generic?  That way, when we have additional permissions that are
like Geolocation, we can just leverage the generic infrastructure.

-Darin


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

> Peter: the question is if we should reuse the existing mechanism via
> "OnBlockedContentChange" to notify the location bar to refresh its icons,
> or if
> we should introduce another one specifically for geolocation. I'm tempted
> to
> reuse (and rename) rather than introduce yet another coupling between the
> two,
> but I'd appreciate your help looking through this code.
>
> Darin, as peter suggested, would you mind taking a look at this one as
> well?
> Again, I think I may need to give you some background here:
> This change was originally made as a whole, and we're in the process of
> splitting it up in smaller chunks:
> http://codereview.chromium.org/650180/show
> (this change is a bit out dated and will not be submitted, but may give you
> an
> overview on where we're heading to..)
>
> Here specifically, we'll store in the tabcontents a mapping between all
> frames
> using geolocation and its permission. This will then be used to provide
> information to create the icon and associated bubble in the location bar.
>
> Peter and Glen are working on finalizing the mocks, you may also want to
> check
> with them to get the 'big picture'.
>
> Thanks,
> Marcus
>
>
> On 2010/03/23 17:46:20, Peter Kasting wrote:
>
>> As with the other patch I commented on, I really think you should get
>> darin to
>> look at how all your geolocation stuff works if you haven't already.
>>
>
>  http://codereview.chromium.org/1143002/diff/1/6
>> File chrome/browser/tab_contents/tab_contents.cc (right):
>>
>
>  http://codereview.chromium.org/1143002/diff/1/6#newcode2039
>> chrome/browser/tab_contents/tab_contents.cc:2039: // TODO(bulach): rename
>> OnBlockedContentChange to OnContentSettingsChange.
>> On 2010/03/23 12:25:43, bulach wrote:
>> > pkasting, WDYT?
>>
>
>  I'm sorry, I can't understand a thing you guys are saying.  Maybe I'd have
>> to
>>
> go
>
>> review this entire patch to figure out what's going on.
>>
>
>
>
> http://codereview.chromium.org/1143002
>

[Use pkasting(at)chromium.org, 2010-03-23 20:41:47]

On Tue, Mar 23, 2010 at 1:32 PM, Darin Fisher <darin@chromium.org> wrote:

> This looks to be about implementing support for mapping a (top-frame,
> requesting-frame) pair to a ContentSetting value.  I see that you are doing
> that specifically for Geolocation.  Wouldn't it be worth it to generalize
> HostContentSettingsMap, or to create something similar to it that is
> likewise generic?  That way, when we have additional permissions that are
> like Geolocation, we can just leverage the generic infrastructure.


What other settings do you foresee being like geolocation?

That said, I realize that when I tried to think how to add geo support in
the existing classes, I only considered (and rejected) the idea of retaining
the existing API and cramming two urls into one.  I never thought of the
idea of making all the APIs take two URLs, and then internally simply ignore
the second for non-geolocation content types.

This would make a lot of code far simpler because we wouldn't have a
"GeolocationContentSettingsMap" object alongside the HostContentSettingsMap,
a situation which (as I implement more of this UI) I'm finding makes things
quite awkward.

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

[darin (slow to review), 2010-03-24 15:19:06]

On 2010/03/23 20:41:47, pkasting wrote:
> On Tue, Mar 23, 2010 at 1:32 PM, Darin Fisher <mailto:darin@chromium.org>
wrote:
> 
> > This looks to be about implementing support for mapping a (top-frame,
> > requesting-frame) pair to a ContentSetting value.  I see that you are doing
> > that specifically for Geolocation.  Wouldn't it be worth it to generalize
> > HostContentSettingsMap, or to create something similar to it that is
> > likewise generic?  That way, when we have additional permissions that are
> > like Geolocation, we can just leverage the generic infrastructure.
> 
> 
> What other settings do you foresee being like geolocation?

I don't have any specific examples in mind, but it seems clear to me that for
geolocation we basically just want to modify the HostContentSettingsMap to
support an additional input constraint.  I can imagine that we might implement
other web platofrm features that want a similar constraint, for privacy reasons
or what have you.


> That said, I realize that when I tried to think how to add geo support in
> the existing classes, I only considered (and rejected) the idea of retaining
> the existing API and cramming two urls into one.  I never thought of the
> idea of making all the APIs take two URLs, and then internally simply ignore
> the second for non-geolocation content types.
> 
> This would make a lot of code far simpler because we wouldn't have a
> "GeolocationContentSettingsMap" object alongside the HostContentSettingsMap,
> a situation which (as I implement more of this UI) I'm finding makes things
> quite awkward.

Exactly.

I'm OK doing such a change separately from this CL.  This CL LGTM.

-Darin


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

[bulach, 2010-03-24 19:21:10]

thanks everyone! I just merged with another change I had, and included tests for
multiple iframes in this change. joth would you mind taking another quick look?

On 2010/03/24 15:19:06, darin wrote:
> On 2010/03/23 20:41:47, pkasting wrote:
> > On Tue, Mar 23, 2010 at 1:32 PM, Darin Fisher <mailto:darin@chromium.org>
> wrote:
> > 
> > > This looks to be about implementing support for mapping a (top-frame,
> > > requesting-frame) pair to a ContentSetting value.  I see that you are
doing
> > > that specifically for Geolocation.  Wouldn't it be worth it to generalize
> > > HostContentSettingsMap, or to create something similar to it that is
> > > likewise generic?  That way, when we have additional permissions that are
> > > like Geolocation, we can just leverage the generic infrastructure.
> > 
> > 
> > What other settings do you foresee being like geolocation?
> 
> I don't have any specific examples in mind, but it seems clear to me that for
> geolocation we basically just want to modify the HostContentSettingsMap to
> support an additional input constraint.  I can imagine that we might implement
> other web platofrm features that want a similar constraint, for privacy
reasons
> or what have you.
> 
> 
> > That said, I realize that when I tried to think how to add geo support in
> > the existing classes, I only considered (and rejected) the idea of retaining
> > the existing API and cramming two urls into one.  I never thought of the
> > idea of making all the APIs take two URLs, and then internally simply ignore
> > the second for non-geolocation content types.
> > 
> > This would make a lot of code far simpler because we wouldn't have a
> > "GeolocationContentSettingsMap" object alongside the HostContentSettingsMap,
> > a situation which (as I implement more of this UI) I'm finding makes things
> > quite awkward.
> 
> Exactly.
> 
> I'm OK doing such a change separately from this CL.  This CL LGTM.
> 
> -Darin
> 
> 
> > 
> > http://codereview.chromium.org/1143002
> > >>
> > >
> >

[joth, 2010-03-24 19:32:50]

LGTM (just read the test & html changes this time)

http://codereview.chromium.org/1143002/diff/18001/19001
File chrome/browser/geolocation/geolocation_browsertest.cc (right):

http://codereview.chromium.org/1143002/diff/18001/19001#newcode206
chrome/browser/geolocation/geolocation_browsertest.cc:206: iframe0_url_ =
iframe0.iframe_url();
we _could_ do
iframe0_url_ = IFrameLoader(current_browser_, 0).iframe_url();
but as mika would say, that's being just a bit too cute.

http://codereview.chromium.org/1143002/diff/18001/19001#newcode286
chrome/browser/geolocation/geolocation_browsertest.cc:286:
EXPECT_EQ(expected_setting, content_settings[requesting_url]);
calling []  has the side-effect of adding the entry if it didn't exist.
maybe on previous line EXPECT_NE(content_setting.end(),
content_setting.find(requesting_url));

[bulach, 2010-03-24 20:11:52]

thanks joth! clarified one comment below, will go ahead as soon as the tries are
happy.

http://codereview.chromium.org/1143002/diff/5001/6005
File chrome/browser/tab_contents/tab_contents.cc (right):

http://codereview.chromium.org/1143002/diff/5001/6005#newcode2037
chrome/browser/tab_contents/tab_contents.cc:2037: const GURL& requesting_origin,
bool allowed) {
On 2010/03/23 16:03:39, brettw wrote:
> Since the args fit, they should be put after the "(", so:
> 
> void TabContents::OnGeolocationPermissionSet(const GURL& requesting_origin,
>                                              bool allowed) {
> 
> The 4-space indent is really only for when they don't fit like this.

Done.

http://codereview.chromium.org/1143002/diff/18001/19001
File chrome/browser/geolocation/geolocation_browsertest.cc (right):

http://codereview.chromium.org/1143002/diff/18001/19001#newcode206
chrome/browser/geolocation/geolocation_browsertest.cc:206: iframe0_url_ =
iframe0.iframe_url();
On 2010/03/24 19:32:50, joth wrote:
> we _could_ do
> iframe0_url_ = IFrameLoader(current_browser_, 0).iframe_url();
> but as mika would say, that's being just a bit too cute.
> 

I'll keep as it is then..

http://codereview.chromium.org/1143002/diff/18001/19001#newcode286
chrome/browser/geolocation/geolocation_browsertest.cc:286:
EXPECT_EQ(expected_setting, content_settings[requesting_url]);
On 2010/03/24 19:32:50, joth wrote:
> calling []  has the side-effect of adding the entry if it didn't exist.
> maybe on previous line EXPECT_NE(content_setting.end(),
> content_setting.find(requesting_url));

283 already checks that:
EXPECT_EQ(1U, content_settings.count(requesting_url));