Changes requested by gab for previously committed CLs.

chrome/browser/prefs/pref_hash_filter_unittest.cc

Index: chrome/browser/prefs/pref_hash_filter_unittest.cc
diff --git a/chrome/browser/prefs/pref_hash_filter_unittest.cc b/chrome/browser/prefs/pref_hash_filter_unittest.cc
index 4fa0ab7233a7bebb631eff6c613b916e90e7e6fd..c576b8869161d0bd7a3e52eb50fd5929568f175f 100644
--- a/chrome/browser/prefs/pref_hash_filter_unittest.cc
+++ b/chrome/browser/prefs/pref_hash_filter_unittest.cc
@@ -29,6 +29,7 @@ namespace {
 const char kAtomicPref[] = "atomic_pref";
 const char kAtomicPref2[] = "atomic_pref2";
 const char kAtomicPref3[] = "pref3";
+const char kAtomicPref4[] = "pref4";
 const char kReportOnlyPref[] = "report_only";
 const char kReportOnlySplitPref[] = "report_only_split_pref";
 const char kSplitPref[] = "split_pref";
@@ -58,6 +59,10 @@ const PrefHashFilter::TrackedPreferenceMetadata kTestTrackedPrefs[] = {
     5, kAtomicPref3, PrefHashFilter::ENFORCE_ON_LOAD,
     PrefHashFilter::TRACKING_STRATEGY_ATOMIC
   },
+  {
+    6, kAtomicPref4, PrefHashFilter::ENFORCE_ON_LOAD,
+    PrefHashFilter::TRACKING_STRATEGY_ATOMIC
+  },
 };
 
 }  // namespace
@@ -309,9 +314,9 @@ class PrefHashFilterTest
   }
 
  protected:
-  // Creates a PrefHashFilter that uses a MockPrefHashStore. The
-  // MockPrefHashStore (owned by the PrefHashFilter) is returned in
-  // |mock_pref_hash_store|.
+  // Initializes |pref_hash_filter_| with a PrefHashFilter that uses a
+  // MockPrefHashStore. The raw pointer to the MockPrefHashStore (owned by the
+  // PrefHashFilter) is stored in |mock_pref_hash_store_|.
   void InitializePrefHashFilter(const std::vector<
       PrefHashFilter::TrackedPreferenceMetadata>& configuration) {
     scoped_ptr<MockPrefHashStore> temp_mock_pref_hash_store(
@@ -881,6 +886,7 @@ TEST_P(PrefHashFilterTest, MigrateValuesTest) {
   source->SetString(kAtomicPref2, "foobar2");
   destination->SetString(kAtomicPref2, "foobar2 preexisting");
   destination->SetString(kAtomicPref3, "foobar3");
+  source->SetString(kAtomicPref4, "foobar4");
   source->SetString(kReportOnlyPref, "helloworld");

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

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).

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

Done.

 
   mock_pref_hash_store_->SetCheckResult(kAtomicPref,
@@ -889,6 +895,8 @@ TEST_P(PrefHashFilterTest, MigrateValuesTest) {
                                         PrefHashStoreTransaction::UNCHANGED);
   mock_pref_hash_store_->SetCheckResult(kAtomicPref3,
                                         PrefHashStoreTransaction::UNCHANGED);
+  mock_pref_hash_store_->SetCheckResult(kAtomicPref4,
+                                        PrefHashStoreTransaction::CHANGED);
   mock_pref_hash_store_->SetCheckResult(kReportOnlyPref,
                                         PrefHashStoreTransaction::UNCHANGED);
 
@@ -901,6 +909,7 @@ TEST_P(PrefHashFilterTest, MigrateValuesTest) {
     ASSERT_FALSE(source->GetValue(kAtomicPref, NULL));
     ASSERT_FALSE(source->GetValue(kAtomicPref2, NULL));
     ASSERT_FALSE(source->GetValue(kAtomicPref3, NULL));
+    ASSERT_FALSE(source->GetValue(kAtomicPref4, NULL));
     ASSERT_TRUE(source->GetString(kReportOnlyPref, &value));
     ASSERT_EQ("helloworld", value);
 
@@ -911,6 +920,7 @@ TEST_P(PrefHashFilterTest, MigrateValuesTest) {
     ASSERT_TRUE(destination->GetString(kAtomicPref3, &value));
     ASSERT_EQ("foobar3", value);
     ASSERT_FALSE(destination->GetValue(kReportOnlyPref, NULL));
+    ASSERT_FALSE(destination->GetValue(kAtomicPref4, NULL));
   } else {
     std::string value;
 
@@ -919,13 +929,17 @@ TEST_P(PrefHashFilterTest, MigrateValuesTest) {
     ASSERT_TRUE(source->GetString(kAtomicPref2, &value));
     ASSERT_EQ("foobar2", value);
     ASSERT_FALSE(source->GetString(kAtomicPref3, &value));
+    ASSERT_TRUE(source->GetString(kAtomicPref4, &value));
+    ASSERT_EQ("foobar4", value);
     ASSERT_TRUE(source->GetString(kReportOnlyPref, &value));
     ASSERT_EQ("helloworld", value);
 
     ASSERT_FALSE(destination->GetValue(kAtomicPref, NULL));
     ASSERT_TRUE(destination->GetString(kAtomicPref2, &value));
     ASSERT_EQ("foobar2 preexisting", value);
-    ASSERT_TRUE(destination->GetValue(kAtomicPref3, NULL));
+    ASSERT_TRUE(destination->GetString(kAtomicPref3, &value));
+    ASSERT_EQ("foobar3", value);
+    ASSERT_FALSE(destination->GetValue(kAtomicPref4, NULL));
     ASSERT_FALSE(destination->GetValue(kReportOnlyPref, NULL));
   }
 }