Added Protector backup for Preferences.

Added Protector backup for Preferences.

BUG=114288
TEST=ProtectedPrefsWatcherTest.*; no user-visible changes

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

[Ivan Korotkov, 2012-03-07 17:56:15]

PTAL
This CL simply adds a signed 'backup' dictionary to Preferences that mirrors
actual protected prefs.

[whywhat, 2012-03-07 18:53:30]

http://codereview.chromium.org/9620010/diff/3001/chrome/browser/prefs/pref_se...
File chrome/browser/prefs/pref_set_observer.cc (right):

http://codereview.chromium.org/9620010/diff/3001/chrome/browser/prefs/pref_se...
chrome/browser/prefs/pref_set_observer.cc:79: // Homepage:
Nit: Style guide recommends ending comments with a dot. Colon here suggests the
next line is a comment too, which is untrue.

http://codereview.chromium.org/9620010/diff/3001/chrome/browser/protector/pro...
File chrome/browser/protector/protected_prefs_watcher.cc (right):

http://codereview.chromium.org/9620010/diff/3001/chrome/browser/protector/pro...
chrome/browser/protector/protected_prefs_watcher.cc:33: const char
kBackupSignature[] = "backup._signature";
why this pref name has underscore?

http://codereview.chromium.org/9620010/diff/3001/chrome/browser/protector/pro...
chrome/browser/protector/protected_prefs_watcher.cc:157: std::stringstream data;
Don't we have StringPrintf in chrome code base?

http://codereview.chromium.org/9620010/diff/3001/chrome/browser/protector/pro...
File chrome/browser/protector/protected_prefs_watcher.h (right):

http://codereview.chromium.org/9620010/diff/3001/chrome/browser/protector/pro...
chrome/browser/protector/protected_prefs_watcher.h:31: void
CheckForPrefChanges();
nit: Maybe ValidateProtectedPrefs or something like this?
write a comment what happens if the backup is invalid, what's the post effect of
this method?

http://codereview.chromium.org/9620010/diff/3001/chrome/browser/protector/pro...
chrome/browser/protector/protected_prefs_watcher.h:47: bool IsSignatureValid();
const?

http://codereview.chromium.org/9620010/diff/3001/chrome/browser/protector/pro...
chrome/browser/protector/protected_prefs_watcher.h:51: bool HasBackup();
const?

http://codereview.chromium.org/9620010/diff/3001/chrome/browser/protector/pro...
chrome/browser/protector/protected_prefs_watcher.h:54: std::string
GetSignatureData(PrefService* prefs);
ditto

http://codereview.chromium.org/9620010/diff/3001/chrome/browser/protector/pro...
chrome/browser/protector/protected_prefs_watcher.h:61: bool
did_startup_settings_change_;
Comment about this member?

http://codereview.chromium.org/9620010/diff/3001/chrome/browser/protector/pro...
File chrome/browser/protector/protector_service_factory.cc (right):

http://codereview.chromium.org/9620010/diff/3001/chrome/browser/protector/pro...
chrome/browser/protector/protector_service_factory.cc:35: // accomplished with
ProfileKeyedService implementation.
File a bug against owners of ExtensionService or ProfileKeyedService?

http://codereview.chromium.org/9620010/diff/3001/chrome/chrome_browser.gypi
File chrome/chrome_browser.gypi (right):

http://codereview.chromium.org/9620010/diff/3001/chrome/chrome_browser.gypi#n...
chrome/chrome_browser.gypi:2177: 'browser/protector/protected_prefs_watcher.cc',
It should be possible to add a unit test to the watcher, no?

[Ivan Korotkov, 2012-03-11 12:59:44]

Added unit test and fixed extension IDs (no blacklist entries anymore).

erikkay: PTAL at extensions/

http://codereview.chromium.org/9620010/diff/3001/chrome/browser/prefs/pref_se...
File chrome/browser/prefs/pref_set_observer.cc (right):

http://codereview.chromium.org/9620010/diff/3001/chrome/browser/prefs/pref_se...
chrome/browser/prefs/pref_set_observer.cc:79: // Homepage:
On 2012/03/07 18:53:30, whywhat wrote:
> Nit: Style guide recommends ending comments with a dot. Colon here suggests
the
> next line is a comment too, which is untrue.

Done.

http://codereview.chromium.org/9620010/diff/3001/chrome/browser/protector/pro...
File chrome/browser/protector/protected_prefs_watcher.cc (right):

http://codereview.chromium.org/9620010/diff/3001/chrome/browser/protector/pro...
chrome/browser/protector/protected_prefs_watcher.cc:33: const char
kBackupSignature[] = "backup._signature";
On 2012/03/07 18:53:30, whywhat wrote:
> why this pref name has underscore?

To emphasize that it's special among other "backup.*" prefs.

http://codereview.chromium.org/9620010/diff/3001/chrome/browser/protector/pro...
chrome/browser/protector/protected_prefs_watcher.cc:157: std::stringstream data;
On 2012/03/07 18:53:30, whywhat wrote:
> Don't we have StringPrintf in chrome code base?

We do. I thought stream IO is a bit more expressive but let's rewrite to
StringPrintf for (hopefully) efficiency.

http://codereview.chromium.org/9620010/diff/3001/chrome/browser/protector/pro...
File chrome/browser/protector/protected_prefs_watcher.h (right):

http://codereview.chromium.org/9620010/diff/3001/chrome/browser/protector/pro...
chrome/browser/protector/protected_prefs_watcher.h:31: void
CheckForPrefChanges();
On 2012/03/07 18:53:30, whywhat wrote:
> nit: Maybe ValidateProtectedPrefs or something like this?
> write a comment what happens if the backup is invalid, what's the post effect
of
> this method?

I didn't document the behaviour for now because it's a stub and subject to
change.

http://codereview.chromium.org/9620010/diff/3001/chrome/browser/protector/pro...
chrome/browser/protector/protected_prefs_watcher.h:47: bool IsSignatureValid();
On 2012/03/07 18:53:30, whywhat wrote:
> const?

Done.

http://codereview.chromium.org/9620010/diff/3001/chrome/browser/protector/pro...
chrome/browser/protector/protected_prefs_watcher.h:51: bool HasBackup();
On 2012/03/07 18:53:30, whywhat wrote:
> const?

Done.

http://codereview.chromium.org/9620010/diff/3001/chrome/browser/protector/pro...
chrome/browser/protector/protected_prefs_watcher.h:54: std::string
GetSignatureData(PrefService* prefs);
On 2012/03/07 18:53:30, whywhat wrote:
> ditto

Done.

http://codereview.chromium.org/9620010/diff/3001/chrome/browser/protector/pro...
chrome/browser/protector/protected_prefs_watcher.h:61: bool
did_startup_settings_change_;
On 2012/03/07 18:53:30, whywhat wrote:
> Comment about this member?

Stale member, forgot to remove.

http://codereview.chromium.org/9620010/diff/3001/chrome/browser/protector/pro...
File chrome/browser/protector/protector_service_factory.cc (right):

http://codereview.chromium.org/9620010/diff/3001/chrome/browser/protector/pro...
chrome/browser/protector/protector_service_factory.cc:35: // accomplished with
ProfileKeyedService implementation.
On 2012/03/07 18:53:30, whywhat wrote:
> File a bug against owners of ExtensionService or ProfileKeyedService?

They are already working on moving services out of Profile to profile-keyed
service.

http://codereview.chromium.org/9620010/diff/3001/chrome/chrome_browser.gypi
File chrome/chrome_browser.gypi (right):

http://codereview.chromium.org/9620010/diff/3001/chrome/chrome_browser.gypi#n...
chrome/chrome_browser.gypi:2177: 'browser/protector/protected_prefs_watcher.cc',
On 2012/03/07 18:53:30, whywhat wrote:
> It should be possible to add a unit test to the watcher, no?

Done.

[Ivan Korotkov, 2012-03-11 13:00:24]

erikkay: PTAL at extensions/

[whywhat, 2012-03-11 13:02:08]

lgtm

[Mattias Nissler (ping if slow), 2012-03-11 15:51:48]

Didn't do a full pass. chrome/browser/prefs LGTM.

[Erik does not do reviews, 2012-03-11 20:22:30]

+aa to review extensions code

[Aaron Boodman, 2012-03-13 19:45:15]

LGTM w/ one nit below

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

http://codereview.chromium.org/9620010/diff/6001/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_prefs.h:127: static void
GetExtensionsFrom(const base::DictionaryValue* extension_prefs,
Just return ExtensionIdSet. Most compilers will optimize away the copy, and even
if they don't clarity is more important anyway.

[Ivan Korotkov, 2012-03-14 13:06:45]

Anton and Aaron, can you please take another look?

ProtectedPrefsWatcher: only prefs that are changed are updated in the backup;
added migration for SessionStartupPref.

ExtensionPrefs: fixed issue that blacklist changes were not always propagated to
"extensions.settings" pref watchers.

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

http://codereview.chromium.org/9620010/diff/6001/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_prefs.h:127: static void
GetExtensionsFrom(const base::DictionaryValue* extension_prefs,
On 2012/03/13 19:45:15, Aaron Boodman wrote:
> Just return ExtensionIdSet. Most compilers will optimize away the copy, and
even
> if they don't clarity is more important anyway.

Done. Is it ok that there is inconsistency between this and GetExtensions() now?

[whywhat, 2012-03-14 13:35:41]

lgtm

[Aaron Boodman, 2012-03-14 18:28:28]

still lgtm

[commit-bot: I haz the power, 2012-03-15 15:13:53]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ivankr@chromium.org/9620010/17019

[commit-bot: I haz the power, 2012-03-15 16:53:50]

Change committed as 126927