Adds GeolocationContentSettingsMap.

Adds GeolocationContentSettingsMap.

This class manages the geolocation permission persistence.
It's based on HostContentSettingsMap but it manages only Geolocation, and instead of mapping host => permission, it maps [top level, frame] => permission.
A follow up will wire up with GeolocationPermissionContext.

TEST=geolocation_content_settings_map_unittest.cc

[bulach, 2010-03-17 17:44:31]

Hi Peter,

As we briefly chatted yesterday, this change adds a separate
GeolocationContentSettingsMap (based on HostContentSettingsMap).

In the spirit of trying to keep small changes :) this is not yet hooked up to
the geolocation infrastructure, however it has some unit tests to exercise it.

Would you please take a look and let me know your thoughts on this?

[Peter Kasting, 2010-03-17 22:26:01]

There are a large number of comments below, but for the most part, they are
nits.  This is in pretty good shape overall.  If you're blocked on submitting
it, you can probably submit after addressing things; I only really need to look
again if you decide one of my suggestions is a lousy idea.  So basically, LGTM

http://codereview.chromium.org/1033004/diff/5001/6002
File chrome/browser/geolocation/geolocation_content_settings_map.cc (right):

http://codereview.chromium.org/1033004/diff/5001/6002#newcode20
chrome/browser/geolocation/geolocation_content_settings_map.cc:20:
GeolocationContentSettingsMap::kTypeName = L"geolocation";
Doesn't look like you use this.  It can probably disappear.

http://codereview.chromium.org/1033004/diff/5001/6002#newcode73
chrome/browser/geolocation/geolocation_content_settings_map.cc:73: if (j !=
i->second.end()) {
Nit: No {}

http://codereview.chromium.org/1033004/diff/5001/6002#newcode92
chrome/browser/geolocation/geolocation_content_settings_map.cc:92: if (setting
!= CONTENT_SETTING_DEFAULT) {
Nit: No {}

http://codereview.chromium.org/1033004/diff/5001/6002#newcode102
chrome/browser/geolocation/geolocation_content_settings_map.cc:102: AutoLock
auto_lock(lock_);
Nit: I would probably enclose the first part of this function in its own block,
just to be consistent about scoping the locking as narrowly as possible; it's
not really a big deal though.

http://codereview.chromium.org/1033004/diff/5001/6002#newcode103
chrome/browser/geolocation/geolocation_content_settings_map.cc:103: if ((setting
== CONTENT_SETTING_DEFAULT) || (setting == kDefaultSetting)) {
Nit: No {} (you may find that using ?: is even better), and you can omit the
second half of this conditional.

http://codereview.chromium.org/1033004/diff/5001/6002#newcode113
chrome/browser/geolocation/geolocation_content_settings_map.cc:113: const GURL&
top_level, const GURL& frame, ContentSetting setting) {
Nit: Personally, I think one arg per line is better...

http://codereview.chromium.org/1033004/diff/5001/6002#newcode116
chrome/browser/geolocation/geolocation_content_settings_map.cc:116: GURL
top_level_origin = top_level.GetOrigin();
Should you be checking whether the inputs are valid GURLs?  If this can
eventually get called directly by UI code that doesn't check, then yes,
otherwise maybe document that they must be valid?

http://codereview.chromium.org/1033004/diff/5001/6002#newcode121
chrome/browser/geolocation/geolocation_content_settings_map.cc:121: {
Your current code doesn't delete top-level origins whose frames are all default,
which we want to do (and do in HostContentSettingsMap) to save space in the
prefs (and memory...).  The following logic does this, and might be a little
easier to follow (or not):

if (setting this pref to the default value) {
  if (the top-level origin doesn't exist ||
      the frame origin doesn't exist)
    return;
  if (the top-level origin has one frame) {
    ClearSettingsForTopLevel(top-level origin);
    return;
  }
  delete the frame for this top-level in memory;
  delete the frame for this top-level in the prefs;
  return;
}
if (the top-level origin doesn't exist) {
  create the top-level origin in memory;
  create the top-level origin in the prefs;
}
set the frame permission in memory;
set the frame permission in prefs;

http://codereview.chromium.org/1033004/diff/5001/6002#newcode155
chrome/browser/geolocation/geolocation_content_settings_map.cc:155: {
Nit: No need for this extra enclosing block.

http://codereview.chromium.org/1033004/diff/5001/6002#newcode190
chrome/browser/geolocation/geolocation_content_settings_map.cc:190: void
GeolocationContentSettingsMap::GetFrameSettingsFromDictionary(
Nit: If you want, you can inline this code in the caller, since there's only one
call.  You don't have to.

http://codereview.chromium.org/1033004/diff/5001/6002#newcode199
chrome/browser/geolocation/geolocation_content_settings_map.cc:199:
(*frame_settings)[GURL(WideToUTF8(frame))] = static_cast<ContentSetting>(
Nit: Might be more readable to break after "="

http://codereview.chromium.org/1033004/diff/5001/6002#newcode205
chrome/browser/geolocation/geolocation_content_settings_map.cc:205:
default_content_setting_ = kDefaultSetting;
You need to only do this if the setting is equal to CONTENT_SETTING_DEFAULT, or
you're overwriting the real value.

(Make sure your unittests catch this bug.)

http://codereview.chromium.org/1033004/diff/5001/6003
File chrome/browser/geolocation/geolocation_content_settings_map.h (right):

http://codereview.chromium.org/1033004/diff/5001/6003#newcode7
chrome/browser/geolocation/geolocation_content_settings_map.h:7: //
HostContentSettingsMaps but differs significantly in two aspects:
Nit: Map, singular

http://codereview.chromium.org/1033004/diff/5001/6003#newcode34
chrome/browser/geolocation/geolocation_content_settings_map.h:34: typedef
std::map<GURL, ContentSetting> FrameSettings;
Nit: Maybe "TargetSettings" or "PerEmbedderSettings" or something would be
clearer?  Not sure

http://codereview.chromium.org/1033004/diff/5001/6003#newcode35
chrome/browser/geolocation/geolocation_content_settings_map.h:35: typedef
std::map<GURL, FrameSettings> TopLevelSettings;
Nit: Maybe "OriginSettings" or "PerOriginSettings" would be clearer?  Again, I'm
not sure.

http://codereview.chromium.org/1033004/diff/5001/6004
File chrome/browser/geolocation/geolocation_content_settings_map_unittest.cc
(right):

http://codereview.chromium.org/1033004/diff/5001/6004#newcode15
chrome/browser/geolocation/geolocation_content_settings_map_unittest.cc:15:
GeolocationContentSettingsMapTests()
Nit: Indent 1 from "public:"

http://codereview.chromium.org/1033004/diff/5001/6004#newcode16
chrome/browser/geolocation/geolocation_content_settings_map_unittest.cc:16: :
ui_thread_(ChromeThread::UI, &message_loop_) {}
Nit: Indent 4 from the classname.  I would probably also put the close brace on
the next line (the style guide is a little clear on this particular edge case).

http://codereview.chromium.org/1033004/diff/5001/6004#newcode48
chrome/browser/geolocation/geolocation_content_settings_map_unittest.cc:48:
top_level_0, no_frame, CONTENT_SETTING_ALLOW);
I think the permission for "foo.com on itself" should be saved as
foo.com/foo.com rather than foo.com/(null).  This lets us enforce on the map
side that the provided URLs are valid; is a little easier to read and understand
for people looking at the prefs on disk; and will make the logic that wants to
check permissions a little easier because it can always pass the top-level URL
instead of having to check if it's the same origin as the requesting frame and
pass an empty GURL if so.

(Applies to a few places below too.)

http://codereview.chromium.org/1033004/diff/5001/6004#newcode116
chrome/browser/geolocation/geolocation_content_settings_map_unittest.cc:116:
EXPECT_TRUE(top_level_settings.end() == top_level_settings.find(top_level_0));
Nit: These statements will be a little shorter if you check that obj.count(...)
== 0 (or !=), instead of obj.find(...) == obj.end().

(Similar comments apply below.)

http://codereview.chromium.org/1033004/diff/5001/6010
File chrome/common/pref_names.h (right):

http://codereview.chromium.org/1033004/diff/5001/6010#newcode255
chrome/common/pref_names.h:255: extern const wchar_t
kGeolocationDefaultContentSetting[];
Nit: You may (or may not) want to name this like the host content settings prefs
are named... I don't have a strong opinion (the current names are fine too)

http://codereview.chromium.org/1033004/diff/5001/6011
File chrome/test/testing_profile.h (right):

http://codereview.chromium.org/1033004/diff/5001/6011#newcode196
chrome/test/testing_profile.h:196: geolocation_content_settings_map_ = new
GeolocationContentSettingsMap(
Nit: Might be more readable to linebreak after "="

[bulach, 2010-03-18 13:50:59]

thanks Peter!
all comments addressed (except two where you mentioned not having a strong
opinion).

since there are no major open issues with either design / impl, I'll go ahead
with it so I can move on to other bits and pieces... 

as usual, I'm happy to follow up should you have any further comments on the
latests snapshot.

http://codereview.chromium.org/1033004/diff/5001/6002
File chrome/browser/geolocation/geolocation_content_settings_map.cc (right):

http://codereview.chromium.org/1033004/diff/5001/6002#newcode20
chrome/browser/geolocation/geolocation_content_settings_map.cc:20:
GeolocationContentSettingsMap::kTypeName = L"geolocation";
On 2010/03/17 22:26:01, Peter Kasting wrote:
> Doesn't look like you use this.  It can probably disappear.

Done.

http://codereview.chromium.org/1033004/diff/5001/6002#newcode73
chrome/browser/geolocation/geolocation_content_settings_map.cc:73: if (j !=
i->second.end()) {
On 2010/03/17 22:26:01, Peter Kasting wrote:
> Nit: No {}

Done.

http://codereview.chromium.org/1033004/diff/5001/6002#newcode92
chrome/browser/geolocation/geolocation_content_settings_map.cc:92: if (setting
!= CONTENT_SETTING_DEFAULT) {
On 2010/03/17 22:26:01, Peter Kasting wrote:
> Nit: No {}

Done.

http://codereview.chromium.org/1033004/diff/5001/6002#newcode102
chrome/browser/geolocation/geolocation_content_settings_map.cc:102: AutoLock
auto_lock(lock_);
On 2010/03/17 22:26:01, Peter Kasting wrote:
> Nit: I would probably enclose the first part of this function in its own
block,
> just to be consistent about scoping the locking as narrowly as possible; it's
> not really a big deal though.

Done.

http://codereview.chromium.org/1033004/diff/5001/6002#newcode103
chrome/browser/geolocation/geolocation_content_settings_map.cc:103: if ((setting
== CONTENT_SETTING_DEFAULT) || (setting == kDefaultSetting)) {
On 2010/03/17 22:26:01, Peter Kasting wrote:
> Nit: No {} (you may find that using ?: is even better), and you can omit the
> second half of this conditional.

done with ?:

http://codereview.chromium.org/1033004/diff/5001/6002#newcode113
chrome/browser/geolocation/geolocation_content_settings_map.cc:113: const GURL&
top_level, const GURL& frame, ContentSetting setting) {
On 2010/03/17 22:26:01, Peter Kasting wrote:
> Nit: Personally, I think one arg per line is better...

Done.

http://codereview.chromium.org/1033004/diff/5001/6002#newcode116
chrome/browser/geolocation/geolocation_content_settings_map.cc:116: GURL
top_level_origin = top_level.GetOrigin();
On 2010/03/17 22:26:01, Peter Kasting wrote:
> Should you be checking whether the inputs are valid GURLs?  If this can
> eventually get called directly by UI code that doesn't check, then yes,
> otherwise maybe document that they must be valid?

DCHECKING them and documented in the .h

http://codereview.chromium.org/1033004/diff/5001/6002#newcode121
chrome/browser/geolocation/geolocation_content_settings_map.cc:121: {
On 2010/03/17 22:26:01, Peter Kasting wrote:
> Your current code doesn't delete top-level origins whose frames are all
default,
> which we want to do (and do in HostContentSettingsMap) to save space in the
> prefs (and memory...).  The following logic does this, and might be a little
> easier to follow (or not):
> 
> if (setting this pref to the default value) {
>   if (the top-level origin doesn't exist ||
>       the frame origin doesn't exist)
>     return;
>   if (the top-level origin has one frame) {
>     ClearSettingsForTopLevel(top-level origin);
>     return;
>   }
>   delete the frame for this top-level in memory;
>   delete the frame for this top-level in the prefs;
>   return;
> }
> if (the top-level origin doesn't exist) {
>   create the top-level origin in memory;
>   create the top-level origin in the prefs;
> }
> set the frame permission in memory;
> set the frame permission in prefs;

very good point! implemented as suggested, and added a unit-test.

http://codereview.chromium.org/1033004/diff/5001/6002#newcode155
chrome/browser/geolocation/geolocation_content_settings_map.cc:155: {
On 2010/03/17 22:26:01, Peter Kasting wrote:
> Nit: No need for this extra enclosing block.

Done.

http://codereview.chromium.org/1033004/diff/5001/6002#newcode190
chrome/browser/geolocation/geolocation_content_settings_map.cc:190: void
GeolocationContentSettingsMap::GetFrameSettingsFromDictionary(
On 2010/03/17 22:26:01, Peter Kasting wrote:
> Nit: If you want, you can inline this code in the caller, since there's only
one
> call.  You don't have to.

kept as a separate function (seems a bit clearer than inlining..)

http://codereview.chromium.org/1033004/diff/5001/6002#newcode199
chrome/browser/geolocation/geolocation_content_settings_map.cc:199:
(*frame_settings)[GURL(WideToUTF8(frame))] = static_cast<ContentSetting>(
On 2010/03/17 22:26:01, Peter Kasting wrote:
> Nit: Might be more readable to break after "="

Done.

http://codereview.chromium.org/1033004/diff/5001/6002#newcode205
chrome/browser/geolocation/geolocation_content_settings_map.cc:205:
default_content_setting_ = kDefaultSetting;
On 2010/03/17 22:26:01, Peter Kasting wrote:
> You need to only do this if the setting is equal to CONTENT_SETTING_DEFAULT,
or
> you're overwriting the real value.
> 
> (Make sure your unittests catch this bug.)

removed this altogether and setting it as appropriate in the ctor and
ResetToDefault.

http://codereview.chromium.org/1033004/diff/5001/6003
File chrome/browser/geolocation/geolocation_content_settings_map.h (right):

http://codereview.chromium.org/1033004/diff/5001/6003#newcode7
chrome/browser/geolocation/geolocation_content_settings_map.h:7: //
HostContentSettingsMaps but differs significantly in two aspects:
On 2010/03/17 22:26:01, Peter Kasting wrote:
> Nit: Map, singular

Done.

http://codereview.chromium.org/1033004/diff/5001/6003#newcode34
chrome/browser/geolocation/geolocation_content_settings_map.h:34: typedef
std::map<GURL, ContentSetting> FrameSettings;
On 2010/03/17 22:26:01, Peter Kasting wrote:
> Nit: Maybe "TargetSettings" or "PerEmbedderSettings" or something would be
> clearer?  Not sure

I liked TargetSettings / OriginSettings, it disambiguates the case where there's
no "frame" or it isn't embedded..

http://codereview.chromium.org/1033004/diff/5001/6003#newcode35
chrome/browser/geolocation/geolocation_content_settings_map.h:35: typedef
std::map<GURL, FrameSettings> TopLevelSettings;
On 2010/03/17 22:26:01, Peter Kasting wrote:
> Nit: Maybe "OriginSettings" or "PerOriginSettings" would be clearer?  Again,
I'm
> not sure.

as above, TargetSettings / OriginSettings

http://codereview.chromium.org/1033004/diff/5001/6004
File chrome/browser/geolocation/geolocation_content_settings_map_unittest.cc
(right):

http://codereview.chromium.org/1033004/diff/5001/6004#newcode15
chrome/browser/geolocation/geolocation_content_settings_map_unittest.cc:15:
GeolocationContentSettingsMapTests()
On 2010/03/17 22:26:01, Peter Kasting wrote:
> Nit: Indent 1 from "public:"

Done.

http://codereview.chromium.org/1033004/diff/5001/6004#newcode16
chrome/browser/geolocation/geolocation_content_settings_map_unittest.cc:16: :
ui_thread_(ChromeThread::UI, &message_loop_) {}
On 2010/03/17 22:26:01, Peter Kasting wrote:
> Nit: Indent 4 from the classname.  I would probably also put the close brace
on
> the next line (the style guide is a little clear on this particular edge
case).

Done.

http://codereview.chromium.org/1033004/diff/5001/6004#newcode48
chrome/browser/geolocation/geolocation_content_settings_map_unittest.cc:48:
top_level_0, no_frame, CONTENT_SETTING_ALLOW);
On 2010/03/17 22:26:01, Peter Kasting wrote:
> I think the permission for "foo.com on itself" should be saved as
> http://foo.com/foo.com rather than foo.com/(null).  This lets us enforce on
the map
> side that the provided URLs are valid; is a little easier to read and
understand
> for people looking at the prefs on disk; and will make the logic that wants to
> check permissions a little easier because it can always pass the top-level URL
> instead of having to check if it's the same origin as the requesting frame and
> pass an empty GURL if so.
> 
> (Applies to a few places below too.)

very good point, agreed.
changed the tests, changed the impl to require valid URL, and also renamed to
origin/target so it's a bit clearer that "foo.com on itself" is
"foo.com/foo.com". thanks!

http://codereview.chromium.org/1033004/diff/5001/6004#newcode116
chrome/browser/geolocation/geolocation_content_settings_map_unittest.cc:116:
EXPECT_TRUE(top_level_settings.end() == top_level_settings.find(top_level_0));
On 2010/03/17 22:26:01, Peter Kasting wrote:
> Nit: These statements will be a little shorter if you check that
obj.count(...)
> == 0 (or !=), instead of obj.find(...) == obj.end().
> 
> (Similar comments apply below.)

Done.

http://codereview.chromium.org/1033004/diff/5001/6010
File chrome/common/pref_names.h (right):

http://codereview.chromium.org/1033004/diff/5001/6010#newcode255
chrome/common/pref_names.h:255: extern const wchar_t
kGeolocationDefaultContentSetting[];
On 2010/03/17 22:26:01, Peter Kasting wrote:
> Nit: You may (or may not) want to name this like the host content settings
prefs
> are named... I don't have a strong opinion (the current names are fine too)

I'm not sure I fully understood, you mean using Geolocation as a suffix rather
than a prefix?
since the object itself uses "Geolocation" as a prefix, I kept as it is.
but I don't have a strong opinion either :) happy to change.

http://codereview.chromium.org/1033004/diff/5001/6011
File chrome/test/testing_profile.h (right):

http://codereview.chromium.org/1033004/diff/5001/6011#newcode196
chrome/test/testing_profile.h:196: geolocation_content_settings_map_ = new
GeolocationContentSettingsMap(
On 2010/03/17 22:26:01, Peter Kasting wrote:
> Nit: Might be more readable to linebreak after "="

Done.

[joth, 2010-03-18 14:34:15]

Thanks for implementing this. A few comments, but almost all minor.

http://codereview.chromium.org/1033004/diff/2001/3003
File chrome/browser/geolocation/geolocation_content_settings_map.h (right):

http://codereview.chromium.org/1033004/diff/2001/3003#newcode35
chrome/browser/geolocation/geolocation_content_settings_map.h:35: typedef
std::map<GURL, FrameSettings> TopLevelSettings;
given the comment below, I'd suggest we optimize the storage for the details
dialog case (i.e. map of geolocation client origins, for each one stating which
top-level domains they may be embedded in)

as discussed though, we can rearrange this in a follow up, as the detailed
dialog is being developed

http://codereview.chromium.org/1033004/diff/2001/3003#newcode57
chrome/browser/geolocation/geolocation_content_settings_map.h:57: void
GetTopLevelSettings(TopLevelSettings* top_level_settings) const;
is the intention that this will be used to populate the bubble?
The intention is that the bubble would only show origins of frames that have
actually made a geolocation api call on the current page.
otherwise all pages will show all the origins that may be embedded anywhere on
that toplevel origin, even if the current page has no embedded frames

(likewise for ClearSettingsForToplevel)

This probably means we'll not use this API at all, so a future CL may just
remove it

http://codereview.chromium.org/1033004/diff/5001/6001
File chrome/browser/browser_prefs.cc (right):

http://codereview.chromium.org/1033004/diff/5001/6001#newcode19
chrome/browser/browser_prefs.cc:19: #include
"chrome/browser/geolocation/geolocation_content_settings_map.h"
How about adding
geolocation::RegisterUserPrefs into geolocation_prefs.h
and moving this include there?

http://codereview.chromium.org/1033004/diff/15001/16002
File chrome/browser/geolocation/geolocation_content_settings_map.cc (right):

http://codereview.chromium.org/1033004/diff/15001/16002#newcode28
chrome/browser/geolocation/geolocation_content_settings_map.cc:28:
prefs->GetInteger(prefs::kGeolocationDefaultContentSetting));
maybe load this as an integer and check it's in range before casting?
(perhaps not needed as we don't allow down-grades, but still...)

http://codereview.chromium.org/1033004/diff/15001/16002#newcode39
chrome/browser/geolocation/geolocation_content_settings_map.cc:39: std::wstring
wide_origin(*i);
nit: const wstring&

http://codereview.chromium.org/1033004/diff/15001/16002#newcode44
chrome/browser/geolocation/geolocation_content_settings_map.cc:44:
TargetSettings target_settings;
minor suggestion:
TargetSettings* target_settings =
&origin_settings_[GURL(WideToUTF8(wide_origin))];
GetTargetSettingsFromDictionary(origin_settings_dictionary, target_settings);

http://codereview.chromium.org/1033004/diff/15001/16002#newcode60
chrome/browser/geolocation/geolocation_content_settings_map.cc:60: return
default_content_setting_;
super strictly, we should AutoLock auto_lock(lock_); in this method too
(or be using base::subtle::Acquire_Load etc --- not advised)
 
conversely, if we don't lock here there's no point doing it in
SetDefaultContentSetting

http://codereview.chromium.org/1033004/diff/15001/16002#newcode68
chrome/browser/geolocation/geolocation_content_settings_map.cc:68:
TargetSettings::const_iterator j(i->second.find(target.GetOrigin()));
I'd prefer it if we could avoid having GetOrigin() calls scattered about, given
we already have a single point (renderer side) that defines the url -> host
mapping, we should leave the logic there and have this class work without
additional duplicate transformations.

Alternatively if we intend to move this resonsibilty into here then I'd how
about defining a function GeolocationOriginFromURL that can encapsulate this
mapping (and besides anything else, be an easy string to grep for)

http://codereview.chromium.org/1033004/diff/15001/16002#newcode80
chrome/browser/geolocation/geolocation_content_settings_map.cc:80: {
as this sub-scope runs right to the end of the method, is it needed?

http://codereview.chromium.org/1033004/diff/15001/16002#newcode100
chrome/browser/geolocation/geolocation_content_settings_map.cc:100:
default_content_setting_= setting == CONTENT_SETTING_DEFAULT ?
space after default_content_setting_

http://codereview.chromium.org/1033004/diff/15001/16002#newcode122
chrome/browser/geolocation/geolocation_content_settings_map.cc:122: {
as this sub-scope runs right to the end of the method, is it needed?

http://codereview.chromium.org/1033004/diff/15001/16002#newcode194
chrome/browser/geolocation/geolocation_content_settings_map.cc:194: std::wstring
target(*i);
nit: const&

http://codereview.chromium.org/1033004/diff/15001/16002#newcode199
chrome/browser/geolocation/geolocation_content_settings_map.cc:199:
static_cast<ContentSetting>(setting);
again, check in range? (could have a local fn that asserts & converts)

http://codereview.chromium.org/1033004/diff/15001/16009
File chrome/common/pref_names.cc (right):

http://codereview.chromium.org/1033004/diff/15001/16009#newcode685
chrome/common/pref_names.cc:685: // Dictionary containing the default
Geolocation content setting.
s/Dictionary/Integer/

http://codereview.chromium.org/1033004/diff/15001/16009#newcode691
chrome/common/pref_names.cc:691: 
these are both profile not local state prefs, so move up to above line 352

[Peter Kasting, 2010-03-18 17:01:42]

I dashed off some notes to bulach via IM this morning that I'll copy in here.  I
also skimmed joth's notes and had a few thoughts (also below).

I want to make it clear that it's fine to check in without everything done, and
maybe ask me to do some of the stuff that doesn't get in the first round, if
doing so will unblock work.  None of the items below would take me very long to
tack on.

* We should probably have the "requesting origin" as the top-level dictionary
and first function arg, and the "embedding page" as the nested setting and
second function arg.

* We'll need the "get all requesting origins which have any non-default
settings" call for the Exceptions dialog.

* We shouldn't need a converse "get all origins which have settings for this
particular embedder" call to implement the bubble, because (a) we want to show
what origins are actually using geo data, not everything possible, and (b) this
info should be on the TabContents; see the implementation of the other content
settings.

* We'll need to allow an empty GURL for the "embedding page" case, since an
origin has the following settings:

foo.com [on itself] -- stored as "foo.com, foo.com"
...embedded on bar.com -- stored as "foo.com, bar.com"
...embedded on baz.com -- stored as "foo.com, baz.com"
...embedded on any other site -- stored as "foo.com, <empty string>"

The logic for retrieving a setting will thus be:
Return the match for this pair, if any;
Otherwise, if the origins don't match each other, return the match for the
requesting origin and the empty string, if any;
Otherwise, return the global default.

* Make sure the "origin" for all valid GURLs is non-empty or you'll have clashes
with this "default embedded" value.  Here I am specifically thinking of file://
URLs, say if a web developer is trying to test something locally.  If we get
empty origins for cases like this, we should probably store and check them as
the GURL.spec() value.

[Peter Kasting, 2010-03-18 17:02:55]

Oh yeah one more.  I agree with joth about acquiring the lock around returning
the default value, and any other places we "merely read" stuff.  I'm ambivalent
on sanity-checking the integer values.  For both of these, if you change this
file, it'd be nice to make the parallel change in the HostContentSettingsMap.

[bulach, 2010-03-18 17:13:35]

Hi Peter, Joth,

Thanks again for the comments!

I changed it slightly and added the ability to have an empty embedder, and added
a test for it.

It's unlikely I'll submit this today anyway, so could you please take another
look?

Peter: would you be fine if I do the changes to HostContentSettingsMap as a
follow up once this is in?

http://codereview.chromium.org/1033004/diff/2001/3003
File chrome/browser/geolocation/geolocation_content_settings_map.h (right):

http://codereview.chromium.org/1033004/diff/2001/3003#newcode35
chrome/browser/geolocation/geolocation_content_settings_map.h:35: typedef
std::map<GURL, FrameSettings> TopLevelSettings;
On 2010/03/18 14:34:15, joth wrote:
> given the comment below, I'd suggest we optimize the storage for the details
> dialog case (i.e. map of geolocation client origins, for each one stating
which
> top-level domains they may be embedded in)
> 
> as discussed though, we can rearrange this in a follow up, as the detailed
> dialog is being developed

yep, let's keep this as it is, and if needs be we change it later.

http://codereview.chromium.org/1033004/diff/5001/6001
File chrome/browser/browser_prefs.cc (right):

http://codereview.chromium.org/1033004/diff/5001/6001#newcode19
chrome/browser/browser_prefs.cc:19: #include
"chrome/browser/geolocation/geolocation_content_settings_map.h"
On 2010/03/18 14:34:15, joth wrote:
> How about adding
> geolocation::RegisterUserPrefs into geolocation_prefs.h
> and moving this include there?

hmm, not sure, I think I prefer to keep geolocationcontentsettingsmap
registering its own prefs..

http://codereview.chromium.org/1033004/diff/5001/6003
File chrome/browser/geolocation/geolocation_content_settings_map.h (right):

http://codereview.chromium.org/1033004/diff/5001/6003#newcode34
chrome/browser/geolocation/geolocation_content_settings_map.h:34: typedef
std::map<GURL, ContentSetting> FrameSettings;
On 2010/03/18 13:50:59, bulach wrote:
> On 2010/03/17 22:26:01, Peter Kasting wrote:
> > Nit: Maybe "TargetSettings" or "PerEmbedderSettings" or something would be
> > clearer?  Not sure
> 
> I liked TargetSettings / OriginSettings, it disambiguates the case where
there's
> no "frame" or it isn't embedded..

sorry, after speaking to joth we decided to move things aroud / clarify:
EmbedderSettings and OriginSettings.
(I think I implemented "the other way round" the first time, so it should be
clearer now..)

http://codereview.chromium.org/1033004/diff/15001/16002
File chrome/browser/geolocation/geolocation_content_settings_map.cc (right):

http://codereview.chromium.org/1033004/diff/15001/16002#newcode28
chrome/browser/geolocation/geolocation_content_settings_map.cc:28:
prefs->GetInteger(prefs::kGeolocationDefaultContentSetting));
On 2010/03/18 14:34:15, joth wrote:
> maybe load this as an integer and check it's in range before casting?
> (perhaps not needed as we don't allow down-grades, but still...)

Done.

http://codereview.chromium.org/1033004/diff/15001/16002#newcode39
chrome/browser/geolocation/geolocation_content_settings_map.cc:39: std::wstring
wide_origin(*i);
On 2010/03/18 14:34:15, joth wrote:
> nit: const wstring&

Done.

http://codereview.chromium.org/1033004/diff/15001/16002#newcode44
chrome/browser/geolocation/geolocation_content_settings_map.cc:44:
TargetSettings target_settings;
On 2010/03/18 14:34:15, joth wrote:
> minor suggestion:
> TargetSettings* target_settings =
> &origin_settings_[GURL(WideToUTF8(wide_origin))];
> GetTargetSettingsFromDictionary(origin_settings_dictionary, target_settings);

not exactly like that, but done (and avoided a deep copy).

http://codereview.chromium.org/1033004/diff/15001/16002#newcode60
chrome/browser/geolocation/geolocation_content_settings_map.cc:60: return
default_content_setting_;
On 2010/03/18 14:34:15, joth wrote:
> super strictly, we should AutoLock auto_lock(lock_); in this method too
> (or be using base::subtle::Acquire_Load etc --- not advised)
>  
> conversely, if we don't lock here there's no point doing it in
> SetDefaultContentSetting
> 

added AutoLock

http://codereview.chromium.org/1033004/diff/15001/16002#newcode68
chrome/browser/geolocation/geolocation_content_settings_map.cc:68:
TargetSettings::const_iterator j(i->second.find(target.GetOrigin()));
On 2010/03/18 14:34:15, joth wrote:
> I'd prefer it if we could avoid having GetOrigin() calls scattered about,
given
> we already have a single point (renderer side) that defines the url -> host
> mapping, we should leave the logic there and have this class work without
> additional duplicate transformations.
> 
> Alternatively if we intend to move this resonsibilty into here then I'd how
> about defining a function GeolocationOriginFromURL that can encapsulate this
> mapping (and besides anything else, be an easy string to grep for)
> 

the mapping on the renderer side will have to change anyway, as the conversion
to "host" is lossy. as we chatted, kept as it is, and if we strongly feel about
it, we'd make our own opaque datatype for this.

http://codereview.chromium.org/1033004/diff/15001/16002#newcode80
chrome/browser/geolocation/geolocation_content_settings_map.cc:80: {
On 2010/03/18 14:34:15, joth wrote:
> as this sub-scope runs right to the end of the method, is it needed?

removed.

http://codereview.chromium.org/1033004/diff/15001/16002#newcode100
chrome/browser/geolocation/geolocation_content_settings_map.cc:100:
default_content_setting_= setting == CONTENT_SETTING_DEFAULT ?
On 2010/03/18 14:34:15, joth wrote:
> space after default_content_setting_

Done.

http://codereview.chromium.org/1033004/diff/15001/16002#newcode122
chrome/browser/geolocation/geolocation_content_settings_map.cc:122: {
On 2010/03/18 14:34:15, joth wrote:
> as this sub-scope runs right to the end of the method, is it needed?

Done.

http://codereview.chromium.org/1033004/diff/15001/16002#newcode194
chrome/browser/geolocation/geolocation_content_settings_map.cc:194: std::wstring
target(*i);
On 2010/03/18 14:34:15, joth wrote:
> nit: const&

Done.

http://codereview.chromium.org/1033004/diff/15001/16002#newcode199
chrome/browser/geolocation/geolocation_content_settings_map.cc:199:
static_cast<ContentSetting>(setting);
On 2010/03/18 14:34:15, joth wrote:
> again, check in range? (could have a local fn that asserts & converts)
> 

Done.

http://codereview.chromium.org/1033004/diff/15001/16009
File chrome/common/pref_names.cc (right):

http://codereview.chromium.org/1033004/diff/15001/16009#newcode685
chrome/common/pref_names.cc:685: // Dictionary containing the default
Geolocation content setting.
On 2010/03/18 14:34:15, joth wrote:
> s/Dictionary/Integer/

Done.

http://codereview.chromium.org/1033004/diff/15001/16009#newcode691
chrome/common/pref_names.cc:691: 
On 2010/03/18 14:34:15, joth wrote:
> these are both profile not local state prefs, so move up to above line 352

ahn, good point, I didn't notice the separate section. done.

[joth, 2010-03-18 18:24:01]

On 2010/03/18 17:01:42, Peter Kasting wrote:
> I dashed off some notes to bulach via IM this morning that I'll copy in here. 
I
> also skimmed joth's notes and had a few thoughts (also below).
> 
> I want to make it clear that it's fine to check in without everything done,
and
> maybe ask me to do some of the stuff that doesn't get in the first round, if
> doing so will unblock work.  None of the items below would take me very long
to
> tack on.
> 
> * We should probably have the "requesting origin" as the top-level dictionary
> and first function arg, and the "embedding page" as the nested setting and
> second function arg.
> 
> * We'll need the "get all requesting origins which have any non-default
> settings" call for the Exceptions dialog.
> 
> * We shouldn't need a converse "get all origins which have settings for this
> particular embedder" call to implement the bubble, because (a) we want to show
> what origins are actually using geo data, not everything possible, and (b)
this
> info should be on the TabContents; see the implementation of the other content
> settings.
> 
> * We'll need to allow an empty GURL for the "embedding page" case, since an
> origin has the following settings:
> 
> http://foo.com [on itself] -- stored as "foo.com, foo.com"
> ...embedded on http://bar.com -- stored as "foo.com, bar.com"
> ...embedded on http://baz.com -- stored as "foo.com, baz.com"
> ...embedded on any other site -- stored as "foo.com, <empty string>"
> 
> The logic for retrieving a setting will thus be:
> Return the match for this pair, if any;
> Otherwise, if the origins don't match each other, return the match for the
> requesting origin and the empty string, if any;
> Otherwise, return the global default.
> 
> * Make sure the "origin" for all valid GURLs is non-empty or you'll have
clashes
> with this "default embedded" value.  Here I am specifically thinking of
file://
> URLs, say if a web developer is trying to test something locally.  If we get
> empty origins for cases like this, we should probably store and check them as
> the GURL.spec() value.

Thanks for copying your notes in here, that all makes total sense to me too.

Cheers
Joth

[Peter Kasting, 2010-03-18 20:59:20]

As I mentioned to bulach on IM, I'm going to try to patch this in locally, apply
the changes suggested below, and land, in order to save turnaround time.  I'll
TBR the results.

http://codereview.chromium.org/1033004/diff/26001/27002
File chrome/browser/geolocation/geolocation_content_settings_map.cc (right):

http://codereview.chromium.org/1033004/diff/26001/27002#newcode27
chrome/browser/geolocation/geolocation_content_settings_map.cc:27: int
default_content_settting =
Nit: 3 ts

http://codereview.chromium.org/1033004/diff/26001/27002#newcode66
chrome/browser/geolocation/geolocation_content_settings_map.cc:66: const GURL&
embedder, const GURL& requesting_origin) const {
Nit: We should check that these are both valid.

http://codereview.chromium.org/1033004/diff/26001/27002#newcode74
chrome/browser/geolocation/geolocation_content_settings_map.cc:74:
EmbedderSettings::const_iterator any_embedder(i->second.find(GURL()));
We should only do this if embedder != requesting_origin.  (And we should
unittest for this case.)

http://codereview.chromium.org/1033004/diff/26001/27002#newcode91
chrome/browser/geolocation/geolocation_content_settings_map.cc:91:
default_content_setting_ = setting == CONTENT_SETTING_DEFAULT ?
Nit: Enclosing the test in parens seems slightly more readable.

http://codereview.chromium.org/1033004/diff/26001/27002#newcode120
chrome/browser/geolocation/geolocation_content_settings_map.cc:120:
embedder_internal))
Nit: I'd indent this 4

http://codereview.chromium.org/1033004/diff/26001/27002#newcode122
chrome/browser/geolocation/geolocation_content_settings_map.cc:122: if
(requesting_origin_settings_[requesting_origin_internal].size() == 1U) {
Nit: No need for U here.

http://codereview.chromium.org/1033004/diff/26001/27002#newcode147
chrome/browser/geolocation/geolocation_content_settings_map.cc:147: void
GeolocationContentSettingsMap::ResetToDefault() {
We also need a function to reset the settings for a single requesting origin.

http://codereview.chromium.org/1033004/diff/26001/27003
File chrome/browser/geolocation/geolocation_content_settings_map.h (right):

http://codereview.chromium.org/1033004/diff/26001/27003#newcode48
chrome/browser/geolocation/geolocation_content_settings_map.h:48: // Note: if
|requesting_origin| is a top-level page, it's the same as
Nit: Better wording might be:
"Note: To determine the setting for a top-level page, as opposed to a frame
embedded in a page, pass the page's URL for both arguments."

http://codereview.chromium.org/1033004/diff/26001/27003#newcode52
chrome/browser/geolocation/geolocation_content_settings_map.h:52: ContentSetting
GetContentSetting(const GURL& embedder,
Nit: I think it would make sense to swap these arguments.

http://codereview.chromium.org/1033004/diff/26001/27003#newcode59
chrome/browser/geolocation/geolocation_content_settings_map.h:59: const
RequestingOriginSettings& requesting_origin_settings() const;
I'm a bit uncomfortable with returning a const ref here.  It means that if
someone changes a setting, the caller's object will change, without any explicit
notification.

Furthermore, if you call this on a non-UI thread, then you have thread safety
problems, because the caller can now look into our state without holding a lock.

Because we don't need to worry about speed of execution on this function, I
think the old signature, while more clunky, was better.

http://codereview.chromium.org/1033004/diff/26001/27003#newcode70
chrome/browser/geolocation/geolocation_content_settings_map.h:70: // An empty
|embedder| is considered a wildcard for any embedder, that is,
Nit: "any embedder except the origin itself"

http://codereview.chromium.org/1033004/diff/26001/27003#newcode72
chrome/browser/geolocation/geolocation_content_settings_map.h:72: //
|requesting_origin| and |embedder|.
Nit: "when |embedder| != |requesting_origin| and there's no specific setting for
(|requesting_origin, |embedder|)."

http://codereview.chromium.org/1033004/diff/26001/27003#newcode73
chrome/browser/geolocation/geolocation_content_settings_map.h:73: //
|requesting_origin| must be is_valid().
Nit: "must be a valid GURL."

http://codereview.chromium.org/1033004/diff/26001/27003#newcode77
chrome/browser/geolocation/geolocation_content_settings_map.h:77: void
SetContentSetting(const GURL& embedder,
Nit: As above, I'd reverse these first two args.

http://codereview.chromium.org/1033004/diff/26001/27003#newcode101
chrome/browser/geolocation/geolocation_content_settings_map.h:101: static
ContentSetting ConvertToContentSetting(int content_setting);
If we want this, it should live in content_settings.h.

http://codereview.chromium.org/1033004/diff/26001/27004
File chrome/browser/geolocation/geolocation_content_settings_map_unittest.cc
(right):

http://codereview.chromium.org/1033004/diff/26001/27004#newcode16
chrome/browser/geolocation/geolocation_content_settings_map_unittest.cc:16: :
ui_thread_(ChromeThread::UI, &message_loop_) {
Nit: This still needs to be indented 4 rather than 2.

[Peter Kasting, 2010-03-18 21:53:29]

http://codereview.chromium.org/1033004/diff/26001/27003
File chrome/browser/geolocation/geolocation_content_settings_map.h (right):

http://codereview.chromium.org/1033004/diff/26001/27003#newcode59
chrome/browser/geolocation/geolocation_content_settings_map.h:59: const
RequestingOriginSettings& requesting_origin_settings() const;
On 2010/03/18 20:59:20, Peter Kasting wrote:
> Because we don't need to worry about speed of execution on this function, I
> think the old signature, while more clunky, was better.

Actually, since we have to perform a deep copy anyway... I'm just going to
return by value.  Simple and correct.

[Peter Kasting, 2010-03-19 00:13:47]

Sigh.  I wrote a huge comment here and Rietveld ate it.  I don't have the will
to write the whole thing again, so here's the summary:

I ended up making more changes to the patch in the form of type, argument, and
temporary naming schemes.  The goal was maximum clarity, and I also got a lot of
condensation (mostly in the unittests) as well.  My original comment explained
and justified each change in detail; if you want to know about anything, or
think any changes were bad choices, just ask.

I'll post the final version somewhere soon.

[Peter Kasting, 2010-03-19 00:50:31]

Posted at http://codereview.chromium.org/1084005 .  Closing this.