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 redesign for http://codereview.chromium.org/4852002/ following discussions.

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. 

This CL requires http://codereview.chromium.org/5204006/

Contributed by battre@google.com 

BUG=50726
TEST=none

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

[battre (please use the other), 2010-11-18 14:00:01]

First version of the CL

[Mattias Nissler (ping if slow), 2010-11-18 16:30:32]

A few initial comments, didn't make it through the whole patch. I'll do that
later or tomorrow.

http://codereview.chromium.org/5213002/diff/36001/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_prefs.cc (right):

http://codereview.chromium.org/5213002/diff/36001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_prefs.cc:86: // extension.
Maybe add "The value is a dictionary mapping preference keys to the values
configured by the extension."

http://codereview.chromium.org/5213002/diff/36001/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_prefs.h (right):

http://codereview.chromium.org/5213002/diff/36001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_prefs.h:30: //       global preferences that
an individual extension wants to set
Maybe not that these are arbitrary preferences (from other subsystems) that the
extension wants to override.

http://codereview.chromium.org/5213002/diff/36001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_prefs.h:31: //
Remove newline.

http://codereview.chromium.org/5213002/diff/36001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_prefs.h:220: // For unit testing
Missing period.

And I think this deserves a more elaborate comment that explains why we would
want to ask the ExtensionPrefs object for a timestamp.

http://codereview.chromium.org/5213002/diff/36001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_prefs.h:287: base::Time GetInstallTime(const
DictionaryValue* extension) const;
Why is that parameter a DictionaryValue*? I'd expect an extension ID.

http://codereview.chromium.org/5213002/diff/36001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_prefs.h:289: // Installs the persistent
extension preferences into prefs_'s extension
|pref_|'s

http://codereview.chromium.org/5213002/diff/36001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_prefs.h:295: const Value*
WinningExtensionControlledPrefValue(const std::string& key)
Again, why a Value* and not an extension ID?

http://codereview.chromium.org/5213002/diff/36001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_prefs.h:298: // Store the
WinningExtensionControlledPrefValue in the PrefValueStore's
what value? ;) Maybe say it in prose :)

[battre (please use the other), 2010-11-18 16:47:33]

Addressed the first set of comments on the new implementation of this feature.

http://codereview.chromium.org/5213002/diff/36001/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_prefs.cc (right):

http://codereview.chromium.org/5213002/diff/36001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_prefs.cc:86: // extension.
On 2010/11/18 16:30:33, Mattias Nissler wrote:
> Maybe add "The value is a dictionary mapping preference keys to the values
> configured by the extension."

Done.

http://codereview.chromium.org/5213002/diff/36001/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_prefs.h (right):

http://codereview.chromium.org/5213002/diff/36001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_prefs.h:30: //       global preferences that
an individual extension wants to set
On 2010/11/18 16:30:33, Mattias Nissler wrote:
> Maybe not that these are arbitrary preferences (from other subsystems) that
the
> extension wants to override.

Done.

http://codereview.chromium.org/5213002/diff/36001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_prefs.h:31: //
On 2010/11/18 16:30:33, Mattias Nissler wrote:
> Remove newline.

Done.

http://codereview.chromium.org/5213002/diff/36001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_prefs.h:220: // For unit testing
On 2010/11/18 16:30:33, Mattias Nissler wrote:
> Missing period.
> 
> And I think this deserves a more elaborate comment that explains why we would
> want to ask the ExtensionPrefs object for a timestamp.

Done.

http://codereview.chromium.org/5213002/diff/36001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_prefs.h:287: base::Time GetInstallTime(const
DictionaryValue* extension) const;
On 2010/11/18 16:30:33, Mattias Nissler wrote:
> Why is that parameter a DictionaryValue*? I'd expect an extension ID.
It is a simple helper method and we need to compare DictionaryValue* elements by
their time. I could add a function std::string GetExtensionId(const
DicationaryValue* extension) and pass the return value to the new
GetInstallTime(std::string) function. But would you recommend to do that?

http://codereview.chromium.org/5213002/diff/36001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_prefs.h:289: // Installs the persistent
extension preferences into prefs_'s extension
On 2010/11/18 16:30:33, Mattias Nissler wrote:
> |pref_|'s

Done.

http://codereview.chromium.org/5213002/diff/36001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_prefs.h:295: const Value*
WinningExtensionControlledPrefValue(const std::string& key)
On 2010/11/18 16:30:33, Mattias Nissler wrote:
> Again, why a Value* and not an extension ID?

this time it is "...Controlle*d*PrefValue", not "Controll*ing*PrefValue". We
could add more indirections. Ask for the winning extension id and then based on
this on its value. Do you prefer this?

http://codereview.chromium.org/5213002/diff/36001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_prefs.h:298: // Store the
WinningExtensionControlledPrefValue in the PrefValueStore's
On 2010/11/18 16:30:33, Mattias Nissler wrote:
> what value? ;) Maybe say it in prose :)

Done.

[Mattias Nissler (ping if slow), 2010-11-19 10:36:12]

More comments, still working :)

http://codereview.chromium.org/5213002/diff/1022/chrome/browser/extensions/ex...
File chrome/browser/extensions/extension_prefs.cc (right):

http://codereview.chromium.org/5213002/diff/1022/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_prefs.cc:571: 
Remove blank line and move the two code lines before the comment.

http://codereview.chromium.org/5213002/diff/1022/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_prefs.cc:614: for
(std::vector<std::string>::iterator i = prefKeys.begin();
It might be useful to have a typedef std::vector<std::string> PrefKeySet in the
class declaration so you don't have to spell it out each time.

http://codereview.chromium.org/5213002/diff/1022/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_prefs.cc:617: }
You have this exact loop earlier in the code, maybe it's useful to factor it
into a UpdateExtensionControlledPrefsForExtension function?

http://codereview.chromium.org/5213002/diff/1022/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_prefs.cc:968: return base::Time::Now();
How does that implementation allow to inject a clock? The GetCurrentTime()
function is not virtual, so you cannot override.

http://codereview.chromium.org/5213002/diff/1022/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_prefs.cc:979: << "Error parsing installation
time of an extension";
period/exclamation mark?

http://codereview.chromium.org/5213002/diff/1022/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_prefs.cc:989: 
No blank line. The rationale is that you're commenting on the code that follows,
so separating the two doesn't make much sense.

http://codereview.chromium.org/5213002/diff/1022/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_prefs.cc:994: std::vector<DictionaryValue*>
sorted_extensions;
How about just using a set with the right comparator?

http://codereview.chromium.org/5213002/diff/1022/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_prefs.cc:1010: SavePrefsAndNotify();
Can we call this after we're done, i.e. outside the loop?

http://codereview.chromium.org/5213002/diff/1022/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_prefs.cc:1015: }
Wouldn't it be more obvious to do the defaulting later? When reading the code
below, I'd think "Wait, what if the dictionary doesn't exist?". So I believe
putting the check there is more understandable.

http://codereview.chromium.org/5213002/diff/1022/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_prefs.cc:1017: // Currently we sort only by
time, no tie-breaker.
Implement sorting inline? Please don't :) And please also add the tie breaker
(e.g. using the extension id).

http://codereview.chromium.org/5213002/diff/1022/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_prefs.cc:1028: scoped_ptr<DictionaryValue>
merged_non_expanded(new DictionaryValue);
why is this a scoped_ptr instead of a plain DictionaryValue var?

http://codereview.chromium.org/5213002/diff/1022/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_prefs.cc:1052: namespace subtle {
you can just use an anonymous namespace here, I that's more common under
chrome/. But then again, since that function has static linkage, you don't need
the namespace at all.

http://codereview.chromium.org/5213002/diff/1022/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_prefs.cc:1056: return a->Equals(b);
this can be rewritten as a == NULL ? b == NULL : a->Equals(b). YMMV though :) I
believe it should really be a static member in Value.

http://codereview.chromium.org/5213002/diff/1022/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_prefs.cc:1060: const Value*
ExtensionPrefs::WinningExtensionControlledPrefValue(
It bothers me a bit that you have the logic that merges down the value in two
different places, i.e. InstallExtensionControlledPreferences and also here. I
don't have good suggestion how to share code other than keeping the dictionaries
in the correct order in memory and merging every time. So I guess I'm OK with
your approach.

http://codereview.chromium.org/5213002/diff/1022/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_prefs.cc:1083: }
No need for curlies.

http://codereview.chromium.org/5213002/diff/1022/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_prefs.cc:1114: pref_key.c_str(),
PrefNotifier::EXTENSION_STORE);
hm, 1103 to here really looks like it should be a method on the pref store.
Problem with that is the store doesn't have access to the pref_notifier. Maybe
we should really provide the stores with the pref_notifier on construction.
WDYT? Put a TODO for now and fix it in a separate patch?

http://codereview.chromium.org/5213002/diff/1022/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_prefs.cc:1125: bool modified =
subtle::equalValues(oldValue, value);
if (!modified)
  return;

http://codereview.chromium.org/5213002/diff/1022/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_prefs.cc:1133:
pref_service()->ScheduleSavePersistentPrefs();
You don't need to do that explicitly, the browser will on shutdown.

http://codereview.chromium.org/5213002/diff/1022/chrome/browser/extensions/ex...
File chrome/browser/extensions/extension_prefs_unittest.cc (right):

http://codereview.chromium.org/5213002/diff/1022/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_prefs_unittest.cc:67: class MockObserver :
public NotificationObserver {
Drop this in favor of chrome/test/mock_notification_observer.h

http://codereview.chromium.org/5213002/diff/1022/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_prefs_unittest.cc:211: // dictionaries (they
contain only the blacklist-property).
Triggering error messages in unit_tests is a common thing to happen, so if they
are really legitimate at this point, you don't need to take special action to
shut them up.

http://codereview.chromium.org/5213002/diff/1022/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_prefs_unittest.cc:397: // Use constants to
avoid confusing std::map with hard-coded strings.
I don't understand? Constants are good, but not for the reason you're giving.

http://codereview.chromium.org/5213002/diff/1022/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_prefs_unittest.cc:407: const char
kDefaultPref4[] = "default pref 4";
These declarations should be moved to the top of the file IMHO. But please
consider the style guide for an authoritive answer.

http://codereview.chromium.org/5213002/diff/1022/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_prefs_unittest.cc:448: void
InstallExtControlledPref(Extension *ext,
Instead of shortening to Ext, why not just InstallPref? The context should be
given be the test class name anyway.

http://codereview.chromium.org/5213002/diff/1022/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_prefs_unittest.cc:450: Value* val) {
fix indentation.

http://codereview.chromium.org/5213002/diff/1022/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_prefs_unittest.cc:471: Extension* ext3;
Missing trailing _

http://codereview.chromium.org/5213002/diff/1022/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_prefs_unittest.cc:511: class
ExtensionPrefsInstallOverwrittenExtensions
I could argue that this test case covers also what the previous one tests, so
maybe remove the previous one?

http://codereview.chromium.org/5213002/diff/1022/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_prefs_unittest.cc:664: // Tests uninstalling
an extension that was winning for only some preferences.
Update comment.

http://codereview.chromium.org/5213002/diff/1022/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_prefs_unittest.cc:732: 
No trailing newlines please.

[Mattias Nissler (ping if slow), 2010-11-19 10:47:39]

some more comments.

http://codereview.chromium.org/5213002/diff/47002/chrome/browser/extensions/t...
File chrome/browser/extensions/test_extension_prefs.cc (right):

http://codereview.chromium.org/5213002/diff/47002/chrome/browser/extensions/t...
chrome/browser/extensions/test_extension_prefs.cc:30: base::Time
GetCurrentTime() const {
note my other comment about GetCurrentTime() not being virtual, so this is no
override.

http://codereview.chromium.org/5213002/diff/47002/chrome/browser/prefs/in_mem...
File chrome/browser/prefs/in_memory_pref_store.h (right):

http://codereview.chromium.org/5213002/diff/47002/chrome/browser/prefs/in_mem...
chrome/browser/prefs/in_memory_pref_store.h:15: // preference values set when a
preference is registered with the PrefService.
Drop the default pref store comment. Perhaps say that it's used for the default
pref store and for the extension pref store.

http://codereview.chromium.org/5213002/diff/47002/chrome/browser/prefs/pref_v...
File chrome/browser/prefs/pref_value_store.cc (right):

http://codereview.chromium.org/5213002/diff/47002/chrome/browser/prefs/pref_v...
chrome/browser/prefs/pref_value_store.cc:57: extension = new InMemoryPrefStore;
This one has no parens, but the one above has. There is no fixed convention for
this in the code base, but I feel you should try and be consistent within a
patch or file.

http://codereview.chromium.org/5213002/diff/47002/chrome/browser/prefs/pref_v...
File chrome/browser/prefs/pref_value_store.h (right):

http://codereview.chromium.org/5213002/diff/47002/chrome/browser/prefs/pref_v...
chrome/browser/prefs/pref_value_store.h:177: void
ReplaceExtensionPrefStore(PrefStore* extension_prefs);
Why would you want to do this if you have the accessor below?

http://codereview.chromium.org/5213002/diff/47002/chrome/browser/profile_impl.h
File chrome/browser/profile_impl.h (right):

http://codereview.chromium.org/5213002/diff/47002/chrome/browser/profile_impl...
chrome/browser/profile_impl.h:277: scoped_ptr<ExtensionPrefs>
tmp_extension_prefs_;
s/owner ship/ownership/
missing period

[battre (please use the other), 2010-11-19 16:03:17]

Addressed Matthias' comments (thanks!!!) and factored out the renaming of
DefaultPrefStore to InMemoryPrefStore to http://codereview.chromium.org/5204006/

http://codereview.chromium.org/5213002/diff/1022/chrome/browser/extensions/ex...
File chrome/browser/extensions/extension_prefs.cc (right):

http://codereview.chromium.org/5213002/diff/1022/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_prefs.cc:571: 
On 2010/11/19 10:36:12, Mattias Nissler wrote:
> Remove blank line and move the two code lines before the comment.

Done.

http://codereview.chromium.org/5213002/diff/1022/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_prefs.cc:614: for
(std::vector<std::string>::iterator i = prefKeys.begin();
On 2010/11/19 10:36:12, Mattias Nissler wrote:
> It might be useful to have a typedef std::vector<std::string> PrefKeySet in
the
> class declaration so you don't have to spell it out each time.

Done.

http://codereview.chromium.org/5213002/diff/1022/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_prefs.cc:617: }
On 2010/11/19 10:36:12, Mattias Nissler wrote:
> You have this exact loop earlier in the code, maybe it's useful to factor it
> into a UpdateExtensionControlledPrefsForExtension function?

It is not that simple: In OnExtensionUninstalled, I first retrieve the set of
extension keys, then remove an extension and finally do the update. I.e. there
is a step between GetExtensionControlledPrefKeys and the UpdateWinningPrefs.

http://codereview.chromium.org/5213002/diff/1022/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_prefs.cc:968: return base::Time::Now();
On 2010/11/19 10:36:12, Mattias Nissler wrote:
> How does that implementation allow to inject a clock? The GetCurrentTime()
> function is not virtual, so you cannot override.

that was a bug. Done.

http://codereview.chromium.org/5213002/diff/1022/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_prefs.cc:979: << "Error parsing installation
time of an extension";
On 2010/11/19 10:36:12, Mattias Nissler wrote:
> period/exclamation mark?

Done.

http://codereview.chromium.org/5213002/diff/1022/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_prefs.cc:989: 
On 2010/11/19 10:36:12, Mattias Nissler wrote:
> No blank line. The rationale is that you're commenting on the code that
follows,
> so separating the two doesn't make much sense.

I considered this a comment about the entire function, not just the following
line. Therefore, I added a blank line. Changed it.

Done.

http://codereview.chromium.org/5213002/diff/1022/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_prefs.cc:994: std::vector<DictionaryValue*>
sorted_extensions;
On 2010/11/19 10:36:12, Mattias Nissler wrote:
> How about just using a set with the right comparator?

Done.

http://codereview.chromium.org/5213002/diff/1022/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_prefs.cc:1010: SavePrefsAndNotify();
On 2010/11/19 10:36:12, Mattias Nissler wrote:
> Can we call this after we're done, i.e. outside the loop?

Done.

http://codereview.chromium.org/5213002/diff/1022/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_prefs.cc:1015: }
On 2010/11/19 10:36:12, Mattias Nissler wrote:
> Wouldn't it be more obvious to do the defaulting later? When reading the code
> below, I'd think "Wait, what if the dictionary doesn't exist?". So I believe
> putting the check there is more understandable.

Done.

http://codereview.chromium.org/5213002/diff/1022/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_prefs.cc:1017: // Currently we sort only by
time, no tie-breaker.
On 2010/11/19 10:36:12, Mattias Nissler wrote:
> Implement sorting inline? Please don't :) And please also add the tie breaker
> (e.g. using the extension id).

Done.

http://codereview.chromium.org/5213002/diff/1022/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_prefs.cc:1028: scoped_ptr<DictionaryValue>
merged_non_expanded(new DictionaryValue);
On 2010/11/19 10:36:12, Mattias Nissler wrote:
> why is this a scoped_ptr instead of a plain DictionaryValue var?

Done.

http://codereview.chromium.org/5213002/diff/1022/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_prefs.cc:1052: namespace subtle {
On 2010/11/19 10:36:12, Mattias Nissler wrote:
> you can just use an anonymous namespace here, I that's more common under
> chrome/. But then again, since that function has static linkage, you don't
need
> the namespace at all.

Done.

http://codereview.chromium.org/5213002/diff/1022/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_prefs.cc:1056: return a->Equals(b);
On 2010/11/19 10:36:12, Mattias Nissler wrote:
> this can be rewritten as a == NULL ? b == NULL : a->Equals(b). YMMV though :)
I
> believe it should really be a static member in Value.

nice. :-)
Done.

http://codereview.chromium.org/5213002/diff/1022/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_prefs.cc:1060: const Value*
ExtensionPrefs::WinningExtensionControlledPrefValue(
On 2010/11/19 10:36:12, Mattias Nissler wrote:
> It bothers me a bit that you have the logic that merges down the value in two
> different places, i.e. InstallExtensionControlledPreferences and also here. I
> don't have good suggestion how to share code other than keeping the
dictionaries
> in the correct order in memory and merging every time. So I guess I'm OK with
> your approach.

I have simplified it a bit at least.

http://codereview.chromium.org/5213002/diff/1022/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_prefs.cc:1083: }
On 2010/11/19 10:36:12, Mattias Nissler wrote:
> No need for curlies.

Done.

http://codereview.chromium.org/5213002/diff/1022/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_prefs.cc:1114: pref_key.c_str(),
PrefNotifier::EXTENSION_STORE);
On 2010/11/19 10:36:12, Mattias Nissler wrote:
> hm, 1103 to here really looks like it should be a method on the pref store.
> Problem with that is the store doesn't have access to the pref_notifier. Maybe
> we should really provide the stores with the pref_notifier on construction.
> WDYT? Put a TODO for now and fix it in a separate patch?

I agree. Added a TODO.

http://codereview.chromium.org/5213002/diff/1022/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_prefs.cc:1125: bool modified =
subtle::equalValues(oldValue, value);
On 2010/11/19 10:36:12, Mattias Nissler wrote:
> if (!modified)
>   return;

Done. Also inserted the missing !.

http://codereview.chromium.org/5213002/diff/1022/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_prefs.cc:1133:
pref_service()->ScheduleSavePersistentPrefs();
On 2010/11/19 10:36:12, Mattias Nissler wrote:
> You don't need to do that explicitly, the browser will on shutdown.

I think is it better to be crash save. WDYT?

http://codereview.chromium.org/5213002/diff/1022/chrome/browser/extensions/ex...
File chrome/browser/extensions/extension_prefs_unittest.cc (right):

http://codereview.chromium.org/5213002/diff/1022/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_prefs_unittest.cc:67: class MockObserver :
public NotificationObserver {
On 2010/11/19 10:36:12, Mattias Nissler wrote:
> Drop this in favor of chrome/test/mock_notification_observer.h

Done.

http://codereview.chromium.org/5213002/diff/1022/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_prefs_unittest.cc:211: // dictionaries (they
contain only the blacklist-property).
On 2010/11/19 10:36:12, Mattias Nissler wrote:
> Triggering error messages in unit_tests is a common thing to happen, so if
they
> are really legitimate at this point, you don't need to take special action to
> shut them up.

Done.

http://codereview.chromium.org/5213002/diff/1022/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_prefs_unittest.cc:397: // Use constants to
avoid confusing std::map with hard-coded strings.
On 2010/11/19 10:36:12, Mattias Nissler wrote:
> I don't understand? Constants are good, but not for the reason you're giving.

just copy and pasted that comment. I have removed it now.

http://codereview.chromium.org/5213002/diff/1022/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_prefs_unittest.cc:407: const char
kDefaultPref4[] = "default pref 4";
On 2010/11/19 10:36:12, Mattias Nissler wrote:
> These declarations should be moved to the top of the file IMHO. But please
> consider the style guide for an authoritive answer.

Moved it, but did not find anything in the style guide.
Done.

http://codereview.chromium.org/5213002/diff/1022/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_prefs_unittest.cc:448: void
InstallExtControlledPref(Extension *ext,
On 2010/11/19 10:36:12, Mattias Nissler wrote:
> Instead of shortening to Ext, why not just InstallPref? The context should be
> given be the test class name anyway.

Unfortunately, Extension Preferences have a different meaning in
|ExtensionPrefs|. I think your suggestion to call the new ones "Extension
Controlled Preferences" makes things clearer and should be used consistently.

http://codereview.chromium.org/5213002/diff/1022/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_prefs_unittest.cc:450: Value* val) {
On 2010/11/19 10:36:12, Mattias Nissler wrote:
> fix indentation.

Done.

http://codereview.chromium.org/5213002/diff/1022/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_prefs_unittest.cc:471: Extension* ext3;
On 2010/11/19 10:36:12, Mattias Nissler wrote:
> Missing trailing _

Was just copy&pasted, but changed now.
Done.

http://codereview.chromium.org/5213002/diff/1022/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_prefs_unittest.cc:511: class
ExtensionPrefsInstallOverwrittenExtensions
On 2010/11/19 10:36:12, Mattias Nissler wrote:
> I could argue that this test case covers also what the previous one tests, so
> maybe remove the previous one?

Done.

http://codereview.chromium.org/5213002/diff/1022/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_prefs_unittest.cc:664: // Tests uninstalling
an extension that was winning for only some preferences.
On 2010/11/19 10:36:12, Mattias Nissler wrote:
> Update comment.

Done.

http://codereview.chromium.org/5213002/diff/1022/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_prefs_unittest.cc:732: 
On 2010/11/19 10:36:12, Mattias Nissler wrote:
> No trailing newlines please.

Hopefully done. Eclipse messes this up.

http://codereview.chromium.org/5213002/diff/47002/chrome/browser/extensions/t...
File chrome/browser/extensions/test_extension_prefs.cc (right):

http://codereview.chromium.org/5213002/diff/47002/chrome/browser/extensions/t...
chrome/browser/extensions/test_extension_prefs.cc:30: base::Time
GetCurrentTime() const {
On 2010/11/19 10:47:40, Mattias Nissler wrote:
> note my other comment about GetCurrentTime() not being virtual, so this is no
> override.

Done.

http://codereview.chromium.org/5213002/diff/47002/chrome/browser/prefs/in_mem...
File chrome/browser/prefs/in_memory_pref_store.h (right):

http://codereview.chromium.org/5213002/diff/47002/chrome/browser/prefs/in_mem...
chrome/browser/prefs/in_memory_pref_store.h:15: // preference values set when a
preference is registered with the PrefService.
On 2010/11/19 10:47:40, Mattias Nissler wrote:
> Drop the default pref store comment. Perhaps say that it's used for the
default
> pref store and for the extension pref store.

Done.

http://codereview.chromium.org/5213002/diff/47002/chrome/browser/prefs/pref_v...
File chrome/browser/prefs/pref_value_store.cc (right):

http://codereview.chromium.org/5213002/diff/47002/chrome/browser/prefs/pref_v...
chrome/browser/prefs/pref_value_store.cc:57: extension = new InMemoryPrefStore;
On 2010/11/19 10:47:40, Mattias Nissler wrote:
> This one has no parens, but the one above has. There is no fixed convention
for
> this in the code base, but I feel you should try and be consistent within a
> patch or file.

Done.

http://codereview.chromium.org/5213002/diff/47002/chrome/browser/prefs/pref_v...
File chrome/browser/prefs/pref_value_store.h (right):

http://codereview.chromium.org/5213002/diff/47002/chrome/browser/prefs/pref_v...
chrome/browser/prefs/pref_value_store.h:177: void
ReplaceExtensionPrefStore(PrefStore* extension_prefs);
On 2010/11/19 10:47:40, Mattias Nissler wrote:
> Why would you want to do this if you have the accessor below?

Done.

http://codereview.chromium.org/5213002/diff/47002/chrome/browser/profile_impl.h
File chrome/browser/profile_impl.h (right):

http://codereview.chromium.org/5213002/diff/47002/chrome/browser/profile_impl...
chrome/browser/profile_impl.h:277: scoped_ptr<ExtensionPrefs>
tmp_extension_prefs_;
On 2010/11/19 10:47:40, Mattias Nissler wrote:
> s/owner ship/ownership/
> missing period

Done.

[Mattias Nissler (ping if slow), 2010-11-19 16:52:15]

a couple more comments.

http://codereview.chromium.org/5213002/diff/1022/chrome/browser/extensions/ex...
File chrome/browser/extensions/extension_prefs_unittest.cc (right):

http://codereview.chromium.org/5213002/diff/1022/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_prefs_unittest.cc:448: void
InstallExtControlledPref(Extension *ext,
On 2010/11/19 16:03:18, battre wrote:
> On 2010/11/19 10:36:12, Mattias Nissler wrote:
> > Instead of shortening to Ext, why not just InstallPref? The context should
be
> > given be the test class name anyway.
> 
> Unfortunately, Extension Preferences have a different meaning in
> |ExtensionPrefs|. I think your suggestion to call the new ones "Extension
> Controlled Preferences" makes things clearer and should be used consistently.

I see.

http://codereview.chromium.org/5213002/diff/1022/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_prefs_unittest.cc:450: Value* val) {
On 2010/11/19 16:03:18, battre wrote:
> On 2010/11/19 10:36:12, Mattias Nissler wrote:
> > fix indentation.
> 
> Done.

Not done?

http://codereview.chromium.org/5213002/diff/1022/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_prefs_unittest.cc:471: Extension* ext3;
On 2010/11/19 16:03:18, battre wrote:
> On 2010/11/19 10:36:12, Mattias Nissler wrote:
> > Missing trailing _
> 
> Was just copy&pasted, but changed now.
> Done.

Not done?

http://codereview.chromium.org/5213002/diff/1022/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_prefs_unittest.cc:511: class
ExtensionPrefsInstallOverwrittenExtensions
On 2010/11/19 16:03:18, battre wrote:
> On 2010/11/19 10:36:12, Mattias Nissler wrote:
> > I could argue that this test case covers also what the previous one tests,
so
> > maybe remove the previous one?
> 
> Done.

No change?

http://codereview.chromium.org/5213002/diff/1022/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_prefs_unittest.cc:664: // Tests uninstalling
an extension that was winning for only some preferences.
On 2010/11/19 16:03:18, battre wrote:
> On 2010/11/19 10:36:12, Mattias Nissler wrote:
> > Update comment.
> 
> Done.

Not done?

http://codereview.chromium.org/5213002/diff/1022/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_prefs_unittest.cc:732: 
On 2010/11/19 16:03:18, battre wrote:
> On 2010/11/19 10:36:12, Mattias Nissler wrote:
> > No trailing newlines please.
> 
> Hopefully done. Eclipse messes this up.

I see, not a big deal :)
However, you should get to the point where you control your editor and not the
other way round :)

http://codereview.chromium.org/5213002/diff/6026/chrome/browser/extensions/ex...
File chrome/browser/extensions/extension_prefs.cc (right):

http://codereview.chromium.org/5213002/diff/6026/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_prefs.cc:514: ExtensionPrefs::PrefKeySet
ExtensionPrefs::GetToolbarOrder() {
Did you also fix callers of this function?

http://codereview.chromium.org/5213002/diff/6026/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_prefs.cc:515: ExtensionPrefs::PrefKeySet
extension_ids;
no need for the ExtensionPrefs qualifier in this line.

http://codereview.chromium.org/5213002/diff/6026/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_prefs.cc:528: const PrefKeySet&
extension_ids) {
Now fits the previous line!

http://codereview.chromium.org/5213002/diff/6026/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_prefs.cc:731: }
I don't see why you add the empty dictionary here. Returning NULL should be
fine?

http://codereview.chromium.org/5213002/diff/6026/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_prefs.cc:1051: if (GetInstallTime(*ext_id)
== base::Time::FromInternalValue(0)) {
base::Time::FromInternalValue(0) == base::Time()

http://codereview.chromium.org/5213002/diff/6026/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_prefs.cc:1058: if (persistRequired) {
AFAICS, persistRequired is always false. Does that mean we don't need the call
to SavePrefsAndNotify()?

http://codereview.chromium.org/5213002/diff/6026/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_prefs.cc:1068: // and store the effective
preferences in the PrefService.
Maybe say "extension pref store" instead of PrefService.

http://codereview.chromium.org/5213002/diff/6026/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_prefs.cc:1089:
merged_non_expanded.begin_keys();
Doesn't this fit the previous line?

http://codereview.chromium.org/5213002/diff/6026/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_prefs.cc:1098: static bool equalValues(const
Value* a, const Value* b) {
Any reason, why you didn't move this to base/values.h as a static member
function of Value?

http://codereview.chromium.org/5213002/diff/6026/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_prefs.cc:1135: // PrefStores, which requires
that they have a pointer to the pref_notifier.
s/PrefStores/InMemoryPrefStore/. I'm not too sure anymore, since that requires
that we know the type of the extension store. Can the PrefService interface
actually guarantee that?

http://codereview.chromium.org/5213002/diff/6026/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_prefs.cc:1144: }
Non need for curlies, but keep them if you like them.

http://codereview.chromium.org/5213002/diff/6026/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_prefs.cc:1166:
extensionPreferences->SetWithoutPathExpansion(pref_key, value);
Well, if there are no curlies here, you shouldn't put them above.

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

http://codereview.chromium.org/5213002/diff/6026/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_prefs.h:77: PrefKeySet GetToolbarOrder();
I believe this should be ExtensionIdSet.

http://codereview.chromium.org/5213002/diff/6026/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_prefs.h:80: void SetToolbarOrder(const
PrefKeySet& extension_ids);
Same here.

http://codereview.chromium.org/5213002/diff/6026/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_prefs.h:308: // in a later Chrome build.
you mean version, not build.

http://codereview.chromium.org/5213002/diff/6026/chrome/browser/extensions/te...
File chrome/browser/extensions/test_extension_prefs.cc (right):

http://codereview.chromium.org/5213002/diff/6026/chrome/browser/extensions/te...
chrome/browser/extensions/test_extension_prefs.cc:20: // Mock ExtensionPrefs
class with artificial clock.
Please put the reason why that is needed into the comment.

http://codereview.chromium.org/5213002/diff/6026/chrome/browser/extensions/te...
chrome/browser/extensions/test_extension_prefs.cc:21: class ExtensionPrefsFake :
public ExtensionPrefs {
Typically we call a subclass of class Foo that is there for testing TestingFoo
or something similar.

http://codereview.chromium.org/5213002/diff/6026/chrome/browser/extensions/te...
chrome/browser/extensions/test_extension_prefs.cc:23: explicit
ExtensionPrefsFake(PrefService* prefs, const FilePath& root_dir_)
No need for explicit, has two arguments.

http://codereview.chromium.org/5213002/diff/6026/chrome/browser/extensions/te...
chrome/browser/extensions/test_extension_prefs.cc:29: // For unit testing
Missing period. Furthermore, this comment doesn't carry any information in its
current form - the whole file is for testing.

http://codereview.chromium.org/5213002/diff/6026/chrome/browser/prefs/pref_va...
File chrome/browser/prefs/pref_value_store.cc (right):

http://codereview.chromium.org/5213002/diff/6026/chrome/browser/prefs/pref_va...
chrome/browser/prefs/pref_value_store.cc:467: return
pref_stores_[PrefNotifier::EXTENSION_STORE].get();
Fix indentation.

http://codereview.chromium.org/5213002/diff/6026/chrome/browser/profile_impl.cc
File chrome/browser/profile_impl.cc (right):

http://codereview.chromium.org/5213002/diff/6026/chrome/browser/profile_impl....
chrome/browser/profile_impl.cc:283: // as early as possible. The constructor
takes care of that.
The style guide advises against doing complicated stuff in constructors. You
might want to review that section and perhaps change the code. I don't feel
strong about this instance though.

[battre (please use the other), 2010-11-19 18:00:39]

Addressed Mattias' comments (thanks!).

Aaron, could you have a look now as well?

Thanks,

Dominic

http://codereview.chromium.org/5213002/diff/1022/chrome/browser/extensions/ex...
File chrome/browser/extensions/extension_prefs_unittest.cc (right):

http://codereview.chromium.org/5213002/diff/1022/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_prefs_unittest.cc:450: Value* val) {
On 2010/11/19 16:52:20, Mattias Nissler wrote:
> On 2010/11/19 16:03:18, battre wrote:
> > On 2010/11/19 10:36:12, Mattias Nissler wrote:
> > > fix indentation.
> > 
> > Done.
> 
> Not done?

Done.

http://codereview.chromium.org/5213002/diff/1022/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_prefs_unittest.cc:471: Extension* ext3;
On 2010/11/19 16:52:20, Mattias Nissler wrote:
> On 2010/11/19 16:03:18, battre wrote:
> > On 2010/11/19 10:36:12, Mattias Nissler wrote:
> > > Missing trailing _
> > 
> > Was just copy&pasted, but changed now.
> > Done.
> 
> Not done?

Done.

http://codereview.chromium.org/5213002/diff/1022/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_prefs_unittest.cc:511: class
ExtensionPrefsInstallOverwrittenExtensions
On 2010/11/19 16:52:20, Mattias Nissler wrote:
> On 2010/11/19 16:03:18, battre wrote:
> > On 2010/11/19 10:36:12, Mattias Nissler wrote:
> > > I could argue that this test case covers also what the previous one tests,
> so
> > > maybe remove the previous one?
> > 
> > Done.
> 
> No change?

Done.

http://codereview.chromium.org/5213002/diff/1022/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_prefs_unittest.cc:664: // Tests uninstalling
an extension that was winning for only some preferences.
On 2010/11/19 16:52:20, Mattias Nissler wrote:
> On 2010/11/19 16:03:18, battre wrote:
> > On 2010/11/19 10:36:12, Mattias Nissler wrote:
> > > Update comment.
> > 
> > Done.
> 
> Not done?

Done.

http://codereview.chromium.org/5213002/diff/6026/chrome/browser/extensions/ex...
File chrome/browser/extensions/extension_prefs.cc (right):

http://codereview.chromium.org/5213002/diff/6026/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_prefs.cc:514: ExtensionPrefs::PrefKeySet
ExtensionPrefs::GetToolbarOrder() {
On 2010/11/19 16:52:20, Mattias Nissler wrote:
> Did you also fix callers of this function?

Reverted.

http://codereview.chromium.org/5213002/diff/6026/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_prefs.cc:515: ExtensionPrefs::PrefKeySet
extension_ids;
On 2010/11/19 16:52:20, Mattias Nissler wrote:
> no need for the ExtensionPrefs qualifier in this line.

Reverted.

http://codereview.chromium.org/5213002/diff/6026/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_prefs.cc:528: const PrefKeySet&
extension_ids) {
On 2010/11/19 16:52:20, Mattias Nissler wrote:
> Now fits the previous line!

Reverted.

http://codereview.chromium.org/5213002/diff/6026/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_prefs.cc:731: }
On 2010/11/19 16:52:20, Mattias Nissler wrote:
> I don't see why you add the empty dictionary here. Returning NULL should be
> fine?

Done. (added a missing NULLity test for the change)

http://codereview.chromium.org/5213002/diff/6026/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_prefs.cc:1051: if (GetInstallTime(*ext_id)
== base::Time::FromInternalValue(0)) {
On 2010/11/19 16:52:20, Mattias Nissler wrote:
> base::Time::FromInternalValue(0) == base::Time()

Done.

http://codereview.chromium.org/5213002/diff/6026/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_prefs.cc:1058: if (persistRequired) {
On 2010/11/19 16:52:20, Mattias Nissler wrote:
> AFAICS, persistRequired is always false. Does that mean we don't need the call
> to SavePrefsAndNotify()?

Done.

http://codereview.chromium.org/5213002/diff/6026/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_prefs.cc:1068: // and store the effective
preferences in the PrefService.
On 2010/11/19 16:52:20, Mattias Nissler wrote:
> Maybe say "extension pref store" instead of PrefService.

Done.

http://codereview.chromium.org/5213002/diff/6026/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_prefs.cc:1089:
merged_non_expanded.begin_keys();
On 2010/11/19 16:52:20, Mattias Nissler wrote:
> Doesn't this fit the previous line?

80 characters on the spot. :-)
Done.

http://codereview.chromium.org/5213002/diff/6026/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_prefs.cc:1098: static bool equalValues(const
Value* a, const Value* b) {
On 2010/11/19 16:52:20, Mattias Nissler wrote:
> Any reason, why you didn't move this to base/values.h as a static member
> function of Value?

Respect for modifying the holy base classes. ;-)

Done.

http://codereview.chromium.org/5213002/diff/6026/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_prefs.cc:1135: // PrefStores, which requires
that they have a pointer to the pref_notifier.
On 2010/11/19 16:52:20, Mattias Nissler wrote:
> s/PrefStores/InMemoryPrefStore/. I'm not too sure anymore, since that requires
> that we know the type of the extension store. Can the PrefService interface
> actually guarantee that?

Removed the comment. It would be a major refactoring.

http://codereview.chromium.org/5213002/diff/6026/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_prefs.cc:1144: }
On 2010/11/19 16:52:20, Mattias Nissler wrote:
> Non need for curlies, but keep them if you like them.

Done.

http://codereview.chromium.org/5213002/diff/6026/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_prefs.cc:1166:
extensionPreferences->SetWithoutPathExpansion(pref_key, value);
On 2010/11/19 16:52:20, Mattias Nissler wrote:
> Well, if there are no curlies here, you shouldn't put them above.

Done.

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

http://codereview.chromium.org/5213002/diff/6026/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_prefs.h:77: PrefKeySet GetToolbarOrder();
On 2010/11/19 16:52:20, Mattias Nissler wrote:
> I believe this should be ExtensionIdSet.

Done.

http://codereview.chromium.org/5213002/diff/6026/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_prefs.h:80: void SetToolbarOrder(const
PrefKeySet& extension_ids);
On 2010/11/19 16:52:20, Mattias Nissler wrote:
> Same here.

Done.

http://codereview.chromium.org/5213002/diff/6026/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_prefs.h:308: // in a later Chrome build.
On 2010/11/19 16:52:20, Mattias Nissler wrote:
> you mean version, not build.

Done.

http://codereview.chromium.org/5213002/diff/6026/chrome/browser/extensions/te...
File chrome/browser/extensions/test_extension_prefs.cc (right):

http://codereview.chromium.org/5213002/diff/6026/chrome/browser/extensions/te...
chrome/browser/extensions/test_extension_prefs.cc:20: // Mock ExtensionPrefs
class with artificial clock.
On 2010/11/19 16:52:20, Mattias Nissler wrote:
> Please put the reason why that is needed into the comment.

Done.

http://codereview.chromium.org/5213002/diff/6026/chrome/browser/extensions/te...
chrome/browser/extensions/test_extension_prefs.cc:21: class ExtensionPrefsFake :
public ExtensionPrefs {
On 2010/11/19 16:52:20, Mattias Nissler wrote:
> Typically we call a subclass of class Foo that is there for testing TestingFoo
> or something similar.

Done.

http://codereview.chromium.org/5213002/diff/6026/chrome/browser/extensions/te...
chrome/browser/extensions/test_extension_prefs.cc:23: explicit
ExtensionPrefsFake(PrefService* prefs, const FilePath& root_dir_)
On 2010/11/19 16:52:20, Mattias Nissler wrote:
> No need for explicit, has two arguments.

Done.

http://codereview.chromium.org/5213002/diff/6026/chrome/browser/extensions/te...
chrome/browser/extensions/test_extension_prefs.cc:29: // For unit testing
On 2010/11/19 16:52:20, Mattias Nissler wrote:
> Missing period. Furthermore, this comment doesn't carry any information in its
> current form - the whole file is for testing.

Done.

http://codereview.chromium.org/5213002/diff/6026/chrome/browser/prefs/pref_va...
File chrome/browser/prefs/pref_value_store.cc (right):

http://codereview.chromium.org/5213002/diff/6026/chrome/browser/prefs/pref_va...
chrome/browser/prefs/pref_value_store.cc:467: return
pref_stores_[PrefNotifier::EXTENSION_STORE].get();
On 2010/11/19 16:52:20, Mattias Nissler wrote:
> Fix indentation.

Done.

http://codereview.chromium.org/5213002/diff/6026/chrome/browser/profile_impl.cc
File chrome/browser/profile_impl.cc (right):

http://codereview.chromium.org/5213002/diff/6026/chrome/browser/profile_impl....
chrome/browser/profile_impl.cc:283: // as early as possible. The constructor
takes care of that.
On 2010/11/19 16:52:20, Mattias Nissler wrote:
> The style guide advises against doing complicated stuff in constructors. You
> might want to review that section and perhaps change the code. I don't feel
> strong about this instance though.

We could change that. But then we need some kind of contract that an init
function is called, which makes the class design more fragile. If you don't
object, I would keep it as it is. Aaron?

[Aaron Boodman, 2010-11-23 20:35:12]

Ok, here we go. 

At a high level, I'm pretty concerned about this approach, and would greatly
prefer changing the interface as described in bug 64232. I think that would
simplify many things. I haven't thought about it too hard, but it doesn't seem
that it would be too much harder than the changes I've requested below.

However, in lieu of that, I have the following structural suggestions for this
change:

1. I'm not crazy about the 'preferences' subtree inside each extension. This
wouldn't be necessary with bug 64232, and since we will hopefully do that, I'm
reluctant to add additional complexity (even though it's small in this case) for
something that's temporary. The temporary workaround I suggest considering is to
have a whitelist of known ECP names and use that whitelist to build up the
DictionaryValue that is needed to fulfill the PrefStore interface.

2. Trying to update the individual winning preferences seems overly complex. I
think it would be better to just rebuild the entire dictionary when an ECP
changes. That would eliminate a lot of code, and I don't think the runtime cost
is anything to worry about. I think this probably requires a small change to
PrefStore interface.

http://codereview.chromium.org/5213002/diff/57002/base/values.cc
File base/values.cc (right):

http://codereview.chromium.org/5213002/diff/57002/base/values.cc#newcode137
base/values.cc:137: bool Value::Equals(const Value* a, const Value* b) {
Needs a unit test.

http://codereview.chromium.org/5213002/diff/57002/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_prefs.cc (right):

http://codereview.chromium.org/5213002/diff/57002/chrome/browser/extensions/e...
chrome/browser/extensions/extension_prefs.cc:92: const char kPrefPreferences[] =
"preferences";
I'm split on having this subtree and the corresponding distinction between
extension-controlled preferences, and other per-extension preferences.

On the one hand it's a namespace separation, which seems like it could be handy,
and it makes the current design cleaner because we don't need a whitelist
anywhere of extension-controlled preferences.

But on the other hand, it adds some complexity, and the whitelist issue would be
fixed automatically with bug 64232.

I guess I lean toward combining the extension-controlled preferences with the
existing tree, and having a whitelist of known ECP in the code that builds up
the tree to hand back to PrefValueStore.

I don't have any suggestion for how to prevent the whitelist from going stale
though, so that is a downside.

Jochen, Dominic, what are your thoughts?

http://codereview.chromium.org/5213002/diff/57002/chrome/browser/extensions/e...
chrome/browser/extensions/extension_prefs.cc:552: UpdateExtensionPref(id,
kPrefPreferences, new DictionaryValue());
Nit: since you know that this will be removed if empty when serialized, and you
create it lazily in a different place, there is no need for this line.

http://codereview.chromium.org/5213002/diff/57002/chrome/browser/extensions/e...
chrome/browser/extensions/extension_prefs.cc:616: UpdateWinningPref(*i);
This pattern is repeated a few times. Factor out a UpdateExtensionPrefStore()
method?

http://codereview.chromium.org/5213002/diff/57002/chrome/browser/extensions/e...
chrome/browser/extensions/extension_prefs.cc:995: const {
Is the const indented on its own correct style? It looks weird to me. I think
maybe the new line should begin with const std::string...

http://codereview.chromium.org/5213002/diff/57002/chrome/browser/extensions/e...
chrome/browser/extensions/extension_prefs.cc:997: if (!extension)
Do we intend for this to happen? If not, we should at least LOG that it was
unexpected.

http://codereview.chromium.org/5213002/diff/57002/chrome/browser/extensions/e...
chrome/browser/extensions/extension_prefs.cc:1122: void
ExtensionPrefs::UpdateWinningPref(const std::string& pref_key) {
Trying to update the individual preference that has changed seems to cause lots
of complexity. Can we just rebuild the entire tree of extension-controlled
preferences and pass it back to PrefValueStore?

http://codereview.chromium.org/5213002/diff/57002/chrome/browser/extensions/e...
chrome/browser/extensions/extension_prefs.cc:1123: PrefStore* extensionPrefStore
=
Here, and many places in this file: Chromium uses under_scores, not camelCase,
for local variables.

http://codereview.chromium.org/5213002/diff/57002/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_prefs.h (right):

http://codereview.chromium.org/5213002/diff/57002/chrome/browser/extensions/e...
chrome/browser/extensions/extension_prefs.h:25: //       not associated with
extensions
comment nit: maybe: internal state for the extension system in general, not
associated with an individual extension, such as lastUpdateTime.

http://codereview.chromium.org/5213002/diff/57002/chrome/browser/extensions/e...
chrome/browser/extensions/extension_prefs.h:27: //       meta-preferences
describing properties of the extension like
internal state of an individual extension, such as...

http://codereview.chromium.org/5213002/diff/57002/chrome/browser/extensions/e...
chrome/browser/extensions/extension_prefs.h:32: //       sub-systems.
TODO(aa): Is this information relevant to readers of this class?

http://codereview.chromium.org/5213002/diff/57002/chrome/browser/extensions/e...
chrome/browser/extensions/extension_prefs.h:282: void
GetEnabledExtensions(ExtensionIdSet* out) const;
This method seems generally useful. Make public?

http://codereview.chromium.org/5213002/diff/57002/chrome/browser/extensions/t...
File chrome/browser/extensions/test_extension_prefs.cc (right):

http://codereview.chromium.org/5213002/diff/57002/chrome/browser/extensions/t...
chrome/browser/extensions/test_extension_prefs.cc:23: class
TestingExtensionPrefs : public ExtensionPrefs {
Maybe name this MockExtensionPrefs to distinguish from the test fixture class
'TestExtensionPrefs' below?

http://codereview.chromium.org/5213002/diff/57002/chrome/browser/profile_impl.cc
File chrome/browser/profile_impl.cc (right):

http://codereview.chromium.org/5213002/diff/57002/chrome/browser/profile_impl...
chrome/browser/profile_impl.cc:284: tmp_extension_prefs_.reset(new
ExtensionPrefs(
I think it is OK for profile to just own ExtensionPrefs permanently and make the
pointer in ExtensionsService raw. That would simplify things slightly.

[Aaron Boodman, 2010-11-23 20:37:31]

On 2010/11/23 20:35:12, Aaron Boodman wrote:
> Ok, here we go. 
> 
> At a high level, I'm pretty concerned about this approach, and would greatly
> prefer changing the interface as described in bug 64232. I think that would
> simplify many things. I haven't thought about it too hard, but it doesn't seem
> that it would be too much harder than the changes I've requested below.
> 
> However, in lieu of that, I have the following structural suggestions for this
> change:

BTW, re-reading this, the tone comes off more strident than it should have.
These are just suggestions, and I don't have the system as internalized as I'm
sure you two do. Please feel free to push back if you disagree.

[Aaron Boodman, 2010-11-23 20:41:59]

On 2010/11/23 20:35:12, Aaron Boodman wrote:
> 1. I'm not crazy about the 'preferences' subtree inside each extension. This
> wouldn't be necessary with bug 64232, and since we will hopefully do that, I'm
> reluctant to add additional complexity (even though it's small in this case)
for
> something that's temporary. The temporary workaround I suggest considering is
to
> have a whitelist of known ECP names and use that whitelist to build up the
> DictionaryValue that is needed to fulfill the PrefStore interface.

Just to expand on how this would work:

- We get rid of most of the conceptual difference between ECP and normal
preferences from ExtensionPrefs' external API.

- Internally, we keep a whitelist of known ECP keys (the whitelist would just
have one entry AFAIK currently).

- To implement GetExtensionControlledPrefs() (which would be the only mention of
this concept externally), we walk the whitelist of ECP keys and build up a new
dictionary containing just those values.

- Later, when something happens inside ExtensionPrefs that could affect
GetExtensionControlledPrefs(), we rebuild the entire Dictionary and (somehow)
notify PrefValueStore or otherwise update PrefValueStore's pointer.

[Mattias Nissler (ping if slow), 2010-11-23 22:02:05]

On 2010/11/23 20:41:59, Aaron Boodman wrote:
> On 2010/11/23 20:35:12, Aaron Boodman wrote:
> > 1. I'm not crazy about the 'preferences' subtree inside each extension. This
> > wouldn't be necessary with bug 64232, and since we will hopefully do that,
I'm
> > reluctant to add additional complexity (even though it's small in this case)
> for
> > something that's temporary. The temporary workaround I suggest considering
is
> to
> > have a whitelist of known ECP names and use that whitelist to build up the
> > DictionaryValue that is needed to fulfill the PrefStore interface.
> 
> Just to expand on how this would work:
> 
> - We get rid of most of the conceptual difference between ECP and normal
> preferences from ExtensionPrefs' external API.
> 
> - Internally, we keep a whitelist of known ECP keys (the whitelist would just
> have one entry AFAIK currently).
> 
> - To implement GetExtensionControlledPrefs() (which would be the only mention
of
> this concept externally), we walk the whitelist of ECP keys and build up a new
> dictionary containing just those values.
> 
> - Later, when something happens inside ExtensionPrefs that could affect
> GetExtensionControlledPrefs(), we rebuild the entire Dictionary and (somehow)
> notify PrefValueStore or otherwise update PrefValueStore's pointer.

So you propose to have ExtensionPrefs implement PrefStore? I think I like that
idea. Notifying PrefValueStore about changes might be a bit tricky though, since
we require the PrefNotifier pointer for that which is only available when the
PrefService gets constructed IIRC.

[Mattias Nissler (ping if slow), 2010-11-23 22:02:44]

http://codereview.chromium.org/5213002/diff/57002/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_prefs.cc (right):

http://codereview.chromium.org/5213002/diff/57002/chrome/browser/extensions/e...
chrome/browser/extensions/extension_prefs.cc:92: const char kPrefPreferences[] =
"preferences";
On 2010/11/23 20:35:13, Aaron Boodman wrote:
> I'm split on having this subtree and the corresponding distinction between
> extension-controlled preferences, and other per-extension preferences.
> 
> On the one hand it's a namespace separation, which seems like it could be
handy,
> and it makes the current design cleaner because we don't need a whitelist
> anywhere of extension-controlled preferences.

My vote is for the namespace separation. This also makes it clearer that these
values don't apply to how the extension system handles the extensions at all,
but only represent a cache for the pref system. To that end, I propose changing
the name from preferences to extension_prefs_cache or something similar.

[Aaron Boodman, 2010-11-23 22:46:30]

On 2010/11/23 22:02:05, Mattias Nissler wrote:
> On 2010/11/23 20:41:59, Aaron Boodman wrote:
> > On 2010/11/23 20:35:12, Aaron Boodman wrote:
> > > 1. I'm not crazy about the 'preferences' subtree inside each extension.
This
> > > wouldn't be necessary with bug 64232, and since we will hopefully do that,
> I'm
> > > reluctant to add additional complexity (even though it's small in this
case)
> > for
> > > something that's temporary. The temporary workaround I suggest considering
> is
> > to
> > > have a whitelist of known ECP names and use that whitelist to build up the
> > > DictionaryValue that is needed to fulfill the PrefStore interface.
> > 
> > Just to expand on how this would work:
> > 
> > - We get rid of most of the conceptual difference between ECP and normal
> > preferences from ExtensionPrefs' external API.
> > 
> > - Internally, we keep a whitelist of known ECP keys (the whitelist would
just
> > have one entry AFAIK currently).
> > 
> > - To implement GetExtensionControlledPrefs() (which would be the only
mention
> of
> > this concept externally), we walk the whitelist of ECP keys and build up a
new
> > dictionary containing just those values.
> > 
> > - Later, when something happens inside ExtensionPrefs that could affect
> > GetExtensionControlledPrefs(), we rebuild the entire Dictionary and
(somehow)
> > notify PrefValueStore or otherwise update PrefValueStore's pointer.
> 
> So you propose to have ExtensionPrefs implement PrefStore? I think I like that
> idea. 

That was my original idea, before I realized that the interface exposed by
PrefStore was fairly coarse. Right now, ExtensionPrefs more ore less implements
PrefStore already. It exposes a dictionary of the values that PrefValueStore
wants, it just doesn't officially implement the interface.

I don't feel too strongly about whether the interface is actually implemented
because right now it's just one method, and that one method has a pretty vague
name. I think it definitely makes sense to make ExtensionPrefs implement
PrefStore as part of bug 64232 though.

> Notifying PrefValueStore about changes might be a bit tricky though, since
> we require the PrefNotifier pointer for that which is only available when the
> PrefService gets constructed IIRC.

One somewhat ghetto way to do this would be to have PrefStores return
DictionaryValue** instead of DictionaryValue*. That way they could replace the
value whenever they want.

[Mattias Nissler (ping if slow), 2010-11-23 22:56:48]

On 2010/11/23 22:46:30, Aaron Boodman wrote:
> On 2010/11/23 22:02:05, Mattias Nissler wrote:
> > On 2010/11/23 20:41:59, Aaron Boodman wrote:
> > > On 2010/11/23 20:35:12, Aaron Boodman wrote:
> > > > 1. I'm not crazy about the 'preferences' subtree inside each extension.
> This
> > > > wouldn't be necessary with bug 64232, and since we will hopefully do
that,
> > I'm
> > > > reluctant to add additional complexity (even though it's small in this
> case)
> > > for
> > > > something that's temporary. The temporary workaround I suggest
considering
> > is
> > > to
> > > > have a whitelist of known ECP names and use that whitelist to build up
the
> > > > DictionaryValue that is needed to fulfill the PrefStore interface.
> > > 
> > > Just to expand on how this would work:
> > > 
> > > - We get rid of most of the conceptual difference between ECP and normal
> > > preferences from ExtensionPrefs' external API.
> > > 
> > > - Internally, we keep a whitelist of known ECP keys (the whitelist would
> just
> > > have one entry AFAIK currently).
> > > 
> > > - To implement GetExtensionControlledPrefs() (which would be the only
> mention
> > of
> > > this concept externally), we walk the whitelist of ECP keys and build up a
> new
> > > dictionary containing just those values.
> > > 
> > > - Later, when something happens inside ExtensionPrefs that could affect
> > > GetExtensionControlledPrefs(), we rebuild the entire Dictionary and
> (somehow)
> > > notify PrefValueStore or otherwise update PrefValueStore's pointer.
> > 
> > So you propose to have ExtensionPrefs implement PrefStore? I think I like
that
> > idea. 
> 
> That was my original idea, before I realized that the interface exposed by
> PrefStore was fairly coarse. Right now, ExtensionPrefs more ore less
implements
> PrefStore already. It exposes a dictionary of the values that PrefValueStore
> wants, it just doesn't officially implement the interface.
> 
> I don't feel too strongly about whether the interface is actually implemented
> because right now it's just one method, and that one method has a pretty vague
> name. I think it definitely makes sense to make ExtensionPrefs implement
> PrefStore as part of bug 64232 though.
> 
> > Notifying PrefValueStore about changes might be a bit tricky though, since
> > we require the PrefNotifier pointer for that which is only available when
the
> > PrefService gets constructed IIRC.
> 
> One somewhat ghetto way to do this would be to have PrefStores return
> DictionaryValue** instead of DictionaryValue*. That way they could replace the
> value whenever they want.

That won't work I'm afraid. When changing pref values, we need to send
PREF_CHANGED notifications. The way this currently works is to tell the
PrefNotifier instance associated with the respective PrefService/PrefValueStore
that a PrefStore changed a value. The notifier will then figure out whether that
change is actually visible to clients and if so, sends the notification.

[Aaron Boodman, 2010-11-23 22:57:10]

On 2010/11/23 22:46:30, Aaron Boodman wrote:
> On 2010/11/23 22:02:05, Mattias Nissler wrote:
> > On 2010/11/23 20:41:59, Aaron Boodman wrote:
> > > On 2010/11/23 20:35:12, Aaron Boodman wrote:
> > > > 1. I'm not crazy about the 'preferences' subtree inside each extension.
> This
> > > > wouldn't be necessary with bug 64232, and since we will hopefully do
that,
> > I'm
> > > > reluctant to add additional complexity (even though it's small in this
> case)
> > > for
> > > > something that's temporary. The temporary workaround I suggest
considering
> > is
> > > to
> > > > have a whitelist of known ECP names and use that whitelist to build up
the
> > > > DictionaryValue that is needed to fulfill the PrefStore interface.
> > > 
> > > Just to expand on how this would work:
> > > 
> > > - We get rid of most of the conceptual difference between ECP and normal
> > > preferences from ExtensionPrefs' external API.
> > > 
> > > - Internally, we keep a whitelist of known ECP keys (the whitelist would
> just
> > > have one entry AFAIK currently).
> > > 
> > > - To implement GetExtensionControlledPrefs() (which would be the only
> mention
> > of
> > > this concept externally), we walk the whitelist of ECP keys and build up a
> new
> > > dictionary containing just those values.
> > > 
> > > - Later, when something happens inside ExtensionPrefs that could affect
> > > GetExtensionControlledPrefs(), we rebuild the entire Dictionary and
> (somehow)
> > > notify PrefValueStore or otherwise update PrefValueStore's pointer.
> > 
> > So you propose to have ExtensionPrefs implement PrefStore? I think I like
that
> > idea. 
> 
> That was my original idea, before I realized that the interface exposed by
> PrefStore was fairly coarse. Right now, ExtensionPrefs more ore less
implements
> PrefStore already. It exposes a dictionary of the values that PrefValueStore
> wants, it just doesn't officially implement the interface.
> 
> I don't feel too strongly about whether the interface is actually implemented
> because right now it's just one method, and that one method has a pretty vague
> name. I think it definitely makes sense to make ExtensionPrefs implement
> PrefStore as part of bug 64232 though.
> 
> > Notifying PrefValueStore about changes might be a bit tricky though, since
> > we require the PrefNotifier pointer for that which is only available when
the
> > PrefService gets constructed IIRC.
> 
> One somewhat ghetto way to do this would be to have PrefStores return
> DictionaryValue** instead of DictionaryValue*. That way they could replace the
> value whenever they want.

Also, I don't know much about this PrefNotifier object, but it looks like you
can get it from PrefService, which ExtensionPref has.

[Mattias Nissler (ping if slow), 2010-11-23 23:04:56]

On Tue, Nov 23, 2010 at 11:57 PM, <aa@chromium.org> wrote:

> On 2010/11/23 22:46:30, Aaron Boodman wrote:
>
>> On 2010/11/23 22:02:05, Mattias Nissler wrote:
>> > On 2010/11/23 20:41:59, Aaron Boodman wrote:
>> > > On 2010/11/23 20:35:12, Aaron Boodman wrote:
>> > > > 1. I'm not crazy about the 'preferences' subtree inside each
>> extension.
>> This
>> > > > wouldn't be necessary with bug 64232, and since we will hopefully do
>>
> that,
>
>> > I'm
>> > > > reluctant to add additional complexity (even though it's small in
>> this
>> case)
>> > > for
>> > > > something that's temporary. The temporary workaround I suggest
>>
> considering
>
>> > is
>> > > to
>> > > > have a whitelist of known ECP names and use that whitelist to build
>> up
>>
> the
>
>> > > > DictionaryValue that is needed to fulfill the PrefStore interface.
>> > >
>> > > Just to expand on how this would work:
>> > >
>> > > - We get rid of most of the conceptual difference between ECP and
>> normal
>> > > preferences from ExtensionPrefs' external API.
>> > >
>> > > - Internally, we keep a whitelist of known ECP keys (the whitelist
>> would
>> just
>> > > have one entry AFAIK currently).
>> > >
>> > > - To implement GetExtensionControlledPrefs() (which would be the only
>> mention
>> > of
>> > > this concept externally), we walk the whitelist of ECP keys and build
>> up a
>> new
>> > > dictionary containing just those values.
>> > >
>> > > - Later, when something happens inside ExtensionPrefs that could
>> affect
>> > > GetExtensionControlledPrefs(), we rebuild the entire Dictionary and
>> (somehow)
>> > > notify PrefValueStore or otherwise update PrefValueStore's pointer.
>> >
>> > So you propose to have ExtensionPrefs implement PrefStore? I think I
>> like
>>
> that
>
>> > idea.
>>
>
>  That was my original idea, before I realized that the interface exposed by
>> PrefStore was fairly coarse. Right now, ExtensionPrefs more ore less
>>
> implements
>
>> PrefStore already. It exposes a dictionary of the values that
>> PrefValueStore
>> wants, it just doesn't officially implement the interface.
>>
>
>  I don't feel too strongly about whether the interface is actually
>> implemented
>> because right now it's just one method, and that one method has a pretty
>> vague
>> name. I think it definitely makes sense to make ExtensionPrefs implement
>> PrefStore as part of bug 64232 though.
>>
>
>  > Notifying PrefValueStore about changes might be a bit tricky though,
>> since
>> > we require the PrefNotifier pointer for that which is only available
>> when
>>
> the
>
>> > PrefService gets constructed IIRC.
>>
>
>  One somewhat ghetto way to do this would be to have PrefStores return
>> DictionaryValue** instead of DictionaryValue*. That way they could replace
>> the
>> value whenever they want.
>>
>
> Also, I don't know much about this PrefNotifier object, but it looks like
> you
> can get it from PrefService, which ExtensionPref has.


That's true, but now we're back at an initialization order problem: Ideally,
we should pass the ExtensionPrefs object to the PrefService constructor, so
it can install it as a PrefStore. But then again, you need the PrefService
to construct ExtensionPrefs. I think the best compromise will probably be to
create a SetExtensionPrefStore() function in PrefService that we can use to
install the ExtensionPrefs object after creating the PrefService.

[Aaron Boodman, 2010-11-23 23:34:21]

On 2010/11/23 23:04:56, Mattias Nissler wrote:
> On Tue, Nov 23, 2010 at 11:57 PM, <mailto:aa@chromium.org> wrote:
> 
> > On 2010/11/23 22:46:30, Aaron Boodman wrote:
> >
> >> On 2010/11/23 22:02:05, Mattias Nissler wrote:
> >> > On 2010/11/23 20:41:59, Aaron Boodman wrote:
> >> > > On 2010/11/23 20:35:12, Aaron Boodman wrote:
> >> > > > 1. I'm not crazy about the 'preferences' subtree inside each
> >> extension.
> >> This
> >> > > > wouldn't be necessary with bug 64232, and since we will hopefully do
> >>
> > that,
> >
> >> > I'm
> >> > > > reluctant to add additional complexity (even though it's small in
> >> this
> >> case)
> >> > > for
> >> > > > something that's temporary. The temporary workaround I suggest
> >>
> > considering
> >
> >> > is
> >> > > to
> >> > > > have a whitelist of known ECP names and use that whitelist to build
> >> up
> >>
> > the
> >
> >> > > > DictionaryValue that is needed to fulfill the PrefStore interface.
> >> > >
> >> > > Just to expand on how this would work:
> >> > >
> >> > > - We get rid of most of the conceptual difference between ECP and
> >> normal
> >> > > preferences from ExtensionPrefs' external API.
> >> > >
> >> > > - Internally, we keep a whitelist of known ECP keys (the whitelist
> >> would
> >> just
> >> > > have one entry AFAIK currently).
> >> > >
> >> > > - To implement GetExtensionControlledPrefs() (which would be the only
> >> mention
> >> > of
> >> > > this concept externally), we walk the whitelist of ECP keys and build
> >> up a
> >> new
> >> > > dictionary containing just those values.
> >> > >
> >> > > - Later, when something happens inside ExtensionPrefs that could
> >> affect
> >> > > GetExtensionControlledPrefs(), we rebuild the entire Dictionary and
> >> (somehow)
> >> > > notify PrefValueStore or otherwise update PrefValueStore's pointer.
> >> >
> >> > So you propose to have ExtensionPrefs implement PrefStore? I think I
> >> like
> >>
> > that
> >
> >> > idea.
> >>
> >
> >  That was my original idea, before I realized that the interface exposed by
> >> PrefStore was fairly coarse. Right now, ExtensionPrefs more ore less
> >>
> > implements
> >
> >> PrefStore already. It exposes a dictionary of the values that
> >> PrefValueStore
> >> wants, it just doesn't officially implement the interface.
> >>
> >
> >  I don't feel too strongly about whether the interface is actually
> >> implemented
> >> because right now it's just one method, and that one method has a pretty
> >> vague
> >> name. I think it definitely makes sense to make ExtensionPrefs implement
> >> PrefStore as part of bug 64232 though.
> >>
> >
> >  > Notifying PrefValueStore about changes might be a bit tricky though,
> >> since
> >> > we require the PrefNotifier pointer for that which is only available
> >> when
> >>
> > the
> >
> >> > PrefService gets constructed IIRC.
> >>
> >
> >  One somewhat ghetto way to do this would be to have PrefStores return
> >> DictionaryValue** instead of DictionaryValue*. That way they could replace
> >> the
> >> value whenever they want.
> >>
> >
> > Also, I don't know much about this PrefNotifier object, but it looks like
> > you
> > can get it from PrefService, which ExtensionPref has.
> 
> 
> That's true, but now we're back at an initialization order problem: Ideally,
> we should pass the ExtensionPrefs object to the PrefService constructor, so
> it can install it as a PrefStore. But then again, you need the PrefService
> to construct ExtensionPrefs. I think the best compromise will probably be to
> create a SetExtensionPrefStore() function in PrefService that we can use to
> install the ExtensionPrefs object after creating the PrefService.

That seems fine too.

[battre (please use the other), 2010-11-30 17:46:53]

I have addressed the issues from the comments. 

After discussions in MUC with mnissler, danno and bauerb, I have moved the
pointer to the (in-memory) extension pref store from PrefValueStore to the
Profile.

The patch requires one more thing to be investigated. Extensions can be started
with a --load-extension command line parameter. I think there are no unit tests
for how this is handled in the ExtensionPrefStore, yet.

Best regards,

Dominic

http://codereview.chromium.org/5213002/diff/57002/base/values.cc
File base/values.cc (right):

http://codereview.chromium.org/5213002/diff/57002/base/values.cc#newcode137
base/values.cc:137: bool Value::Equals(const Value* a, const Value* b) {
On 2010/11/23 20:35:13, Aaron Boodman wrote:
> Needs a unit test.

Done.

http://codereview.chromium.org/5213002/diff/57002/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_prefs.cc (right):

http://codereview.chromium.org/5213002/diff/57002/chrome/browser/extensions/e...
chrome/browser/extensions/extension_prefs.cc:92: const char kPrefPreferences[] =
"preferences";
On 2010/11/23 22:02:44, Mattias Nissler wrote:
> On 2010/11/23 20:35:13, Aaron Boodman wrote:
> > I'm split on having this subtree and the corresponding distinction between
> > extension-controlled preferences, and other per-extension preferences.
> > 
> > On the one hand it's a namespace separation, which seems like it could be
> handy,
> > and it makes the current design cleaner because we don't need a whitelist
> > anywhere of extension-controlled preferences.
> 
> My vote is for the namespace separation. This also makes it clearer that these
> values don't apply to how the extension system handles the extensions at all,
> but only represent a cache for the pref system. To that end, I propose
changing
> the name from preferences to extension_prefs_cache or something similar.

We have discussed this with several people in MUC (mnissler, danno, battre,
bauerb) and prefer to keep it as a preferences subtree. How is a fix for 64232
going to fix this automatically?

http://codereview.chromium.org/5213002/diff/57002/chrome/browser/extensions/e...
chrome/browser/extensions/extension_prefs.cc:552: UpdateExtensionPref(id,
kPrefPreferences, new DictionaryValue());
On 2010/11/23 20:35:13, Aaron Boodman wrote:
> Nit: since you know that this will be removed if empty when serialized, and
you
> create it lazily in a different place, there is no need for this line.

That is true, but on the other hand, I like having one place that shows me which
preferences to expect. I think it is unfortunate that we need the lazy creation
but having this here, makes stuff cleaner.

http://codereview.chromium.org/5213002/diff/57002/chrome/browser/extensions/e...
chrome/browser/extensions/extension_prefs.cc:616: UpdateWinningPref(*i);
On 2010/11/23 20:35:13, Aaron Boodman wrote:
> This pattern is repeated a few times. Factor out a UpdateExtensionPrefStore()
> method?

I think the pattern is repeated only twice (UpdateWinningPref is called only
three times, twice in such a pattern). The two occurrences of the pattern are
not identical however. In OnExtensionUninstalled we first store the prefKeys,
then remove ECPs and then do the update. The intermission makes a significant
difference.

http://codereview.chromium.org/5213002/diff/57002/chrome/browser/extensions/e...
chrome/browser/extensions/extension_prefs.cc:995: const {
On 2010/11/23 20:35:13, Aaron Boodman wrote:
> Is the const indented on its own correct style? It looks weird to me. I think
> maybe the new line should begin with const std::string...

Done.

http://codereview.chromium.org/5213002/diff/57002/chrome/browser/extensions/e...
chrome/browser/extensions/extension_prefs.cc:997: if (!extension)
On 2010/11/23 20:35:13, Aaron Boodman wrote:
> Do we intend for this to happen? If not, we should at least LOG that it was
> unexpected.

Done. Added comment: // If the extension doesn't have a pref, it's a
--load-extension.

http://codereview.chromium.org/5213002/diff/57002/chrome/browser/extensions/e...
chrome/browser/extensions/extension_prefs.cc:1122: void
ExtensionPrefs::UpdateWinningPref(const std::string& pref_key) {
On 2010/11/23 20:35:13, Aaron Boodman wrote:
> Trying to update the individual preference that has changed seems to cause
lots
> of complexity. Can we just rebuild the entire tree of extension-controlled
> preferences and pass it back to PrefValueStore?

We could do this and DictionaryValue::GetDifferingPaths would nicely allow to
send notifications of changed preferences. However, bug 64232 (get rid of
prefs() function in PrefStore interface) will create problems. Therefore, I have
removed the code duplication by making
ExtensionPrefs::InstallPersistedExtensionControlledPrefs use UpdateWinningPref
as well.

Done.

http://codereview.chromium.org/5213002/diff/57002/chrome/browser/extensions/e...
chrome/browser/extensions/extension_prefs.cc:1123: PrefStore* extensionPrefStore
=
On 2010/11/23 20:35:13, Aaron Boodman wrote:
> Here, and many places in this file: Chromium uses under_scores, not camelCase,
> for local variables.

Done.

http://codereview.chromium.org/5213002/diff/57002/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_prefs.h (right):

http://codereview.chromium.org/5213002/diff/57002/chrome/browser/extensions/e...
chrome/browser/extensions/extension_prefs.h:25: //       not associated with
extensions
On 2010/11/23 20:35:13, Aaron Boodman wrote:
> comment nit: maybe: internal state for the extension system in general, not
> associated with an individual extension, such as lastUpdateTime.

Done.

http://codereview.chromium.org/5213002/diff/57002/chrome/browser/extensions/e...
chrome/browser/extensions/extension_prefs.h:27: //       meta-preferences
describing properties of the extension like
On 2010/11/23 20:35:13, Aaron Boodman wrote:
> internal state of an individual extension, such as...

I think "internal state" does not really fit stuff like installation time for
two reasons:
a) "internal state" sounds like "managed by the extension itself" to me.
b) "state" sounds like "can change over time" to me.
I think the current description fits better.

http://codereview.chromium.org/5213002/diff/57002/chrome/browser/extensions/e...
chrome/browser/extensions/extension_prefs.h:32: //       sub-systems.
On 2010/11/23 20:35:13, Aaron Boodman wrote:
> TODO(aa): Is this information relevant to readers of this class?

Do you refer to the nomenclature of the three different kinds of preference
types? I think we should keep this here. I think that the term "preferences" has
been significantly abused (IMO it describes something the user defines - maybe
indirectly, but an Extension Installation Date is not a preference).

http://codereview.chromium.org/5213002/diff/57002/chrome/browser/extensions/e...
chrome/browser/extensions/extension_prefs.h:282: void
GetEnabledExtensions(ExtensionIdSet* out) const;
On 2010/11/23 20:35:13, Aaron Boodman wrote:
> This method seems generally useful. Make public?

Done.

http://codereview.chromium.org/5213002/diff/57002/chrome/browser/extensions/t...
File chrome/browser/extensions/test_extension_prefs.cc (right):

http://codereview.chromium.org/5213002/diff/57002/chrome/browser/extensions/t...
chrome/browser/extensions/test_extension_prefs.cc:23: class
TestingExtensionPrefs : public ExtensionPrefs {
On 2010/11/23 20:35:13, Aaron Boodman wrote:
> Maybe name this MockExtensionPrefs to distinguish from the test fixture class
> 'TestExtensionPrefs' below?

Done.

http://codereview.chromium.org/5213002/diff/57002/chrome/browser/profile_impl.cc
File chrome/browser/profile_impl.cc (right):

http://codereview.chromium.org/5213002/diff/57002/chrome/browser/profile_impl...
chrome/browser/profile_impl.cc:284: tmp_extension_prefs_.reset(new
ExtensionPrefs(
On 2010/11/23 20:35:13, Aaron Boodman wrote:
> I think it is OK for profile to just own ExtensionPrefs permanently and make
the
> pointer in ExtensionsService raw. That would simplify things slightly.

Done.

[Mattias Nissler (ping if slow), 2010-12-01 10:36:36]

I think this is much closer to what we want.

http://codereview.chromium.org/5213002/diff/114001/base/values.h
File base/values.h (right):

http://codereview.chromium.org/5213002/diff/114001/base/values.h#newcode103
base/values.h:103: // Compares if two Value objects have equal contents. Can
hanndle NULLs.
s/hanndle/handle/
You might want to mention that NULLs compare equals.

http://codereview.chromium.org/5213002/diff/114001/base/values_unittest.cc
File base/values_unittest.cc (right):

http://codereview.chromium.org/5213002/diff/114001/base/values_unittest.cc#ne...
base/values_unittest.cc:512: Value* null_ptr = NULL;
Why have a var for this that even has more characters than the literal NULL?

http://codereview.chromium.org/5213002/diff/114001/chrome/browser/extensions/...
File chrome/browser/extensions/extension_prefs.cc (right):

http://codereview.chromium.org/5213002/diff/114001/chrome/browser/extensions/...
chrome/browser/extensions/extension_prefs.cc:731: UpdateWinningPref(*i, false);
I believe Aaron already asked to factor this loop into its own function, since
you have it 3 times in the code. Any reason not to do so?

http://codereview.chromium.org/5213002/diff/114001/chrome/browser/extensions/...
chrome/browser/extensions/extension_prefs.cc:870: return NULL;
Add a newline here and above before the comment? Jochen wouldn't like it, but I
think the rest of this file has these newlines, so we should be consistent.

http://codereview.chromium.org/5213002/diff/114001/chrome/browser/extensions/...
chrome/browser/extensions/extension_prefs.cc:1144: return base::Time::Time();
Some whitespace here, too?

http://codereview.chromium.org/5213002/diff/114001/chrome/browser/extensions/...
chrome/browser/extensions/extension_prefs.cc:1198: std::set<std::string>
ext_controlled_prefs;
Isn't this a PrefKeySet?

http://codereview.chromium.org/5213002/diff/114001/chrome/browser/extensions/...
chrome/browser/extensions/extension_prefs.cc:1201: DictionaryValue* preferences
= GetExtensionControlledPrefs(*ext_id);
Why not just call GetExtensionControlledPrefKeys(&ext_controlled_prefs) here?

http://codereview.chromium.org/5213002/diff/114001/chrome/browser/extensions/...
chrome/browser/extensions/extension_prefs.cc:1213: // is called.
I don't understand why it would be necessary to suppress the notifications then?

http://codereview.chromium.org/5213002/diff/114001/chrome/browser/extensions/...
chrome/browser/extensions/extension_prefs.cc:1259:
winning_pref_value->DeepCopy());
Need curlies here.

http://codereview.chromium.org/5213002/diff/114001/chrome/browser/extensions/...
chrome/browser/extensions/extension_prefs.cc:1263: if (changed &&
!suppressNotification)
FYI to other reviewers: Note that there is work underway that will change the
PrefStore interface to not allow to write to prefs(). This means that the whole
business of updating the value in ExtensionPrefStore and sending out
notifications will move to there.

http://codereview.chromium.org/5213002/diff/114001/chrome/browser/extensions/...
chrome/browser/extensions/extension_prefs.cc:1265: pref_key.c_str(),
PrefNotifier::EXTENSION_STORE);
curlies here as well.

http://codereview.chromium.org/5213002/diff/114001/chrome/browser/extensions/...
chrome/browser/extensions/extension_prefs.cc:1278: // extension might be null.
They just can't override prefs? Or should we create the entry? If we had
separate pref paths for regular prefs and the override cache, we wouldn't run
into this problem, right? I don't want to start this discussion again though.
Aaron?

However, the CHECK() is the wrong answer, since that will make us crash
reproducibly. So please at least turn it into a LOG(WARNING) + return

http://codereview.chromium.org/5213002/diff/114001/chrome/browser/extensions/...
File chrome/browser/extensions/extension_prefs.h (right):

http://codereview.chromium.org/5213002/diff/114001/chrome/browser/extensions/...
chrome/browser/extensions/extension_prefs.h:62: const FilePath& root_dir);
Can we guarantee that the ExtensionPrefStore outlives ExtensionPrefs? In that
case I'd vote for passing the ExtensionPrefs pointer here instead of the whole
profile. If not, passing both profile and prefs is redundant, since we can
always call profile->GetPrefs() to get to the PrefService.

http://codereview.chromium.org/5213002/diff/114001/chrome/browser/extensions/...
chrome/browser/extensions/extension_prefs.h:356: // Helper method to acquire the
installation time of an extension
Missing period.

http://codereview.chromium.org/5213002/diff/114001/chrome/browser/extensions/...
chrome/browser/extensions/extension_prefs.h:370: const;
"If your function is const, the const keyword should be on the same line as the
last parameter"

http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Function_Decla...

http://codereview.chromium.org/5213002/diff/114001/chrome/browser/extensions/...
File chrome/browser/extensions/extension_prefs_unittest.cc (right):

http://codereview.chromium.org/5213002/diff/114001/chrome/browser/extensions/...
chrome/browser/extensions/extension_prefs_unittest.cc:329: 
any reason for the added newline?

http://codereview.chromium.org/5213002/diff/114001/chrome/browser/extensions/...
File chrome/browser/extensions/test_extension_prefs.cc (left):

http://codereview.chromium.org/5213002/diff/114001/chrome/browser/extensions/...
chrome/browser/extensions/test_extension_prefs.cc:43:
pref_service_.reset(PrefService::CreateUserPrefService(preferences_file_));
Why doesn't this work any longer? Anyway, these tests should verify that the
ExtensionPrefs reinitializes correctly, not that the PrefService is capable of
persisting values. So I think recreating the PrefService is really not necessary
here.

http://codereview.chromium.org/5213002/diff/114001/chrome/test/testing_pref_s...
File chrome/test/testing_pref_service.cc (right):

http://codereview.chromium.org/5213002/diff/114001/chrome/test/testing_pref_s...
chrome/test/testing_pref_service.cc:47: managed_platform_prefs_ =
managed_platform_prefs,
I'd expect two spaces more indentation here, because the statement starts at
PrefService. Not totally sure though.

http://codereview.chromium.org/5213002/diff/114001/chrome/test/testing_profile.h
File chrome/test/testing_profile.h (right):

http://codereview.chromium.org/5213002/diff/114001/chrome/test/testing_profil...
chrome/test/testing_profile.h:415: PrefStore* ext_pref_store_;
Can we name this extension_pref_store? The profile has lots of different
pointers, so abbreviations make understanding the thing harder.

[battre (please use the other), 2010-12-01 17:44:38]

Addressed the issues raised by Mattias.

http://codereview.chromium.org/5213002/diff/114001/base/values.h
File base/values.h (right):

http://codereview.chromium.org/5213002/diff/114001/base/values.h#newcode103
base/values.h:103: // Compares if two Value objects have equal contents. Can
hanndle NULLs.
On 2010/12/01 10:36:36, Mattias Nissler wrote:
> s/hanndle/handle/
> You might want to mention that NULLs compare equals.

Done.

http://codereview.chromium.org/5213002/diff/114001/base/values_unittest.cc
File base/values_unittest.cc (right):

http://codereview.chromium.org/5213002/diff/114001/base/values_unittest.cc#ne...
base/values_unittest.cc:512: Value* null_ptr = NULL;
On 2010/12/01 10:36:36, Mattias Nissler wrote:
> Why have a var for this that even has more characters than the literal NULL?

Done.

http://codereview.chromium.org/5213002/diff/114001/chrome/browser/extensions/...
File chrome/browser/extensions/extension_prefs.cc (right):

http://codereview.chromium.org/5213002/diff/114001/chrome/browser/extensions/...
chrome/browser/extensions/extension_prefs.cc:731: UpdateWinningPref(*i, false);
On 2010/12/01 10:36:36, Mattias Nissler wrote:
> I believe Aaron already asked to factor this loop into its own function, since
> you have it 3 times in the code. Any reason not to do so?

Done.

http://codereview.chromium.org/5213002/diff/114001/chrome/browser/extensions/...
chrome/browser/extensions/extension_prefs.cc:870: return NULL;
On 2010/12/01 10:36:36, Mattias Nissler wrote:
> Add a newline here and above before the comment? Jochen wouldn't like it, but
I
> think the rest of this file has these newlines, so we should be consistent.

Done.

http://codereview.chromium.org/5213002/diff/114001/chrome/browser/extensions/...
chrome/browser/extensions/extension_prefs.cc:1144: return base::Time::Time();
On 2010/12/01 10:36:36, Mattias Nissler wrote:
> Some whitespace here, too?

Done.

http://codereview.chromium.org/5213002/diff/114001/chrome/browser/extensions/...
chrome/browser/extensions/extension_prefs.cc:1198: std::set<std::string>
ext_controlled_prefs;
On 2010/12/01 10:36:36, Mattias Nissler wrote:
> Isn't this a PrefKeySet?

No, we need to filter for *unique* entries. The PrefKeySet is a vector.

I have changed this to collect everything in a PrefKeySet, then sort and
unique-ify it. Then we have three invocations of the same loop of
UpdateWinningPref(...) calls, which I have factored out into a function.

Done.

http://codereview.chromium.org/5213002/diff/114001/chrome/browser/extensions/...
chrome/browser/extensions/extension_prefs.cc:1201: DictionaryValue* preferences
= GetExtensionControlledPrefs(*ext_id);
On 2010/12/01 10:36:36, Mattias Nissler wrote:
> Why not just call GetExtensionControlledPrefKeys(&ext_controlled_prefs) here?

See above. Done.

http://codereview.chromium.org/5213002/diff/114001/chrome/browser/extensions/...
chrome/browser/extensions/extension_prefs.cc:1213: // is called.
On 2010/12/01 10:36:36, Mattias Nissler wrote:
> I don't understand why it would be necessary to suppress the notifications
then?

The notification mechanism checked that the value for which we send
notifications has a governor. Usually the default pref store becomes the
governor for preferences when they are registered. However, this has not
happened at this time. Therefore, everything we insert here, has not default
preference, yet. Therefore, the notification triggered a DCHECK violation.

After discussion with mnissler, I have changed PrefValueStore to ignore
notifications of preferences that have not been registered, yet. Now
ExtensionPrefs verifies in SetExtensionControlledPref that preferences have been
registered before they are set.

Done.

http://codereview.chromium.org/5213002/diff/114001/chrome/browser/extensions/...
chrome/browser/extensions/extension_prefs.cc:1259:
winning_pref_value->DeepCopy());
On 2010/12/01 10:36:36, Mattias Nissler wrote:
> Need curlies here.

Done.

http://codereview.chromium.org/5213002/diff/114001/chrome/browser/extensions/...
chrome/browser/extensions/extension_prefs.cc:1265: pref_key.c_str(),
PrefNotifier::EXTENSION_STORE);
On 2010/12/01 10:36:36, Mattias Nissler wrote:
> curlies here as well.

Done.

http://codereview.chromium.org/5213002/diff/114001/chrome/browser/extensions/...
chrome/browser/extensions/extension_prefs.cc:1278: // extension might be null.
On 2010/12/01 10:36:36, Mattias Nissler wrote:
> They just can't override prefs? Or should we create the entry? If we had
> separate pref paths for regular prefs and the override cache, we wouldn't run
> into this problem, right? I don't want to start this discussion again though.
> Aaron?
> 
> However, the CHECK() is the wrong answer, since that will make us crash
> reproducibly. So please at least turn it into a LOG(WARNING) + return

Done.

http://codereview.chromium.org/5213002/diff/114001/chrome/browser/extensions/...
File chrome/browser/extensions/extension_prefs.h (right):

http://codereview.chromium.org/5213002/diff/114001/chrome/browser/extensions/...
chrome/browser/extensions/extension_prefs.h:62: const FilePath& root_dir);
On 2010/12/01 10:36:36, Mattias Nissler wrote:
> Can we guarantee that the ExtensionPrefStore outlives ExtensionPrefs? In that
> case I'd vote for passing the ExtensionPrefs pointer here instead of the whole
> profile. If not, passing both profile and prefs is redundant, since we can
> always call profile->GetPrefs() to get to the PrefService.

Done. Moved PrefService in ProfileImpl accordingly.

http://codereview.chromium.org/5213002/diff/114001/chrome/browser/extensions/...
chrome/browser/extensions/extension_prefs.h:356: // Helper method to acquire the
installation time of an extension
On 2010/12/01 10:36:36, Mattias Nissler wrote:
> Missing period.

Done.

http://codereview.chromium.org/5213002/diff/114001/chrome/browser/extensions/...
chrome/browser/extensions/extension_prefs.h:370: const;
On 2010/12/01 10:36:36, Mattias Nissler wrote:
> "If your function is const, the const keyword should be on the same line as
the
> last parameter"
> 
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Function_Decla...

Done.

http://codereview.chromium.org/5213002/diff/114001/chrome/browser/extensions/...
File chrome/browser/extensions/extension_prefs_unittest.cc (right):

http://codereview.chromium.org/5213002/diff/114001/chrome/browser/extensions/...
chrome/browser/extensions/extension_prefs_unittest.cc:329: 
On 2010/12/01 10:36:36, Mattias Nissler wrote:
> any reason for the added newline?

Done.

http://codereview.chromium.org/5213002/diff/114001/chrome/browser/extensions/...
File chrome/browser/extensions/test_extension_prefs.cc (left):

http://codereview.chromium.org/5213002/diff/114001/chrome/browser/extensions/...
chrome/browser/extensions/test_extension_prefs.cc:43:
pref_service_.reset(PrefService::CreateUserPrefService(preferences_file_));
On 2010/12/01 10:36:36, Mattias Nissler wrote:
> Why doesn't this work any longer? Anyway, these tests should verify that the
> ExtensionPrefs reinitializes correctly, not that the PrefService is capable of
> persisting values. So I think recreating the PrefService is really not
necessary
> here.

@Why doesn't this work any longer?
The PrefService is now part of the Profile.

@I think creating the PrefService is really not necessary here.
There are two possible choices:
a) remove the persistence and reloading of the PrefValueStore
b) keep it as it is.

The advantage of a) is that we have a stricter unit-test. We not not test the
persistence of PrefValueStores.

Unfortunately, persisting and reloading a PrefValueStore has a side effect:
every empty dictionary is discarded and the ExtensionPrefs need to be able to
deal with this. Therefore, we need to test it. If we go for a), we need to
implement pruning the PrefStores.

The advantage of b) is actually that we have this kind of integration testing
and can be sure that the side effects and changes therein are covered. I tend to
prefer b) while mnissler prefers a). I have addressed the issue with a comment.
Please let me know if you have a strong feeling for either a) or b).

http://codereview.chromium.org/5213002/diff/114001/chrome/test/testing_pref_s...
File chrome/test/testing_pref_service.cc (right):

http://codereview.chromium.org/5213002/diff/114001/chrome/test/testing_pref_s...
chrome/test/testing_pref_service.cc:47: managed_platform_prefs_ =
managed_platform_prefs,
On 2010/12/01 10:36:36, Mattias Nissler wrote:
> I'd expect two spaces more indentation here, because the statement starts at
> PrefService. Not totally sure though.

I agree. Done.

http://codereview.chromium.org/5213002/diff/114001/chrome/test/testing_profile.h
File chrome/test/testing_profile.h (right):

http://codereview.chromium.org/5213002/diff/114001/chrome/test/testing_profil...
chrome/test/testing_profile.h:415: PrefStore* ext_pref_store_;
On 2010/12/01 10:36:36, Mattias Nissler wrote:
> Can we name this extension_pref_store? The profile has lots of different
> pointers, so abbreviations make understanding the thing harder.

Done.

[Aaron Boodman, 2010-12-03 08:58:10]

http://codereview.chromium.org/5213002/diff/136001/chrome/browser/extensions/...
File chrome/browser/extensions/extension_prefs.cc (right):

http://codereview.chromium.org/5213002/diff/136001/chrome/browser/extensions/...
chrome/browser/extensions/extension_prefs.cc:101: const char kPrefPreferences[]
= "preferences";
I think it is OK to assume the reader has read the class header, so you could
just say something like:

Contains any extension-controlled preferences.

http://codereview.chromium.org/5213002/diff/136001/chrome/browser/extensions/...
chrome/browser/extensions/extension_prefs.cc:1203:
std::sort(ext_controlled_prefs.begin(), ext_controlled_prefs.end());
Did you consider having PrefKeySet actually be a std::set<> ? It seems that
could eliminate this sorting/uniquing code.

http://codereview.chromium.org/5213002/diff/136001/chrome/browser/extensions/...
chrome/browser/extensions/extension_prefs.cc:1284: LOG(WARNING) << "Not setting
preference for " << pref_key
This warning doesn't seem quite right... We do store preferences for unpacked
(--load-extension) extensions. The only type of extension we don't store prefs
for that I can think of is component extensions.

http://codereview.chromium.org/5213002/diff/136001/chrome/browser/extensions/...
File chrome/browser/extensions/extension_prefs.h (right):

http://codereview.chromium.org/5213002/diff/136001/chrome/browser/extensions/...
chrome/browser/extensions/extension_prefs.h:20: // This class should be accessed
via ExtensionsService.
This last sentence no longer seems useful. It can be removed.

http://codereview.chromium.org/5213002/diff/136001/chrome/browser/extensions/...
chrome/browser/extensions/extension_prefs.h:32: //       sub-systems.
A quick example might clarify things here.

Also, in this overview, you should introduce the collaboration with
PrefValueStore.

http://codereview.chromium.org/5213002/diff/136001/chrome/browser/extensions/...
chrome/browser/extensions/extension_prefs.h:361: // Installs the persistent
extension preferences into |prefs_|'s extension
It seems like what this thing really does is intialize the ExtensionPrefStore,
so how about calling it that? "InitPrefStore"? The name requires familiarity
with the idea of a 'pref store', but once you have that and the understanding of
how this class relates to pref stores, that name should make it clear what is
going on.

http://codereview.chromium.org/5213002/diff/136001/chrome/browser/extensions/...
chrome/browser/extensions/extension_prefs.h:367: const Value*
WinningExtensionControlledPrefValue(
Method names should be verbs, so "GetWinning...", or how about
"GetMostRecent..." so the reader doesn't have to guess at what defines
'winning'.

http://codereview.chromium.org/5213002/diff/136001/chrome/browser/extensions/...
chrome/browser/extensions/extension_prefs.h:371: void UpdateWinningPrefs(const
PrefKeySet& pref_keys);
UpdatePrefStore?

http://codereview.chromium.org/5213002/diff/136001/chrome/browser/extensions/...
chrome/browser/extensions/extension_prefs.h:376: void UpdateWinningPref(const
std::string& pref_key);
Also UpdatePrefStore? (method overload)

http://codereview.chromium.org/5213002/diff/136001/chrome/browser/extensions/...
chrome/browser/extensions/extension_prefs.h:379: // intents to manage. Keys are
always appended, |out| is not cleared.
s/intents/intends

http://codereview.chromium.org/5213002/diff/136001/chrome/browser/profile.h
File chrome/browser/profile.h (right):

http://codereview.chromium.org/5213002/diff/136001/chrome/browser/profile.h#n...
chrome/browser/profile.h:284: virtual PrefStore* GetExtensionPrefStore() = 0;
Why is it necessary to store a pointer to extension's pref store in Profile?

It seems like ExtensionPrefs could get it directly from PrefValueStore (via
Profile).

Or if that is too coupled, Profile::GetExtensionPrefStore() could get it from
PrefValueStore.

http://codereview.chromium.org/5213002/diff/136001/chrome/browser/profile_impl.h
File chrome/browser/profile_impl.h (right):

http://codereview.chromium.org/5213002/diff/136001/chrome/browser/profile_imp...
chrome/browser/profile_impl.h:175: scoped_ptr<PrefService> prefs_;  // Keep this
on top for destruction order
Scary comment is scary. What is causing the destruction order dependency? Can we
get rid of it?

[Aaron Boodman, 2010-12-03 09:02:11]

http://codereview.chromium.org/5213002/diff/136001/chrome/browser/extensions/...
File chrome/browser/extensions/extension_pref_store.cc (right):

http://codereview.chromium.org/5213002/diff/136001/chrome/browser/extensions/...
chrome/browser/extensions/extension_pref_store.cc:11: if (profile_)
In Profile, I commented that I didn't understand why we need to store a pointer
to ExtensionPrefStore there. If we can get rid of that, it seems like you could
get rid of this entire class, and just use InMemoryPrefStore directly.

[battre (please use the other), 2010-12-03 19:32:58]

As discussed, I have moved the Extension Pref Store back into the PrefValueStore
again. I have also rebased to current trunk.

I am looking forward to your comments.

http://codereview.chromium.org/5213002/diff/136001/chrome/browser/extensions/...
File chrome/browser/extensions/extension_pref_store.cc (right):

http://codereview.chromium.org/5213002/diff/136001/chrome/browser/extensions/...
chrome/browser/extensions/extension_pref_store.cc:11: if (profile_)
On 2010/12/03 09:02:11, Aaron Boodman wrote:
> In Profile, I commented that I didn't understand why we need to store a
pointer
> to ExtensionPrefStore there. If we can get rid of that, it seems like you
could
> get rid of this entire class, and just use InMemoryPrefStore directly.

Removed pointer from profile as discussed. Done.

http://codereview.chromium.org/5213002/diff/136001/chrome/browser/extensions/...
File chrome/browser/extensions/extension_prefs.cc (right):

http://codereview.chromium.org/5213002/diff/136001/chrome/browser/extensions/...
chrome/browser/extensions/extension_prefs.cc:101: const char kPrefPreferences[]
= "preferences";
On 2010/12/03 08:58:10, Aaron Boodman wrote:
> I think it is OK to assume the reader has read the class header, so you could
> just say something like:
> 
> Contains any extension-controlled preferences.

Done.

http://codereview.chromium.org/5213002/diff/136001/chrome/browser/extensions/...
chrome/browser/extensions/extension_prefs.cc:1203:
std::sort(ext_controlled_prefs.begin(), ext_controlled_prefs.end());
On 2010/12/03 08:58:10, Aaron Boodman wrote:
> Did you consider having PrefKeySet actually be a std::set<> ? It seems that
> could eliminate this sorting/uniquing code.

Done.

http://codereview.chromium.org/5213002/diff/136001/chrome/browser/extensions/...
chrome/browser/extensions/extension_prefs.cc:1284: LOG(WARNING) << "Not setting
preference for " << pref_key
On 2010/12/03 08:58:10, Aaron Boodman wrote:
> This warning doesn't seem quite right... We do store preferences for unpacked
> (--load-extension) extensions. The only type of extension we don't store prefs
> for that I can think of is component extensions.

Done.

http://codereview.chromium.org/5213002/diff/136001/chrome/browser/extensions/...
File chrome/browser/extensions/extension_prefs.h (right):

http://codereview.chromium.org/5213002/diff/136001/chrome/browser/extensions/...
chrome/browser/extensions/extension_prefs.h:20: // This class should be accessed
via ExtensionsService.
On 2010/12/03 08:58:10, Aaron Boodman wrote:
> This last sentence no longer seems useful. It can be removed.

Done.

http://codereview.chromium.org/5213002/diff/136001/chrome/browser/extensions/...
chrome/browser/extensions/extension_prefs.h:32: //       sub-systems.
On 2010/12/03 08:58:10, Aaron Boodman wrote:
> A quick example might clarify things here.
> 
> Also, in this overview, you should introduce the collaboration with
> PrefValueStore.

Done.

http://codereview.chromium.org/5213002/diff/136001/chrome/browser/extensions/...
chrome/browser/extensions/extension_prefs.h:361: // Installs the persistent
extension preferences into |prefs_|'s extension
On 2010/12/03 08:58:10, Aaron Boodman wrote:
> It seems like what this thing really does is intialize the ExtensionPrefStore,
> so how about calling it that? "InitPrefStore"? The name requires familiarity
> with the idea of a 'pref store', but once you have that and the understanding
of
> how this class relates to pref stores, that name should make it clear what is
> going on.

Done.

http://codereview.chromium.org/5213002/diff/136001/chrome/browser/extensions/...
chrome/browser/extensions/extension_prefs.h:367: const Value*
WinningExtensionControlledPrefValue(
On 2010/12/03 08:58:10, Aaron Boodman wrote:
> Method names should be verbs, so "GetWinning...", or how about
> "GetMostRecent..." so the reader doesn't have to guess at what defines
> 'winning'.

I have changed it to GetWinning. GetMostRecent does not quite nail it. It is
about the recency of extensions, not about the recency when an extension
controlled pref value has been set.

Done.

http://codereview.chromium.org/5213002/diff/136001/chrome/browser/extensions/...
chrome/browser/extensions/extension_prefs.h:371: void UpdateWinningPrefs(const
PrefKeySet& pref_keys);
On 2010/12/03 08:58:10, Aaron Boodman wrote:
> UpdatePrefStore?

Done.

http://codereview.chromium.org/5213002/diff/136001/chrome/browser/extensions/...
chrome/browser/extensions/extension_prefs.h:376: void UpdateWinningPref(const
std::string& pref_key);
On 2010/12/03 08:58:10, Aaron Boodman wrote:
> Also UpdatePrefStore? (method overload)

Done.

http://codereview.chromium.org/5213002/diff/136001/chrome/browser/extensions/...
chrome/browser/extensions/extension_prefs.h:379: // intents to manage. Keys are
always appended, |out| is not cleared.
On 2010/12/03 08:58:10, Aaron Boodman wrote:
> s/intents/intends

Done.

http://codereview.chromium.org/5213002/diff/136001/chrome/browser/profile.h
File chrome/browser/profile.h (right):

http://codereview.chromium.org/5213002/diff/136001/chrome/browser/profile.h#n...
chrome/browser/profile.h:284: virtual PrefStore* GetExtensionPrefStore() = 0;
On 2010/12/03 08:58:10, Aaron Boodman wrote:
> Why is it necessary to store a pointer to extension's pref store in Profile?
> 
> It seems like ExtensionPrefs could get it directly from PrefValueStore (via
> Profile).
> 
> Or if that is too coupled, Profile::GetExtensionPrefStore() could get it from
> PrefValueStore.

Gone.

http://codereview.chromium.org/5213002/diff/136001/chrome/browser/profile_impl.h
File chrome/browser/profile_impl.h (right):

http://codereview.chromium.org/5213002/diff/136001/chrome/browser/profile_imp...
chrome/browser/profile_impl.h:175: scoped_ptr<PrefService> prefs_;  // Keep this
on top for destruction order
On 2010/12/03 08:58:10, Aaron Boodman wrote:
> Scary comment is scary. What is causing the destruction order dependency? Can
we
> get rid of it?

Well, this is just playing save. The situation has not changed by this CL.
Probably we don't have a real issue here because of threading guarantees. But I
don't know them, yet. I propose to leave this but added a comment "Please remove
comment if you are certain that we don't have an issue here".

[Aaron Boodman, 2010-12-03 20:00:44]

LGTM w/ the below nits and a question.

In general, I'd take a close look at your NOTREACHED()/DCHECK()s and apply the
following test:

If the condition is impossible, even in the face of corrupt disk data, failing
network, etc, then CHECK. If the CHECK fails it means our code has a bad bug,
and we want to know about that.

If the condition could happen if, eg, somebody twiddled the data on disk behind
our back, or another system did not behave the way we thought it would, then
LOG(WARNING) or DCHECK is more appropriate.

http://codereview.chromium.org/5213002/diff/136001/chrome/browser/profile_impl.h
File chrome/browser/profile_impl.h (right):

http://codereview.chromium.org/5213002/diff/136001/chrome/browser/profile_imp...
chrome/browser/profile_impl.h:175: scoped_ptr<PrefService> prefs_;  // Keep this
on top for destruction order
On 2010/12/03 19:32:58, battre wrote:
> On 2010/12/03 08:58:10, Aaron Boodman wrote:
> > Scary comment is scary. What is causing the destruction order dependency?
Can
> we
> > get rid of it?
> 
> Well, this is just playing save. The situation has not changed by this CL.
> Probably we don't have a real issue here because of threading guarantees. But
I
> don't know them, yet. I propose to leave this but added a comment "Please
remove
> comment if you are certain that we don't have an issue here".

Sorry, I still don't understand. Can you clarify what you are worried about
happening?

http://codereview.chromium.org/5213002/diff/181001/chrome/browser/extensions/...
File chrome/browser/extensions/extension_prefs.cc (right):

http://codereview.chromium.org/5213002/diff/181001/chrome/browser/extensions/...
chrome/browser/extensions/extension_prefs.cc:1137: NOTREACHED();
If this really can't happen, then CHECK(extension). NOTREACHED() is compiled out
of release builds.

http://codereview.chromium.org/5213002/diff/181001/chrome/browser/extensions/...
chrome/browser/extensions/extension_prefs.cc:1150: DCHECK(out);
Don't be a wuss: CHECK(out).

http://codereview.chromium.org/5213002/diff/181001/chrome/browser/extensions/...
chrome/browser/extensions/extension_prefs.cc:1169: if (extension == NULL) {
CHECK(extension)

http://codereview.chromium.org/5213002/diff/181001/chrome/browser/extensions/...
File chrome/browser/extensions/extension_prefs.h (right):

http://codereview.chromium.org/5213002/diff/181001/chrome/browser/extensions/...
chrome/browser/extensions/extension_prefs.h:33: // As several extensions may
want to set extension controlled preferences
What I meant was something like this, for the section under "extension
controlled preferences":

Browser preferences that an extension controls. For example, an extension could
use the proxy API to specify the browser's proxy preference.
Extension-controlled preferences are stored in
PrefValueStore::extension_prefs(), which this class populates and maintains as
the underlying extensions change.

Then the last paragraph of this comment could be removed. WDYT?

(sorry for micromanaging the comments here, but this is complex and now that I
have it in my brain I want to make sure it is written down so that I can
remember how it works next time)

[battre (please use the other), 2010-12-06 10:28:49]

Thanks for the feedback.

http://codereview.chromium.org/5213002/diff/136001/chrome/browser/profile_impl.h
File chrome/browser/profile_impl.h (right):

http://codereview.chromium.org/5213002/diff/136001/chrome/browser/profile_imp...
chrome/browser/profile_impl.h:175: scoped_ptr<PrefService> prefs_;  // Keep this
on top for destruction order
On 2010/12/03 20:00:44, Aaron Boodman wrote:
> On 2010/12/03 19:32:58, battre wrote:
> > On 2010/12/03 08:58:10, Aaron Boodman wrote:
> > > Scary comment is scary. What is causing the destruction order dependency?
> Can
> > we
> > > get rid of it?
> > 
> > Well, this is just playing save. The situation has not changed by this CL.
> > Probably we don't have a real issue here because of threading guarantees.
But
> I
> > don't know them, yet. I propose to leave this but added a comment "Please
> remove
> > comment if you are certain that we don't have an issue here".
> 
> Sorry, I still don't understand. Can you clarify what you are worried about
> happening?

Maybe this comment is clearer:
  // Keep prefs_ on top for destruction order because extension_prefs_,
  // net_pref_observer_, web_resource_service_ and background_contents_service_
  // store pointers to prefs_ and shall be destructed first.

http://codereview.chromium.org/5213002/diff/181001/chrome/browser/extensions/...
File chrome/browser/extensions/extension_prefs.cc (right):

http://codereview.chromium.org/5213002/diff/181001/chrome/browser/extensions/...
chrome/browser/extensions/extension_prefs.cc:1137: NOTREACHED();
On 2010/12/03 20:00:44, Aaron Boodman wrote:
> If this really can't happen, then CHECK(extension). NOTREACHED() is compiled
out
> of release builds.

In can happen if the programmer passed an invalid extension_id. However, we can
still handle the case gracefully by returning "now" as the install timestamp. We
don't need to crash the browser.

http://codereview.chromium.org/5213002/diff/181001/chrome/browser/extensions/...
chrome/browser/extensions/extension_prefs.cc:1150: DCHECK(out);
On 2010/12/03 20:00:44, Aaron Boodman wrote:
> Don't be a wuss: CHECK(out).

Done.

http://codereview.chromium.org/5213002/diff/181001/chrome/browser/extensions/...
chrome/browser/extensions/extension_prefs.cc:1169: if (extension == NULL) {
On 2010/12/03 20:00:44, Aaron Boodman wrote:
> CHECK(extension)

Done.

http://codereview.chromium.org/5213002/diff/181001/chrome/browser/extensions/...
File chrome/browser/extensions/extension_prefs.h (right):

http://codereview.chromium.org/5213002/diff/181001/chrome/browser/extensions/...
chrome/browser/extensions/extension_prefs.h:33: // As several extensions may
want to set extension controlled preferences
On 2010/12/03 20:00:44, Aaron Boodman wrote:
> What I meant was something like this, for the section under "extension
> controlled preferences":
> 
> Browser preferences that an extension controls. For example, an extension
could
> use the proxy API to specify the browser's proxy preference.
> Extension-controlled preferences are stored in
> PrefValueStore::extension_prefs(), which this class populates and maintains as
> the underlying extensions change.
> 
> Then the last paragraph of this comment could be removed. WDYT?
> 
> (sorry for micromanaging the comments here, but this is complex and now that I
> have it in my brain I want to make sure it is written down so that I can
> remember how it works next time)

Done.

[Mattias Nissler (ping if slow), 2010-12-06 16:29:18]

LGTM from my side too.

[Aaron Boodman, 2010-12-06 18:20:56]

http://codereview.chromium.org/5213002/diff/136001/chrome/browser/profile_impl.h
File chrome/browser/profile_impl.h (right):

http://codereview.chromium.org/5213002/diff/136001/chrome/browser/profile_imp...
chrome/browser/profile_impl.h:175: scoped_ptr<PrefService> prefs_;  // Keep this
on top for destruction order
On 2010/12/06 10:28:50, battre wrote:
> On 2010/12/03 20:00:44, Aaron Boodman wrote:
> > On 2010/12/03 19:32:58, battre wrote:
> > > On 2010/12/03 08:58:10, Aaron Boodman wrote:
> > > > Scary comment is scary. What is causing the destruction order
dependency?
> > Can
> > > we
> > > > get rid of it?
> > > 
> > > Well, this is just playing save. The situation has not changed by this CL.
> > > Probably we don't have a real issue here because of threading guarantees.
> But
> > I
> > > don't know them, yet. I propose to leave this but added a comment "Please
> > remove
> > > comment if you are certain that we don't have an issue here".
> > 
> > Sorry, I still don't understand. Can you clarify what you are worried about
> > happening?
> 
> Maybe this comment is clearer:
>   // Keep prefs_ on top for destruction order because extension_prefs_,
>   // net_pref_observer_, web_resource_service_ and
background_contents_service_
>   // store pointers to prefs_ and shall be destructed first.

I talked to a few people in this office about this, and we all agreed. This
problem probably exists all over Chrome, and we never comment about it in any of
those places.

If somebody actually changed ExtensionPrefs such that it actually needed to call
PrefService in its destructor, then a better way to document this would be to
reset the extension_prefs_ pointer explicitly in ~Profile.

In general, relying on destruction order seems pretty ugly.

[Mattias Nissler (ping if slow), 2010-12-06 19:29:50]

http://codereview.chromium.org/5213002/diff/136001/chrome/browser/profile_impl.h
File chrome/browser/profile_impl.h (right):

http://codereview.chromium.org/5213002/diff/136001/chrome/browser/profile_imp...
chrome/browser/profile_impl.h:175: scoped_ptr<PrefService> prefs_;  // Keep this
on top for destruction order
On 2010/12/06 18:20:56, Aaron Boodman wrote:
> On 2010/12/06 10:28:50, battre wrote:
> > On 2010/12/03 20:00:44, Aaron Boodman wrote:
> > > On 2010/12/03 19:32:58, battre wrote:
> > > > On 2010/12/03 08:58:10, Aaron Boodman wrote:
> > > > > Scary comment is scary. What is causing the destruction order
> dependency?
> > > Can
> > > > we
> > > > > get rid of it?
> > > > 
> > > > Well, this is just playing save. The situation has not changed by this
CL.
> > > > Probably we don't have a real issue here because of threading
guarantees.
> > But
> > > I
> > > > don't know them, yet. I propose to leave this but added a comment
"Please
> > > remove
> > > > comment if you are certain that we don't have an issue here".
> > > 
> > > Sorry, I still don't understand. Can you clarify what you are worried
about
> > > happening?
> > 
> > Maybe this comment is clearer:
> >   // Keep prefs_ on top for destruction order because extension_prefs_,
> >   // net_pref_observer_, web_resource_service_ and
> background_contents_service_
> >   // store pointers to prefs_ and shall be destructed first.
> 
> I talked to a few people in this office about this, and we all agreed. This
> problem probably exists all over Chrome, and we never comment about it in any
of
> those places.
> 
> If somebody actually changed ExtensionPrefs such that it actually needed to
call
> PrefService in its destructor, then a better way to document this would be to
> reset the extension_prefs_ pointer explicitly in ~Profile.
> 
> In general, relying on destruction order seems pretty ugly.

I agree that relying on implicit destruction order for something as central as
the Profile is really ugly.

Actually, while working on a recent CL that touched the profile, I tried to make
PrefService destruction explicit in the ProfileImpl constructor. I gave up after
I realized that I had to put resets for pretty much everything in the profile
before releasing prefs_. This shows that the current code heavily relies on the
ordering here.

Created http://crbug.com/65637 to track this.