Update DataReductionProxyNetworkDelegate/BypassStats to post tasks safely.

Update DataReductionProxyNetworkDelegate/BypassStats to post tasks safely.

- DRP NetworkDelegate/BypassStats are no longer aware of the UI thread.
- Add thread hopping functionality to DRP IOData/Service, using the
  correct WeakPtr to post tasks safely.
- Construct BypassStats in the IOData constructor.

BUG=472290
Committed: https://crrev.com/d0856baaed6f3817a96c657e4eab4b201655bb3a
Cr-Commit-Position: refs/heads/master@{#324375}

[jeremyim, 2015-04-03 08:02:40]

jeremyim@chromium.org changed reviewers:
+ bengr@chromium.org

[jeremyim, 2015-04-03 08:02:41]

PTAL!

[bengr, 2015-04-06 17:48:05]

a few comments to get started...

https://codereview.chromium.org/1054363002/diff/20001/components/data_reducti...
File
components/data_reduction_proxy/core/browser/data_reduction_proxy_bypass_stats.cc
(right):

https://codereview.chromium.org/1054363002/diff/20001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_bypass_stats.cc:108:
scoped_refptr<base::SingleThreadTaskRunner> io_task_runner)
I don't see why you need an io task runner. Consider removing.

https://codereview.chromium.org/1054363002/diff/20001/components/data_reducti...
File
components/data_reduction_proxy/core/browser/data_reduction_proxy_io_data.cc
(right):

https://codereview.chromium.org/1054363002/diff/20001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_io_data.cc:68:
// synchronously on the IO thread.
and this object outlives the caller, which this object owns.

https://codereview.chromium.org/1054363002/diff/20001/components/data_reducti...
File
components/data_reduction_proxy/core/browser/data_reduction_proxy_service.cc
(right):

https://codereview.chromium.org/1054363002/diff/20001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_service.cc:44:
void DataReductionProxyService::UpdateContentLengthPrefs(
As much as possible, DRPService should avoid business logic, and should just map
to the underlying UI objects' methods.

https://codereview.chromium.org/1054363002/diff/20001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_service.cc:60:
compression_stats_->SetInt64(
This seems like a horrible layering violation. Why is SetInt64 public?  No one
outside of DRPCompressionStats should know about the prefs that it manages.
Please change this and related methods to private. I'm open to the idea of
providing a method called:

void IncreaseContentLengthTotals(int64 original_length, int64 received_length);


possibly with a bool return. The method should get, update and set.

(I'm open to a better name.)

https://codereview.chromium.org/1054363002/diff/20001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_service.cc:64:
UpdateContentLengthPrefsForDataReductionProxy(
Uggh. Why is this static in metrics? Can this be a method (non-static) of
CompressionStats?

Can this be called by IncreaseContentLengthTotals instead?

[jeremyim, 2015-04-06 20:43:56]

Patchset #3 (id:40001) has been deleted

[jeremyim, 2015-04-06 21:26:28]

Patchset #3 (id:60001) has been deleted

[jeremyim, 2015-04-06 21:27:23]

https://codereview.chromium.org/1054363002/diff/20001/components/data_reducti...
File
components/data_reduction_proxy/core/browser/data_reduction_proxy_bypass_stats.cc
(right):

https://codereview.chromium.org/1054363002/diff/20001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_bypass_stats.cc:108:
scoped_refptr<base::SingleThreadTaskRunner> io_task_runner)
On 2015/04/06 17:48:04, bengr wrote:
> I don't see why you need an io task runner. Consider removing.

Done.

https://codereview.chromium.org/1054363002/diff/20001/components/data_reducti...
File
components/data_reduction_proxy/core/browser/data_reduction_proxy_io_data.cc
(right):

https://codereview.chromium.org/1054363002/diff/20001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_io_data.cc:68:
// synchronously on the IO thread.
On 2015/04/06 17:48:04, bengr wrote:
> and this object outlives the caller, which this object owns.

Done.

https://codereview.chromium.org/1054363002/diff/20001/components/data_reducti...
File
components/data_reduction_proxy/core/browser/data_reduction_proxy_service.cc
(right):

https://codereview.chromium.org/1054363002/diff/20001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_service.cc:44:
void DataReductionProxyService::UpdateContentLengthPrefs(
On 2015/04/06 17:48:04, bengr wrote:
> As much as possible, DRPService should avoid business logic, and should just
map
> to the underlying UI objects' methods.

Done.

https://codereview.chromium.org/1054363002/diff/20001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_service.cc:60:
compression_stats_->SetInt64(
On 2015/04/06 17:48:04, bengr wrote:
> This seems like a horrible layering violation. Why is SetInt64 public?  No one
> outside of DRPCompressionStats should know about the prefs that it manages.
> Please change this and related methods to private. I'm open to the idea of
> providing a method called:
> 
> void IncreaseContentLengthTotals(int64 original_length, int64
received_length);
> 
> 
> possibly with a bool return. The method should get, update and set.
> 
> (I'm open to a better name.)

Done.

https://codereview.chromium.org/1054363002/diff/20001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_service.cc:64:
UpdateContentLengthPrefsForDataReductionProxy(
On 2015/04/06 17:48:04, bengr wrote:
> Uggh. Why is this static in metrics? Can this be a method (non-static) of
> CompressionStats?
> 
> Can this be called by IncreaseContentLengthTotals instead?

Done.

[bengr, 2015-04-07 00:20:35]

https://codereview.chromium.org/1054363002/diff/60002/components/data_reducti...
File
components/data_reduction_proxy/core/browser/data_reduction_proxy_compression_stats.cc
(right):

https://codereview.chromium.org/1054363002/diff/60002/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_compression_stats.cc:29:
// Returns the |index|'th value of |list_value| as an int64.
"th" doesn't always work as a suffix.

https://codereview.chromium.org/1054363002/diff/60002/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_compression_stats.cc:301:
}  // namespace
I'm assuming everything in this anonymous namespace was moved without
modification.

https://codereview.chromium.org/1054363002/diff/60002/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_compression_stats.cc:492:
for (size_t i = 0; i < kNumDaysInHistory; ++i) {
Remove curlies.

https://codereview.chromium.org/1054363002/diff/60002/components/data_reducti...
File
components/data_reduction_proxy/core/browser/data_reduction_proxy_compression_stats.h
(right):

https://codereview.chromium.org/1054363002/diff/60002/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_compression_stats.h:62:
int64 GetLastUpdateJsTime();
Why JsTime?

https://codereview.chromium.org/1054363002/diff/60002/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_compression_stats.h:64:
// Resets certain statistics when the Data Reduction Proxy is enabled after
Which ones, and why?

https://codereview.chromium.org/1054363002/diff/60002/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_compression_stats.h:107:
void SetInt64(const char* pref_path, int64 pref_value);
Thank you.

https://codereview.chromium.org/1054363002/diff/60002/components/data_reducti...
File
components/data_reduction_proxy/core/browser/data_reduction_proxy_io_data_unittest.cc
(right):

https://codereview.chromium.org/1054363002/diff/60002/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_io_data_unittest.cc:117:
EXPECT_NE(nullptr, io_data->bypass_stats());
Why the change?

[jeremyim, 2015-04-07 06:21:11]

https://codereview.chromium.org/1054363002/diff/60002/components/data_reducti...
File
components/data_reduction_proxy/core/browser/data_reduction_proxy_compression_stats.cc
(right):

https://codereview.chromium.org/1054363002/diff/60002/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_compression_stats.cc:29:
// Returns the |index|'th value of |list_value| as an int64.
On 2015/04/07 00:20:35, bengr wrote:
> "th" doesn't always work as a suffix.

Done.

https://codereview.chromium.org/1054363002/diff/60002/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_compression_stats.cc:301:
}  // namespace
On 2015/04/07 00:20:35, bengr wrote:
> I'm assuming everything in this anonymous namespace was moved without
> modification.

Mostly:
- GetInt64PrefValue came from DRPsettings, the rest from DRPMetrics
- DailyDataSavingsUpdate/DailyContentLengthUpdate used to take CompressionStats
in the constructor; now they take the base::ListValue directly

https://codereview.chromium.org/1054363002/diff/60002/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_compression_stats.cc:492:
for (size_t i = 0; i < kNumDaysInHistory; ++i) {
On 2015/04/07 00:20:35, bengr wrote:
> Remove curlies.

Done.

https://codereview.chromium.org/1054363002/diff/60002/components/data_reducti...
File
components/data_reduction_proxy/core/browser/data_reduction_proxy_compression_stats.h
(right):

https://codereview.chromium.org/1054363002/diff/60002/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_compression_stats.h:62:
int64 GetLastUpdateJsTime();
On 2015/04/07 00:20:35, bengr wrote:
> Why JsTime?

Fixed. Updated comment too (which was a originally incorrect).

https://codereview.chromium.org/1054363002/diff/60002/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_compression_stats.h:64:
// Resets certain statistics when the Data Reduction Proxy is enabled after
On 2015/04/07 00:20:35, bengr wrote:
> Which ones, and why?

Done.

Looks like they are reset the first time DRP is ever (maybe) enabled. There's an
interesting TODO at
https://code.google.com/p/chromium/codesearch#chromium/src/components/data_re...
related to this.

https://codereview.chromium.org/1054363002/diff/60002/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_compression_stats.h:107:
void SetInt64(const char* pref_path, int64 pref_value);
On 2015/04/07 00:20:35, bengr wrote:
> Thank you.

Acknowledged.

https://codereview.chromium.org/1054363002/diff/60002/components/data_reducti...
File
components/data_reduction_proxy/core/browser/data_reduction_proxy_io_data_unittest.cc
(right):

https://codereview.chromium.org/1054363002/diff/60002/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_io_data_unittest.cc:117:
EXPECT_NE(nullptr, io_data->bypass_stats());
On 2015/04/07 00:20:35, bengr wrote:
> Why the change?

Bypass stats are now created in the constructor.

[bengr, 2015-04-09 03:09:09]

lgtm

[jeremyim, 2015-04-09 03:10:05]

The CQ bit was checked by jeremyim@chromium.org

[commit-bot: I haz the power, 2015-04-09 03:11:03]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1054363002/110001

[commit-bot: I haz the power, 2015-04-09 04:48:00]

Committed patchset #5 (id:110001)

[commit-bot: I haz the power, 2015-04-09 04:48:39]

Patchset 5 (id:??) landed as
https://crrev.com/d0856baaed6f3817a96c657e4eab4b201655bb3a
Cr-Commit-Position: refs/heads/master@{#324375}