Changes requested by gab for previously committed CLs.

chrome/browser/prefs/tracked/segregated_pref_store.cc

 // Copyright 2014 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "chrome/browser/prefs/tracked/segregated_pref_store.h"
 
 #include "base/logging.h"
 #include "base/stl_util.h"
 #include "base/values.h"
 
@@ skipping 17 matching lines @@
 void SegregatedPrefStore::AggregatingObserver::OnInitializationCompleted(
     bool succeeded) {
   if (succeeded)
     ++successful_sub_initializations_;
   else
     ++failed_sub_initializations_;
 
   DCHECK_LE(failed_sub_initializations_ + successful_sub_initializations_, 2);
 
   if (failed_sub_initializations_ + successful_sub_initializations_ == 2) {
-
     if (!outer_->on_initialization_.is_null())
       outer_->on_initialization_.Run();
 
     if (successful_sub_initializations_ == 2 && outer_->read_error_delegate_) {
       PersistentPrefStore::PrefReadError read_error = outer_->GetReadError();
       if (read_error != PersistentPrefStore::PREF_READ_ERROR_NONE)
         outer_->read_error_delegate_->OnError(read_error);
     }
 
     FOR_EACH_OBSERVER(
@@ skipping 95 matching lines @@
 void SegregatedPrefStore::CommitPendingWrite() {
   default_pref_store_->CommitPendingWrite();
   selected_pref_store_->CommitPendingWrite();
 }
 
 SegregatedPrefStore::~SegregatedPrefStore() {
   default_pref_store_->RemoveObserver(&aggregating_observer_);
   selected_pref_store_->RemoveObserver(&aggregating_observer_);
 }
 
-const PersistentPrefStore*
-SegregatedPrefStore::StoreForKey(const std::string& key) const {
+const PersistentPrefStore* SegregatedPrefStore::StoreForKey(
+    const std::string& key) const {
   if (ContainsKey(selected_preference_names_, key) ||
       selected_pref_store_->GetValue(key, NULL)) {
     return selected_pref_store_.get();
   }
   return default_pref_store_.get();
 }
 
 PersistentPrefStore* SegregatedPrefStore::StoreForKey(const std::string& key) {
   if (ContainsKey(selected_preference_names_, key))
     return selected_pref_store_.get();
 
   // Check if this unselected value was previously selected. If so, migrate it
   // back to the unselected store.
   // It's hard to do this in a single pass at startup because PrefStore does not
   // permit us to enumerate its contents.
   const base::Value* value = NULL;
   if (selected_pref_store_->GetValue(key, &value)) {
-    scoped_ptr<base::Value> migrated_value(value->DeepCopy());
+    default_pref_store_->SetValue(key, value->DeepCopy());
+    default_pref_store_->CommitPendingWrite();

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

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.

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

Done.

+
     value = NULL;

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

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]

AfterRemoveValue, |value| is invalid.

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

[gab, 2014/04/03 15:47:13]

Oh right, good point.

-    default_pref_store_->SetValue(key, migrated_value.release());
-    default_pref_store_->CommitPendingWrite();
     selected_pref_store_->RemoveValue(key);
-    selected_pref_store_->CommitPendingWrite();
   }
 
   return default_pref_store_.get();
 }