Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2257533003:
Adding a previews object, PreviewsDataSavings (Closed)
|
Created:
4 years, 4 months ago by RyanSturm Modified:
4 years, 3 months ago CC:
chromium-reviews, blundell+watchlist_chromium.org, sdefresne+watchlist_chromium.org, asvitkine+watch_chromium.org, droger+watchlist_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdding a previews object, PreviewsDataSavings
This object hooks into existing data saver code to track savings based
on host, data used, and original size of the data.
BUG=615565
Committed: https://crrev.com/794e7d9b05188df4006282ae68f7b7596fda625d
Cr-Commit-Position: refs/heads/master@{#417359}
Patch Set 1 #Patch Set 2 : Changing histograms #Patch Set 3 : moving histograms to another CL #Patch Set 4 : updating dependent cl #
Total comments: 26
Patch Set 5 : bengr comments #Patch Set 6 : fixing a comment #Patch Set 7 : rebase and design change. #
Total comments: 6
Patch Set 8 : rebase & bengr comments #
Total comments: 2
Patch Set 9 : bengr comment #Dependent Patchsets: Messages
Total messages: 47 (32 generated)
This depends on https://codereview.chromium.org/2249903004/ and https://codereview.chromium.org/2255603002/ (adding two depends on patchsets didn't seem possible)
The CQ bit was checked by ryansturm@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
ryansturm@chromium.org changed reviewers: + bengr@chromium.org
bengr: PTAL @ *
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
kundaji@chromium.org changed reviewers: + kundaji@chromium.org
Happened to see a data use/savings cl, so took a quick look. https://codereview.chromium.org/2257533003/diff/60001/components/previews/pre... File components/previews/previews_data_savings.h (right): https://codereview.chromium.org/2257533003/diff/60001/components/previews/pre... components/previews/previews_data_savings.h:24: class PreviewsDataSavings { 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.
https://codereview.chromium.org/2257533003/diff/60001/components/previews/pre... File components/previews/previews_data_savings.h (right): https://codereview.chromium.org/2257533003/diff/60001/components/previews/pre... components/previews/previews_data_savings.h:24: class PreviewsDataSavings { On 2016/08/25 19:24:48, kundaji wrote: > Would it make sense for PreviewsDataSavings to be a subclass of > DataSavingsRecorder? 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. > That way, DataReductionProxyService, which currently calls UpdateDataSavings() > on DRPCompressionStats could also call the method on other DataSavingsRecorders > such as PreviewsDataSavings. > I would hope that DRPService wouldn't call UpdateDataSavings on PreviewsDataSavings and on DRPCompressionStats, as PreviewsDataSavings calls into DRPCompressionStats and it would double count. > RecordDataSavings() has the same signature as UpdateDataSavings() so seems > better to give them the same name. > 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. > 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. 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.
Here are some random comments. https://codereview.chromium.org/2257533003/diff/60001/components/previews/pre... File components/previews/previews_data_savings.cc (right): https://codereview.chromium.org/2257533003/diff/60001/components/previews/pre... components/previews/previews_data_savings.cc:32: // Only record savings when data saver is enabled. Again, this class shouldn't know about Data Saver. https://codereview.chromium.org/2257533003/diff/60001/components/previews/pre... components/previews/previews_data_savings.cc:33: if (!data_savings_recorder_ || !data_saver_status_ || 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. https://codereview.chromium.org/2257533003/diff/60001/components/previews/pre... components/previews/previews_data_savings.cc:34: !data_saver_status_->IsDataSaverEnabled()) { Use a callback called should_record_data_savings_ or something similar. https://codereview.chromium.org/2257533003/diff/60001/components/previews/pre... File components/previews/previews_data_savings.h (right): https://codereview.chromium.org/2257533003/diff/60001/components/previews/pre... components/previews/previews_data_savings.h:22: // Provides an interface for previews/ to record data savings and report is as This class shouldn't care how it is going to be used. Say: // Provides an interface for previews to report data savings. https://codereview.chromium.org/2257533003/diff/60001/components/previews/pre... components/previews/previews_data_savings.h:26: PreviewsDataSavings( This needs a comment. https://codereview.chromium.org/2257533003/diff/60001/components/previews/pre... components/previews/previews_data_savings.h:31: // Records the amount of data used by that preview, and the amount of data that -> the without -> if the original page had been loaded instead of the preview. https://codereview.chromium.org/2257533003/diff/60001/components/previews/pre... components/previews/previews_data_savings.h:33: // adjustment to previosuly recorded data (all URLRequest data_used is previously Also, what does "adjustment" mean in the context and why is it necessary? https://codereview.chromium.org/2257533003/diff/60001/components/previews/pre... components/previews/previews_data_savings.h:34: // tracked). |host| is the host name to attribute the data savings to (e.g. Is host the right term? FQDM might be better. https://codereview.chromium.org/2257533003/diff/60001/components/previews/pre... components/previews/previews_data_savings.h:41: // Owned by DataReductionProxyService, which will outlive the WebContents 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. https://codereview.chromium.org/2257533003/diff/60001/components/previews/pre... components/previews/previews_data_savings.h:45: // Owned by DataReductionProxyService, which will outlive the WebContents Here too. https://codereview.chromium.org/2257533003/diff/60001/components/previews/pre... components/previews/previews_data_savings.h:48: data_reduction_proxy::DataSaverStatus* data_saver_status_; 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.
https://codereview.chromium.org/2257533003/diff/60001/components/previews/pre... File components/previews/previews_data_savings.h (right): https://codereview.chromium.org/2257533003/diff/60001/components/previews/pre... components/previews/previews_data_savings.h:24: class PreviewsDataSavings { On 2016/08/25 20:22:45, RyanSturm wrote: > On 2016/08/25 19:24:48, kundaji wrote: > > Would it make sense for PreviewsDataSavings to be a subclass of > > DataSavingsRecorder? > > 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. > > > That way, DataReductionProxyService, which currently calls UpdateDataSavings() > > on DRPCompressionStats could also call the method on other > DataSavingsRecorders > > such as PreviewsDataSavings. > > > > I would hope that DRPService wouldn't call UpdateDataSavings on > PreviewsDataSavings and on DRPCompressionStats, as PreviewsDataSavings calls > into DRPCompressionStats and it would double count. > > > RecordDataSavings() has the same signature as UpdateDataSavings() so seems > > better to give them the same name. > > > > 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. > > > 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. > > 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. > 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?
The CQ bit was checked by ryansturm@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #5 (id:80001) has been deleted
Dry run: Try jobs failed on following builders: android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...) android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...) android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...) cast_shell_android on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...) linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...) cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...) chromeos_amd64-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...) chromeos_x86-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_x86-ge...) chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...) linux_chromium_chromeos_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by ryansturm@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
bengr, kundaji: PTAL, thanks https://codereview.chromium.org/2257533003/diff/60001/components/previews/pre... File components/previews/previews_data_savings.cc (right): https://codereview.chromium.org/2257533003/diff/60001/components/previews/pre... components/previews/previews_data_savings.cc:32: // Only record savings when data saver is enabled. On 2016/08/25 23:04:19, bengr wrote: > Again, this class shouldn't know about Data Saver. 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. https://codereview.chromium.org/2257533003/diff/60001/components/previews/pre... components/previews/previews_data_savings.cc:33: if (!data_savings_recorder_ || !data_saver_status_ || On 2016/08/25 23:04:19, bengr wrote: > 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. Good point, these were left over from when this was a KeyedService using Shutdown. https://codereview.chromium.org/2257533003/diff/60001/components/previews/pre... components/previews/previews_data_savings.cc:34: !data_saver_status_->IsDataSaverEnabled()) { On 2016/08/25 23:04:19, bengr wrote: > Use a callback called should_record_data_savings_ or something similar. See other comment. https://codereview.chromium.org/2257533003/diff/60001/components/previews/pre... File components/previews/previews_data_savings.h (right): https://codereview.chromium.org/2257533003/diff/60001/components/previews/pre... components/previews/previews_data_savings.h:22: // Provides an interface for previews/ to record data savings and report is as On 2016/08/25 23:04:19, bengr wrote: > This class shouldn't care how it is going to be used. Say: > > // Provides an interface for previews to report data savings. Done. https://codereview.chromium.org/2257533003/diff/60001/components/previews/pre... components/previews/previews_data_savings.h:24: class PreviewsDataSavings { On 2016/08/26 00:36:10, kundaji wrote: > On 2016/08/25 20:22:45, RyanSturm wrote: > > On 2016/08/25 19:24:48, kundaji wrote: > > > Would it make sense for PreviewsDataSavings to be a subclass of > > > DataSavingsRecorder? > > > > 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. > > > > > That way, DataReductionProxyService, which currently calls > UpdateDataSavings() > > > on DRPCompressionStats could also call the method on other > > DataSavingsRecorders > > > such as PreviewsDataSavings. > > > > > > > I would hope that DRPService wouldn't call UpdateDataSavings on > > PreviewsDataSavings and on DRPCompressionStats, as PreviewsDataSavings calls > > into DRPCompressionStats and it would double count. > > > > > RecordDataSavings() has the same signature as UpdateDataSavings() so seems > > > better to give them the same name. > > > > > > > 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. > > > > > 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. > > > > 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. > > > > How will this class be initialized and hooked up? > Still not finalized; here is a CL with a possibility: https://codereview.chromium.org/2257693004/ > 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. > 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. > 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? > //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. > 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? > > 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. https://codereview.chromium.org/2257533003/diff/60001/components/previews/pre... components/previews/previews_data_savings.h:26: PreviewsDataSavings( On 2016/08/25 23:04:19, bengr wrote: > This needs a comment. Done. https://codereview.chromium.org/2257533003/diff/60001/components/previews/pre... components/previews/previews_data_savings.h:31: // Records the amount of data used by that preview, and the amount of data On 2016/08/25 23:04:19, bengr wrote: > that -> the > without -> if the original page had been loaded instead of the preview. Done. https://codereview.chromium.org/2257533003/diff/60001/components/previews/pre... components/previews/previews_data_savings.h:33: // adjustment to previosuly recorded data (all URLRequest data_used is On 2016/08/25 23:04:19, bengr wrote: > previously > > Also, what does "adjustment" mean in the context and why is it necessary? It's not, done. https://codereview.chromium.org/2257533003/diff/60001/components/previews/pre... components/previews/previews_data_savings.h:34: // tracked). |host| is the host name to attribute the data savings to (e.g. On 2016/08/25 23:04:19, bengr wrote: > Is host the right term? FQDM might be better. Done. https://codereview.chromium.org/2257533003/diff/60001/components/previews/pre... components/previews/previews_data_savings.h:41: // Owned by DataReductionProxyService, which will outlive the WebContents On 2016/08/25 23:04:19, bengr wrote: > 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. Done. https://codereview.chromium.org/2257533003/diff/60001/components/previews/pre... components/previews/previews_data_savings.h:45: // Owned by DataReductionProxyService, which will outlive the WebContents On 2016/08/25 23:04:19, bengr wrote: > Here too. Done. https://codereview.chromium.org/2257533003/diff/60001/components/previews/pre... components/previews/previews_data_savings.h:48: data_reduction_proxy::DataSaverStatus* data_saver_status_; On 2016/08/25 23:04:19, bengr wrote: > 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. See other comment.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by ryansturm@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2257533003/diff/140001/components/data_reduct... File components/data_reduction_proxy/core/common/data_savings_recorder.h (right): https://codereview.chromium.org/2257533003/diff/140001/components/data_reduct... components/data_reduction_proxy/core/common/data_savings_recorder.h:22: virtual void UpdateDataSavings(const std::string& data_usage_host, Should this only be called if ShouldRecordDataSavings() returns true? How is that enforced? https://codereview.chromium.org/2257533003/diff/140001/components/data_reduct... components/data_reduction_proxy/core/common/data_savings_recorder.h:26: // Whether data savings should be recorded. Make this a complete sentence: Returns true if data savings should be recorded. https://codereview.chromium.org/2257533003/diff/140001/components/previews/pr... File components/previews/previews_data_savings.cc (right): https://codereview.chromium.org/2257533003/diff/140001/components/previews/pr... components/previews/previews_data_savings.cc:30: if (!data_savings_recorder_->ShouldRecordDataSavings()) { Can this be an implementation detail of UpdateDataSavings?
The CQ bit was checked by ryansturm@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by ryansturm@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #8 (id:160001) has been deleted
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Bengr: ptal https://codereview.chromium.org/2257533003/diff/140001/components/data_reduct... File components/data_reduction_proxy/core/common/data_savings_recorder.h (right): https://codereview.chromium.org/2257533003/diff/140001/components/data_reduct... components/data_reduction_proxy/core/common/data_savings_recorder.h:22: virtual void UpdateDataSavings(const std::string& data_usage_host, On 2016/08/31 22:02:25, bengr wrote: > Should this only be called if ShouldRecordDataSavings() returns true? How is > that enforced? I'm combining the methods to return a bool and do the right thing instead of letting the caller make the decision. https://codereview.chromium.org/2257533003/diff/140001/components/data_reduct... components/data_reduction_proxy/core/common/data_savings_recorder.h:26: // Whether data savings should be recorded. On 2016/08/31 22:02:26, bengr wrote: > Make this a complete sentence: Returns true if data savings should be recorded. Done. https://codereview.chromium.org/2257533003/diff/140001/components/previews/pr... File components/previews/previews_data_savings.cc (right): https://codereview.chromium.org/2257533003/diff/140001/components/previews/pr... components/previews/previews_data_savings.cc:30: if (!data_savings_recorder_->ShouldRecordDataSavings()) { On 2016/08/31 22:02:26, bengr wrote: > Can this be an implementation detail of UpdateDataSavings? Done.
See last nit. lgtm otherwise. https://codereview.chromium.org/2257533003/diff/180001/components/data_reduct... File components/data_reduction_proxy/core/common/data_savings_recorder.h (right): https://codereview.chromium.org/2257533003/diff/180001/components/data_reduct... components/data_reduction_proxy/core/common/data_savings_recorder.h:22: // Returns true if data savings recording is allowed. Shouldn't this return true if data savings were recorded?
The CQ bit was checked by ryansturm@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by ryansturm@chromium.org
thanks for the review. https://codereview.chromium.org/2257533003/diff/180001/components/data_reduct... File components/data_reduction_proxy/core/common/data_savings_recorder.h (right): https://codereview.chromium.org/2257533003/diff/180001/components/data_reduct... components/data_reduction_proxy/core/common/data_savings_recorder.h:22: // Returns true if data savings recording is allowed. On 2016/09/08 00:24:40, bengr wrote: > Shouldn't this return true if data savings were recorded? Done.
The CQ bit was checked by ryansturm@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from bengr@chromium.org Link to the patchset: https://codereview.chromium.org/2257533003/#ps200001 (title: "bengr comment")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Committed patchset #9 (id:200001)
Message was sent while issue was closed.
Description was changed from ========== Adding a previews object, PreviewsDataSavings This object hooks into existing data saver code to track savings based on host, data used, and original size of the data. BUG=615565 ========== to ========== Adding a previews object, PreviewsDataSavings This object hooks into existing data saver code to track savings based on host, data used, and original size of the data. BUG=615565 Committed: https://crrev.com/794e7d9b05188df4006282ae68f7b7596fda625d Cr-Commit-Position: refs/heads/master@{#417359} ==========
Message was sent while issue was closed.
Patchset 9 (id:??) landed as https://crrev.com/794e7d9b05188df4006282ae68f7b7596fda625d Cr-Commit-Position: refs/heads/master@{#417359} |
