Add a Restore() method to ValueStore and make StorageAPI use it

chrome/browser/extensions/api/storage/syncable_settings_storage.cc

 // Copyright (c) 2012 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/extensions/api/storage/syncable_settings_storage.h"
 
 #include "base/strings/stringprintf.h"
 #include "chrome/browser/extensions/api/storage/settings_namespace.h"
 #include "chrome/browser/extensions/api/storage/settings_sync_processor.h"
 #include "chrome/browser/extensions/api/storage/settings_sync_util.h"
@@ skipping 104 matching lines @@
 ValueStore::WriteResult SyncableSettingsStorage::Clear() {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE));
   WriteResult result = delegate_->Clear();
   if (result->HasError()) {
     return result.Pass();
   }
   SyncResultIfEnabled(result);
   return result.Pass();
 }
 
+bool SyncableSettingsStorage::Restore() {
+  // If we're syncing, stop - we don't want to push the deletion of any data.
+  StopSyncing();

[not at google - send to devlin, 2014/02/14 19:35:56]

For this method and below:

I would have thought that
(1) if restore is successful and
(2) we're syncing

then this class can actually properly restore its state from the sync processor.

did you plan to do this in a follow-up? Because maybe that would be best (though
add a TODO) since I already have this change in my head :)

[Devlin, 2014/02/18 23:55:22]

Right now, my understanding of what will happen is:
- We Restore() (hopefully successfully), and lose some (or all) data.
- At next startup, we'll sync, and get back any data we can because we take the
union of everything in sync and on disk, and we didn't break anything on sync
(hopefully).

It would be better if we can trigger this sync automatically, but I'm not sure
there's a way to do that.  Similarly, it might work that we could StopSyncing(),
Restore(), and StartSyncing().  I'll investigate more and find out.

[not at google - send to devlin, 2014/02/19 21:25:29]

Cool. This is basically the same discussion we had with the policy store then.
What you have is fine, just, I think the behaviour is subtle enough to warrant a
slightly more helpful comment.

[Devlin, 2014/02/19 23:12:28]

Already done on patch set 2.

+  return delegate_->Restore();
+}
+
+bool SyncableSettingsStorage::RestoreKey(const std::string& key) {
+  // If we're syncing, stop - we don't want to push the deletion of any data.
+  StopSyncing();
+  return delegate_->RestoreKey(key);
+}
+
 void SyncableSettingsStorage::SyncResultIfEnabled(
     const ValueStore::WriteResult& result) {
   if (result->changes().empty())
     return;
 
   if (sync_processor_.get()) {
     syncer::SyncError error = sync_processor_->SendChanges(result->changes());
     if (error.IsSet())
       StopSyncing();
   } else {
@@ skipping 259 matching lines @@
         syncer::SyncError::DATATYPE_ERROR,
         base::StringPrintf("Error pushing sync remove to local settings: %s",
             result->error().message.c_str()),
         sync_processor_->type());
   }
   changes->push_back(ValueStoreChange(key, old_value, NULL));
   return syncer::SyncError();
 }
 
 }  // namespace extensions