DataReductionProxyStatisticsPrefs should support WeakPtr

DataReductionProxyStatisticsPrefs should support WeakPtr

The DRPStatisticsPrefs object is referenced via a raw pointer in
DRPNetworkDelegate and DRPSettings. It should instead be referenced
via WeakPtr.

BUG=453155, 455559, 444939
Committed: https://crrev.com/149bd770d674e8125f81cd649a8deece3a851682
Cr-Commit-Position: refs/heads/master@{#315419}

Committed: https://crrev.com/85594ccf495b97ccd4642dd5f5ab98e31d1d2003
Cr-Commit-Position: refs/heads/master@{#315621}

[megjablon, 2015-01-29 20:16:24]

Patchset #1 (id:1) has been deleted

[megjablon, 2015-01-29 20:20:24]

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

[megjablon, 2015-01-29 20:20:24]

https://codereview.chromium.org/888713002/diff/20001/components/data_reductio...
File
components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate_unittest.cc
(right):

https://codereview.chromium.org/888713002/diff/20001/components/data_reductio...
components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate_unittest.cc:463:
data_reduction_proxy::prefs::kDataReductionProxyEnabled, false);
This is copied from drp_metrics_unittest. Any suggestions on how to prevent this
duplication?

[bengr, 2015-01-30 17:48:10]

On 2015/01/29 20:20:24, megjablon wrote:
>
https://codereview.chromium.org/888713002/diff/20001/components/data_reductio...
> File
>
components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate_unittest.cc
> (right):
> 
>
https://codereview.chromium.org/888713002/diff/20001/components/data_reductio...
>
components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate_unittest.cc:463:
> data_reduction_proxy::prefs::kDataReductionProxyEnabled, false);
> This is copied from drp_metrics_unittest. Any suggestions on how to prevent
this
> duplication?

I think it's best to change ownership so that DRPSettings owns
DRPStatisticsPrefs, and passes a WeakPtr to IOData. I'll review once you make
that change.

[megjablon, 2015-02-03 00:45:52]

Patchset #3 (id:60001) has been deleted

[megjablon, 2015-02-03 02:38:56]

Patchset #6 (id:130001) has been deleted

[megjablon, 2015-02-03 02:39:05]

Patchset #4 (id:100001) has been deleted

[megjablon, 2015-02-03 02:40:25]

Patchset #5 (id:150001) has been deleted

[megjablon, 2015-02-03 18:52:14]

Patchset #5 (id:170001) has been deleted

[megjablon, 2015-02-03 19:56:09]

Patchset #3 (id:80001) has been deleted

[megjablon, 2015-02-03 19:56:16]

Patchset #3 (id:120001) has been deleted

[megjablon, 2015-02-03 19:56:25]

Patchset #3 (id:190001) has been deleted

[megjablon, 2015-02-03 19:57:59]

DRPSettings now owns DRPStatisticsPrefs

[bengr, 2015-02-03 23:51:17]

https://codereview.chromium.org/888713002/diff/210001/chrome/browser/net/spdy...
File chrome/browser/net/spdyproxy/data_reduction_proxy_chrome_io_data.cc
(right):

https://codereview.chromium.org/888713002/diff/210001/chrome/browser/net/spdy...
chrome/browser/net/spdyproxy/data_reduction_proxy_chrome_io_data.cc:47:
scoped_ptr<data_reduction_proxy::DataReductionProxyStatisticsPrefs>(
#include "base/memory/scoped_ptr.h"
#include
"components/data_reduction_proxy/core/browser/data_reduction_proxy_statistics_prefs.h"

https://codereview.chromium.org/888713002/diff/210001/chrome/browser/net/spdy...
File
chrome/browser/net/spdyproxy/data_reduction_proxy_settings_unittest_android.cc
(right):

https://codereview.chromium.org/888713002/diff/210001/chrome/browser/net/spdy...
chrome/browser/net/spdyproxy/data_reduction_proxy_settings_unittest_android.cc:86:
base::TimeDelta())));
#include "base/memory/ref_counted.h"
#include "base/memory/scoped_ptr.h"
#include "base/time/time.h"
#include
"components/data_reduction_proxy/core/browser/data_reduction_proxy_statistics_prefs.h"

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

https://codereview.chromium.org/888713002/diff/210001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_io_data.cc:43:
statistics_prefs_ = settings->statistics_prefs()->GetWeakPtr();
#include "...statistics_prefs.h"

https://codereview.chromium.org/888713002/diff/210001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_io_data.cc:70:
statistics_prefs_->ShutdownOnUIThread();
This should be don on Settings destruction, right, not here.

https://codereview.chromium.org/888713002/diff/210001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_io_data.cc:94:
settings_->SetDataReductionProxyStatisticsPrefs(
This shouldn't be in IOData.

[bengr, 2015-02-04 00:06:06]

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

https://codereview.chromium.org/888713002/diff/210001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_io_data.cc:94:
settings_->SetDataReductionProxyStatisticsPrefs(
On 2015/02/03 23:51:17, bengr wrote:
> This shouldn't be in IOData.

You can pass one into settings via InitDataReductionProxySettings, and you can
pass a WeakPtr to IOData in the implementation of that method too.

[megjablon, 2015-02-04 02:16:59]

Patchset #5 (id:250001) has been deleted

[megjablon, 2015-02-04 18:47:54]

https://codereview.chromium.org/888713002/diff/210001/chrome/browser/net/spdy...
File chrome/browser/net/spdyproxy/data_reduction_proxy_chrome_io_data.cc
(right):

https://codereview.chromium.org/888713002/diff/210001/chrome/browser/net/spdy...
chrome/browser/net/spdyproxy/data_reduction_proxy_chrome_io_data.cc:47:
scoped_ptr<data_reduction_proxy::DataReductionProxyStatisticsPrefs>(
On 2015/02/03 23:51:17, bengr wrote:
> #include "base/memory/scoped_ptr.h"
> #include
>
"components/data_reduction_proxy/core/browser/data_reduction_proxy_statistics_prefs.h"

scoped_ptr is included in the .h and statistics_prefs is included

https://codereview.chromium.org/888713002/diff/210001/chrome/browser/net/spdy...
File
chrome/browser/net/spdyproxy/data_reduction_proxy_settings_unittest_android.cc
(right):

https://codereview.chromium.org/888713002/diff/210001/chrome/browser/net/spdy...
chrome/browser/net/spdyproxy/data_reduction_proxy_settings_unittest_android.cc:86:
base::TimeDelta())));
On 2015/02/03 23:51:17, bengr wrote:
> #include "base/memory/ref_counted.h"
> #include "base/memory/scoped_ptr.h"
> #include "base/time/time.h"
> #include
>
"components/data_reduction_proxy/core/browser/data_reduction_proxy_statistics_prefs.h"

Done.

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

https://codereview.chromium.org/888713002/diff/210001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_io_data.cc:43:
statistics_prefs_ = settings->statistics_prefs()->GetWeakPtr();
On 2015/02/03 23:51:17, bengr wrote:
> #include "...statistics_prefs.h"

Don't need anymore.

https://codereview.chromium.org/888713002/diff/210001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_io_data.cc:70:
statistics_prefs_->ShutdownOnUIThread();
On 2015/02/03 23:51:17, bengr wrote:
> This should be don on Settings destruction, right, not here.

Done.

[bengr, 2015-02-04 23:39:11]

https://codereview.chromium.org/888713002/diff/270001/chrome/browser/net/spdy...
File chrome/browser/net/spdyproxy/data_reduction_proxy_chrome_io_data.cc
(right):

https://codereview.chromium.org/888713002/diff/270001/chrome/browser/net/spdy...
chrome/browser/net/spdyproxy/data_reduction_proxy_chrome_io_data.cc:14: #include
"components/data_reduction_proxy/core/browser/data_reduction_proxy_statistics_prefs.h"
No longer needed.

https://codereview.chromium.org/888713002/diff/270001/chrome/browser/net/spdy...
File chrome/browser/net/spdyproxy/data_reduction_proxy_chrome_settings.cc
(right):

https://codereview.chromium.org/888713002/diff/270001/chrome/browser/net/spdy...
chrome/browser/net/spdyproxy/data_reduction_proxy_chrome_settings.cc:97:
DataReductionProxySettings::statistics_prefs()->GetWeakPtr());
Add a comment to the declaration of InitDRPSettings that mentioned that io_data
is given a weak pointer to the statistics prefs that is owned by settings.

https://codereview.chromium.org/888713002/diff/270001/components/data_reducti...
File
components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate_unittest.cc
(right):

https://codereview.chromium.org/888713002/diff/270001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate_unittest.cc:92:
// loop_proxy_ is just the current message loop. This means loop_proxy_
What is loop_proxy_?

https://codereview.chromium.org/888713002/diff/270001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate_unittest.cc:426:
PrefRegistrySimple* registry = pref_service.registry();
Can't you use the DRP registry functions, e.g.,
https://code.google.com/p/chromium/codesearch#chromium/src/components/data_re...

https://codereview.chromium.org/888713002/diff/270001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate_unittest.cc:504:
DataReductionProxyHistoricNetworkStatsTest() {}
Move end curly to new line.

https://codereview.chromium.org/888713002/diff/270001/components/data_reducti...
File
components/data_reduction_proxy/core/browser/data_reduction_proxy_settings.cc
(right):

https://codereview.chromium.org/888713002/diff/270001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_settings.cc:109:
if (statistics_prefs_) {
#include statistics_prefs.h if you remove the include from the .h

https://codereview.chromium.org/888713002/diff/270001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_settings.cc:110:
statistics_prefs_->WritePrefs();
Verify that this works.

https://codereview.chromium.org/888713002/diff/270001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_settings.cc:133:
void DataReductionProxySettings::InitDataReductionProxySettings(
In the case of WebView, which is called first? This or
EnableCompressionStatisticsLogging? 

Suggest adding DCHECK(!statistics_prefs_) to enforce the ordering.

https://codereview.chromium.org/888713002/diff/270001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_settings.cc:168:
statistics_prefs_.reset(
DCHECK(!statistics_prefs_)

https://codereview.chromium.org/888713002/diff/270001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_settings.cc:170:
prefs, ui_task_runner, commit_delay));
Can this fit on the previous line?

https://codereview.chromium.org/888713002/diff/270001/components/data_reducti...
File
components/data_reduction_proxy/core/browser/data_reduction_proxy_settings.h
(right):

https://codereview.chromium.org/888713002/diff/270001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_settings.h:19:
#include
"components/data_reduction_proxy/core/browser/data_reduction_proxy_statistics_prefs.h"
Can this be forward declared here instead?

https://codereview.chromium.org/888713002/diff/270001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_settings.h:92:
// prefs is passed into the constructor.
Is it safe to call it if a valid statistics prefs was passed into the
constructor? I think probably not, so we should probably enforce that.

https://codereview.chromium.org/888713002/diff/270001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_settings.h:99:
void SetDataReductionProxyStatisticsPrefs(
Does this need to be public? Also, move this down after all the non-test
methods.

https://codereview.chromium.org/888713002/diff/270001/components/data_reducti...
File
components/data_reduction_proxy/core/browser/data_reduction_proxy_settings_test_utils.cc
(right):

https://codereview.chromium.org/888713002/diff/270001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_settings_test_utils.cc:90:
&pref_service_,
#include drp_statistics_prefs.h

https://codereview.chromium.org/888713002/diff/270001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_settings_test_utils.cc:97:
settings_->SetDataReductionProxyStatisticsPrefs(
Is this needed because Init is not called in every test? I'd prefer we don't
have this method.

https://codereview.chromium.org/888713002/diff/270001/components/data_reducti...
File
components/data_reduction_proxy/core/browser/data_reduction_proxy_statistics_prefs.cc
(right):

https://codereview.chromium.org/888713002/diff/270001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_statistics_prefs.cc:39:
void DataReductionProxyStatisticsPrefs::ShutdownOnUIThread() {
Since this will destroyed on the UI thread, can't you just move this stuff to
the destructor?

https://codereview.chromium.org/888713002/diff/270001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_statistics_prefs.cc:131:
LOG(WARNING) << "Starting write prefs";
Remove.

https://codereview.chromium.org/888713002/diff/270001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_statistics_prefs.cc:148:
LOG(WARNING) << "Finished write prefs";
Remove.

[megjablon, 2015-02-06 21:32:51]

Patchset #6 (id:290001) has been deleted

[megjablon, 2015-02-06 23:03:45]

Patchset #6 (id:310001) has been deleted

[megjablon, 2015-02-06 23:40:42]

https://codereview.chromium.org/888713002/diff/270001/chrome/browser/net/spdy...
File chrome/browser/net/spdyproxy/data_reduction_proxy_chrome_io_data.cc
(right):

https://codereview.chromium.org/888713002/diff/270001/chrome/browser/net/spdy...
chrome/browser/net/spdyproxy/data_reduction_proxy_chrome_io_data.cc:14: #include
"components/data_reduction_proxy/core/browser/data_reduction_proxy_statistics_prefs.h"
On 2015/02/04 23:39:10, bengr wrote:
> No longer needed.

Acknowledged.

https://codereview.chromium.org/888713002/diff/270001/chrome/browser/net/spdy...
File chrome/browser/net/spdyproxy/data_reduction_proxy_chrome_settings.cc
(right):

https://codereview.chromium.org/888713002/diff/270001/chrome/browser/net/spdy...
chrome/browser/net/spdyproxy/data_reduction_proxy_chrome_settings.cc:97:
DataReductionProxySettings::statistics_prefs()->GetWeakPtr());
On 2015/02/04 23:39:10, bengr wrote:
> Add a comment to the declaration of InitDRPSettings that mentioned that
io_data
> is given a weak pointer to the statistics prefs that is owned by settings.

Done.

https://codereview.chromium.org/888713002/diff/270001/components/data_reducti...
File
components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate_unittest.cc
(right):

https://codereview.chromium.org/888713002/diff/270001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate_unittest.cc:92:
// loop_proxy_ is just the current message loop. This means loop_proxy_
On 2015/02/04 23:39:11, bengr wrote:
> What is loop_proxy_?

Not sure what this comment was, just copied when moving this duplicate code.

https://codereview.chromium.org/888713002/diff/270001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate_unittest.cc:426:
PrefRegistrySimple* registry = pref_service.registry();
On 2015/02/04 23:39:10, bengr wrote:
> Can't you use the DRP registry functions, e.g.,
>
https://code.google.com/p/chromium/codesearch#chromium/src/components/data_re...
> 

Good to know about this

https://codereview.chromium.org/888713002/diff/270001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate_unittest.cc:504:
DataReductionProxyHistoricNetworkStatsTest() {}
On 2015/02/04 23:39:11, bengr wrote:
> Move end curly to new line.

Done.

https://codereview.chromium.org/888713002/diff/270001/components/data_reducti...
File
components/data_reduction_proxy/core/browser/data_reduction_proxy_settings.cc
(right):

https://codereview.chromium.org/888713002/diff/270001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_settings.cc:109:
if (statistics_prefs_) {
On 2015/02/04 23:39:11, bengr wrote:
> #include statistics_prefs.h if you remove the include from the .h

Done.

https://codereview.chromium.org/888713002/diff/270001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_settings.cc:110:
statistics_prefs_->WritePrefs();
On 2015/02/04 23:39:11, bengr wrote:
> Verify that this works.

Verified. Also moved to statistics prefs destructor.

https://codereview.chromium.org/888713002/diff/270001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_settings.cc:133:
void DataReductionProxySettings::InitDataReductionProxySettings(
On 2015/02/04 23:39:11, bengr wrote:
> In the case of WebView, which is called first? This or
> EnableCompressionStatisticsLogging? 
> 
> Suggest adding DCHECK(!statistics_prefs_) to enforce the ordering.

This is called first. Done.

https://codereview.chromium.org/888713002/diff/270001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_settings.cc:168:
statistics_prefs_.reset(
On 2015/02/04 23:39:11, bengr wrote:
> DCHECK(!statistics_prefs_)

Done.

https://codereview.chromium.org/888713002/diff/270001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_settings.cc:170:
prefs, ui_task_runner, commit_delay));
On 2015/02/04 23:39:11, bengr wrote:
> Can this fit on the previous line?

No. 2 characters too long.

https://codereview.chromium.org/888713002/diff/270001/components/data_reducti...
File
components/data_reduction_proxy/core/browser/data_reduction_proxy_settings.h
(right):

https://codereview.chromium.org/888713002/diff/270001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_settings.h:19:
#include
"components/data_reduction_proxy/core/browser/data_reduction_proxy_statistics_prefs.h"
On 2015/02/04 23:39:11, bengr wrote:
> Can this be forward declared here instead?

Done.

https://codereview.chromium.org/888713002/diff/270001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_settings.h:92:
// prefs is passed into the constructor.
On 2015/02/04 23:39:11, bengr wrote:
> Is it safe to call it if a valid statistics prefs was passed into the
> constructor? I think probably not, so we should probably enforce that.

Done.

https://codereview.chromium.org/888713002/diff/270001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_settings.h:99:
void SetDataReductionProxyStatisticsPrefs(
On 2015/02/04 23:39:11, bengr wrote:
> Does this need to be public? Also, move this down after all the non-test
> methods.

Yes, it needs to be public since DRPSettingsTestBase isn't a subclass.

https://codereview.chromium.org/888713002/diff/270001/components/data_reducti...
File
components/data_reduction_proxy/core/browser/data_reduction_proxy_settings_test_utils.cc
(right):

https://codereview.chromium.org/888713002/diff/270001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_settings_test_utils.cc:90:
&pref_service_,
On 2015/02/04 23:39:11, bengr wrote:
> #include drp_statistics_prefs.h

Already included.

https://codereview.chromium.org/888713002/diff/270001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_settings_test_utils.cc:97:
settings_->SetDataReductionProxyStatisticsPrefs(
On 2015/02/04 23:39:11, bengr wrote:
> Is this needed because Init is not called in every test? I'd prefer we don't
> have this method.

Ya, Init is not called for a lot of tests. Changing this will require fixing a
lot. If we want to change this, can we do it in another CL?

https://codereview.chromium.org/888713002/diff/270001/components/data_reducti...
File
components/data_reduction_proxy/core/browser/data_reduction_proxy_statistics_prefs.cc
(right):

https://codereview.chromium.org/888713002/diff/270001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_statistics_prefs.cc:39:
void DataReductionProxyStatisticsPrefs::ShutdownOnUIThread() {
On 2015/02/04 23:39:11, bengr wrote:
> Since this will destroyed on the UI thread, can't you just move this stuff to
> the destructor?

Done.

https://codereview.chromium.org/888713002/diff/270001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_statistics_prefs.cc:131:
LOG(WARNING) << "Starting write prefs";
On 2015/02/04 23:39:11, bengr wrote:
> Remove.

Done.

https://codereview.chromium.org/888713002/diff/270001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_statistics_prefs.cc:148:
LOG(WARNING) << "Finished write prefs";
On 2015/02/04 23:39:11, bengr wrote:
> Remove.

Done.

[bengr, 2015-02-06 23:56:15]

https://codereview.chromium.org/888713002/diff/270001/components/data_reducti...
File
components/data_reduction_proxy/core/browser/data_reduction_proxy_settings_test_utils.cc
(right):

https://codereview.chromium.org/888713002/diff/270001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_settings_test_utils.cc:90:
&pref_service_,
On 2015/02/06 23:40:42, megjablon wrote:
> On 2015/02/04 23:39:11, bengr wrote:
> > #include drp_statistics_prefs.h
> 
> Already included.

Ah. Missed that.

https://codereview.chromium.org/888713002/diff/270001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_settings_test_utils.cc:97:
settings_->SetDataReductionProxyStatisticsPrefs(
On 2015/02/06 23:40:42, megjablon wrote:
> On 2015/02/04 23:39:11, bengr wrote:
> > Is this needed because Init is not called in every test? I'd prefer we don't
> > have this method.
> 
> Ya, Init is not called for a lot of tests. Changing this will require fixing a
> lot. If we want to change this, can we do it in another CL?

Yes. Please file a bug.

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

https://codereview.chromium.org/888713002/diff/330001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_io_data.cc:81:
DataReductionProxyIOData::GetDataReductionProxyStatisticsPrefs() {
Why not call this statistics_prefs()?

https://codereview.chromium.org/888713002/diff/330001/components/data_reducti...
File components/data_reduction_proxy/core/browser/data_reduction_proxy_io_data.h
(right):

https://codereview.chromium.org/888713002/diff/330001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_io_data.h:86:
return statistics_prefs_weak_ptr_.get();
Who uses this now? I thought all accesses would be via WeakPtr.

https://codereview.chromium.org/888713002/diff/330001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_io_data.h:110:
scoped_ptr<DataReductionProxyStatisticsPrefs> statistics_prefs_scoped_ptr_;
Add a comment that the scoped_ptr is used only until settings initialization and
is dead there after.

https://codereview.chromium.org/888713002/diff/330001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_io_data.h:111:
base::WeakPtr<DataReductionProxyStatisticsPrefs> statistics_prefs_weak_ptr_;
Change the name to statistics_prefs_;

https://codereview.chromium.org/888713002/diff/330001/components/data_reducti...
File
components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate_unittest.cc
(right):

https://codereview.chromium.org/888713002/diff/330001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate_unittest.cc:94:
// message_loop_proxy() gets the current message loop. This means
Delete this comment. It doesn't make any sense to me.

[bengr, 2015-02-07 00:21:54]

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

https://codereview.chromium.org/888713002/diff/330001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_io_data.cc:81:
DataReductionProxyIOData::GetDataReductionProxyStatisticsPrefs() {
On 2015/02/06 23:56:15, bengr wrote:
> Why not call this statistics_prefs()?

Better, call it PassStatisticsPrefs()

[megjablon, 2015-02-07 00:38:57]

Patchset #7 (id:350001) has been deleted

[megjablon, 2015-02-07 00:48:29]

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

https://codereview.chromium.org/888713002/diff/330001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_io_data.cc:81:
DataReductionProxyIOData::GetDataReductionProxyStatisticsPrefs() {
On 2015/02/06 23:56:15, bengr wrote:
> Why not call this statistics_prefs()?

Done.

https://codereview.chromium.org/888713002/diff/330001/components/data_reducti...
File components/data_reduction_proxy/core/browser/data_reduction_proxy_io_data.h
(right):

https://codereview.chromium.org/888713002/diff/330001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_io_data.h:86:
return statistics_prefs_weak_ptr_.get();
On 2015/02/06 23:56:15, bengr wrote:
> Who uses this now? I thought all accesses would be via WeakPtr.

Done.

https://codereview.chromium.org/888713002/diff/330001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_io_data.h:110:
scoped_ptr<DataReductionProxyStatisticsPrefs> statistics_prefs_scoped_ptr_;
On 2015/02/06 23:56:15, bengr wrote:
> Add a comment that the scoped_ptr is used only until settings initialization
and
> is dead there after.

Done.

https://codereview.chromium.org/888713002/diff/330001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_io_data.h:111:
base::WeakPtr<DataReductionProxyStatisticsPrefs> statistics_prefs_weak_ptr_;
On 2015/02/06 23:56:15, bengr wrote:
> Change the name to statistics_prefs_;

Done.

https://codereview.chromium.org/888713002/diff/330001/components/data_reducti...
File
components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate_unittest.cc
(right):

https://codereview.chromium.org/888713002/diff/330001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate_unittest.cc:94:
// message_loop_proxy() gets the current message loop. This means
On 2015/02/06 23:56:15, bengr wrote:
> Delete this comment. It doesn't make any sense to me.

SGTM

[bengr, 2015-02-07 01:57:44]

lgtm with one issue.

https://codereview.chromium.org/888713002/diff/390001/components/data_reducti...
File components/data_reduction_proxy/core/browser/data_reduction_proxy_io_data.h
(right):

https://codereview.chromium.org/888713002/diff/390001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_io_data.h:58:
base::WeakPtr<DataReductionProxyStatisticsPrefs> statistics_prefs();
#include "base/memory/weak_ptr.h"

[megjablon, 2015-02-07 02:00:10]

New patchsets have been uploaded after l-g-t-m from bengr@chromium.org

[megjablon, 2015-02-07 02:00:33]

Patchset #9 (id:410001) has been deleted

[megjablon, 2015-02-07 03:18:44]

megjablon@chromium.org changed reviewers:
+ mmenke@chromium.org, sgurun@chromium.org

[megjablon, 2015-02-07 03:18:45]

sgurun: aw_browser_context.cc

mmenke: please look at ownership

https://codereview.chromium.org/888713002/diff/390001/components/data_reducti...
File components/data_reduction_proxy/core/browser/data_reduction_proxy_io_data.h
(right):

https://codereview.chromium.org/888713002/diff/390001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_io_data.h:58:
base::WeakPtr<DataReductionProxyStatisticsPrefs> statistics_prefs();
On 2015/02/07 01:57:44, bengr wrote:
> #include "base/memory/weak_ptr.h"

Done.

[mmenke, 2015-02-09 18:33:57]

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

https://codereview.chromium.org/888713002/diff/430001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_io_data.cc:82:
return temporary_statistics_prefs_.Pass();
This temporary ownership thing seems ugly.

The problem is the IOData object wants a weak pointer to this UI object, and the
main UI thread object wants pointers to two IO thread objects (The events store
and the URLRequestContextGetter).

To make things more complicated, the Getter can only be initialized only after
the ProxyIOData is good to go, so can't just make a single method to set them
both up, creating the shared objects first, before returning the IO and UI
objects for use.

I currently don't have a suggestion on how to do this more cleanly,
unfortunately.

[sgurun-gerrit only, 2015-02-09 22:07:15]

On 2015/02/09 18:33:57, mmenke wrote:
>
https://codereview.chromium.org/888713002/diff/430001/components/data_reducti...
> File
> components/data_reduction_proxy/core/browser/data_reduction_proxy_io_data.cc
> (right):
> 
>
https://codereview.chromium.org/888713002/diff/430001/components/data_reducti...
>
components/data_reduction_proxy/core/browser/data_reduction_proxy_io_data.cc:82:
> return temporary_statistics_prefs_.Pass();
> This temporary ownership thing seems ugly.
> 
> The problem is the IOData object wants a weak pointer to this UI object, and
the
> main UI thread object wants pointers to two IO thread objects (The events
store
> and the URLRequestContextGetter).
> 
> To make things more complicated, the Getter can only be initialized only after
> the ProxyIOData is good to go, so can't just make a single method to set them
> both up, creating the shared objects first, before returning the IO and UI
> objects for use.
> 
> I currently don't have a suggestion on how to do this more cleanly,
> unfortunately.

aw lgtm. only looked at android_webview

[megjablon, 2015-02-09 22:38:12]

The CQ bit was checked by megjablon@chromium.org

[commit-bot: I haz the power, 2015-02-09 22:39:10]

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

[commit-bot: I haz the power, 2015-02-09 22:44:19]

Committed patchset #10 (id:450001)

[commit-bot: I haz the power, 2015-02-09 22:46:18]

Patchset 10 (id:??) landed as
https://crrev.com/149bd770d674e8125f81cd649a8deece3a851682
Cr-Commit-Position: refs/heads/master@{#315419}

[jiayl, 2015-02-09 23:19:30]

A revert of this CL (patchset #10 id:450001) has been created in
https://codereview.chromium.org/896713003/ by jiayl@chromium.org.

The reason for reverting is: Suspected to cause compile failure:

http://build.chromium.org/p/chromium.mac/builders/iOS%20Device/builds/23577/s....

[bengr, 2015-02-10 19:31:33]

The CQ bit was checked by bengr@chromium.org

[commit-bot: I haz the power, 2015-02-10 19:32:41]

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

[commit-bot: I haz the power, 2015-02-10 20:06:47]

Committed patchset #10 (id:450001)

[commit-bot: I haz the power, 2015-02-10 20:07:08]

Patchset 10 (id:??) landed as
https://crrev.com/85594ccf495b97ccd4642dd5f5ab98e31d1d2003
Cr-Commit-Position: refs/heads/master@{#315621}