Adding a previews object, PreviewsDataSavings

components/previews/previews_data_savings.cc

+// Copyright 2016 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/previews/previews_data_savings.h"
+
+#include "base/logging.h"
+#include "base/metrics/histogram_macros.h"
+#include "components/data_reduction_proxy/core/common/data_saver_status.h"
+#include "components/data_reduction_proxy/core/common/data_savings_recorder.h"
+
+namespace previews {
+
+PreviewsDataSavings::PreviewsDataSavings(
+    data_reduction_proxy::DataSavingsRecorder* data_savings_recorder,
+    data_reduction_proxy::DataSaverStatus* data_saver_status)
+    : data_savings_recorder_(data_savings_recorder),
+      data_saver_status_(data_saver_status) {
+  DCHECK(data_savings_recorder);
+  DCHECK(data_saver_status);
+}
+
+PreviewsDataSavings::~PreviewsDataSavings() {
+  DCHECK(thread_checker_.CalledOnValidThread());
+}
+
+void PreviewsDataSavings::RecordDataSavings(const std::string& host,
+                                            int64_t data_used,
+                                            int64_t original_size) {
+  DCHECK(thread_checker_.CalledOnValidThread());
+
+  // Only record savings when data saver is enabled.

[bengr, 2016/08/25 23:04:19]

Again, this class shouldn't know about Data Saver.

[RyanSturm, 2016/08/26 17:47:39]

I think it's fundamental to previews/ to understand what data saver is. I can
understand the desire to use a callback, but I think explicit layering is more
clear with an interface and a raw pointer. I think a callback makes sense for a
class that is embedded in multiple ways with multiple embedders, but IMO
previews should be aware of data saver and data savings recorder.

I'm not opposed to changing these to callbacks, but I don't understand how
making them callbacks improves layering. Let me know some more detail on why
callbacks are better in this case. also, let me know if you have a different
understanding of DRP/previews layering than I do.

+  if (!data_savings_recorder_ || !data_saver_status_ ||

[bengr, 2016/08/25 23:04:19]

You don't need to check these if they're expected to be non-null. So long as you
document, you also don't need the DHECKs in the constructor, because it will be
painfully obvious why you're crashing.

[RyanSturm, 2016/08/26 17:47:39]

Good point, these were left over from when this was a KeyedService using
Shutdown.

+      !data_saver_status_->IsDataSaverEnabled()) {

[bengr, 2016/08/25 23:04:19]

Use a callback called should_record_data_savings_ or something similar.

[RyanSturm, 2016/08/26 17:47:39]

See other comment.

+    return;
+  }
+  data_savings_recorder_->UpdateDataSavings(host, data_used, original_size);
+}
+
+}  // namespace previews