Migrate data reduction proxy statistics prefs to profile

Migrate data reduction proxy statistics prefs to profile

Data reduction proxy statistics are accumulated in local
state preferences. To support desktop and other platforms
with multiple profiles, these are being ported to profile
preferences.

BUG=411373
Committed: https://crrev.com/e7ec38df8d1966cc871961e72dda8865ce68a3cc
Cr-Commit-Position: refs/heads/master@{#300369}

[bengr, 2014-10-14 18:25:14]

Patchset #2 (id:70001) has been deleted

[bengr, 2014-10-14 18:25:23]

Patchset #1 (id:1) has been deleted

[bengr, 2014-10-14 18:27:09]

bengr@chromium.org changed reviewers:
+ marq@chromium.org, mmenke@chromium.org

[bengr, 2014-10-14 18:27:10]

mmenke: profile_impl.cc
marq: everything else

[marq (ping after 24h), 2014-10-15 08:29:35]

LGTM with nits.

As far as I can tell this change will transparently carry over to iOS.

https://codereview.chromium.org/652743004/diff/90001/components/data_reductio...
File components/data_reduction_proxy/browser/data_reduction_proxy_prefs.cc
(right):

https://codereview.chromium.org/652743004/diff/90001/components/data_reductio...
components/data_reduction_proxy/browser/data_reduction_proxy_prefs.cc:15: void
CopyList(const char* pref_path, PrefService* src, PrefService* dest) {
Rename to CopyPrefList for clarity.

https://codereview.chromium.org/652743004/diff/90001/components/data_reductio...
components/data_reduction_proxy/browser/data_reduction_proxy_prefs.cc:16:
ListPrefUpdate update(dest, pref_path);
What happens if |pref_path| doesn't have a list value? Is a DCHECK needed here?

https://codereview.chromium.org/652743004/diff/90001/components/data_reductio...
File components/data_reduction_proxy/browser/data_reduction_proxy_prefs.h
(right):

https://codereview.chromium.org/652743004/diff/90001/components/data_reductio...
components/data_reduction_proxy/browser/data_reduction_proxy_prefs.h:35:
PrefService* dest);
No need for the line break.

[mmenke, 2014-10-15 15:49:35]

https://codereview.chromium.org/652743004/diff/90001/chrome/browser/profiles/...
File chrome/browser/profiles/profile_impl.cc (right):

https://codereview.chromium.org/652743004/diff/90001/chrome/browser/profiles/...
chrome/browser/profiles/profile_impl.cc:672: prefs_.get());
Add a TODO, with a milestone, about when this can be removed (Could additionally
add it to the function declaration - think it may make sense to have it in both
places, to make sure it's removed).

https://codereview.chromium.org/652743004/diff/90001/components/data_reductio...
File components/data_reduction_proxy/browser/data_reduction_proxy_prefs.cc
(right):

https://codereview.chromium.org/652743004/diff/90001/components/data_reductio...
components/data_reduction_proxy/browser/data_reduction_proxy_prefs.cc:18:
update->Swap(src_list.get());
Shouldn't we also be deleting the old values?  Or does that happen
automatically, when we stop registering them?

https://codereview.chromium.org/652743004/diff/90001/components/data_reductio...
components/data_reduction_proxy/browser/data_reduction_proxy_prefs.cc:21: }
nit:

}  // namespace

https://codereview.chromium.org/652743004/diff/90001/components/data_reductio...
components/data_reduction_proxy/browser/data_reduction_proxy_prefs.cc:129:
prefs::kDailyContentLengthViaDataReductionProxy);
Should we still register all these prefs?

[bengr, 2014-10-15 22:52:51]

https://codereview.chromium.org/652743004/diff/90001/chrome/browser/profiles/...
File chrome/browser/profiles/profile_impl.cc (right):

https://codereview.chromium.org/652743004/diff/90001/chrome/browser/profiles/...
chrome/browser/profiles/profile_impl.cc:672: prefs_.get());
On 2014/10/15 15:49:34, mmenke wrote:
> Add a TODO, with a milestone, about when this can be removed (Could
additionally
> add it to the function declaration - think it may make sense to have it in
both
> places, to make sure it's removed).

Done.

https://codereview.chromium.org/652743004/diff/90001/components/data_reductio...
File components/data_reduction_proxy/browser/data_reduction_proxy_prefs.cc
(right):

https://codereview.chromium.org/652743004/diff/90001/components/data_reductio...
components/data_reduction_proxy/browser/data_reduction_proxy_prefs.cc:15: void
CopyList(const char* pref_path, PrefService* src, PrefService* dest) {
On 2014/10/15 08:29:35, marq_ wrote:
> Rename to CopyPrefList for clarity.

Done.

https://codereview.chromium.org/652743004/diff/90001/components/data_reductio...
components/data_reduction_proxy/browser/data_reduction_proxy_prefs.cc:16:
ListPrefUpdate update(dest, pref_path);
On 2014/10/15 08:29:35, marq_ wrote:
> What happens if |pref_path| doesn't have a list value? Is a DCHECK needed
here?

Done.

https://codereview.chromium.org/652743004/diff/90001/components/data_reductio...
components/data_reduction_proxy/browser/data_reduction_proxy_prefs.cc:18:
update->Swap(src_list.get());
On 2014/10/15 15:49:34, mmenke wrote:
> Shouldn't we also be deleting the old values?  Or does that happen
> automatically, when we stop registering them?

Done.

https://codereview.chromium.org/652743004/diff/90001/components/data_reductio...
components/data_reduction_proxy/browser/data_reduction_proxy_prefs.cc:21: }
On 2014/10/15 15:49:34, mmenke wrote:
> nit:
> 
> }  // namespace

Done.

https://codereview.chromium.org/652743004/diff/90001/components/data_reductio...
File components/data_reduction_proxy/browser/data_reduction_proxy_prefs.h
(right):

https://codereview.chromium.org/652743004/diff/90001/components/data_reductio...
components/data_reduction_proxy/browser/data_reduction_proxy_prefs.h:35:
PrefService* dest);
On 2014/10/15 08:29:35, marq_ wrote:
> No need for the line break.

Done.

[mmenke, 2014-10-16 15:38:46]

profiles/ LGTM.  Did skim over the rest, but didn't do a careful review.

On 2014/10/15 22:52:51, bengr1 wrote:
>
https://codereview.chromium.org/652743004/diff/90001/chrome/browser/profiles/...
> File chrome/browser/profiles/profile_impl.cc (right):
> 
>
https://codereview.chromium.org/652743004/diff/90001/chrome/browser/profiles/...
> chrome/browser/profiles/profile_impl.cc:672: prefs_.get());
> On 2014/10/15 15:49:34, mmenke wrote:
> > Add a TODO, with a milestone, about when this can be removed (Could
> additionally
> > add it to the function declaration - think it may make sense to have it in
> both
> > places, to make sure it's removed).
> 
> Done.
> 
>
https://codereview.chromium.org/652743004/diff/90001/components/data_reductio...
> File components/data_reduction_proxy/browser/data_reduction_proxy_prefs.cc
> (right):
> 
>
https://codereview.chromium.org/652743004/diff/90001/components/data_reductio...
> components/data_reduction_proxy/browser/data_reduction_proxy_prefs.cc:15: void
> CopyList(const char* pref_path, PrefService* src, PrefService* dest) {
> On 2014/10/15 08:29:35, marq_ wrote:
> > Rename to CopyPrefList for clarity.
> 
> Done.
> 
>
https://codereview.chromium.org/652743004/diff/90001/components/data_reductio...
> components/data_reduction_proxy/browser/data_reduction_proxy_prefs.cc:16:
> ListPrefUpdate update(dest, pref_path);
> On 2014/10/15 08:29:35, marq_ wrote:
> > What happens if |pref_path| doesn't have a list value? Is a DCHECK needed
> here?
> 
> Done.
> 
>
https://codereview.chromium.org/652743004/diff/90001/components/data_reductio...
> components/data_reduction_proxy/browser/data_reduction_proxy_prefs.cc:18:
> update->Swap(src_list.get());
> On 2014/10/15 15:49:34, mmenke wrote:
> > Shouldn't we also be deleting the old values?  Or does that happen
> > automatically, when we stop registering them?
> 
> Done.
> 
>
https://codereview.chromium.org/652743004/diff/90001/components/data_reductio...
> components/data_reduction_proxy/browser/data_reduction_proxy_prefs.cc:21: }
> On 2014/10/15 15:49:34, mmenke wrote:
> > nit:
> > 
> > }  // namespace
> 
> Done.
> 
>
https://codereview.chromium.org/652743004/diff/90001/components/data_reductio...
> File components/data_reduction_proxy/browser/data_reduction_proxy_prefs.h
> (right):
> 
>
https://codereview.chromium.org/652743004/diff/90001/components/data_reductio...
> components/data_reduction_proxy/browser/data_reduction_proxy_prefs.h:35:
> PrefService* dest);
> On 2014/10/15 08:29:35, marq_ wrote:
> > No need for the line break.
> 
> Done.

[bengr, 2014-10-20 20:19:59]

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

[bengr, 2014-10-20 20:20:00]

sgurun: android_webview/*

[sgurun-gerrit only, 2014-10-20 20:52:37]

On 2014/10/20 20:20:00, bengr1 wrote:
> sgurun: android_webview/*

aw lgtm since this simply seems like removing a duped registry operation.
However, we should also fix the pref crash when flywheel is enabled :)

[bengr, 2014-10-20 21:08:36]

The CQ bit was checked by bengr@chromium.org

[commit-bot: I haz the power, 2014-10-20 21:10:12]

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

[commit-bot: I haz the power, 2014-10-20 22:49:00]

Committed patchset #5 (id:170001)

[commit-bot: I haz the power, 2014-10-20 22:49:32]

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