Fix for Bug 50726 "Save extension list and "winning" prefs from extensions"

Fix for Bug 50726 "Save extension list and "winning" prefs from extensions"

This is a continuation of
  http://codereview.chromium.org/4438001/
  http://codereview.chromium.org/4551001/
(see these CLs for previous discussions).
I created a new CL because the approach is significantly different from the previous CLs.

The goal of this patch is to ensure that Chrome maintains the order in which extensions apply their preferences between restarts. This may be an issue if two extensions overwrite each others preferences. Furthermore, it ensures that preferences are persisted to disk between browser restarts. Therefore, previous settings are immediately available when the browser is restarted.

A description of the design has been published https://docs.google.com/a/google.com/document/d/1E_HX_cUpET1gH2gDunGIU1EOywMM6FEOuVU6TlpnSwo/edit?hl=en for review and comments. - Sorry, accessible Google internally only.

Contributed by battre@google.com 

BUG=50726
TEST=none

[battre (please use the other), 2010-11-12 15:50:58]

Hi.

This is a follow-up to http://codereview.chromium.org/4438001/

I created a new issue because the code grew very different from 4438001.

The CL is now feature complete to fix bug 50726. It remains to be discussed
whether the approach is a good one or whether we want to do a major refactoring.
I have written a design doc that gives an overview of the architecture (both
existing and of the solution):
https://docs.google.com/a/google.com/document/d/1E_HX_cUpET1gH2gDunGIU1EOywMM...

If there is a way to make it accessible to the public outside of Google, we can
discuss the patch also in a wider scope (chromium-dev, as suggested by Aaron).

I am looking forward to your comments.

[Bernhard Bauer, 2010-11-12 18:16:57]

http://codereview.chromium.org/4852002/diff/5001/chrome/browser/extensions/ex...
File chrome/browser/extensions/extension_pref_store.cc (right):

http://codereview.chromium.org/4852002/diff/5001/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_pref_store.cc:31: DCHECK(dict_->size()==2);
Use DCHECK_EQ(2, dict->size()) for better error messages.

http://codereview.chromium.org/4852002/diff/5001/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_pref_store.cc:35: {}
Nit: Either both braces on the previous line, or the opening one on the previous
line, and the closing one here. Same for ExtensionStack below.

http://codereview.chromium.org/4852002/diff/5001/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_pref_store.cc:47:
CHECK(dict_->GetString(kIdKey, &string_value));
I think in release builds we try to only crash to avoid something even worse,
like memory corruption. If the "id" key is missing, that points to preference
corruption (either caused by the user, or by us), and we should try to recover
from it.

TL;DR: My gut feeling says DCHECK here.

http://codereview.chromium.org/4852002/diff/5001/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_pref_store.cc:68: return value;  // NULL if
Get failed.
Nit: That comment is unnecessary.

http://codereview.chromium.org/4852002/diff/5001/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_pref_store.cc:188:
(*result).push_back(extension_stack_.get(i).extension_id());
Nit: result->push_back

http://codereview.chromium.org/4852002/diff/5001/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_pref_store.cc:212: if (has_pref) {
Nit: You could inline |has_pref|.

http://codereview.chromium.org/4852002/diff/5001/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_pref_store.cc:261: // If profile_==NULL,
this ExtensionPrefStore is for local-state.
Nit: "If |profile_| is NULL"? In whole sentences, I find prose easier to read
than code. If you want to keep the |==|, please add spaces around it (even code
in comments should keep to the style guide ;-)

http://codereview.chromium.org/4852002/diff/5001/chrome/browser/extensions/ex...
File chrome/browser/extensions/extension_pref_store.h (right):

http://codereview.chromium.org/4852002/diff/5001/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_pref_store.h:43: // In this example,
extension 1 has higher precendence than extension 2.
Nit: "precedence"

http://codereview.chromium.org/4852002/diff/5001/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_pref_store.h:123: void set(const
std::string& key, Value* value);
Nit-ish: Use |Set| and |Get|. Normally we only use hacker_style method names for
accessors (so they're consistent with the field they're accessing). Same for
ExtensionStack.

http://codereview.chromium.org/4852002/diff/5001/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_pref_store.h:183: void LazyInit() const;
Are you sure that declaring this const is the way to go? I wouldn't have a big
problem with making PrefStore::prefs non-const.

http://codereview.chromium.org/4852002/diff/5001/chrome/browser/extensions/ex...
File chrome/browser/extensions/extension_pref_store_unittest.cc (right):

http://codereview.chromium.org/4852002/diff/5001/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_pref_store_unittest.cc:36: :
ui_thread_(BrowserThread::UI, &message_loop_), eps_(NULL), dps_(NULL),
Nit: every parameter on its own line.

http://codereview.chromium.org/4852002/diff/5001/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_pref_store_unittest.cc:40: // Sets up the
test fixture.
Nit: comment unnecessary.

http://codereview.chromium.org/4852002/diff/5001/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_pref_store_unittest.cc:152: NULL, eps_,
NULL, ups_, NULL, dps_);
I wouldn't be too opposed to keeping all parameters on the same line (they're
just a list of |PrefStore|s), but then you should indent the line by four
spaces.

http://codereview.chromium.org/4852002/diff/5001/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_pref_store_unittest.cc:473:
ASSERT_TRUE(eps.ext1 != NULL);
Nit: Just ASSERT_TRUE(eps.ext1);

http://codereview.chromium.org/4852002/diff/5001/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_pref_store_unittest.cc:475: ListValue
*extensionPrefs = new ListValue();
Nit: ListValue* extensionPrefs

[battre (please use the other), 2010-11-12 19:07:09]

Thank you very much, Bernhard. I have addressed the issues, except for

"""
 179 
 180   // Initializes the extension_stack_ with empty dictionaries for all
 181   // extensions registered in the precedence list.
 182   // Declared const for "DictionaryValue* prefs() const"
 183   void LazyInit() const;

Bernhard Bauer 2010/11/12 18:16:57 
Are you sure that declaring this const is the way to go? I wouldn't have a big
problem with making PrefStore::prefs non-const.

(Draft) 2010/11/12 18:41:24 
I leave that up to discussion. I have no strong preference. As prefs() returns a
non-const pointer, it is questionable anyway whether it should be const.
"""

If there are no objections I make prefs() non-const.

http://codereview.chromium.org/4852002/diff/5001/chrome/browser/extensions/ex...
File chrome/browser/extensions/extension_pref_store.cc (right):

http://codereview.chromium.org/4852002/diff/5001/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_pref_store.cc:31: DCHECK(dict_->size()==2);
On 2010/11/12 18:16:57, Bernhard Bauer wrote:
> Use DCHECK_EQ(2, dict->size()) for better error messages.

Done.

http://codereview.chromium.org/4852002/diff/5001/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_pref_store.cc:35: {}
On 2010/11/12 18:16:57, Bernhard Bauer wrote:
> Nit: Either both braces on the previous line, or the opening one on the
previous
> line, and the closing one here. Same for ExtensionStack below.

Done.

http://codereview.chromium.org/4852002/diff/5001/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_pref_store.cc:47:
CHECK(dict_->GetString(kIdKey, &string_value));
On 2010/11/12 18:16:57, Bernhard Bauer wrote:
> I think in release builds we try to only crash to avoid something even worse,
> like memory corruption. If the "id" key is missing, that points to preference
> corruption (either caused by the user, or by us), and we should try to recover
> from it.
> 
> TL;DR: My gut feeling says DCHECK here.

Done. If this fails, there won't be a crash but the issue that preference values
might never be purged even if all extensions are uninstalled. I have converted
this to DCHECK.

http://codereview.chromium.org/4852002/diff/5001/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_pref_store.cc:68: return value;  // NULL if
Get failed.
On 2010/11/12 18:16:57, Bernhard Bauer wrote:
> Nit: That comment is unnecessary.

Done.

http://codereview.chromium.org/4852002/diff/5001/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_pref_store.cc:188:
(*result).push_back(extension_stack_.get(i).extension_id());
On 2010/11/12 18:16:57, Bernhard Bauer wrote:
> Nit: result->push_back

Done.

http://codereview.chromium.org/4852002/diff/5001/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_pref_store.cc:212: if (has_pref) {
On 2010/11/12 18:16:57, Bernhard Bauer wrote:
> Nit: You could inline |has_pref|.

Had it inlined before and found this easier to read. Would keep it unless you
object.

http://codereview.chromium.org/4852002/diff/5001/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_pref_store.cc:261: // If profile_==NULL,
this ExtensionPrefStore is for local-state.
On 2010/11/12 18:16:57, Bernhard Bauer wrote:
> Nit: "If |profile_| is NULL"? In whole sentences, I find prose easier to read
> than code. If you want to keep the |==|, please add spaces around it (even
code
> in comments should keep to the style guide ;-)

Done.

http://codereview.chromium.org/4852002/diff/5001/chrome/browser/extensions/ex...
File chrome/browser/extensions/extension_pref_store.h (right):

http://codereview.chromium.org/4852002/diff/5001/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_pref_store.h:43: // In this example,
extension 1 has higher precendence than extension 2.
On 2010/11/12 18:16:57, Bernhard Bauer wrote:
> Nit: "precedence"

Done.

http://codereview.chromium.org/4852002/diff/5001/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_pref_store.h:123: void set(const
std::string& key, Value* value);
On 2010/11/12 18:16:57, Bernhard Bauer wrote:
> Nit-ish: Use |Set| and |Get|. Normally we only use hacker_style method names
for
> accessors (so they're consistent with the field they're accessing). Same for
> ExtensionStack.

Done.

http://codereview.chromium.org/4852002/diff/5001/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_pref_store.h:183: void LazyInit() const;
On 2010/11/12 18:16:57, Bernhard Bauer wrote:
> Are you sure that declaring this const is the way to go? I wouldn't have a big
> problem with making PrefStore::prefs non-const.

I leave that up to discussion. I have no strong preference. As prefs() returns a
non-const pointer, it is questionable anyway whether it should be const.

http://codereview.chromium.org/4852002/diff/5001/chrome/browser/extensions/ex...
File chrome/browser/extensions/extension_pref_store_unittest.cc (right):

http://codereview.chromium.org/4852002/diff/5001/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_pref_store_unittest.cc:36: :
ui_thread_(BrowserThread::UI, &message_loop_), eps_(NULL), dps_(NULL),
On 2010/11/12 18:16:57, Bernhard Bauer wrote:
> Nit: every parameter on its own line.

Done.

http://codereview.chromium.org/4852002/diff/5001/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_pref_store_unittest.cc:40: // Sets up the
test fixture.
On 2010/11/12 18:16:57, Bernhard Bauer wrote:
> Nit: comment unnecessary.

Done.

http://codereview.chromium.org/4852002/diff/5001/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_pref_store_unittest.cc:152: NULL, eps_,
NULL, ups_, NULL, dps_);
On 2010/11/12 18:16:57, Bernhard Bauer wrote:
> I wouldn't be too opposed to keeping all parameters on the same line (they're
> just a list of |PrefStore|s), but then you should indent the line by four
> spaces.

Done.

http://codereview.chromium.org/4852002/diff/5001/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_pref_store_unittest.cc:473:
ASSERT_TRUE(eps.ext1 != NULL);
On 2010/11/12 18:16:57, Bernhard Bauer wrote:
> Nit: Just ASSERT_TRUE(eps.ext1);

did a grep over the source and ASSERT_TRUE(foo != NULL) seems to be the more
common pattern

http://codereview.chromium.org/4852002/diff/5001/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_pref_store_unittest.cc:475: ListValue
*extensionPrefs = new ListValue();
On 2010/11/12 18:16:57, Bernhard Bauer wrote:
> Nit: ListValue* extensionPrefs

Done.

[Paweł Hajdan Jr., 2010-11-13 11:51:10]

Drive-by with a minor test comment. No need to wait for another review by me.

http://codereview.chromium.org/4852002/diff/12001/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_pref_store_unittest.cc (right):

http://codereview.chromium.org/4852002/diff/12001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_pref_store_unittest.cc:164:
testing::Test::SetUp();
huh? Did you mean TearDown?

[battre (please use the other), 2010-11-15 09:27:07]

Thank you, fixed.

Dominic

http://codereview.chromium.org/4852002/diff/12001/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_pref_store_unittest.cc (right):

http://codereview.chromium.org/4852002/diff/12001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_pref_store_unittest.cc:164:
testing::Test::SetUp();
On 2010/11/13 11:51:10, Paweł Hajdan Jr. wrote:
> huh? Did you mean TearDown?

Done.

[battre (please use the other), 2010-11-15 16:48:05]

Hi.

Mattias and I went to the white board and discussed some strategies for
addressing Aaron's comments raised in
https://docs.google.com/a/google.com/document/d/1E_HX_cUpET1gH2gDunGIU1EOywMM...
.

Basically it is possible to integrate the cached preferences in the tree but it
creates some new dependencies that Mattias was not very happy about. Pam
mentioned that she did not have strong feelings for either approach.

Aaron would you like to do a VC about this? Based on the discussion we had here,
we think it is much easier to talk about this interactively. Would you be
available tomorrow morning? (Currently Calendar is offline, so it difficult to
schedule a VC room)

Best,
Dominic

[Aaron Boodman, 2010-11-15 17:04:17]

VC tomorrow sounds good.

- a

On Mon, Nov 15, 2010 at 8:48 AM,  <battre@google.com> wrote:
> Hi.
>
> Mattias and I went to the white board and discussed some strategies for
> addressing Aaron's comments raised in
>
https://docs.google.com/a/google.com/document/d/1E_HX_cUpET1gH2gDunGIU1EOywMM...
> .
>
> Basically it is possible to integrate the cached preferences in the tree but
> it
> creates some new dependencies that Mattias was not very happy about. Pam
> mentioned that she did not have strong feelings for either approach.
>
> Aaron would you like to do a VC about this? Based on the discussion we had
> here,
> we think it is much easier to talk about this interactively. Would you be
> available tomorrow morning? (Currently Calendar is offline, so it difficult
> to
> schedule a VC room)
>
> Best,
> Dominic
>
>
>
> http://codereview.chromium.org/4852002/
>