Changes requested by gab for previously committed CLs.

Changes requested by gab for previously committed CLs.

Mainly nits related to preference protection.

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

[erikwright (departed), 2014-04-02 13:07:21]

gab: PTAL.

[gab, 2014-04-02 17:02:15]

https://codereview.chromium.org/220203011/diff/1/chrome/browser/prefs/pref_ha...
File chrome/browser/prefs/pref_hash_filter_unittest.cc (right):

https://codereview.chromium.org/220203011/diff/1/chrome/browser/prefs/pref_ha...
chrome/browser/prefs/pref_hash_filter_unittest.cc:890:
source->SetString(kReportOnlyPref, "helloworld");
I think a comment on each pref specifying what it's testing specifically would
make this test easier to read (i.e. from the comments a reader could tell what
each pref tests without having to parse the entire test).

https://codereview.chromium.org/220203011/diff/1/chrome/browser/prefs/tracked...
File chrome/browser/prefs/tracked/segregated_pref_store.cc (right):

https://codereview.chromium.org/220203011/diff/1/chrome/browser/prefs/tracked...
chrome/browser/prefs/tracked/segregated_pref_store.cc:173:
default_pref_store_->CommitPendingWrite();
Add a comment as to why this commit is required, i.e.:

// Commit |default_pref_store_| to guarantee that the migrated value is flushed
to disk before the removal from |selected_pref_store_| is eventually flushed to
disk.

https://codereview.chromium.org/220203011/diff/1/chrome/browser/prefs/tracked...
chrome/browser/prefs/tracked/segregated_pref_store.cc:175: value = NULL;
Why even do this? It doesn't seem to matter that |value| keeps its pointer value
until the end of this method.

[erikwright (departed), 2014-04-03 13:25:47]

PTAL. Note that this is based off of my other CL under review by you, so the
inter-patchset diff contains some extra stuff.

Sorry about that.

https://codereview.chromium.org/220203011/diff/1/chrome/browser/prefs/pref_ha...
File chrome/browser/prefs/pref_hash_filter_unittest.cc (right):

https://codereview.chromium.org/220203011/diff/1/chrome/browser/prefs/pref_ha...
chrome/browser/prefs/pref_hash_filter_unittest.cc:890:
source->SetString(kReportOnlyPref, "helloworld");
On 2014/04/02 17:02:15, gab wrote:
> I think a comment on each pref specifying what it's testing specifically would
> make this test easier to read (i.e. from the comments a reader could tell what
> each pref tests without having to parse the entire test).

Done.

https://codereview.chromium.org/220203011/diff/1/chrome/browser/prefs/tracked...
File chrome/browser/prefs/tracked/segregated_pref_store.cc (right):

https://codereview.chromium.org/220203011/diff/1/chrome/browser/prefs/tracked...
chrome/browser/prefs/tracked/segregated_pref_store.cc:173:
default_pref_store_->CommitPendingWrite();
On 2014/04/02 17:02:15, gab wrote:
> Add a comment as to why this commit is required, i.e.:
> 
> // Commit |default_pref_store_| to guarantee that the migrated value is
flushed
> to disk before the removal from |selected_pref_store_| is eventually flushed
to
> disk.

Done.

https://codereview.chromium.org/220203011/diff/1/chrome/browser/prefs/tracked...
chrome/browser/prefs/tracked/segregated_pref_store.cc:175: value = NULL;
On 2014/04/02 17:02:15, gab wrote:
> Why even do this? It doesn't seem to matter that |value| keeps its pointer
value
> until the end of this method.

AfterRemoveValue, |value| is invalid.

If this 'if' block is later extended, that can lead to a bug.

[gab, 2014-04-03 15:47:12]

lgtm w/ nits from https://codereview.chromium.org/218583003/ and one more
comment below.

Cheers,
Gab

https://codereview.chromium.org/220203011/diff/1/chrome/browser/prefs/tracked...
File chrome/browser/prefs/tracked/segregated_pref_store.cc (right):

https://codereview.chromium.org/220203011/diff/1/chrome/browser/prefs/tracked...
chrome/browser/prefs/tracked/segregated_pref_store.cc:175: value = NULL;
On 2014/04/03 13:25:47, erikwright wrote:
> On 2014/04/02 17:02:15, gab wrote:
> > Why even do this? It doesn't seem to matter that |value| keeps its pointer
> value
> > until the end of this method.
> 
> AfterRemoveValue, |value| is invalid.
> 
> If this 'if' block is later extended, that can lead to a bug.

Oh right, good point.

https://codereview.chromium.org/220203011/diff/20001/chrome/browser/prefs/pre...
File chrome/browser/prefs/pref_hash_filter_unittest.cc (right):

https://codereview.chromium.org/220203011/diff/20001/chrome/browser/prefs/pre...
chrome/browser/prefs/pref_hash_filter_unittest.cc:898:
destination->SetString(kAtomicPref2, "foobar2 preexisting");
This line should go with the previous block, right?

i.e., 

// If enforcing, should be discarded due to pre-existing value in
// |destination|.
source->SetString(kAtomicPref2, "foobar2");
destination->SetString(kAtomicPref2, "foobar2 preexisting");
mock_pref_hash_store_->SetCheckResult(kAtomicPref2,
                                      PrefHashStoreTransaction::UNCHANGED);

[gab, 2014-04-04 14:42:47]

https://codereview.chromium.org/220203011/diff/20001/chrome/browser/prefs/pre...
File chrome/browser/prefs/pref_hash_filter.cc (right):

https://codereview.chromium.org/220203011/diff/20001/chrome/browser/prefs/pre...
chrome/browser/prefs/pref_hash_filter.cc:128: source->CommitPendingWrite();
Actually I just realized that these unconditional commits are bad.

Since MigrateValues is called every startup this may induce unnecessary IO early
on startup.

We should only do these commits if a value was actually migrated above.

(in practice this may not matter on untampered chromes since I think the only
writes that can actually have happened at this point are resets and migrations
-- and if nothing was written, the commits will no-op -- so maybe we don't need
to merge this, but still a necessary cleanup IMO)

[erikwright (departed), 2014-04-04 19:15:48]

PTAL and re-l g t m.

https://codereview.chromium.org/220203011/diff/20001/chrome/browser/prefs/pre...
File chrome/browser/prefs/pref_hash_filter.cc (right):

https://codereview.chromium.org/220203011/diff/20001/chrome/browser/prefs/pre...
chrome/browser/prefs/pref_hash_filter.cc:128: source->CommitPendingWrite();
On 2014/04/04 14:42:48, gab wrote:
> Actually I just realized that these unconditional commits are bad.
> 
> Since MigrateValues is called every startup this may induce unnecessary IO
early
> on startup.
> 
> We should only do these commits if a value was actually migrated above.
> 
> (in practice this may not matter on untampered chromes since I think the only
> writes that can actually have happened at this point are resets and migrations
> -- and if nothing was written, the commits will no-op -- so maybe we don't
need
> to merge this, but still a necessary cleanup IMO)

Done.

https://codereview.chromium.org/220203011/diff/20001/chrome/browser/prefs/pre...
File chrome/browser/prefs/pref_hash_filter_unittest.cc (right):

https://codereview.chromium.org/220203011/diff/20001/chrome/browser/prefs/pre...
chrome/browser/prefs/pref_hash_filter_unittest.cc:898:
destination->SetString(kAtomicPref2, "foobar2 preexisting");
On 2014/04/03 15:47:13, gab wrote:
> This line should go with the previous block, right?
> 
> i.e., 
> 
> // If enforcing, should be discarded due to pre-existing value in
> // |destination|.
> source->SetString(kAtomicPref2, "foobar2");
> destination->SetString(kAtomicPref2, "foobar2 preexisting");
> mock_pref_hash_store_->SetCheckResult(kAtomicPref2,
>                                       PrefHashStoreTransaction::UNCHANGED);

Well, I wanted to also document what was expected to happen to the pre-existing
value. So to do that I separated them.

[gab, 2014-04-04 19:33:24]

lgtm w/ comment

https://codereview.chromium.org/220203011/diff/20001/chrome/browser/prefs/pre...
File chrome/browser/prefs/pref_hash_filter_unittest.cc (right):

https://codereview.chromium.org/220203011/diff/20001/chrome/browser/prefs/pre...
chrome/browser/prefs/pref_hash_filter_unittest.cc:898:
destination->SetString(kAtomicPref2, "foobar2 preexisting");
On 2014/04/04 19:15:48, erikwright wrote:
> On 2014/04/03 15:47:13, gab wrote:
> > This line should go with the previous block, right?
> > 
> > i.e., 
> > 
> > // If enforcing, should be discarded due to pre-existing value in
> > // |destination|.
> > source->SetString(kAtomicPref2, "foobar2");
> > destination->SetString(kAtomicPref2, "foobar2 preexisting");
> > mock_pref_hash_store_->SetCheckResult(kAtomicPref2,
> >                                       PrefHashStoreTransaction::UNCHANGED);
> 
> Well, I wanted to also document what was expected to happen to the
pre-existing
> value. So to do that I separated them.

Ok, then at least add a new line between this and the comment on the next line
regarding foobar3.

[gab, 2014-04-09 20:02:20]

On 2014/04/04 19:33:24, gab wrote:
> lgtm w/ comment
> 
>
https://codereview.chromium.org/220203011/diff/20001/chrome/browser/prefs/pre...
> File chrome/browser/prefs/pref_hash_filter_unittest.cc (right):
> 
>
https://codereview.chromium.org/220203011/diff/20001/chrome/browser/prefs/pre...
> chrome/browser/prefs/pref_hash_filter_unittest.cc:898:
> destination->SetString(kAtomicPref2, "foobar2 preexisting");
> On 2014/04/04 19:15:48, erikwright wrote:
> > On 2014/04/03 15:47:13, gab wrote:
> > > This line should go with the previous block, right?
> > > 
> > > i.e., 
> > > 
> > > // If enforcing, should be discarded due to pre-existing value in
> > > // |destination|.
> > > source->SetString(kAtomicPref2, "foobar2");
> > > destination->SetString(kAtomicPref2, "foobar2 preexisting");
> > > mock_pref_hash_store_->SetCheckResult(kAtomicPref2,
> > >                                      
PrefHashStoreTransaction::UNCHANGED);
> > 
> > Well, I wanted to also document what was expected to happen to the
> pre-existing
> > value. So to do that I separated them.
> 
> Ok, then at least add a new line between this and the comment on the next line
> regarding foobar3.

ping? should we land this now?

[erikwright (departed), 2014-04-10 00:22:24]

The CQ bit was checked by erikwright@chromium.org

[commit-bot: I haz the power, 2014-04-10 00:25:30]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/erikwright@chromium.org/220203011/60001

[commit-bot: I haz the power, 2014-04-10 13:25:57]

Change committed as 262962