Split the data use into foreground, background and unknown

Split the data use into foreground, background and unknown

Currently data use is measured as foreground and background. In some
cases where the app was transitioning from one state to different state,
data use at that time should be logged as unknown.

This CL changes the UMA to log on every network TX/RX of the URLRequest.
Earlier it was logged only when URLRequest completes.

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

[Raj, 2016-10-06 20:51:16]

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

[Raj, 2016-10-06 20:51:17]

mmenke: ptal chrome_network_delegate_unittest.cc

[mmenke, 2016-10-06 20:53:49]

LGTM, rubberstamp

https://codereview.chromium.org/2399783003/diff/1/chrome/browser/net/chrome_n...
File chrome/browser/net/chrome_network_delegate_unittest.cc (right):

https://codereview.chromium.org/2399783003/diff/1/chrome/browser/net/chrome_n...
chrome/browser/net/chrome_network_delegate_unittest.cc:229: .empty());
Why are you no longer testing exact counts?

[Raj, 2016-10-06 21:02:14]

Thanks

https://codereview.chromium.org/2399783003/diff/1/chrome/browser/net/chrome_n...
File chrome/browser/net/chrome_network_delegate_unittest.cc (right):

https://codereview.chromium.org/2399783003/diff/1/chrome/browser/net/chrome_n...
chrome/browser/net/chrome_network_delegate_unittest.cc:229: .empty());
On 2016/10/06 20:53:49, mmenke wrote:
> Why are you no longer testing exact counts?

Because this CL changes the UMA to log on every network TX/RX of the URLRequest.
Earlier it was logged only when URLRequest completes.

[mmenke, 2016-10-06 21:07:35]

https://codereview.chromium.org/2399783003/diff/1/chrome/browser/net/chrome_n...
File chrome/browser/net/chrome_network_delegate_unittest.cc (right):

https://codereview.chromium.org/2399783003/diff/1/chrome/browser/net/chrome_n...
chrome/browser/net/chrome_network_delegate_unittest.cc:229: .empty());
On 2016/10/06 21:02:14, Raj wrote:
> On 2016/10/06 20:53:49, mmenke wrote:
> > Why are you no longer testing exact counts?
> 
> Because this CL changes the UMA to log on every network TX/RX of the
URLRequest.
> Earlier it was logged only when URLRequest completes.

Mention that in CL description?

[Raj, 2016-10-06 21:09:19]

Description was changed from

==========
Split the data use into foreground, background and unknown

Currently data use is measured as foreground and background. In some
cases where the app was transitioning from one state to different state,
data use at that time should be logged as unknown.

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

to

==========
Split the data use into foreground, background and unknown

Currently data use is measured as foreground and background. In some
cases where the app was transitioning from one state to different state,
data use at that time should be logged as unknown.

This CL changes the UMA to log on every network TX/RX of the URLRequest. Earlier
it was logged only when URLRequest completes.

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

[Raj, 2016-10-06 21:09:41]

Description was changed from

==========
Split the data use into foreground, background and unknown

Currently data use is measured as foreground and background. In some
cases where the app was transitioning from one state to different state,
data use at that time should be logged as unknown.

This CL changes the UMA to log on every network TX/RX of the URLRequest. Earlier
it was logged only when URLRequest completes.

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

to

==========
Split the data use into foreground, background and unknown

Currently data use is measured as foreground and background. In some
cases where the app was transitioning from one state to different state,
data use at that time should be logged as unknown.

This CL changes the UMA to log on every network TX/RX of the URLRequest.
Earlier it was logged only when URLRequest completes.

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

[Not at Google. Contact bengr, 2016-10-06 22:17:48]

https://codereview.chromium.org/2399783003/diff/1/components/data_use_measure...
File components/data_use_measurement/content/data_use_measurement.cc (right):

https://codereview.chromium.org/2399783003/diff/1/components/data_use_measure...
components/data_use_measurement/content/data_use_measurement.cc:83:
ReportDataUseUMA(request);
Is this called here as well as in OnCompleted because GetTotalSentBytes() and
GetTotalReceivedBytes() don't track data use from previous jobs? If so, we
should remember to take this out when those methods are fixed. See
http://crbug/651957.

https://codereview.chromium.org/2399783003/diff/1/components/data_use_measure...
components/data_use_measurement/content/data_use_measurement.cc:125:
DataUseUserData::ServiceName service_name =
To avoid repeating the ternary operator, suggest:

DataUseUserData::ServiceName service_name = DataUseUserData::NOT_TAGGED;
DataUseUserData::AppState old_app_state = DataUseUserData::FOREGROUND;
DataUseUserData* attached_service_data = ...
if (attached_service_data) {
  service_name = attached_service_data->service_name();
  old_app_state = attached_service_data->app_state();
  if (old_app_state != new_app_state)
    attached_service_data->set_app_state(CurrentAppState());
}

https://codereview.chromium.org/2399783003/diff/1/components/data_use_measure...
components/data_use_measurement/content/data_use_measurement.cc:131:
DataUseUserData::AppState new_app_state = old_app_state == CurrentAppState()
Why not just use CurrentAppState()?

https://codereview.chromium.org/2399783003/diff/1/components/data_use_measure...
components/data_use_measurement/content/data_use_measurement.cc:150: void
DataUseMeasurement::ReportDataUseUMA(
Looks like this is just updating prefs and not reporting UMA. If so, please
rename.

https://codereview.chromium.org/2399783003/diff/1/components/data_use_measure...
components/data_use_measurement/content/data_use_measurement.cc:156:
DataUseUserData* attached_service_data = reinterpret_cast<DataUseUserData*>(
Can this be a static_cast?

[Raj, 2016-10-06 23:17:51]

https://codereview.chromium.org/2399783003/diff/1/components/data_use_measure...
File components/data_use_measurement/content/data_use_measurement.cc (right):

https://codereview.chromium.org/2399783003/diff/1/components/data_use_measure...
components/data_use_measurement/content/data_use_measurement.cc:83:
ReportDataUseUMA(request);
On 2016/10/06 22:17:48, kundaji wrote:
> Is this called here as well as in OnCompleted because GetTotalSentBytes() and
> GetTotalReceivedBytes() don't track data use from previous jobs? If so, we
> should remember to take this out when those methods are fixed. See
> http://crbug/651957. 

Added comment.

https://codereview.chromium.org/2399783003/diff/1/components/data_use_measure...
components/data_use_measurement/content/data_use_measurement.cc:125:
DataUseUserData::ServiceName service_name =
On 2016/10/06 22:17:47, kundaji wrote:
> To avoid repeating the ternary operator, suggest:
> 
> DataUseUserData::ServiceName service_name = DataUseUserData::NOT_TAGGED;
> DataUseUserData::AppState old_app_state = DataUseUserData::FOREGROUND;
> DataUseUserData* attached_service_data = ...
> if (attached_service_data) {
>   service_name = attached_service_data->service_name();
>   old_app_state = attached_service_data->app_state();
>   if (old_app_state != new_app_state)
>     attached_service_data->set_app_state(CurrentAppState());
> }

Done.

https://codereview.chromium.org/2399783003/diff/1/components/data_use_measure...
components/data_use_measurement/content/data_use_measurement.cc:131:
DataUseUserData::AppState new_app_state = old_app_state == CurrentAppState()
On 2016/10/06 22:17:48, kundaji wrote:
> Why not just use CurrentAppState()?

CurrentAppState() only returns FOREGROUND or BACKGROUND.
It should be compared with old_app_state to determine if the current data use
should be logged as FOREGROUND or BACKGROUND or UNKNOWN.

If the old and current app state is same, the current data use will be logged as
the same state - FOREGROUND or BACKGROUND.
If the old and current app state are different, the current data use will be
logged as UNKNOWN.

https://codereview.chromium.org/2399783003/diff/1/components/data_use_measure...
components/data_use_measurement/content/data_use_measurement.cc:150: void
DataUseMeasurement::ReportDataUseUMA(
On 2016/10/06 22:17:47, kundaji wrote:
> Looks like this is just updating prefs and not reporting UMA. If so, please
> rename.

Done.

https://codereview.chromium.org/2399783003/diff/1/components/data_use_measure...
components/data_use_measurement/content/data_use_measurement.cc:156:
DataUseUserData* attached_service_data = reinterpret_cast<DataUseUserData*>(
On 2016/10/06 22:17:47, kundaji wrote:
> Can this be a static_cast?

Done.

[Not at Google. Contact bengr, 2016-10-06 23:28:07]

lgtm

[Raj, 2016-10-06 23:31:08]

The CQ bit was checked by rajendrant@chromium.org

[Raj, 2016-10-06 23:31:08]

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/2399783003/#ps20001
(title: "Addressed kundaji comments")

[commit-bot: I haz the power, 2016-10-06 23:32:34]

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-10-07 00:30:11]

Description was changed from

==========
Split the data use into foreground, background and unknown

Currently data use is measured as foreground and background. In some
cases where the app was transitioning from one state to different state,
data use at that time should be logged as unknown.

This CL changes the UMA to log on every network TX/RX of the URLRequest.
Earlier it was logged only when URLRequest completes.

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

to

==========
Split the data use into foreground, background and unknown

Currently data use is measured as foreground and background. In some
cases where the app was transitioning from one state to different state,
data use at that time should be logged as unknown.

This CL changes the UMA to log on every network TX/RX of the URLRequest.
Earlier it was logged only when URLRequest completes.

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

[commit-bot: I haz the power, 2016-10-07 00:30:13]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-10-07 00:32:26]

Description was changed from

==========
Split the data use into foreground, background and unknown

Currently data use is measured as foreground and background. In some
cases where the app was transitioning from one state to different state,
data use at that time should be logged as unknown.

This CL changes the UMA to log on every network TX/RX of the URLRequest.
Earlier it was logged only when URLRequest completes.

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

to

==========
Split the data use into foreground, background and unknown

Currently data use is measured as foreground and background. In some
cases where the app was transitioning from one state to different state,
data use at that time should be logged as unknown.

This CL changes the UMA to log on every network TX/RX of the URLRequest.
Earlier it was logged only when URLRequest completes.

BUG=648808

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

[commit-bot: I haz the power, 2016-10-07 00:32:27]

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