Fix foregound vs background data use measurement

Fix foregound vs background data use measurement

The application state when the request was created should be used to
record foreground vs background data use. Currently, application state at
the time of data use is used.

BUG=648808
Committed: https://crrev.com/da6c2ca5df87b5f74c615c45f8b5b6faf82cb158
Cr-Commit-Position: refs/heads/master@{#421126}

[Raj, 2016-09-21 00:34:46]

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

[Raj, 2016-09-21 00:34:46]

ptal

[mmenke, 2016-09-21 16:49:05]

On 2016/09/21 00:34:46, Raj wrote:
> ptal

ChromeNetworkDelegate LGTM, rubberstamp.

[mmenke, 2016-09-21 16:50:31]

On 2016/09/21 16:49:05, mmenke wrote:
> On 2016/09/21 00:34:46, Raj wrote:
> > ptal
> 
> ChromeNetworkDelegate LGTM, rubberstamp.

Random question:  Should we consider using a LayeredNetworkDelegate for this?

[bengr, 2016-09-23 18:27:31]

On 2016/09/21 16:50:31, mmenke wrote:
> On 2016/09/21 16:49:05, mmenke wrote:
> > On 2016/09/21 00:34:46, Raj wrote:
> > > ptal
> > 
> > ChromeNetworkDelegate LGTM, rubberstamp.
> 
> Random question:  Should we consider using a LayeredNetworkDelegate for this?

Maybe eventually, but the footprint in CND is really small right now and I don't
see a ton of potential for use with other embedders yet.

[mmenke, 2016-09-23 18:28:58]

On 2016/09/23 18:27:31, bengr wrote:
> On 2016/09/21 16:50:31, mmenke wrote:
> > On 2016/09/21 16:49:05, mmenke wrote:
> > > On 2016/09/21 00:34:46, Raj wrote:
> > > > ptal
> > > 
> > > ChromeNetworkDelegate LGTM, rubberstamp.
> > 
> > Random question:  Should we consider using a LayeredNetworkDelegate for
this?
> 
> Maybe eventually, but the footprint in CND is really small right now and I
don't
> see a ton of potential for use with other embedders yet.

I'm thinking more in terms of code health for ChromeNetworkDelegate, and
minimizing code that depends on it, than making it easy to plug in somewhere
else, actually.

[bengr, 2016-09-23 18:31:59]

On 2016/09/23 18:28:58, mmenke wrote:
> On 2016/09/23 18:27:31, bengr wrote:
> > On 2016/09/21 16:50:31, mmenke wrote:
> > > On 2016/09/21 16:49:05, mmenke wrote:
> > > > On 2016/09/21 00:34:46, Raj wrote:
> > > > > ptal
> > > > 
> > > > ChromeNetworkDelegate LGTM, rubberstamp.
> > > 
> > > Random question:  Should we consider using a LayeredNetworkDelegate for
> this?
> > 
> > Maybe eventually, but the footprint in CND is really small right now and I
> don't
> > see a ton of potential for use with other embedders yet.
> 
> I'm thinking more in terms of code health for ChromeNetworkDelegate, and
> minimizing code that depends on it, than making it easy to plug in somewhere
> else, actually.

Another random thought: Could this be part of the base URLRequest::Delegate?

[Raj, 2016-09-23 18:35:51]

Eventually this could move to DataUseNetworkDelegate
https://codereview.chromium.org/2354323002/

[bengr, 2016-09-23 18:45:32]

https://codereview.chromium.org/2358663004/diff/20001/components/data_use_mea...
File components/data_use_measurement/content/data_use_measurement.cc (right):

https://codereview.chromium.org/2358663004/diff/20001/components/data_use_mea...
components/data_use_measurement/content/data_use_measurement.cc:107: bool
started_in_foreground =
Might be better to have an "unknown" state.

https://codereview.chromium.org/2358663004/diff/20001/components/data_use_mea...
File components/data_use_measurement/core/data_use_user_data.h (right):

https://codereview.chromium.org/2358663004/diff/20001/components/data_use_mea...
components/data_use_measurement/core/data_use_user_data.h:72: // True if the url
request started while the app was in foreground.
nit: url request -> request
nit: in -> in the

https://codereview.chromium.org/2358663004/diff/20001/components/data_use_mea...
components/data_use_measurement/core/data_use_user_data.h:73: const bool
started_in_foreground_;
I'd make this an enum with states FOREGROUND, BACKGROUND, and UNKNOWN.

[Raj, 2016-09-23 22:33:38]

Patchset #3 (id:40001) has been deleted

[Raj, 2016-09-23 22:34:10]

https://codereview.chromium.org/2358663004/diff/20001/components/data_use_mea...
File components/data_use_measurement/content/data_use_measurement.cc (right):

https://codereview.chromium.org/2358663004/diff/20001/components/data_use_mea...
components/data_use_measurement/content/data_use_measurement.cc:107: bool
started_in_foreground =
On 2016/09/23 18:45:31, bengr wrote:
> Might be better to have an "unknown" state.

Done.

https://codereview.chromium.org/2358663004/diff/20001/components/data_use_mea...
File components/data_use_measurement/core/data_use_user_data.h (right):

https://codereview.chromium.org/2358663004/diff/20001/components/data_use_mea...
components/data_use_measurement/core/data_use_user_data.h:73: const bool
started_in_foreground_;
On 2016/09/23 18:45:31, bengr wrote:
> I'd make this an enum with states FOREGROUND, BACKGROUND, and UNKNOWN.

Done.

[mmenke, 2016-09-23 23:42:45]

One other thing about the LayeredNetworkDelegate approach...You can actually
test it with a real network stack, as opposed to mocked out calls, or a full
BrowserTest.  Anyhow, I'll be quiet now.  :)

[bengr, 2016-09-26 16:10:30]

On 2016/09/23 23:42:45, mmenke wrote:
> One other thing about the LayeredNetworkDelegate approach...You can actually
> test it with a real network stack, as opposed to mocked out calls, or a full
> BrowserTest.  Anyhow, I'll be quiet now.  :)

I think we should move to a LayeredNetworkDelegate, but not necessarily in this
CL. Raj, wanna do that in a follow-up when you have time?

[Raj, 2016-09-26 16:49:08]

On 2016/09/26 16:10:30, bengr wrote:
> On 2016/09/23 23:42:45, mmenke wrote:
> > One other thing about the LayeredNetworkDelegate approach...You can actually
> > test it with a real network stack, as opposed to mocked out calls, or a full
> > BrowserTest.  Anyhow, I'll be quiet now.  :)
> 
> I think we should move to a LayeredNetworkDelegate, but not necessarily in
this
> CL. Raj, wanna do that in a follow-up when you have time?

Yes. All the current code could move to DataUseNetworkDelegate, and we do not
need to depend on ChromeNetworkDelegate.
I will do that.

[bengr, 2016-09-26 17:07:14]

lgtm

[Raj, 2016-09-26 18:33:26]

The CQ bit was checked by rajendrant@chromium.org

[Raj, 2016-09-26 18:33:26]

The patchset sent to the CQ was uploaded after l-g-t-m from mmenke@chromium.org
Link to the patchset: https://codereview.chromium.org/2358663004/#ps60001
(title: "Addressed bengr comments")

[commit-bot: I haz the power, 2016-09-26 18:34:50]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Not at Google. Contact bengr, 2016-09-26 18:40:18]

kundaji@chromium.org changed reviewers:
+ kundaji@chromium.org

[Not at Google. Contact bengr, 2016-09-26 18:40:19]

lgtm

Just a nit/question. Changes is any can be made in a different cl.

https://codereview.chromium.org/2358663004/diff/60001/components/data_use_mea...
File components/data_use_measurement/content/data_use_measurement.cc (right):

https://codereview.chromium.org/2358663004/diff/60001/components/data_use_mea...
components/data_use_measurement/content/data_use_measurement.cc:71:
DataUseUserData* data_use_user_data = reinterpret_cast<DataUseUserData*>(
Is there a reason for reinterpret_cast here? Could this be a static_cast?

[commit-bot: I haz the power, 2016-09-26 18:42:28]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-26 18:42:29]

Try jobs failed on following builders:
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)

[Raj, 2016-09-26 18:55:15]

The CQ bit was checked by rajendrant@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-26 18:56:15]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-26 19:09:02]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-26 19:09:03]

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_...)

[Raj, 2016-09-26 20:25:16]

Patchset #4 (id:80001) has been deleted

[Raj, 2016-09-26 20:28:25]

The CQ bit was checked by rajendrant@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-26 20:29:21]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-26 21:49:00]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-26 21:49:01]

Dry run: Try jobs failed on following builders:
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[Raj, 2016-09-27 05:36:55]

The CQ bit was checked by rajendrant@chromium.org

[Raj, 2016-09-27 05:36:56]

The patchset sent to the CQ was uploaded after l-g-t-m from bengr@chromium.org,
kundaji@chromium.org, mmenke@chromium.org
Link to the patchset: https://codereview.chromium.org/2358663004/#ps100001
(title: "rebased")

[commit-bot: I haz the power, 2016-09-27 05:37:16]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-27 06:13:37]

Committed patchset #4 (id:100001)

[commit-bot: I haz the power, 2016-09-27 06:17:21]

Description was changed from

==========
Fix foregound vs background data use measurement

The application state when the request was created should be used to
record foreground vs background data use. Currently, application state at
the time of data use is used.

BUG=648808
==========

to

==========
Fix foregound vs background data use measurement

The application state when the request was created should be used to
record foreground vs background data use. Currently, application state at
the time of data use is used.

BUG=648808

Committed: https://crrev.com/da6c2ca5df87b5f74c615c45f8b5b6faf82cb158
Cr-Commit-Position: refs/heads/master@{#421126}
==========

[commit-bot: I haz the power, 2016-09-27 06:17:22]

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