Expand the JsonPrefStore's interface to allow for an alternate filename to be specified.

Expand the JsonPrefStore's interface to allow for an alternate filename to be specified.

The alternate filename is intended to be the old name of the pref file being read, JsonPrefStore handles the migration if required. JsonPrefStore's FileThreadDeserializer is the best place to do this as it's the only code in the prefs stack running on a thread allowed to do IO (another solution would be to let JsonPrefStore take a PreReadOnFileThreadCallback to abstract the required work away, but the direct approach seemed easier here without breaking encapsulation).

BUG=372547
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=278776

[gab, 2014-06-19 15:23:15]

Bernhard PTAL.

Thanks!
Gab

[gab, 2014-06-19 15:24:42]

+sky for chrome_constants.[cc|h] changes.

[Bernhard Bauer, 2014-06-19 15:38:09]

https://codereview.chromium.org/347793002/diff/1/base/prefs/json_pref_store.h
File base/prefs/json_pref_store.h (right):

https://codereview.chromium.org/347793002/diff/1/base/prefs/json_pref_store.h...
base/prefs/json_pref_store.h:55: // could be read from. If |pref_filename|
exists, |pref_alternate_filename|
Why do we delete the alternate pref file if the primary one exists?

https://codereview.chromium.org/347793002/diff/1/chrome/common/chrome_constan...
File chrome/common/chrome_constants.h (right):

https://codereview.chromium.org/347793002/diff/1/chrome/common/chrome_constan...
chrome/common/chrome_constants.h:80: extern const base::FilePath::CharType
kSecurePreferencesFilename[];
Move this down so the constants stay in alphabetical order?

[gab, 2014-06-19 16:03:52]

Thanks for the quick review, reply inline.

https://codereview.chromium.org/347793002/diff/1/base/prefs/json_pref_store.h
File base/prefs/json_pref_store.h (right):

https://codereview.chromium.org/347793002/diff/1/base/prefs/json_pref_store.h...
base/prefs/json_pref_store.h:55: // could be read from. If |pref_filename|
exists, |pref_alternate_filename|
On 2014/06/19 15:38:09, Bernhard Bauer wrote:
> Why do we delete the alternate pref file if the primary one exists?

The alternate pref file is the old name of the file, if we already have the
primary then its safe to delete the old one.

I guess that since the new name was introduced in this CL it's not really
possible to have both (as the new one will always first be created by moving the
old one), but I thought it was nice extra cleanup to avoid having the same prefs
lying around in various deprecated files.

https://codereview.chromium.org/347793002/diff/1/chrome/common/chrome_constan...
File chrome/common/chrome_constants.h (right):

https://codereview.chromium.org/347793002/diff/1/chrome/common/chrome_constan...
chrome/common/chrome_constants.h:80: extern const base::FilePath::CharType
kSecurePreferencesFilename[];
On 2014/06/19 15:38:09, Bernhard Bauer wrote:
> Move this down so the constants stay in alphabetical order?

I debated that, but I opted to have them side-by-side; FWIW this list is already
not quite alphabetically sorted (i.e. kSupervisedUserSettingsFilename).

I don't really care, if you feel strongly I'll make the change; it just felt
like having kPref.., kDeprecatedProtectedPref.., and kSecurePref.. together was
easier to read.

[sky, 2014-06-19 16:03:58]

LGTM

https://codereview.chromium.org/347793002/diff/1/chrome/common/chrome_constan...
File chrome/common/chrome_constants.h (right):

https://codereview.chromium.org/347793002/diff/1/chrome/common/chrome_constan...
chrome/common/chrome_constants.h:80: extern const base::FilePath::CharType
kSecurePreferencesFilename[];
On 2014/06/19 15:38:09, Bernhard Bauer wrote:
> Move this down so the constants stay in alphabetical order?

+1

[Bernhard Bauer, 2014-06-19 17:20:20]

https://codereview.chromium.org/347793002/diff/1/base/prefs/json_pref_store.h
File base/prefs/json_pref_store.h (right):

https://codereview.chromium.org/347793002/diff/1/base/prefs/json_pref_store.h...
base/prefs/json_pref_store.h:55: // could be read from. If |pref_filename|
exists, |pref_alternate_filename|
On 2014/06/19 16:03:51, gab wrote:
> On 2014/06/19 15:38:09, Bernhard Bauer wrote:
> > Why do we delete the alternate pref file if the primary one exists?
> 
> The alternate pref file is the old name of the file, if we already have the
> primary then its safe to delete the old one.
> 
> I guess that since the new name was introduced in this CL it's not really
> possible to have both (as the new one will always first be created by moving
the
> old one), but I thought it was nice extra cleanup to avoid having the same
prefs
> lying around in various deprecated files.

Yeah, I'm just a bit wary of deleting a file that shouldn't exist anyway if
everything is going normal. I think I would be a little happier if we DCHECK
that the file doesn't exist, to document our assumptions and to be notified (at
least in Debug builds) if they are incorrect.

https://codereview.chromium.org/347793002/diff/1/chrome/common/chrome_constan...
File chrome/common/chrome_constants.h (right):

https://codereview.chromium.org/347793002/diff/1/chrome/common/chrome_constan...
chrome/common/chrome_constants.h:80: extern const base::FilePath::CharType
kSecurePreferencesFilename[];
On 2014/06/19 16:03:52, gab wrote:
> On 2014/06/19 15:38:09, Bernhard Bauer wrote:
> > Move this down so the constants stay in alphabetical order?
> 
> I debated that, but I opted to have them side-by-side; FWIW this list is
already
> not quite alphabetically sorted (i.e. kSupervisedUserSettingsFilename).
> 
> I don't really care, if you feel strongly I'll make the change; it just felt
> like having kPref.., kDeprecatedProtectedPref.., and kSecurePref.. together
was
> easier to read.

I do prefer alphabetic.

The kSupervisedUserSettingsFilename is an artifact of a recent renaming, which
I'm about to fix :)

[gab, 2014-06-19 17:50:11]

https://codereview.chromium.org/347793002/diff/1/base/prefs/json_pref_store.h
File base/prefs/json_pref_store.h (right):

https://codereview.chromium.org/347793002/diff/1/base/prefs/json_pref_store.h...
base/prefs/json_pref_store.h:55: // could be read from. If |pref_filename|
exists, |pref_alternate_filename|
On 2014/06/19 17:20:20, Bernhard Bauer wrote:
> On 2014/06/19 16:03:51, gab wrote:
> > On 2014/06/19 15:38:09, Bernhard Bauer wrote:
> > > Why do we delete the alternate pref file if the primary one exists?
> > 
> > The alternate pref file is the old name of the file, if we already have the
> > primary then its safe to delete the old one.
> > 
> > I guess that since the new name was introduced in this CL it's not really
> > possible to have both (as the new one will always first be created by moving
> the
> > old one), but I thought it was nice extra cleanup to avoid having the same
> prefs
> > lying around in various deprecated files.
> 
> Yeah, I'm just a bit wary of deleting a file that shouldn't exist anyway if
> everything is going normal. I think I would be a little happier if we DCHECK
> that the file doesn't exist, to document our assumptions and to be notified
(at
> least in Debug builds) if they are incorrect.

I'm not really worried of this assumption being incorrect on developer machines,
I'm more worried that some users end up in weird states (e.g., tampered users)
where they have both yet one doesn't matter.

https://codereview.chromium.org/347793002/diff/1/chrome/common/chrome_constan...
File chrome/common/chrome_constants.h (right):

https://codereview.chromium.org/347793002/diff/1/chrome/common/chrome_constan...
chrome/common/chrome_constants.h:80: extern const base::FilePath::CharType
kSecurePreferencesFilename[];
On 2014/06/19 17:20:20, Bernhard Bauer wrote:
> On 2014/06/19 16:03:52, gab wrote:
> > On 2014/06/19 15:38:09, Bernhard Bauer wrote:
> > > Move this down so the constants stay in alphabetical order?
> > 
> > I debated that, but I opted to have them side-by-side; FWIW this list is
> already
> > not quite alphabetically sorted (i.e. kSupervisedUserSettingsFilename).
> > 
> > I don't really care, if you feel strongly I'll make the change; it just felt
> > like having kPref.., kDeprecatedProtectedPref.., and kSecurePref.. together
> was
> > easier to read.
> 
> I do prefer alphabetic.
> 
> The kSupervisedUserSettingsFilename is an artifact of a recent renaming, which
> I'm about to fix :)

Done (and changed kDeprecatedProtectedPreferencesFilename to
kProtectedPreferencesFilenameDeprecated so that it at least doesn't end up alone
at the top).

[Bernhard Bauer, 2014-06-20 14:16:29]

https://codereview.chromium.org/347793002/diff/1/base/prefs/json_pref_store.h
File base/prefs/json_pref_store.h (right):

https://codereview.chromium.org/347793002/diff/1/base/prefs/json_pref_store.h...
base/prefs/json_pref_store.h:55: // could be read from. If |pref_filename|
exists, |pref_alternate_filename|
On 2014/06/19 17:50:10, gab wrote:
> On 2014/06/19 17:20:20, Bernhard Bauer wrote:
> > On 2014/06/19 16:03:51, gab wrote:
> > > On 2014/06/19 15:38:09, Bernhard Bauer wrote:
> > > > Why do we delete the alternate pref file if the primary one exists?
> > > 
> > > The alternate pref file is the old name of the file, if we already have
the
> > > primary then its safe to delete the old one.
> > > 
> > > I guess that since the new name was introduced in this CL it's not really
> > > possible to have both (as the new one will always first be created by
moving
> > the
> > > old one), but I thought it was nice extra cleanup to avoid having the same
> > prefs
> > > lying around in various deprecated files.
> > 
> > Yeah, I'm just a bit wary of deleting a file that shouldn't exist anyway if
> > everything is going normal. I think I would be a little happier if we DCHECK
> > that the file doesn't exist, to document our assumptions and to be notified
> (at
> > least in Debug builds) if they are incorrect.
> 
> I'm not really worried of this assumption being incorrect on developer
machines,
> I'm more worried that some users end up in weird states (e.g., tampered users)
> where they have both yet one doesn't matter.

See, my immediate reaction to that would be to touch as little on disk as
possible, if we can simply use the new file :)

I don't feel strongly about though, so LGTM.

[gab, 2014-06-20 14:48:28]

Thanks!

https://codereview.chromium.org/347793002/diff/1/base/prefs/json_pref_store.h
File base/prefs/json_pref_store.h (right):

https://codereview.chromium.org/347793002/diff/1/base/prefs/json_pref_store.h...
base/prefs/json_pref_store.h:55: // could be read from. If |pref_filename|
exists, |pref_alternate_filename|
On 2014/06/20 14:16:28, Bernhard Bauer wrote:
> On 2014/06/19 17:50:10, gab wrote:
> > On 2014/06/19 17:20:20, Bernhard Bauer wrote:
> > > On 2014/06/19 16:03:51, gab wrote:
> > > > On 2014/06/19 15:38:09, Bernhard Bauer wrote:
> > > > > Why do we delete the alternate pref file if the primary one exists?
> > > > 
> > > > The alternate pref file is the old name of the file, if we already have
> the
> > > > primary then its safe to delete the old one.
> > > > 
> > > > I guess that since the new name was introduced in this CL it's not
really
> > > > possible to have both (as the new one will always first be created by
> moving
> > > the
> > > > old one), but I thought it was nice extra cleanup to avoid having the
same
> > > prefs
> > > > lying around in various deprecated files.
> > > 
> > > Yeah, I'm just a bit wary of deleting a file that shouldn't exist anyway
if
> > > everything is going normal. I think I would be a little happier if we
DCHECK
> > > that the file doesn't exist, to document our assumptions and to be
notified
> > (at
> > > least in Debug builds) if they are incorrect.
> > 
> > I'm not really worried of this assumption being incorrect on developer
> machines,
> > I'm more worried that some users end up in weird states (e.g., tampered
users)
> > where they have both yet one doesn't matter.
> 
> See, my immediate reaction to that would be to touch as little on disk as
> possible, if we can simply use the new file :)
> 
> I don't feel strongly about though, so LGTM.

Thanks, as discussed this shouldn't happen in normal checkouts and it will work
just fine in unexpected checkouts if we don't clean up the old file. Therefore I
removed the DeleteFile as you suggested.

[gab, 2014-06-20 14:48:31]

The CQ bit was checked by gab@chromium.org

[commit-bot: I haz the power, 2014-06-20 14:50:40]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/gab@chromium.org/347793002/30001

[commit-bot: I haz the power, 2014-06-20 18:29:50]

Change committed as 278776