Simplify the geolocation content settings map: it's only ever used from the U...

Simplify the geolocation content settings map: it's only ever used from the UI thread, so no need for it to cache a copy of the prefs values

BUG=none
TEST=GeolocationContentSettingsMapTests.*


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

[bulach, 2010-04-08 15:45:15]

LGTM, nice simplification, thanks!

couple of nits:

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

http://codereview.chromium.org/1525018/diff/5001/6002#newcode88
chrome/browser/geolocation/geolocation_content_settings_map.cc:88:
ReadExceptions(&content_settings);
roll this one here..

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

http://codereview.chromium.org/1525018/diff/5001/6003#newcode24
chrome/browser/geolocation/geolocation_content_settings_map.h:24: #include
"googleurl/src/gurl.h"
I think <utility>, <vector> and "lock" can all go?

[joth, 2010-04-08 15:49:09]

Thanks! It was a useful experiment.

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

http://codereview.chromium.org/1525018/diff/5001/6002#newcode88
chrome/browser/geolocation/geolocation_content_settings_map.cc:88:
ReadExceptions(&content_settings);
On 2010/04/08 15:45:15, bulach wrote:
> roll this one here..

Done.

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

http://codereview.chromium.org/1525018/diff/5001/6003#newcode24
chrome/browser/geolocation/geolocation_content_settings_map.h:24: #include
"googleurl/src/gurl.h"
On 2010/04/08 15:45:15, bulach wrote:
> I think <utility>, <vector> and "lock" can all go?

Done.

[albertb, 2010-04-08 16:48:23]

http://codereview.chromium.org/1525018/diff/13001/1005
File chrome/browser/geolocation/geolocation_content_settings_map.cc (right):

http://codereview.chromium.org/1525018/diff/13001/1005#newcode116
chrome/browser/geolocation/geolocation_content_settings_map.cc:116:
ScopedPrefUpdate update(prefs, prefs::kGeolocationDefaultContentSetting);
You only need ScopedPrefUpdate when updating Dictionaries or Lists. All other
types correctly fire notifications when modified.

http://codereview.chromium.org/1525018/diff/13001/1005#newcode173
chrome/browser/geolocation/geolocation_content_settings_map.cc:173:
ScopedPrefUpdate update(prefs, prefs::kGeolocationDefaultContentSetting);
ClearPref correctly fires a notification, no need for a ScopedPrefUpdate here.

http://codereview.chromium.org/1525018/diff/13001/1005#newcode177
chrome/browser/geolocation/geolocation_content_settings_map.cc:177:
ScopedPrefUpdate update(prefs, prefs::kGeolocationContentSettings);
Same comment.

[joth, 2010-04-08 16:52:27]

Thanks for clarifying that, all fixed.

On 2010/04/08 16:48:23, albertb wrote:
> http://codereview.chromium.org/1525018/diff/13001/1005
> File chrome/browser/geolocation/geolocation_content_settings_map.cc (right):
> 
> http://codereview.chromium.org/1525018/diff/13001/1005#newcode116
> chrome/browser/geolocation/geolocation_content_settings_map.cc:116:
> ScopedPrefUpdate update(prefs, prefs::kGeolocationDefaultContentSetting);
> You only need ScopedPrefUpdate when updating Dictionaries or Lists. All other
> types correctly fire notifications when modified.
> 
> http://codereview.chromium.org/1525018/diff/13001/1005#newcode173
> chrome/browser/geolocation/geolocation_content_settings_map.cc:173:
> ScopedPrefUpdate update(prefs, prefs::kGeolocationDefaultContentSetting);
> ClearPref correctly fires a notification, no need for a ScopedPrefUpdate here.
> 
> http://codereview.chromium.org/1525018/diff/13001/1005#newcode177
> chrome/browser/geolocation/geolocation_content_settings_map.cc:177:
> ScopedPrefUpdate update(prefs, prefs::kGeolocationContentSettings);
> Same comment.

[albertb, 2010-04-08 17:08:34]

LGTM

http://codereview.chromium.org/1525018/diff/17001/10003
File chrome/browser/geolocation/geolocation_content_settings_map.cc (right):

http://codereview.chromium.org/1525018/diff/17001/10003#newcode135
chrome/browser/geolocation/geolocation_content_settings_map.cc:135:
prefs::kGeolocationContentSettings);
This is indented too far.