Keep emtpy List/Dictionary pref value with non-empty default.

Keep emtpy List/Dictionary pref value with non-empty default.

- Add a MarkNeedsEmptyValue method to JsonPrefStore;
- In PrefService::RegisterPreference, mark ListValue and DictionaryValue pref
  with non-empty default as needing empty value in |user_pref_store_|;
- Update ChromeLauncherDelegate to put back default pinned apps and add
  migration logic for M19 users;
- Add unit tests;

BUG=122679
TEST=Verify fix for issue 122679.


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

[xiyuan, 2012-04-11 20:48:42]

battre: Please review all the other changes.
sky: if you want, you can just look at the ChromeLauncherDelegate part;

=====
A little background for battre:

We have a ListValue pref kUseDefaultPinnedApps to track the user pinned apps,
which has a non-empty default apps list. When user unpin all the apps, the
ListValue of the pref is empty. And we get the default list value on Chrome
restart.

This is because the empty ListValue/DictionaryValue is stripped out in
DeepCopyWithoutEmptyChildren in JsonPrefStore::SerializeData. And on Chrome
restart, since the empty value is gone, it falls into default store.

This CL adds ability to tag certains keys as need empty value in JsonPrefStore
and use that in PrefService::RegisterPreference.

[battre, 2012-04-11 21:55:50]

http://codereview.chromium.org/10055003/diff/1/chrome/browser/prefs/pref_serv...
File chrome/browser/prefs/pref_service.cc (right):

http://codereview.chromium.org/10055003/diff/1/chrome/browser/prefs/pref_serv...
chrome/browser/prefs/pref_service.cc:736: base::ListValue * list =
static_cast<base::ListValue*>(default_value);
nit: base::ListValue*

http://codereview.chromium.org/10055003/diff/1/chrome/browser/prefs/pref_serv...
chrome/browser/prefs/pref_service.cc:736: base::ListValue * list =
static_cast<base::ListValue*>(default_value);
instead of the static_cast, I would recommend to use
default_value->GetAsList(&list); as this is more idiomatic, same below and in
other files.

http://codereview.chromium.org/10055003/diff/1/chrome/browser/prefs/pref_serv...
File chrome/browser/prefs/pref_service_unittest.cc (right):

http://codereview.chromium.org/10055003/diff/1/chrome/browser/prefs/pref_serv...
chrome/browser/prefs/pref_service_unittest.cc:204: updater.Get()->Clear();
nit, optional: you can simplify this to updater->Clear();

http://codereview.chromium.org/10055003/diff/1/chrome/browser/prefs/pref_serv...
chrome/browser/prefs/pref_service_unittest.cc:227:
data_dir_.AppendASCII("read.need_empty_value.json"),
nit: indentation

http://codereview.chromium.org/10055003/diff/1/chrome/browser/prefs/pref_serv...
chrome/browser/prefs/pref_service_unittest.cc:231:
builder.WithUserFilePrefs(pref_file, message_loop_proxy_.get());
optional: do you want to inline message_loop_proxy_? It is written and read
exactly once.

http://codereview.chromium.org/10055003/diff/1/chrome/browser/prefs/pref_serv...
chrome/browser/prefs/pref_service_unittest.cc:267:
ASSERT_TRUE(file_util::Delete(pref_file, false));
nit: necessary? should be deleted by ScopedTempDir

http://codereview.chromium.org/10055003/diff/1/chrome/common/json_pref_store.h
File chrome/common/json_pref_store.h (right):

http://codereview.chromium.org/10055003/diff/1/chrome/common/json_pref_store....
chrome/common/json_pref_store.h:88: std::set<std::string>
keys_needs_empty_value_;
grammar: keys (plural) vs. needs (singular)

http://codereview.chromium.org/10055003/diff/1/chrome/common/json_pref_store_...
File chrome/common/json_pref_store_unittest.cc (right):

http://codereview.chromium.org/10055003/diff/1/chrome/common/json_pref_store_...
chrome/common/json_pref_store_unittest.cc:253: pref_file));
nit: indentation

http://codereview.chromium.org/10055003/diff/1/chrome/common/persistent_pref_...
File chrome/common/persistent_pref_store.h (right):

http://codereview.chromium.org/10055003/diff/1/chrome/common/persistent_pref_...
chrome/common/persistent_pref_store.h:68: // Marks the |key| with empty
ListValue/DictionaryValue need to be persisted.
grammar: the |key| (singular) vs. need (plural)

Marks that the |key| with empty ListValue/DictionaryValue needs to be persisted.

[xiyuan, 2012-04-11 22:36:46]

CL updated. Please take another look.
Thanks.

http://codereview.chromium.org/10055003/diff/1/chrome/browser/prefs/pref_serv...
File chrome/browser/prefs/pref_service.cc (right):

http://codereview.chromium.org/10055003/diff/1/chrome/browser/prefs/pref_serv...
chrome/browser/prefs/pref_service.cc:736: base::ListValue * list =
static_cast<base::ListValue*>(default_value);
On 2012/04/11 21:55:51, battre wrote:
> nit: base::ListValue*

Done.

http://codereview.chromium.org/10055003/diff/1/chrome/browser/prefs/pref_serv...
chrome/browser/prefs/pref_service.cc:736: base::ListValue * list =
static_cast<base::ListValue*>(default_value);
On 2012/04/11 21:55:51, battre wrote:
> instead of the static_cast, I would recommend to use
> default_value->GetAsList(&list); as this is more idiomatic, same below and in
> other files.

Done.

http://codereview.chromium.org/10055003/diff/1/chrome/browser/prefs/pref_serv...
File chrome/browser/prefs/pref_service_unittest.cc (right):

http://codereview.chromium.org/10055003/diff/1/chrome/browser/prefs/pref_serv...
chrome/browser/prefs/pref_service_unittest.cc:204: updater.Get()->Clear();
On 2012/04/11 21:55:51, battre wrote:
> nit, optional: you can simplify this to updater->Clear();

Done.

http://codereview.chromium.org/10055003/diff/1/chrome/browser/prefs/pref_serv...
chrome/browser/prefs/pref_service_unittest.cc:227:
data_dir_.AppendASCII("read.need_empty_value.json"),
On 2012/04/11 21:55:51, battre wrote:
> nit: indentation

Done.

http://codereview.chromium.org/10055003/diff/1/chrome/browser/prefs/pref_serv...
chrome/browser/prefs/pref_service_unittest.cc:231:
builder.WithUserFilePrefs(pref_file, message_loop_proxy_.get());
On 2012/04/11 21:55:51, battre wrote:
> optional: do you want to inline message_loop_proxy_? It is written and read
> exactly once.

Done.

http://codereview.chromium.org/10055003/diff/1/chrome/browser/prefs/pref_serv...
chrome/browser/prefs/pref_service_unittest.cc:267:
ASSERT_TRUE(file_util::Delete(pref_file, false));
On 2012/04/11 21:55:51, battre wrote:
> nit: necessary? should be deleted by ScopedTempDir

Removed.

http://codereview.chromium.org/10055003/diff/1/chrome/common/json_pref_store.h
File chrome/common/json_pref_store.h (right):

http://codereview.chromium.org/10055003/diff/1/chrome/common/json_pref_store....
chrome/common/json_pref_store.h:88: std::set<std::string>
keys_needs_empty_value_;
On 2012/04/11 21:55:51, battre wrote:
> grammar: keys (plural) vs. needs (singular)

Done.

http://codereview.chromium.org/10055003/diff/1/chrome/common/json_pref_store_...
File chrome/common/json_pref_store_unittest.cc (right):

http://codereview.chromium.org/10055003/diff/1/chrome/common/json_pref_store_...
chrome/common/json_pref_store_unittest.cc:253: pref_file));
On 2012/04/11 21:55:51, battre wrote:
> nit: indentation

Done.

http://codereview.chromium.org/10055003/diff/1/chrome/common/persistent_pref_...
File chrome/common/persistent_pref_store.h (right):

http://codereview.chromium.org/10055003/diff/1/chrome/common/persistent_pref_...
chrome/common/persistent_pref_store.h:68: // Marks the |key| with empty
ListValue/DictionaryValue need to be persisted.
On 2012/04/11 21:55:51, battre wrote:
> grammar: the |key| (singular) vs. need (plural)
> 
> Marks that the |key| with empty ListValue/DictionaryValue needs to be
persisted.

Done.

[sky, 2012-04-12 00:18:08]

ChromeLauncherDelegate LGTM

[battre, 2012-04-12 01:07:16]

lgtm

[commit-bot: I haz the power, 2012-04-12 01:57:35]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/[email protected]/10055003/5002

[commit-bot: I haz the power, 2012-04-12 03:41:39]

Change committed as 131919