Abstract pref storage from net::SdchOwner

chrome/browser/net/sdch_owner_pref_storage.cc

+// Copyright 2015 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/net/sdch_owner_pref_storage.h"
+
+#include "base/prefs/persistent_pref_store.h"
+#include "base/values.h"
+
+namespace chrome_browser_net {
+
+SdchOwnerPrefStorage::SdchOwnerPrefStorage(PersistentPrefStore* storage)
+    : storage_(storage),
+      storage_key_("SDCH"),

[Randy Smith (Not in Mondays), 2016/01/27 21:48:17]

I'd like this to be abstracted out into a constant.  In an ideal world, there
would be some way that multiple different clients using the same
PersistentPrefStore could automatically confirm that they weren't colliding on
the same key, but I assume for the moment that's not a concern so I won't worry
about it.

+      init_observer_(nullptr) {
+}
+
+SdchOwnerPrefStorage::~SdchOwnerPrefStorage() {
+  if (init_observer_)
+    storage_->RemoveObserver(this);
+}
+
+net::SdchOwner::PrefStorage::ReadError
+SdchOwnerPrefStorage::GetReadError() const {
+  PersistentPrefStore::PrefReadError error = storage_->GetReadError();
+
+  DCHECK_NE(error,
+            PersistentPrefStore::PREF_READ_ERROR_ASYNCHRONOUS_TASK_INCOMPLETE);
+  DCHECK_NE(error, PersistentPrefStore::PREF_READ_ERROR_MAX_ENUM);
+
+  switch (error) {
+    case PersistentPrefStore::PREF_READ_ERROR_NONE:
+      return PERSISTENCE_FAILURE_NONE;
+
+    case PersistentPrefStore::PREF_READ_ERROR_NO_FILE:
+      return PERSISTENCE_FAILURE_REASON_NO_FILE;
+
+    case PersistentPrefStore::PREF_READ_ERROR_JSON_PARSE:
+    case PersistentPrefStore::PREF_READ_ERROR_JSON_TYPE:
+    case PersistentPrefStore::PREF_READ_ERROR_FILE_OTHER:
+    case PersistentPrefStore::PREF_READ_ERROR_FILE_LOCKED:
+    case PersistentPrefStore::PREF_READ_ERROR_JSON_REPEAT:
+      return PERSISTENCE_FAILURE_REASON_READ_FAILED;
+
+    case PersistentPrefStore::PREF_READ_ERROR_ACCESS_DENIED:
+    case PersistentPrefStore::PREF_READ_ERROR_FILE_NOT_SPECIFIED:
+    case PersistentPrefStore::PREF_READ_ERROR_ASYNCHRONOUS_TASK_INCOMPLETE:
+    case PersistentPrefStore::PREF_READ_ERROR_MAX_ENUM:
+    default:
+      // This may seem a bit odd but our goal here is to map errors we expect
+      // to a histogram that the SDCH system cares about. We don't expect to
+      // get these errors with our usage, and there is no histogram value for
+      // them, which is why we return "none" rather than having a general
+      // "other" error value.

[Randy Smith (Not in Mondays), 2016/01/27 21:48:17]

Could you say a bit more about your logic here?  I would imagine that we'd want
to map these to an "other" error value, so that there's the possibility of UMA
raising a flag if unanticipated things start happening.  If we map to NONE, I
don't think there's any way we'd ever notice.

[brettw, 2016/01/27 22:23:54]

I was trying to avoid updating the histogram but now can't think of why it's
necessary to avoid doing that. I added OTHER here and to the histogram.

+      NOTREACHED();
+      return PERSISTENCE_FAILURE_NONE;
+  }
+}
+
+bool SdchOwnerPrefStorage::GetValue(
+      const base::DictionaryValue** result) const {
+  const base::Value* result_value = nullptr;
+  if (!storage_->GetValue(storage_key_, &result_value))
+    return false;
+  return result_value->GetAsDictionary(result);
+}
+
+bool SdchOwnerPrefStorage::GetMutableValue(base::DictionaryValue** result) {
+  base::Value* result_value = nullptr;
+  if (!storage_->GetMutableValue(storage_key_, &result_value))
+    return false;
+  return result_value->GetAsDictionary(result);
+}
+
+void SdchOwnerPrefStorage::SetValue(scoped_ptr<base::DictionaryValue> value) {
+  storage_->SetValue(storage_key_, std::move(value),
+                     WriteablePrefStore::DEFAULT_PREF_WRITE_FLAGS);
+}
+
+void SdchOwnerPrefStorage::ReportValueChanged() {
+  storage_->ReportValueChanged(storage_key_,
+                               WriteablePrefStore::DEFAULT_PREF_WRITE_FLAGS);
+}
+
+bool SdchOwnerPrefStorage::IsInitializationComplete() {
+  return storage_->IsInitializationComplete();
+}
+
+void SdchOwnerPrefStorage::StartObservingInit(net::SdchOwner* observer) {
+  DCHECK(!init_observer_);
+  init_observer_ = observer;
+  storage_->AddObserver(this);
+}
+
+void SdchOwnerPrefStorage::StopObservingInit() {
+  DCHECK(init_observer_);
+  init_observer_ = nullptr;
+  storage_->RemoveObserver(this);
+}
+
+void SdchOwnerPrefStorage::OnPrefValueChanged(const std::string& key) {
+}
+
+void SdchOwnerPrefStorage::OnInitializationCompleted(bool succeeded) {
+  init_observer_->OnPrefStorageInitializationComplete(succeeded);

[Randy Smith (Not in Mondays), 2016/01/27 21:48:17]

There's an implication in this line about the interface contract (i.e. you must
call StartObservingInit before the initialization could be complete, and you
can't call StopObservingInit() until after it has completed).  Would you be
willing to either document the requirements in the header file or relax them
here (presumably by testing init_observer_ for nullity)?   

(I'm torn about which of those is better, as testing init_observer_ for nullity
opens us up to silent races; I *think* I'd prefer documentation + some DCHECKs,
but I'm not sure, so I'll leaving it up to you.)

[brettw, 2016/01/27 22:23:54]

I changed the comments in the interface to make them a bit more precise and
added a DCHECK here for null. But I don't agree with what you wrote here. You
can register or unregister the observer whenever you want as long as there's
only one. This class will only be a pref store observer when the SdchOwner is
registered as an observer, so we'll only get here when the SdchOwner is
registered.

[Randy Smith (Not in Mondays), 2016/01/28 17:45:56]

Ah, gotcha.  That makes sense.  Sorry about the misunderstanding.

+}
+
+}  // namespace chrome_browser_net