Adding a previews object, PreviewsDataSavings

components/previews/previews_data_savings.h

+// 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.
+
+#ifndef CHROME_BROWSER_PREVIEWS_PREVIEWS_DATA_SAVINGS_H_
+#define CHROME_BROWSER_PREVIEWS_PREVIEWS_DATA_SAVINGS_H_
+
+#include <stdint.h>
+
+#include <string>
+
+#include "base/macros.h"
+#include "base/threading/thread_checker.h"
+
+namespace data_reduction_proxy {
+class DataSavingsRecorder;
+class DataSaverStatus;
+}
+
+namespace previews {
+
+// Provides an interface for previews/ to record data savings and report is as

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

This class shouldn't care how it is going to be used. Say:

// Provides an interface for previews to report data savings.

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

Done.

+// part of the data savings seen by data saver users.
+class PreviewsDataSavings {

[Not at Google. Contact bengr, 2016/08/25 19:24:48]

Would it make sense for PreviewsDataSavings to be a subclass of
DataSavingsRecorder?

That way, DataReductionProxyService, which currently calls UpdateDataSavings()
on DRPCompressionStats could also call the method on other DataSavingsRecorders
such as PreviewsDataSavings.

RecordDataSavings() has the same signature as UpdateDataSavings() so seems
better to give them the same name.

Eventually would be nice to do this within the page size measuring
infrastructure so we have savings for individual pages that could be potentially
exposed to users.

[RyanSturm, 2016/08/25 20:22:45]

DataReductionProxy shouldn't know anything about previews/ (so the
DataSavingsRecorder interface needs to be in DRP code or another common code
area). Previews shouldn't really expose anything about DRP to consumers to
maintain layering, so Offline and other consumers would not even use the
DataSavingsRecorder interface since they only know about previews, not DRP.
I would hope that DRPService wouldn't call UpdateDataSavings on
PreviewsDataSavings and on DRPCompressionStats, as PreviewsDataSavings calls
into DRPCompressionStats and it would double count.
While I could name them the same thing, Consumers of this API are trying to
record data savings, whereas this class knows calling into DRPCompressionStats
is an update as opposed to just recording. Consumers of this class don't need to
understand how data recording is implemented, so they don't need to know about
update vs non-update and the API should reflect that.
I suspect the DRPCompressionStats class can move the core functionality to the
new component (that is what I was hoping for eventually). At that point,
DataReductionProxy and Previews can both use page_size::DataSavingsRecorder as
the API interface into data recording. Obviously that interface can be expanded
to include the UpdatecontentLengths method (or a rename of that method), so DRP
can use the interface. I still think that PreviewsDataSavings class won't want
to look exactly like the DataSavingsRecorder interface at that point either.

Overall, this class is exposed to consumers of previews and needs to keep
consumers from knowing about DRP and other components as best as possible. While
previews depends on DRP, the consumers of previews shouldn't have any idea about
DRP.

[Not at Google. Contact bengr, 2016/08/26 00:36:10]

How will this class be initialized and hooked up? 

It seems to me that as it is currently written, DRP must know about this class.
This is because the data use information is available on the IO thread and DRP
is making the jump to copy it over to the UI thread. This class depends on this
information being on the UI thread since it is not doing this jump itself.

Ideally, both components would not know about the other. However, not sure if
this is possible in practice. If both are running simultaneously, does
computation of data savings require collaboration between them?

Also, can you please clarify what you mean by DRPCompressionStats is doing an
update whereas this class is recording? Isn't UpdateContentLengths() basically
just recording data use and savings? Or am I missing something?

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

Still not finalized; here is a CL with a possibility:
https://codereview.chromium.org/2257693004/
DRP doesn't need to know about this class. DRP isn't going to rely on this. DRP
will report data savings using UpdateContentLengths as it already does, instead
of DRP -> previews -> DRP it will stay as DRP -> DRP. Eventually, it might be
DRP -> data_savings_component and previews -> data_savings_component.
//components/ have strict layering meaning if A depends on B, B can't depend on
A, directly or indirectly.

In a sense, yes, they do coordinate. There are fundamentally two types of ways
to record data savings, an UPDATE and a non-UPDATE. The typical path for
non-UPDATE is through UpdateContentLengths (how DRP currently operates), this
will record the data used at the URLRequest level, so original and actual data
are reported for the URLRequest. An UPDATE would generally come from a
page-level optimization such as offline pages -- since the actual data is
already recorded, an UPDATE will only change the original content length by the
the difference between original content length and content length (see
UpdateDataSavings). Ultimately, I imagine a lot of the data recording code could
move to a data_recorder (or another name) component that would record all of the
bytes used, and data reduction proxy and previews would independently be able to
report Updates to the data_recorder that would affect original content length.
However, for now, by calling UpdateDataSavings, original content length will be
added to without modifying content length.
UpdateContentLengths is recording, but UpdateDataSavings is only modifying
original content length; the distinction is that even if a non-zero actual bytes
used is passed in here, actual bytes used won't change. Previews understands the
distinction between UPDATES and non-UPDATES, but non-DRP consumers of previews
don't need to understand this distinction.

The overall design is *not* perfect, but without another component that both DRP
and previews can depend on, and significant refactoring, there's not a better
layering for consumers of previews to data savings (excluding the possibility of
callbacks if we decide that layering is better).

This design seems to be a step in the right direction for DRP and previews to
share data recording, but it is not the end of the story.


Let me know if there is a place I can add more comments that would clarify
anything.

+ public:
+  PreviewsDataSavings(

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

This needs a comment.

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

Done.

+      data_reduction_proxy::DataSavingsRecorder* data_savings_recorder,
+      data_reduction_proxy::DataSaverStatus* data_reduction_proxy_settings);
+  ~PreviewsDataSavings();
+
+  // Records the amount of data used by that preview, and the amount of data

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

that -> the
without -> if the original page had been loaded instead of the preview.

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

Done.

+  // that would have been used without the preview. |is_update| allows
+  // adjustment to previosuly recorded data (all URLRequest data_used is

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

previously

Also, what does "adjustment" mean in the context and why is it necessary?

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

It's not, done.

+  // tracked). |host| is the host name to attribute the data savings to (e.g.

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

Is host the right term? FQDM might be better.

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

Done.

+  // codereview.chromium.org).
+  void RecordDataSavings(const std::string& host,
+                         int64_t data_used,
+                         int64_t original_size);
+
+ private:
+  // Owned by DataReductionProxyService, which will outlive the WebContents

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

This interface shouldn't know anything about its embedder. Just say that it is
owned by the embedder and expected to be valid for the lifetime of this.

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

Done.

+  // that ultimately owns |this|.
+  // Provides a method to record data savings.
+  data_reduction_proxy::DataSavingsRecorder* data_savings_recorder_;
+  // Owned by DataReductionProxyService, which will outlive the WebContents

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

Here too.

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

Done.

+  // that ultimately owns |this|.
+  // Provides a method to determine if data saver is enabled.
+  data_reduction_proxy::DataSaverStatus* data_saver_status_;

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

It might be better to store a callback instead of a raw pointer here. Also, this
class shouldn't know anything about Data Saver. I suppose it just needs to know
if data savings should be tracked.

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

See other comment.

+
+  // Enforce usage on the UI thread.
+  base::ThreadChecker thread_checker_;
+
+  DISALLOW_COPY_AND_ASSIGN(PreviewsDataSavings);
+};
+
+}  // namespace previews
+
+#endif  // CHROME_BROWSER_PREVIEWS_PREVIEWS_DATA_SAVINGS_H_