Split incognito extension content settings in session-only and persistent.

Split incognito extension content settings in session-only and persistent.


BUG=71067
TEST=none


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

[Bernhard Bauer, 2011-06-03 02:12:05]

Please review.

For the extension API, we're going to have to make more changes to get access to
incognito content settings before the OffTheRecordProfile exists, as the
HostContentSettingsMap and its providers depend on the profile in a *lot* of
places.

[battre, 2011-06-03 18:48:36]

LGTM with some nits

http://codereview.chromium.org/7088017/diff/2001/chrome/browser/content_setti...
File chrome/browser/content_settings/content_settings_details.h (right):

http://codereview.chromium.org/7088017/diff/2001/chrome/browser/content_setti...
chrome/browser/content_settings/content_settings_details.h:29: bool update_all()
const { return !pattern_.IsValid(); }
should this be renamed to update_all_patterns() given that we have
update_all_types() below?

http://codereview.chromium.org/7088017/diff/2001/chrome/browser/extensions/ex...
File chrome/browser/extensions/extension_content_settings_store.cc (right):

http://codereview.chromium.org/7088017/diff/2001/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_content_settings_store.cc:313:
extension_prefs_scope::kIncognitoPersistent));
This can create duplicated entires in |rules|. Is this ok?

http://codereview.chromium.org/7088017/diff/2001/chrome/browser/extensions/ex...
File chrome/browser/extensions/extension_content_settings_store.h (right):

http://codereview.chromium.org/7088017/diff/2001/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_content_settings_store.h:95: const
ListValue* dict,
nit: |dict| -> |list| (see line 92)

http://codereview.chromium.org/7088017/diff/2001/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_content_settings_store.h:153: static void
AddRules(ContentSettingsType type,
Add a comment?

http://codereview.chromium.org/7088017/diff/2001/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_content_settings_store.h:156: const
ContentSettingSpecList* setting_spec_list);
Order of parameters: You search for a match of |type| and |identifier| in
|setting_spec_list|. Make |setting_spec_list| the third parameter?

http://codereview.chromium.org/7088017/diff/2001/chrome/browser/extensions/ex...
File chrome/browser/extensions/extension_prefs.h (right):

http://codereview.chromium.org/7088017/diff/2001/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_prefs.h:330: // TODO
why TODO?

http://codereview.chromium.org/7088017/diff/2001/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_prefs.h:331: void
ClearIncognitoSessionOnlySettings();
"ClearIncognitoSessionOnly***Content***Settings"?

http://codereview.chromium.org/7088017/diff/2001/chrome/browser/geolocation/g...
File chrome/browser/geolocation/geolocation_content_settings_map_unittest.cc
(right):

http://codereview.chromium.org/7088017/diff/2001/chrome/browser/geolocation/g...
chrome/browser/geolocation/geolocation_content_settings_map_unittest.cc:297: //
Test if a GEOLOCATION_CONTENT_SETTINGS_CHANGED notification is sent after
The event is called "GEOLOCATION_SETTINGS_CHANGED"

http://codereview.chromium.org/7088017/diff/2001/chrome/browser/geolocation/g...
chrome/browser/geolocation/geolocation_content_settings_map_unittest.cc:306: //
Test if a GEOLOCATION_CONTENT_SETTINGS_CHANGED notification is sent after
same here

[Bernhard Bauer, 2011-06-03 20:34:27]

http://codereview.chromium.org/7088017/diff/2001/chrome/browser/extensions/ex...
File chrome/browser/extensions/extension_content_settings_store.cc (right):

http://codereview.chromium.org/7088017/diff/2001/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_content_settings_store.cc:313:
extension_prefs_scope::kIncognitoPersistent));
On 2011/06/03 18:48:36, battre wrote:
> This can create duplicated entires in |rules|. Is this ok?

The entries are going to be sorted (and un-duped) by the HostContentSettingsMap
before showing them e.g. in the UI, but we should put session-only settings
after persistent ones to make sure that they're going to overwrite the others.

http://codereview.chromium.org/7088017/diff/2001/chrome/browser/extensions/ex...
File chrome/browser/extensions/extension_content_settings_store.h (right):

http://codereview.chromium.org/7088017/diff/2001/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_content_settings_store.h:95: const
ListValue* dict,
On 2011/06/03 18:48:36, battre wrote:
> nit: |dict| -> |list| (see line 92)

Done.

http://codereview.chromium.org/7088017/diff/2001/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_content_settings_store.h:153: static void
AddRules(ContentSettingsType type,
On 2011/06/03 18:48:36, battre wrote:
> Add a comment?

Done.

http://codereview.chromium.org/7088017/diff/2001/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_content_settings_store.h:156: const
ContentSettingSpecList* setting_spec_list);
On 2011/06/03 18:48:36, battre wrote:
> Order of parameters: You search for a match of |type| and |identifier| in
> |setting_spec_list|. Make |setting_spec_list| the third parameter?

Good idea. Done.

http://codereview.chromium.org/7088017/diff/2001/chrome/browser/extensions/ex...
File chrome/browser/extensions/extension_prefs.h (right):

http://codereview.chromium.org/7088017/diff/2001/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_prefs.h:330: // TODO
On 2011/06/03 18:48:36, battre wrote:
> why TODO?

As a reminder for me to add a comment, and it worked. Sorta ;-)

http://codereview.chromium.org/7088017/diff/2001/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_prefs.h:331: void
ClearIncognitoSessionOnlySettings();
On 2011/06/03 18:48:36, battre wrote:
> "ClearIncognitoSessionOnly***Content***Settings"?

Done.

http://codereview.chromium.org/7088017/diff/2001/chrome/browser/geolocation/g...
File chrome/browser/geolocation/geolocation_content_settings_map_unittest.cc
(right):

http://codereview.chromium.org/7088017/diff/2001/chrome/browser/geolocation/g...
chrome/browser/geolocation/geolocation_content_settings_map_unittest.cc:297: //
Test if a GEOLOCATION_CONTENT_SETTINGS_CHANGED notification is sent after
On 2011/06/03 18:48:36, battre wrote:
> The event is called "GEOLOCATION_SETTINGS_CHANGED"

Hooray for consistent naming! Done.

[commit-bot: I haz the power, 2011-06-03 22:50:59]

Change committed as 87891