Take out and generalize user prefs overriding from IncognitoUserPrefStore.

Take out and generalize user prefs overriding from IncognitoUserPrefStore.

Add support for "local" and "global" user preferences where
a "local" preference set in the overlay shadows the "global" preference.

BUG=none
TEST=OverlayUserPrefStoreTest*

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

[Mattias Nissler (ping if slow), 2011-11-10 12:45:17]

mostly good, some suggestions on making the test code more concise.

http://codereview.chromium.org/8515002/diff/1/chrome/browser/prefs/overlay_us...
File chrome/browser/prefs/overlay_user_pref_store.h (right):

http://codereview.chromium.org/8515002/diff/1/chrome/browser/prefs/overlay_us...
chrome/browser/prefs/overlay_user_pref_store.h:60: typedef
base::hash_map<std::string, std::string> NamesMap;
any reason not to use std::map (which we usually do if we don't have a good
reason not to)?

http://codereview.chromium.org/8515002/diff/1/chrome/browser/prefs/overlay_us...
File chrome/browser/prefs/overlay_user_pref_store_unittest.cc (right):

http://codereview.chromium.org/8515002/diff/1/chrome/browser/prefs/overlay_us...
chrome/browser/prefs/overlay_user_pref_store_unittest.cc:25: class
OverlayUserPrefStoreImpl : public OverlayUserPrefStore {
You might get away with just giving the test access to
RegisterOverlayProperty(). If you prefer to keep this class, I suggest you
rename it to TestOverlayUserPrefStore to make it clear that it's only useful for
testing.

http://codereview.chromium.org/8515002/diff/1/chrome/browser/prefs/overlay_us...
chrome/browser/prefs/overlay_user_pref_store_unittest.cc:34: 
remove newline

http://codereview.chromium.org/8515002/diff/1/chrome/browser/prefs/overlay_us...
chrome/browser/prefs/overlay_user_pref_store_unittest.cc:41: public:
this can be protected

http://codereview.chromium.org/8515002/diff/1/chrome/browser/prefs/overlay_us...
chrome/browser/prefs/overlay_user_pref_store_unittest.cc:115: EXPECT_EQ(42, i);
you could rewrite these three-line blocks as one line like this:

EXPECT_TRUE(FundamentalValue(42).Equals(value));

http://codereview.chromium.org/8515002/diff/1/chrome/browser/prefs/overlay_us...
chrome/browser/prefs/overlay_user_pref_store_unittest.cc:156:
ASSERT_TRUE(modify->GetType() == Value::TYPE_DICTIONARY);
type checks are better written using Value::IsType().

http://codereview.chromium.org/8515002/diff/1/chrome/browser/prefs/overlay_us...
chrome/browser/prefs/overlay_user_pref_store_unittest.cc:194:
ASSERT_TRUE(value);
FundamentalValue.Equals() shortcut also works here.

http://codereview.chromium.org/8515002/diff/1/chrome/browser/prefs/overlay_us...
chrome/browser/prefs/overlay_user_pref_store_unittest.cc:257:
ASSERT_TRUE(value);
FundamentalValue::Equals shortcut possible here (and below)

[mnaganov (inactive), 2011-11-10 14:49:40]

-40 LOC thanks to your comments!

http://codereview.chromium.org/8515002/diff/1/chrome/browser/prefs/overlay_us...
File chrome/browser/prefs/overlay_user_pref_store.h (right):

http://codereview.chromium.org/8515002/diff/1/chrome/browser/prefs/overlay_us...
chrome/browser/prefs/overlay_user_pref_store.h:60: typedef
base::hash_map<std::string, std::string> NamesMap;
On 2011/11/10 12:45:17, Mattias Nissler wrote:
> any reason not to use std::map (which we usually do if we don't have a good
> reason not to)?

No, I simply has misguided myself. Fixed.

http://codereview.chromium.org/8515002/diff/1/chrome/browser/prefs/overlay_us...
File chrome/browser/prefs/overlay_user_pref_store_unittest.cc (right):

http://codereview.chromium.org/8515002/diff/1/chrome/browser/prefs/overlay_us...
chrome/browser/prefs/overlay_user_pref_store_unittest.cc:25: class
OverlayUserPrefStoreImpl : public OverlayUserPrefStore {
On 2011/11/10 12:45:17, Mattias Nissler wrote:
> You might get away with just giving the test access to
> RegisterOverlayProperty(). If you prefer to keep this class, I suggest you
> rename it to TestOverlayUserPrefStore to make it clear that it's only useful
for
> testing.

A good idea! Made test class a friend.

http://codereview.chromium.org/8515002/diff/1/chrome/browser/prefs/overlay_us...
chrome/browser/prefs/overlay_user_pref_store_unittest.cc:34: 
On 2011/11/10 12:45:17, Mattias Nissler wrote:
> remove newline

Removed class.

http://codereview.chromium.org/8515002/diff/1/chrome/browser/prefs/overlay_us...
chrome/browser/prefs/overlay_user_pref_store_unittest.cc:41: public:
On 2011/11/10 12:45:17, Mattias Nissler wrote:
> this can be protected

Done.

http://codereview.chromium.org/8515002/diff/1/chrome/browser/prefs/overlay_us...
chrome/browser/prefs/overlay_user_pref_store_unittest.cc:115: EXPECT_EQ(42, i);
On 2011/11/10 12:45:17, Mattias Nissler wrote:
> you could rewrite these three-line blocks as one line like this:
> 
> EXPECT_TRUE(FundamentalValue(42).Equals(value));

Neat! That significantly reduces the number of lines :)

http://codereview.chromium.org/8515002/diff/1/chrome/browser/prefs/overlay_us...
chrome/browser/prefs/overlay_user_pref_store_unittest.cc:156:
ASSERT_TRUE(modify->GetType() == Value::TYPE_DICTIONARY);
On 2011/11/10 12:45:17, Mattias Nissler wrote:
> type checks are better written using Value::IsType().

Done.

http://codereview.chromium.org/8515002/diff/1/chrome/browser/prefs/overlay_us...
chrome/browser/prefs/overlay_user_pref_store_unittest.cc:194:
ASSERT_TRUE(value);
On 2011/11/10 12:45:17, Mattias Nissler wrote:
> FundamentalValue.Equals() shortcut also works here.

Done.

http://codereview.chromium.org/8515002/diff/1/chrome/browser/prefs/overlay_us...
chrome/browser/prefs/overlay_user_pref_store_unittest.cc:257:
ASSERT_TRUE(value);
On 2011/11/10 12:45:17, Mattias Nissler wrote:
> FundamentalValue::Equals shortcut possible here (and below)

Done.

[mnaganov (inactive), 2011-11-10 14:51:46]

Sorry, it looks like my change wasn't rebased correctly. Will reupload in a
minute

[mnaganov (inactive), 2011-11-14 10:15:23]

On 2011/11/10 14:51:46, Mikhail Naganov (Chromium) wrote:
> Sorry, it looks like my change wasn't rebased correctly. Will reupload in a
> minute

Mattias, can you take another look, please?

[Mattias Nissler (ping if slow), 2011-11-14 12:04:55]

LGTM with a nit.

http://codereview.chromium.org/8515002/diff/8009/chrome/browser/prefs/overlay...
File chrome/browser/prefs/overlay_user_pref_store.h (right):

http://codereview.chromium.org/8515002/diff/8009/chrome/browser/prefs/overlay...
chrome/browser/prefs/overlay_user_pref_store.h:12: #include "base/hash_tables.h"
remove this and instead #include <map>

[mnaganov (inactive), 2011-11-14 12:07:31]

Thank you very much!

http://codereview.chromium.org/8515002/diff/8009/chrome/browser/prefs/overlay...
File chrome/browser/prefs/overlay_user_pref_store.h (right):

http://codereview.chromium.org/8515002/diff/8009/chrome/browser/prefs/overlay...
chrome/browser/prefs/overlay_user_pref_store.h:12: #include "base/hash_tables.h"
On 2011/11/14 12:04:55, Mattias Nissler wrote:
> remove this and instead #include <map>

Done.