Fixed regression: various preferences were not persisted when changed from incognito window

Fixed regression: various preferences were not persisted when changed from incognito window

This CL renames the OverlayPrefStore to an IncognitoPrefStore. This IncognitoPrefStore stores write operations in memory and prevents persisting them to the user prefs file. The CL also blacklists two preferences from being stored in the in-memory IncognitoPrefStore to fix the regressions mentioned in the bugs.

BUG=87191, 84472
TEST=see bug descriptions

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

[battre, 2011-07-13 11:29:13]

This is a different approach for some regressions due to the introduction of the
incognito pref store.

It fixes http://crbug.com/87191 and http://crbug.com/84472.

It turns out that http://crbug.com/88355 actually has a different root cause, so
this is not affected.

The CL intentionally does not "fix" http://crbug.com/82089 as per Peter's
request.

Note that this approach black-lists some preferences from being stored in the
in-memory overlay of the incognito user pref store. Everything else defaults to
be stored only in the incognito pref store.

We could invert the logic to only store a selected set of preferences in the
incognito user pref store and default to the regular user pref store.

I am open to discussions which approach is favorable.

[Mattias Nissler (ping if slow), 2011-07-13 12:15:32]

I guess I'm not convinced that whitelisting is better than blacklisting. Do you
have any rationale and/or data on whether or not the write through situation is
the more common case?

http://codereview.chromium.org/7342043/diff/1/chrome/browser/prefs/incognito_...
File chrome/browser/prefs/incognito_user_pref_store.cc (right):

http://codereview.chromium.org/7342043/diff/1/chrome/browser/prefs/incognito_...
chrome/browser/prefs/incognito_user_pref_store.cc:40: if (!StoreInOverlay(key))
This shouldn't be necessary, since we should not place values in the overlay in
this case anyway.

http://codereview.chromium.org/7342043/diff/1/chrome/browser/prefs/incognito_...
File chrome/browser/prefs/incognito_user_pref_store.h (right):

http://codereview.chromium.org/7342043/diff/1/chrome/browser/prefs/incognito_...
chrome/browser/prefs/incognito_user_pref_store.h:62: virtual bool
StoreInOverlay(const std::string& key) const;
The function name suggests that something actually gets stored, which is
misleading.

[sky, 2011-07-13 14:03:01]

I'm inclined to go for blacklisting too. I don't have any data to back
that up though, it just feels more correct.

  -Scott

On Wed, Jul 13, 2011 at 5:15 AM,  <mnissler@chromium.org> wrote:
> I guess I'm not convinced that whitelisting is better than blacklisting. Do
> you
> have any rationale and/or data on whether or not the write through situation
> is
> the more common case?
>
>
>
http://codereview.chromium.org/7342043/diff/1/chrome/browser/prefs/incognito_...
> File chrome/browser/prefs/incognito_user_pref_store.cc (right):
>
>
http://codereview.chromium.org/7342043/diff/1/chrome/browser/prefs/incognito_...
> chrome/browser/prefs/incognito_user_pref_store.cc:40: if
> (!StoreInOverlay(key))
> This shouldn't be necessary, since we should not place values in the
> overlay in this case anyway.
>
>
http://codereview.chromium.org/7342043/diff/1/chrome/browser/prefs/incognito_...
> File chrome/browser/prefs/incognito_user_pref_store.h (right):
>
>
http://codereview.chromium.org/7342043/diff/1/chrome/browser/prefs/incognito_...
> chrome/browser/prefs/incognito_user_pref_store.h:62: virtual bool
> StoreInOverlay(const std::string& key) const;
> The function name suggests that something actually gets stored, which is
> misleading.
>
> http://codereview.chromium.org/7342043/
>

[Peter Kasting, 2011-07-13 22:02:05]

I can't tell what you guys are arguing for/against.  It sounds like battre meant
"blacklist" to mean "incognito pref store unless otherwise noted" but mnissler
and sky mean the opposite -- that they'd like to see things go in the normal
pref store unless otherwise noted.

I don't personally have much of an opinion.  I guess maybe the current approach
seems a little better than the opposite, but I haven't really thought about it.

http://codereview.chromium.org/7342043/diff/1/chrome/browser/prefs/incognito_...
File chrome/browser/prefs/incognito_user_pref_store.cc (right):

http://codereview.chromium.org/7342043/diff/1/chrome/browser/prefs/incognito_...
chrome/browser/prefs/incognito_user_pref_store.cc:61: if (read_result ==
READ_OK) {
Nit: Shorter:

if (read_result != READ_OK)
  return read_result;
... deep copy ...

http://codereview.chromium.org/7342043/diff/1/chrome/browser/prefs/incognito_...
chrome/browser/prefs/incognito_user_pref_store.cc:66: // Return read failure if
underlay stores no value for |key|.
Nit: This comment doesn't really add much to the code.

http://codereview.chromium.org/7342043/diff/1/chrome/browser/prefs/incognito_...
chrome/browser/prefs/incognito_user_pref_store.cc:83: if (!StoreInOverlay(key))
{
Nit: Shorter:

if (StoreInOverlay(key))
  overlay_.SetValue(key, value);
else
  underlay_->SetValueSilently(key, value);

http://codereview.chromium.org/7342043/diff/1/chrome/browser/prefs/incognito_...
chrome/browser/prefs/incognito_user_pref_store.cc:152: if (key ==
prefs::kCheckDefaultBrowser)
Nit: Seems just as good, and shorter:

return (key != prefs::kCheckDefaultBrowser) &&
       (key != prefs::kShowBookmarkBar);

[battre, 2011-07-13 22:28:32]

Can I consider your statement

> I can't tell what you guys are arguing for/against.  It sounds like battre
meant
> "blacklist" to mean "incognito pref store unless otherwise noted" but mnissler
> and sky mean the opposite -- that they'd like to see things go in the normal
> pref store unless otherwise noted.
>
> I don't personally have much of an opinion.  I guess maybe the current
approach
> seems a little better than the opposite, but I haven't really thought about
it.

as 1 "don't care" votes vs. 2 "write things into the normal pref store unless
otherwise noted" votes?

I will implement your comments tomorrow. Please let me know in case you want to
pass the task of LGTMing to Mattias (I think the remaining issues are very
straight forward). Otherwise I will wait for another round of input.

http://codereview.chromium.org/7342043/diff/1/chrome/browser/prefs/incognito_...
File chrome/browser/prefs/incognito_user_pref_store.cc (right):

http://codereview.chromium.org/7342043/diff/1/chrome/browser/prefs/incognito_...
chrome/browser/prefs/incognito_user_pref_store.cc:83: if (!StoreInOverlay(key))
{
On 2011/07/13 22:02:06, Peter Kasting wrote:
> Nit: Shorter:
> 
> if (StoreInOverlay(key))
>   overlay_.SetValue(key, value);
> else
>   underlay_->SetValueSilently(key, value);

Hm, do you prefer brevity over common patterns? All functions followed the "if
(!StoreInOverlay(...))" pattern. I prefer my proposed style, but I will change
it to your proposed style, if I don't hear anything else.

[Peter Kasting, 2011-07-13 22:43:51]

On 2011/07/13 22:28:32, battre wrote:
> Can I consider your statement
> as 1 "don't care" votes

Yeah, I don't care.  Whatever you all think is best.  I might care more if it
became obvious that doing things one way would be substantially more code or
less safe or something.

> I will implement your comments tomorrow. Please let me know in case you want
to
> pass the task of LGTMing to Mattias (I think the remaining issues are very
> straight forward). Otherwise I will wait for another round of input.

I'm fine if he LGTMs.

> Hm, do you prefer brevity over common patterns? All functions followed the "if
> (!StoreInOverlay(...))" pattern. I prefer my proposed style, but I will change
> it to your proposed style, if I don't hear anything else.

It's a nit, so you can ignore it if you prefer.  You could also do various other
things such as reverse the conditional here (or in the other functions).  In
general, I don't really value making one funciton look like another, but this
sort of thing is super-minor and you can write it however you like.

[battre, 2011-07-14 12:31:56]

I have addressed the comments. Mattias, please have a look. Thanks.

http://codereview.chromium.org/7342043/diff/1/chrome/browser/prefs/incognito_...
File chrome/browser/prefs/incognito_user_pref_store.cc (right):

http://codereview.chromium.org/7342043/diff/1/chrome/browser/prefs/incognito_...
chrome/browser/prefs/incognito_user_pref_store.cc:40: if (!StoreInOverlay(key))
On 2011/07/13 12:15:32, Mattias Nissler wrote:
> This shouldn't be necessary, since we should not place values in the overlay
in
> this case anyway.

Done.

http://codereview.chromium.org/7342043/diff/1/chrome/browser/prefs/incognito_...
chrome/browser/prefs/incognito_user_pref_store.cc:61: if (read_result ==
READ_OK) {
On 2011/07/13 22:02:06, Peter Kasting wrote:
> Nit: Shorter:
> 
> if (read_result != READ_OK)
>   return read_result;
> ... deep copy ...

Done.

http://codereview.chromium.org/7342043/diff/1/chrome/browser/prefs/incognito_...
chrome/browser/prefs/incognito_user_pref_store.cc:66: // Return read failure if
underlay stores no value for |key|.
On 2011/07/13 22:02:06, Peter Kasting wrote:
> Nit: This comment doesn't really add much to the code.

Done.

http://codereview.chromium.org/7342043/diff/1/chrome/browser/prefs/incognito_...
chrome/browser/prefs/incognito_user_pref_store.cc:152: if (key ==
prefs::kCheckDefaultBrowser)
On 2011/07/13 22:02:06, Peter Kasting wrote:
> Nit: Seems just as good, and shorter:
> 
> return (key != prefs::kCheckDefaultBrowser) &&
>        (key != prefs::kShowBookmarkBar);

Done.

http://codereview.chromium.org/7342043/diff/1/chrome/browser/prefs/incognito_...
File chrome/browser/prefs/incognito_user_pref_store.h (right):

http://codereview.chromium.org/7342043/diff/1/chrome/browser/prefs/incognito_...
chrome/browser/prefs/incognito_user_pref_store.h:62: virtual bool
StoreInOverlay(const std::string& key) const;
On 2011/07/13 12:15:32, Mattias Nissler wrote:
> The function name suggests that something actually gets stored, which is
> misleading.

Done.

[Mattias Nissler (ping if slow), 2011-07-14 13:47:16]

LGTM with a suggestion that you may choose to ignore.

http://codereview.chromium.org/7342043/diff/7001/chrome/browser/prefs/incogni...
File chrome/browser/prefs/incognito_user_pref_store_unittest.cc (right):

http://codereview.chromium.org/7342043/diff/7001/chrome/browser/prefs/incogni...
chrome/browser/prefs/incognito_user_pref_store_unittest.cc:27: return key ==
incognito_key;
why not just use regular pref names and avoid creating a subclass?

http://codereview.chromium.org/7342043/diff/7001/chrome/browser/prefs/incogni...
chrome/browser/prefs/incognito_user_pref_store_unittest.cc:28: }
DISALLOW_COPY_AND_ASSIGN

[battre, 2011-07-14 14:49:56]

Thanks

http://codereview.chromium.org/7342043/diff/7001/chrome/browser/prefs/incogni...
File chrome/browser/prefs/incognito_user_pref_store_unittest.cc (right):

http://codereview.chromium.org/7342043/diff/7001/chrome/browser/prefs/incogni...
chrome/browser/prefs/incognito_user_pref_store_unittest.cc:27: return key ==
incognito_key;
On 2011/07/14 13:47:16, Mattias Nissler wrote:
> why not just use regular pref names and avoid creating a subclass?

Done.

[commit-bot: I haz the power, 2011-07-14 14:50:42]

Can't process patch for file chrome/browser/prefs/incognito_user_pref_store.h.
A +