Don't clear existing extension-defined preferences and content settings when reloading or updating…

Don't clear existing extension-defined preferences and content settings when reloading or updating an extension.

BUG=284385
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=223124

[falken, 2013-09-06 06:24:36]

Not sure if this was ready for a review, but lgtm. Thanks!

I also verified this fixes the reset when updating an extension.

[Bernhard Bauer, 2013-09-06 07:03:32]

Yes, I think I just forgot to send it out :)

[commit-bot: I haz the power, 2013-09-06 07:04:31]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/bauerb@chromium.org/23694020/8001

[commit-bot: I haz the power, 2013-09-06 07:19:04]

Retried try job too often on chromium_presubmit for step(s) presubmit
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...

[Bernhard Bauer, 2013-09-06 11:26:29]

Jeffrey, can I get an OWNERS review?

[Bernhard Bauer, 2013-09-06 11:26:29]

Jeffrey, can I get an OWNERS review?

[Jeffrey Yasskin, 2013-09-06 21:31:56]

lgtm to avoid timezone delay, with the following comments:

https://codereview.chromium.org/23694020/diff/8001/chrome/browser/extensions/...
File chrome/browser/extensions/api/content_settings/content_settings_apitest.cc
(right):

https://codereview.chromium.org/23694020/diff/8001/chrome/browser/extensions/...
chrome/browser/extensions/api/content_settings/content_settings_apitest.cc:108:
// The settings should not be reset when the extension is reloaded.
Could you also check that an uninstall+reinstall does reset the settings?

https://codereview.chromium.org/23694020/diff/8001/chrome/browser/extensions/...
File chrome/browser/extensions/api/content_settings/content_settings_store.cc
(right):

https://codereview.chromium.org/23694020/diff/8001/chrome/browser/extensions/...
chrome/browser/extensions/api/content_settings/content_settings_store.cc:131:
entry->enabled = is_enabled;
Similarly here, do we need to update |enabled| or DCHECK it?

https://codereview.chromium.org/23694020/diff/8001/chrome/browser/extensions/...
File chrome/browser/extensions/api/preference/preference_apitest.cc (right):

https://codereview.chromium.org/23694020/diff/8001/chrome/browser/extensions/...
chrome/browser/extensions/api/preference/preference_apitest.cc:50: // The
settings should not be reset when the extension is reloaded.
Could you also check that an uninstall+reinstall does reset the settings?

https://codereview.chromium.org/23694020/diff/8001/chrome/browser/extensions/...
File chrome/browser/extensions/extension_pref_value_map.cc (right):

https://codereview.chromium.org/23694020/diff/8001/chrome/browser/extensions/...
chrome/browser/extensions/extension_pref_value_map.cc:116: void
ExtensionPrefValueMap::RegisterExtension(const std::string& ext_id,
This is called from PreferenceAPI::InitExtensionControlledPrefs() and
ExtensionPrefs::FinishExtensionInfoPrefs(). Is the new behavior what we want in
both places?

https://codereview.chromium.org/23694020/diff/8001/chrome/browser/extensions/...
chrome/browser/extensions/extension_pref_value_map.cc:123:
entries_[ext_id]->install_time = install_time;
Using the older install time is probably right in either case. An extension
upgrade probably shouldn't change the users effective preferences.

https://codereview.chromium.org/23694020/diff/8001/chrome/browser/extensions/...
chrome/browser/extensions/extension_pref_value_map.cc:124:
entries_[ext_id]->enabled = is_enabled;
When we re-install an extension, it seems like we'd want to update the enabled
state, unless we're somehow guaranteed that it'll already be up to date. Add a
DCHECK if the second is true?

https://codereview.chromium.org/23694020/diff/8001/chrome/browser/extensions/...
File chrome/browser/extensions/extension_prefs.cc (right):

https://codereview.chromium.org/23694020/diff/8001/chrome/browser/extensions/...
chrome/browser/extensions/extension_prefs.cc:1838: if
(!extension_dict->HasKey(pref_names::kPrefPreferences))
Nit: This might be clearer with a helper function:

template<typename PrefType>
static void InitializeIfNotPresent(DictionaryValue* dict, const std::string&
key) {
  if (!dict->HasKey(key))
    dict->Set(key, new PrefType);
}

https://codereview.chromium.org/23694020/diff/8001/chrome/browser/extensions/...
chrome/browser/extensions/extension_prefs.cc:1838: if
(!extension_dict->HasKey(pref_names::kPrefPreferences))
I double-checked that uninstalling an extension removes the prefs (assuming I
can read code corectly), so this should be an ok change even if users are
getting around buggy extensions by uninstalling and reinstalling them.

[Jeffrey Yasskin, 2013-09-06 21:32:55]

Oh, also please make sure your CL description is clear and complete. "installing
an extension (which implies it's being reloaded)" doesn't look correct.

[Bernhard Bauer, 2013-09-09 21:01:59]

Hmph.

Turns out reloading the extension closes the browser window, which can cause the
message loop to quit prematurely, causing test flakiness. I'll see whether I can
fix that tomorrow.

https://codereview.chromium.org/23694020/diff/8001/chrome/browser/extensions/...
File chrome/browser/extensions/extension_pref_value_map.cc (right):

https://codereview.chromium.org/23694020/diff/8001/chrome/browser/extensions/...
chrome/browser/extensions/extension_pref_value_map.cc:116: void
ExtensionPrefValueMap::RegisterExtension(const std::string& ext_id,
On 2013/09/06 21:31:56, Jeffrey Yasskin wrote:
> This is called from PreferenceAPI::InitExtensionControlledPrefs() and
> ExtensionPrefs::FinishExtensionInfoPrefs(). Is the new behavior what we want
in
> both places?

Yes, PreferenceAPI::InitExtensionControlledPrefs() is called during startup,
where the map should be empty anyway.

https://codereview.chromium.org/23694020/diff/8001/chrome/browser/extensions/...
chrome/browser/extensions/extension_pref_value_map.cc:124:
entries_[ext_id]->enabled = is_enabled;
On 2013/09/06 21:31:56, Jeffrey Yasskin wrote:
> When we re-install an extension, it seems like we'd want to update the enabled
> state, unless we're somehow guaranteed that it'll already be up to date. Add a
> DCHECK if the second is true?

What do you mean by up to date?

[Jeffrey Yasskin, 2013-09-09 21:15:27]

Thanks for looking into the test flakiness. Still lgtm.

https://codereview.chromium.org/23694020/diff/8001/chrome/browser/extensions/...
File chrome/browser/extensions/extension_pref_value_map.cc (right):

https://codereview.chromium.org/23694020/diff/8001/chrome/browser/extensions/...
chrome/browser/extensions/extension_pref_value_map.cc:124:
entries_[ext_id]->enabled = is_enabled;
On 2013/09/09 21:02:03, Bernhard Bauer wrote:
> On 2013/09/06 21:31:56, Jeffrey Yasskin wrote:
> > When we re-install an extension, it seems like we'd want to update the
enabled
> > state, unless we're somehow guaranteed that it'll already be up to date. Add
a
> > DCHECK if the second is true?
> 
> What do you mean by up to date?

Say the extension is disabled, but RegisterExtension() is called with
is_enabled==true. That's what I mean by entries_[ext_id]->enabled being "not up
to date". Now, I think that's impossible because is_enabled is read out of
entries_[ext_id] in these cases, but a DCHECK would make sure that doesn't get
broken in the future.

[Bernhard Bauer, 2013-09-09 21:31:45]

https://codereview.chromium.org/23694020/diff/8001/chrome/browser/extensions/...
File chrome/browser/extensions/extension_pref_value_map.cc (right):

https://codereview.chromium.org/23694020/diff/8001/chrome/browser/extensions/...
chrome/browser/extensions/extension_pref_value_map.cc:124:
entries_[ext_id]->enabled = is_enabled;
On 2013/09/09 21:15:28, Jeffrey Yasskin wrote:
> On 2013/09/09 21:02:03, Bernhard Bauer wrote:
> > On 2013/09/06 21:31:56, Jeffrey Yasskin wrote:
> > > When we re-install an extension, it seems like we'd want to update the
> enabled
> > > state, unless we're somehow guaranteed that it'll already be up to date.
Add
> a
> > > DCHECK if the second is true?
> > 
> > What do you mean by up to date?
> 
> Say the extension is disabled, but RegisterExtension() is called with
> is_enabled==true. That's what I mean by entries_[ext_id]->enabled being "not
up
> to date". Now, I think that's impossible because is_enabled is read out of
> entries_[ext_id] in these cases, but a DCHECK would make sure that doesn't get
> broken in the future.

Gotcha. Yes, that makes sense.

[Bernhard Bauer, 2013-09-10 13:28:18]

Scott, can I get an OWNERS review for in_process_browser_test.cc?

https://codereview.chromium.org/23694020/diff/8001/chrome/browser/extensions/...
File chrome/browser/extensions/api/content_settings/content_settings_store.cc
(right):

https://codereview.chromium.org/23694020/diff/8001/chrome/browser/extensions/...
chrome/browser/extensions/api/content_settings/content_settings_store.cc:131:
entry->enabled = is_enabled;
On 2013/09/06 21:31:56, Jeffrey Yasskin wrote:
> Similarly here, do we need to update |enabled| or DCHECK it?

DCHECKed.

https://codereview.chromium.org/23694020/diff/8001/chrome/browser/extensions/...
File chrome/browser/extensions/api/preference/preference_apitest.cc (right):

https://codereview.chromium.org/23694020/diff/8001/chrome/browser/extensions/...
chrome/browser/extensions/api/preference/preference_apitest.cc:50: // The
settings should not be reset when the extension is reloaded.
On 2013/09/06 21:31:56, Jeffrey Yasskin wrote:
> Could you also check that an uninstall+reinstall does reset the settings?

Done.

[sky, 2013-09-10 15:49:29]

LGTM

[commit-bot: I haz the power, 2013-09-11 21:01:32]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/bauerb@chromium.org/23694020/66001

[commit-bot: I haz the power, 2013-09-11 22:38:23]

Retried try job too often on mac_rel for step(s) browser_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...

[commit-bot: I haz the power, 2013-09-13 18:06:34]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/bauerb@chromium.org/23694020/1004

[commit-bot: I haz the power, 2013-09-13 21:24:21]

Change committed as 223124