Add GeolocationContentSettingsMap, a geolocation-specific variant of HostCont...

Add GeolocationContentSettingsMap, a geolocation-specific variant of HostContentSettingsMap.

This was originally written by bulach and posted at http://codereview.chromium.org/1033004 ; modified and landed by me.

BUG=37206
TEST=Tested by unittests
TBR=bulach,joth

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

[Peter Kasting, 2010-03-19 00:51:11]

Here's my final take on this patch.  Landing once trybots are green.

[bulach, 2010-03-19 10:53:01]

LGTM

thanks for taking over my change Peter!

just one minor question / suggestion below:

http://codereview.chromium.org/1084005/diff/1/9
File chrome/browser/geolocation/geolocation_content_settings_map.h (right):

http://codereview.chromium.org/1084005/diff/1/9#newcode58
chrome/browser/geolocation/geolocation_content_settings_map.h:58:
AllOriginsSettings GetAllOriginsSettings() const;
hmm, I the relationship between "requesting_url" and "embedding_url" as used by
Get/SetContentSettings does not have a direct relationship to
"AllOriginsSettings==>OneOriginSettings" without inspecting the
implementation...

I don't feel strongly about it but would like to suggest two options:
1. expand the comments here to explain the cardinality, something like
[requesting_url:[embedding_url:content_setting]]

2. more drastic, use something like the previous names for the types
(RequestingSettings and EmbeddingSettings) and rename this to
GetAllRequestingSettings() ?

let me know if you agree with any of these, and I'll do this change.

[Peter Kasting, 2010-03-19 17:35:09]

On 2010/03/19 10:53:01, bulach wrote:
> just one minor question / suggestion below:
> 
> http://codereview.chromium.org/1084005/diff/1/9
> File chrome/browser/geolocation/geolocation_content_settings_map.h (right):
> 
> http://codereview.chromium.org/1084005/diff/1/9#newcode58
> chrome/browser/geolocation/geolocation_content_settings_map.h:58:
> AllOriginsSettings GetAllOriginsSettings() const;
> hmm, I the relationship between "requesting_url" and "embedding_url" as used
by
> Get/SetContentSettings does not have a direct relationship to
> "AllOriginsSettings==>OneOriginSettings" without inspecting the
> implementation...

I agree that this is by far the most confusing and contentious part of the
change.  I devoted the most space in my original (lost) writeup to justifying
it.

To summarize what I wrote there (which doesn't answer your question, but just
gives background): the only bit of info one needs in order to grasp this is that
an "origin" refers to an origin that wants geo data.  This is what (I think) a
reader would naively guess anyway.  At that point it's clear that the leaves are
settings related to one requesting origin, and the total collection is the
settings for all requesting origins.  With the old names, my biggest concern was
that the second-level maps sounded like they held all the settings for one
embedding origin, or something like that; the secondary concern was that it
wasn't obvious from the typedef names which one was the more narrowly scoped
object.

> 1. expand the comments here to explain the cardinality, something like
> [requesting_url:[embedding_url:content_setting]]

More comments are never a bad thing, if you think they add clarity.  I'm totally
fine with anything you want to add.

> 2. more drastic, use something like the previous names for the types
> (RequestingSettings and EmbeddingSettings) and rename this to
> GetAllRequestingSettings() ?

These names are slightly better than the old ones.  I still prefer the current
names, because these don't have the "which one is more narrowly scoped" clarity,
and also because in the absence of other context, it's not really obvious what
the phrase "requesting settings" means.

I admit, this is a subjective call.

[joth, 2010-03-19 17:45:54]

On 19 March 2010 17:35, <pkasting@chromium.org> wrote:

> On 2010/03/19 10:53:01, bulach wrote:
>
>> just one minor question / suggestion below:
>>
>
>  http://codereview.chromium.org/1084005/diff/1/9
>> File chrome/browser/geolocation/geolocation_content_settings_map.h
>> (right):
>>
>
>  http://codereview.chromium.org/1084005/diff/1/9#newcode58
>> chrome/browser/geolocation/geolocation_content_settings_map.h:58:
>> AllOriginsSettings GetAllOriginsSettings() const;
>> hmm, I the relationship between "requesting_url" and "embedding_url" as
>> used
>>
> by
>
>> Get/SetContentSettings does not have a direct relationship to
>> "AllOriginsSettings==>OneOriginSettings" without inspecting the
>> implementation...
>>
>
> I agree that this is by far the most confusing and contentious part of the
> change.  I devoted the most space in my original (lost) writeup to
> justifying
> it.
>
> To summarize what I wrote there (which doesn't answer your question, but
> just
> gives background): the only bit of info one needs in order to grasp this is
> that
> an "origin" refers to an origin that wants geo data.  This is what (I
> think) a
> reader would naively guess anyway.  At that point it's clear that the
> leaves are
> settings related to one requesting origin, and the total collection is the
> settings for all requesting origins.  With the old names, my biggest
> concern was
> that the second-level maps sounded like they held all the settings for one
> embedding origin, or something like that; the secondary concern was that it
> wasn't obvious from the typedef names which one was the more narrowly
> scoped
> object.
>
>
>  1. expand the comments here to explain the cardinality, something like
>> [requesting_url:[embedding_url:content_setting]]
>>
>
> More comments are never a bad thing, if you think they add clarity.  I'm
> totally
> fine with anything you want to add.
>
> Yep, I think having a one liner like this next to the main map instance
variable will be great value (I think we could spend for ever discussing the
type names and I'll still end up referring to this when I need to refresh
memory on it)




>  2. more drastic, use something like the previous names for the types
>> (RequestingSettings and EmbeddingSettings) and rename this to
>> GetAllRequestingSettings() ?
>>
>
> These names are slightly better than the old ones.  I still prefer the
> current
> names, because these don't have the "which one is more narrowly scoped"
> clarity,
> and also because in the absence of other context, it's not really obvious
> what
> the phrase "requesting settings" means.
>
> I admit, this is a subjective call.
>
>
> http://codereview.chromium.org/1084005
>