Update data reduction proxy statistics prefs less often on desktop

components/data_reduction_proxy/browser/data_reduction_proxy_delayed_pref_service.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 "components/data_reduction_proxy/browser/data_reduction_proxy_delayed_pref_service.h"
+
+#include "base/bind.h"
+#include "base/location.h"
+#include "base/prefs/pref_service.h"
+#include "base/prefs/scoped_user_pref_update.h"
+#include "base/sequenced_task_runner.h"
+#include "base/strings/string_number_conversions.h"
+#include "base/time/time.h"
+#include "base/values.h"
+
+namespace data_reduction_proxy {
+
+DataReductionProxyDelayedPrefService::DataReductionProxyDelayedPrefService(
+    PrefService* prefs,
+    scoped_refptr<base::SequencedTaskRunner> task_runner,
+    base::TimeDelta delay)
+    : pref_service_(prefs),
+      task_runner_(task_runner),
+      delay_(delay),
+      delayed_task_posted_(false) {
+  DCHECK(prefs);

[bengr, 2014/08/14 17:39:42]

Also, DCHECK that delay is non-negative

[megjablon, 2014/08/26 19:28:40]

Done.

+  if (delay_ <= base::TimeDelta()) {

[bengr, 2014/08/14 17:39:42]

Don't you mean >=?. Also, afaict, these don't need to be on the heap.

[megjablon, 2014/08/26 19:28:41]

Done.

+    pref_map_ = new DataReductionProxyPrefMap();
+    list_pref_map_ = new DataReductionProxyListPrefMap();
+  }
+}
+
+DataReductionProxyDelayedPrefService::~DataReductionProxyDelayedPrefService() {
+  WritePrefs();
+}
+
+int64 DataReductionProxyDelayedPrefService::GetInt64(const char* pref_path) {
+  if (!(delay_ <= base::TimeDelta())) {

[bengr, 2014/08/14 17:39:41]

if (delay_ == base::TimeDelta())
  return pref_service_->GetInt64(pref_path);

[megjablon, 2014/08/26 19:28:41]

Done.

+    return pref_service_->GetInt64(pref_path);
+  }
+
+  DataReductionProxyPrefMap::iterator iter = pref_map_->find(pref_path);
+  if (iter == pref_map_->end()) {
+    int64 pref_value = pref_service_->GetInt64(pref_path);
+    pref_map_->insert(std::pair<const char*, int64>(pref_path, pref_value));

[bengr, 2014/08/14 17:39:42]

You can do pref_map_[pref_path] = pref_value, I think. 

[megjablon, 2014/08/26 19:28:40]

Done.

+    return pref_value;
+  }
+  return iter->second;
+}
+
+void DataReductionProxyDelayedPrefService::SetInt64(const char* pref_path,
+                                                    int64 pref_value) {
+  if (!(delay_ <= base::TimeDelta())) {

[bengr, 2014/08/14 17:39:41]

if (delay_ == base::TimeDelta())
  ...

[megjablon, 2014/08/26 19:28:41]

Done.

+    pref_service_->SetInt64(pref_path, pref_value);
+    return;
+  }
+
+  if (!delayed_task_posted_)
+    WritePrefsAndPost();
+  DataReductionProxyPrefMap::iterator iter = pref_map_->find(pref_path);

[bengr, 2014/08/14 17:39:42]

Can you factor out the initialization of the maps? Two ways to do this:
1) create an init function that takes a list of pref names and reads those prefs
into the map. This is more efficient, but requires you to name all the prefs up
front.
2) Write a function called MaybeAddPrefToMap() that reads the pref and writes to
the map if necessary.

[megjablon, 2014/08/26 19:28:40]

Done.

+  if (iter == pref_map_->end()) {
+    int64 pref_value = pref_service_->GetInt64(pref_path);
+    pref_map_->insert(std::pair<const char*, int64>(pref_path, pref_value));

[bengr, 2014/08/14 17:39:41]

again, use []

[megjablon, 2014/08/26 19:28:40]

Done.

+  }
+  (*pref_map_)[pref_path] = pref_value;
+}
+
+base::ListValue* DataReductionProxyDelayedPrefService::GetList(
+    const char* pref_path) {
+  if (!(delay_ <= base::TimeDelta())) {
+    return ListPrefUpdate(pref_service_, pref_path).Get();
+  }
+
+  if (!delayed_task_posted_)
+    WritePrefsAndPost();
+  DataReductionProxyListPrefMap::iterator iter =
+      list_pref_map_->find(pref_path);
+  if (iter == list_pref_map_->end()) {
+    base::ListValue* pref_value = pref_service_->GetList(pref_path)->DeepCopy();

[bengr, 2014/08/14 17:39:41]

This looks like a memory leak. Where do you deallocate this? Maybe use a
scoped_ptr?

[megjablon, 2014/08/26 19:28:40]

Using a ScopedPtrHashMap

[megjablon, 2014/08/26 19:28:40]

Done.

+    list_pref_map_->insert(std::pair<const char*, base::ListValue*>
+                      (pref_path, pref_value));
+    return pref_value;
+  }
+  return iter->second;
+}
+
+void DataReductionProxyDelayedPrefService::WritePrefs() {
+  for(DataReductionProxyPrefMap::iterator iter = pref_map_->begin();

[bengr, 2014/08/14 17:39:41]

for (

[megjablon, 2014/08/26 19:28:41]

Done.

+      iter != pref_map_->end(); ++iter) {
+    pref_service_->SetInt64(iter->first, iter->second);
+  }
+
+  for (DataReductionProxyListPrefMap::iterator iter = list_pref_map_->begin();
+      iter != list_pref_map_->end(); ++iter) {

[bengr, 2014/08/14 17:39:41]

indent one more

[megjablon, 2014/08/26 19:28:40]

Done.

+    TransferList(*(iter->second),
+                 ListPrefUpdate(pref_service_, iter->first).Get());
+  }
+}
+
+void DataReductionProxyDelayedPrefService::WritePrefsAndPost() {
+  // Only write after the first time posting the task
+  if (!delayed_task_posted_) {
+    delayed_task_posted_ = true;
+  } else {
+    WritePrefs();
+  }
+  task_runner_->PostDelayedTask(
+      FROM_HERE,
+      base::Bind(&DataReductionProxyDelayedPrefService::WritePrefsAndPost,
+                 base::Unretained(this)),

[bengr, 2014/08/14 17:39:41]

What happens after this is destroyed? I think you need to use a
CancelableTaskTracker

[bengr, 2014/08/14 23:09:49]

Ignore that. I think you can just pass a WeakPtr instead of
base::Unretained(this)

[megjablon, 2014/08/26 19:28:40]

Done.

+                 delay_);
+}
+
+void DataReductionProxyDelayedPrefService::TransferList(
+    const base::ListValue& from_list,
+    base::ListValue* to_list) {
+  to_list->Clear();
+  for (size_t i = 0; i < from_list.GetSize(); ++i) {
+    int64 value = GetListPrefInt64Value(from_list, i);
+    to_list->Set(i, new base::StringValue(base::Int64ToString(value)));

[bengr, 2014/08/14 17:39:41]

you can combine this line with the previous.

[megjablon, 2014/08/26 19:28:41]

Done.

+  }
+}
+
+int64 DataReductionProxyDelayedPrefService::GetListPrefInt64Value(
+    const base::ListValue& list,
+    size_t index) {
+  std::string string_value;
+  if (!list.GetString(index, &string_value)) {
+    NOTREACHED();
+    return 0;
+  }
+
+  int64 value = 0;
+  bool rv = base::StringToInt64(string_value, &value);
+  DCHECK(rv);
+  return value;
+}
+
+} // namespace data_reduction_proxy