Expose API to store and load data usage stats from UI task runner.

Expose API to store and load data usage stats from UI task runner.

Initialize and hookup DB task runner and DB objects.
|DataReductionProxyService| exposes APIs on UI task runner to read and
modify data usage stats. Initialize webview with a dummy store
where load and store are no-ops.

BUG=482442
Committed: https://crrev.com/2ac5656adb9cb3e68127cdc118756de2879454f5
Cr-Commit-Position: refs/heads/master@{#338914}

[Not at Google. Contact bengr, 2015-07-10 19:38:56]

kundaji@chromium.org changed reviewers:
+ jeremyim@chromium.org, jochen@chromium.org, sgurun@chromium.org

[Not at Google. Contact bengr, 2015-07-10 20:25:59]

This change was originally part of https://codereview.chromium.org/1173343009/
but was split to make the cls more manageable.

jeremyim:
  chrome/browser/net/spdyproxy/data_reduction_proxy_chrome_settings.cc
  chrome/browser/net/spdyproxy/data_reduction_proxy_chrome_settings.h
 
components/data_reduction_proxy/core/browser/data_reduction_proxy_compression_stats.cc
 
components/data_reduction_proxy/core/browser/data_reduction_proxy_compression_stats.h
  components/data_reduction_proxy/core/browser/data_reduction_proxy_service.cc
  components/data_reduction_proxy/core/browser/data_reduction_proxy_service.h
 
components/data_reduction_proxy/core/browser/data_reduction_proxy_test_utils.cc
  components/data_reduction_proxy/core/browser/data_reduction_proxy_test_utils.h

jochen:
  chrome/browser/profiles/profile_impl_io_data.cc
  chrome/browser/renderer_context_menu/render_view_context_menu_unittest.cc

sgurun:
  android_webview/browser/aw_browser_context.cc

[jochen (gone - plz use gerrit), 2015-07-13 11:08:48]

lgtm

[jeremyim, 2015-07-13 20:19:41]

https://codereview.chromium.org/1221663009/diff/100001/chrome/browser/net/spd...
File chrome/browser/net/spdyproxy/data_reduction_proxy_chrome_settings.h
(right):

https://codereview.chromium.org/1221663009/diff/100001/chrome/browser/net/spd...
chrome/browser/net/spdyproxy/data_reduction_proxy_chrome_settings.h:61: //
request context getter, and task runner.
Update comment with new params

https://codereview.chromium.org/1221663009/diff/100001/chrome/browser/rendere...
File chrome/browser/renderer_context_menu/render_view_context_menu_unittest.cc
(right):

https://codereview.chromium.org/1221663009/diff/100001/chrome/browser/rendere...
chrome/browser/renderer_context_menu/render_view_context_menu_unittest.cc:396:
base::ThreadTaskRunnerHandle::Get());
It would be nice (but not necessary) if more of the boilerplate could be removed
(possibly by having drp_test_context_ expose a method to initialize an external
settings object).

https://codereview.chromium.org/1221663009/diff/100001/components/data_reduct...
File
components/data_reduction_proxy/core/browser/data_reduction_proxy_compression_stats.h
(right):

https://codereview.chromium.org/1221663009/diff/100001/components/data_reduct...
components/data_reduction_proxy/core/browser/data_reduction_proxy_compression_stats.h:88:
}
Looks like this is part of a future CL that crept in here? |service_| appears to
be unused.

https://codereview.chromium.org/1221663009/diff/100001/components/data_reduct...
File components/data_reduction_proxy/core/browser/data_reduction_proxy_service.h
(right):

https://codereview.chromium.org/1221663009/diff/100001/components/data_reduct...
components/data_reduction_proxy/core/browser/data_reduction_proxy_service.h:82:
scoped_refptr<base::SequencedTaskRunner> db_task_runner,
nit: make these const &

[Not at Google. Contact bengr, 2015-07-14 16:41:31]

https://codereview.chromium.org/1221663009/diff/100001/chrome/browser/net/spd...
File chrome/browser/net/spdyproxy/data_reduction_proxy_chrome_settings.h
(right):

https://codereview.chromium.org/1221663009/diff/100001/chrome/browser/net/spd...
chrome/browser/net/spdyproxy/data_reduction_proxy_chrome_settings.h:61: //
request context getter, and task runner.
On 2015/07/13 20:19:40, jeremyim wrote:
> Update comment with new params

Done.

https://codereview.chromium.org/1221663009/diff/100001/chrome/browser/rendere...
File chrome/browser/renderer_context_menu/render_view_context_menu_unittest.cc
(right):

https://codereview.chromium.org/1221663009/diff/100001/chrome/browser/rendere...
chrome/browser/renderer_context_menu/render_view_context_menu_unittest.cc:396:
base::ThreadTaskRunnerHandle::Get());
On 2015/07/13 20:19:40, jeremyim wrote:
> It would be nice (but not necessary) if more of the boilerplate could be
removed
> (possibly by having drp_test_context_ expose a method to initialize an
external
> settings object).

It seems confusing to have 2 settings objects, so I am hesitant to put code in
drp_test_context_ that encourages this.

Also, the DRPChromeSettings::InitDRPSettings is only called from this test, so
doesn't get us any reduction in total code lines.

Ideally, the builder would take an external DRPChromeSettings object.

https://codereview.chromium.org/1221663009/diff/100001/components/data_reduct...
File
components/data_reduction_proxy/core/browser/data_reduction_proxy_compression_stats.h
(right):

https://codereview.chromium.org/1221663009/diff/100001/components/data_reduct...
components/data_reduction_proxy/core/browser/data_reduction_proxy_compression_stats.h:88:
}
On 2015/07/13 20:19:40, jeremyim wrote:
> Looks like this is part of a future CL that crept in here? |service_| appears
to
> be unused.

I intended to have this be part of this cl, since this is part of the hookup.

All of the functionality in this cl is unused. Cl is already pretty big, so
seemed better to split from cl that uses this functionality.

https://codereview.chromium.org/1221663009/diff/100001/components/data_reduct...
File components/data_reduction_proxy/core/browser/data_reduction_proxy_service.h
(right):

https://codereview.chromium.org/1221663009/diff/100001/components/data_reduct...
components/data_reduction_proxy/core/browser/data_reduction_proxy_service.h:82:
scoped_refptr<base::SequencedTaskRunner> db_task_runner,
On 2015/07/13 20:19:41, jeremyim wrote:
> nit: make these const &

Done.

[jeremyim, 2015-07-14 22:39:54]

https://codereview.chromium.org/1221663009/diff/100001/chrome/browser/rendere...
File chrome/browser/renderer_context_menu/render_view_context_menu_unittest.cc
(right):

https://codereview.chromium.org/1221663009/diff/100001/chrome/browser/rendere...
chrome/browser/renderer_context_menu/render_view_context_menu_unittest.cc:396:
base::ThreadTaskRunnerHandle::Get());
On 2015/07/14 16:41:30, kundaji wrote:
> On 2015/07/13 20:19:40, jeremyim wrote:
> > It would be nice (but not necessary) if more of the boilerplate could be
> removed
> > (possibly by having drp_test_context_ expose a method to initialize an
> external
> > settings object).
> 
> It seems confusing to have 2 settings objects, so I am hesitant to put code in
> drp_test_context_ that encourages this.
> 
> Also, the DRPChromeSettings::InitDRPSettings is only called from this test, so
> doesn't get us any reduction in total code lines.
> 
> Ideally, the builder would take an external DRPChromeSettings object.

I'm not looking for reduction in total code lines; I'm looking for a way that
changes in the way DRP classes are hooked up don't bleed into test code
(especially outside of the DRP component) and require updates elsewhere.

Having the builder take an external DRPChromeSettings seems like a great idea;
it might have solved the previous issues you had trying to get the test to work.
=)

https://codereview.chromium.org/1221663009/diff/100001/components/data_reduct...
File
components/data_reduction_proxy/core/browser/data_reduction_proxy_compression_stats.h
(right):

https://codereview.chromium.org/1221663009/diff/100001/components/data_reduct...
components/data_reduction_proxy/core/browser/data_reduction_proxy_compression_stats.h:88:
}
On 2015/07/14 16:41:30, kundaji wrote:
> On 2015/07/13 20:19:40, jeremyim wrote:
> > Looks like this is part of a future CL that crept in here? |service_|
appears
> to
> > be unused.
> 
> I intended to have this be part of this cl, since this is part of the hookup.
> 
> All of the functionality in this cl is unused. Cl is already pretty big, so
> seemed better to split from cl that uses this functionality. 

I'll likely have some comments about this in the next CL; in general, I'm not a
fan of having to perform a set call, since it introduces the possibility of data
races.

[jeremyim, 2015-07-14 23:06:04]

https://codereview.chromium.org/1221663009/diff/200001/android_webview/browse...
File android_webview/browser/aw_browser_context.cc (right):

https://codereview.chromium.org/1221663009/diff/200001/android_webview/browse...
android_webview/browser/aw_browser_context.cc:399:
base::SequencedWorkerPool::SKIP_ON_SHUTDOWN);
This shouldn't be necessary, as drp_service_ already has the db_task_runner.

https://codereview.chromium.org/1221663009/diff/200001/components/data_reduct...
File
components/data_reduction_proxy/core/browser/data_reduction_proxy_service.cc
(right):

https://codereview.chromium.org/1221663009/diff/200001/components/data_reduct...
components/data_reduction_proxy/core/browser/data_reduction_proxy_service.cc:44:
compression_stats_->set_data_reduction_proxy_service(this);
Can we conditionally create the compression_stats here based on a non-null prefs
object? This would mean passing in the ui_task_runner here, but would mean that
DRPCompressionStats could receive the service object in the constructor instead
of a set method.

https://codereview.chromium.org/1221663009/diff/200001/components/data_reduct...
components/data_reduction_proxy/core/browser/data_reduction_proxy_service.cc:95:
prefs_, ui_task_runner, commit_delay));
set_data_reduction_proxy_service is not being called here...

[Not at Google. Contact bengr, 2015-07-15 00:08:17]

https://codereview.chromium.org/1221663009/diff/100001/chrome/browser/rendere...
File chrome/browser/renderer_context_menu/render_view_context_menu_unittest.cc
(right):

https://codereview.chromium.org/1221663009/diff/100001/chrome/browser/rendere...
chrome/browser/renderer_context_menu/render_view_context_menu_unittest.cc:396:
base::ThreadTaskRunnerHandle::Get());
On 2015/07/14 22:39:54, jeremyim wrote:
> On 2015/07/14 16:41:30, kundaji wrote:
> > On 2015/07/13 20:19:40, jeremyim wrote:
> > > It would be nice (but not necessary) if more of the boilerplate could be
> > removed
> > > (possibly by having drp_test_context_ expose a method to initialize an
> > external
> > > settings object).
> > 
> > It seems confusing to have 2 settings objects, so I am hesitant to put code
in
> > drp_test_context_ that encourages this.
> > 
> > Also, the DRPChromeSettings::InitDRPSettings is only called from this test,
so
> > doesn't get us any reduction in total code lines.
> > 
> > Ideally, the builder would take an external DRPChromeSettings object.
> 
> I'm not looking for reduction in total code lines; I'm looking for a way that
> changes in the way DRP classes are hooked up don't bleed into test code
> (especially outside of the DRP component) and require updates elsewhere.
> 
> Having the builder take an external DRPChromeSettings seems like a great idea;
> it might have solved the previous issues you had trying to get the test to
work.
> =)

Sounds good. I'll do this in a separate cl.

https://codereview.chromium.org/1221663009/diff/100001/components/data_reduct...
File
components/data_reduction_proxy/core/browser/data_reduction_proxy_compression_stats.h
(right):

https://codereview.chromium.org/1221663009/diff/100001/components/data_reduct...
components/data_reduction_proxy/core/browser/data_reduction_proxy_compression_stats.h:88:
}
On 2015/07/14 22:39:54, jeremyim wrote:
> On 2015/07/14 16:41:30, kundaji wrote:
> > On 2015/07/13 20:19:40, jeremyim wrote:
> > > Looks like this is part of a future CL that crept in here? |service_|
> appears
> > to
> > > be unused.
> > 
> > I intended to have this be part of this cl, since this is part of the
hookup.
> > 
> > All of the functionality in this cl is unused. Cl is already pretty big, so
> > seemed better to split from cl that uses this functionality. 
> 
> I'll likely have some comments about this in the next CL; in general, I'm not
a
> fan of having to perform a set call, since it introduces the possibility of
data
> races.

Done.

https://codereview.chromium.org/1221663009/diff/200001/android_webview/browse...
File android_webview/browser/aw_browser_context.cc (right):

https://codereview.chromium.org/1221663009/diff/200001/android_webview/browse...
android_webview/browser/aw_browser_context.cc:399:
base::SequencedWorkerPool::SKIP_ON_SHUTDOWN);
On 2015/07/14 23:06:03, jeremyim wrote:
> This shouldn't be necessary, as drp_service_ already has the db_task_runner.

Done.

https://codereview.chromium.org/1221663009/diff/200001/components/data_reduct...
File
components/data_reduction_proxy/core/browser/data_reduction_proxy_service.cc
(right):

https://codereview.chromium.org/1221663009/diff/200001/components/data_reduct...
components/data_reduction_proxy/core/browser/data_reduction_proxy_service.cc:44:
compression_stats_->set_data_reduction_proxy_service(this);
On 2015/07/14 23:06:03, jeremyim wrote:
> Can we conditionally create the compression_stats here based on a non-null
prefs
> object? This would mean passing in the ui_task_runner here, but would mean
that
> DRPCompressionStats could receive the service object in the constructor
instead
> of a set method.

Done.

https://codereview.chromium.org/1221663009/diff/200001/components/data_reduct...
components/data_reduction_proxy/core/browser/data_reduction_proxy_service.cc:95:
prefs_, ui_task_runner, commit_delay));
On 2015/07/14 23:06:03, jeremyim wrote:
> set_data_reduction_proxy_service is not being called here...

Done.

[jeremyim, 2015-07-15 00:36:18]

https://codereview.chromium.org/1221663009/diff/240001/components/data_reduct...
File
components/data_reduction_proxy/core/browser/data_reduction_proxy_compression_stats.cc
(right):

https://codereview.chromium.org/1221663009/diff/240001/components/data_reduct...
components/data_reduction_proxy/core/browser/data_reduction_proxy_compression_stats.cc:375:
: service_(service),
It would be nice if we could DCHECK this. That does mean that we'd have to
update the unit test, though.

[Not at Google. Contact bengr, 2015-07-15 17:46:54]

https://codereview.chromium.org/1221663009/diff/240001/components/data_reduct...
File
components/data_reduction_proxy/core/browser/data_reduction_proxy_compression_stats.cc
(right):

https://codereview.chromium.org/1221663009/diff/240001/components/data_reduct...
components/data_reduction_proxy/core/browser/data_reduction_proxy_compression_stats.cc:375:
: service_(service),
On 2015/07/15 00:36:18, jeremyim wrote:
> It would be nice if we could DCHECK this. That does mean that we'd have to
> update the unit test, though.

Done.

[jeremyim, 2015-07-15 20:10:10]

lgtm

[sgurun-gerrit only, 2015-07-15 20:37:54]

On 2015/07/15 20:10:10, jeremyim wrote:
> lgtm

lgtm

[Not at Google. Contact bengr, 2015-07-15 20:38:24]

The CQ bit was checked by kundaji@chromium.org

[Not at Google. Contact bengr, 2015-07-15 20:38:25]

The patchset sent to the CQ was uploaded after l-g-t-m from jochen@chromium.org
Link to the patchset: https://codereview.chromium.org/1221663009/#ps260001
(title: "Add DCHECK")

[commit-bot: I haz the power, 2015-07-15 20:40:26]

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

[commit-bot: I haz the power, 2015-07-15 21:06:51]

Committed patchset #14 (id:260001)

[commit-bot: I haz the power, 2015-07-15 21:08:56]

Patchset 14 (id:??) landed as
https://crrev.com/2ac5656adb9cb3e68127cdc118756de2879454f5
Cr-Commit-Position: refs/heads/master@{#338914}

[Not at Google. Contact bengr, 2015-07-15 22:34:09]

https://codereview.chromium.org/1221663009/diff/100001/chrome/browser/rendere...
File chrome/browser/renderer_context_menu/render_view_context_menu_unittest.cc
(right):

https://codereview.chromium.org/1221663009/diff/100001/chrome/browser/rendere...
chrome/browser/renderer_context_menu/render_view_context_menu_unittest.cc:396:
base::ThreadTaskRunnerHandle::Get());
On 2015/07/15 00:08:17, kundaji wrote:
> On 2015/07/14 22:39:54, jeremyim wrote:
> > On 2015/07/14 16:41:30, kundaji wrote:
> > > On 2015/07/13 20:19:40, jeremyim wrote:
> > > > It would be nice (but not necessary) if more of the boilerplate could be
> > > removed
> > > > (possibly by having drp_test_context_ expose a method to initialize an
> > > external
> > > > settings object).
> > > 
> > > It seems confusing to have 2 settings objects, so I am hesitant to put
code
> in
> > > drp_test_context_ that encourages this.
> > > 
> > > Also, the DRPChromeSettings::InitDRPSettings is only called from this
test,
> so
> > > doesn't get us any reduction in total code lines.
> > > 
> > > Ideally, the builder would take an external DRPChromeSettings object.
> > 
> > I'm not looking for reduction in total code lines; I'm looking for a way
that
> > changes in the way DRP classes are hooked up don't bleed into test code
> > (especially outside of the DRP component) and require updates elsewhere.
> > 
> > Having the builder take an external DRPChromeSettings seems like a great
idea;
> > it might have solved the previous issues you had trying to get the test to
> work.
> > =)
> 
> Sounds good. I'll do this in a separate cl.

Jeremy: I investigated this further. Moving initialization of DRPChromeSettings
to DRPTestContext will add a dependency (in DRPTestContext) on the following
files:
       
'../chrome/browser/net/spdyproxy/data_reduction_proxy_chrome_settings.cc',
       
'../chrome/browser/net/spdyproxy/data_reduction_proxy_chrome_settings.h',
        '../chrome/common/pref_names.cc',
        '../chrome/common/pref_names.h',
        '../chrome/browser/browser_process.cc',
        '../chrome/browser/browser_process.h',
        '../chrome/browser/metrics/chrome_metrics_service_accessor.cc',
        '../chrome/browser/metrics/chrome_metrics_service_accessor.h',
        '../chrome/browser/profiles/profile.cc',
        '../chrome/browser/profiles/profile.h',
        '../chrome/browser/profiles/profile_manager.cc',
        '../chrome/browser/profiles/profile_manager.h',
        '../chrome/common/pref_names.cc',
        '../chrome/common/pref_names.h',
With this in mind, I don't think this is a good idea. What do you think?