Record the tab state during data use

Record the tab state during data use

DataUse.AppTabState UMA is recorded with the following dimensions.
  Upstream.AppBackground
  Upstream.AppForeground.TabBackground
  Upstream.AppForeground.TabForeground
  Downstream.AppBackground
  Downstream.AppForeground.TabBackground
  Downstream.AppForeground.TabForeground

BUG=673820
Committed: https://crrev.com/f3ccb26f8d14aa3aff875d0b4e097568b2ec9b8a
Cr-Commit-Position: refs/heads/master@{#439742}

[Raj, 2016-12-15 23:46:58]

rajendrant@chromium.org changed reviewers:
+ rkaplow@chromium.org, ryansturm@chromium.org

[Raj, 2016-12-15 23:46:59]

ryansturm: ptal components/data_use_measurement/*
rkaplow: ptal histograms.xml

[RyanSturm, 2016-12-16 16:26:12]

https://codereview.chromium.org/2583703002/diff/1/chrome/browser/data_use_mea...
File chrome/browser/data_use_measurement/chrome_data_use_ascriber.cc (right):

https://codereview.chromium.org/2583703002/diff/1/chrome/browser/data_use_mea...
chrome/browser/data_use_measurement/chrome_data_use_ascriber.cc:54: const
DataUseRecorder* ChromeDataUseAscriber::GetDataUseRecorderIfExists(
Since you are adding this for const-ness, can you remove the "bool
can_create_new" arg from GetDataUseRecorder and GetDataUseRecorderEntry. Passing
false to that method does the same thing as this method without the const
request or the const on the method. DataReductionProxyData has a similar pair of
methods (except they are static instead of const methods).

That will also require removing the const from the return type of this method.

https://codereview.chromium.org/2583703002/diff/1/chrome/browser/data_use_mea...
chrome/browser/data_use_measurement/chrome_data_use_ascriber.cc:59: return
(user_data && user_data->recorder_entry() != data_use_recorders_.end())
You shouldn't need to check user_data->recorder_entry() !=
data_use_recorders_.end(), as an assumption of the user data class is that a
valid entry is being passed to the constructor.

https://codereview.chromium.org/2583703002/diff/1/chrome/browser/data_use_mea...
chrome/browser/data_use_measurement/chrome_data_use_ascriber.cc:411: void
ChromeDataUseAscriber::RenderFrameHostChanged(int old_render_process_id,
You copied this method from above somehow I think.

https://codereview.chromium.org/2583703002/diff/1/chrome/browser/data_use_mea...
File chrome/browser/data_use_measurement/chrome_data_use_ascriber_service.cc
(right):

https://codereview.chromium.org/2583703002/diff/1/chrome/browser/data_use_mea...
chrome/browser/data_use_measurement/chrome_data_use_ascriber_service.cc:222:
void ChromeDataUseAscriberService::RenderFrameHostChanged(
You copied this method from above accidentally.

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

https://codereview.chromium.org/2583703002/diff/1/components/data_use_measure...
components/data_use_measurement/core/data_use_measurement.cc:70:
DataUseAscriber* ascriber,
nit: can you order the arguments in the same order as the initialization list;
i.e., move ascriber to be the last argument.

https://codereview.chromium.org/2583703002/diff/1/components/data_use_measure...
components/data_use_measurement/core/data_use_measurement.cc:200: const auto
recorder = ascriber_->GetDataUseRecorderIfExists(&request);
nit: s/auto/DataUseRecorder*/

It's a little fuzzy in the style guide whether the method name Get<type>IfExists
is enough to allow auto:
https://google.github.io/styleguide/cppguide.html#auto

https://codereview.chromium.org/2583703002/diff/1/components/data_use_measure...
components/data_use_measurement/core/data_use_measurement.cc:357:
base::StringPrintf("DataUse.AppTabState.%s.%s",
This expression might be a little more readable if you built the histogram
string in steps.

std::string histogram_name = "DataUse.AppTabState.";
histogram_name.append(...)
if (app_state == brackground)
{...}
else
{...}

RecordUMAHistogramCount(histogram_name...)

https://codereview.chromium.org/2583703002/diff/1/components/data_use_measure...
File components/data_use_measurement/core/data_use_measurement.h (right):

https://codereview.chromium.org/2583703002/diff/1/components/data_use_measure...
components/data_use_measurement/core/data_use_measurement.h:79: enum TabState {
FOREGROUND, BACKGROUND, UNKNOWN };
Is this enum really necessary, you treat it as boolean wherever you use it, so
maybe it is better to pass is_visible or is_in_foreground.

https://codereview.chromium.org/2583703002/diff/1/components/data_use_measure...
components/data_use_measurement/core/data_use_measurement.h:135: // A helper
function used to record the app and tab state when data use
How about "Records data use histograms split on TrafficDirection, AppState and
TabState."

https://codereview.chromium.org/2583703002/diff/1/components/data_use_measure...
components/data_use_measurement/core/data_use_measurement.h:151: //
DataUseAscriber used to get the attributes of data use. Not owned by
I don't think you need to say "Not owned by |this|" since it is a raw pointer.

https://codereview.chromium.org/2583703002/diff/1/tools/metrics/histograms/hi...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/2583703002/diff/1/tools/metrics/histograms/hi...
tools/metrics/histograms/histograms.xml:109174: +  <affected-histogram
name="DataUse.AppTabState"/>
I think it would be better to use an already existing affected-histogram. Does
DataUse.TrafficSize.User already measure this same thing not broken up by
direction, and state? If so you should just use that as the affected histogram
so that these would be "DataUse.TrafficSize.User.Upstream.AppBackground".

WDYT?

[Raj, 2016-12-16 19:52:55]

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

[commit-bot: I haz the power, 2016-12-16 19:53:34]

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

[Raj, 2016-12-16 19:56:04]

Patchset #2 (id:20001) has been deleted

[Raj, 2016-12-16 19:58:31]

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

[Raj, 2016-12-16 19:59:11]

https://codereview.chromium.org/2583703002/diff/1/chrome/browser/data_use_mea...
File chrome/browser/data_use_measurement/chrome_data_use_ascriber.cc (right):

https://codereview.chromium.org/2583703002/diff/1/chrome/browser/data_use_mea...
chrome/browser/data_use_measurement/chrome_data_use_ascriber.cc:54: const
DataUseRecorder* ChromeDataUseAscriber::GetDataUseRecorderIfExists(
On 2016/12/16 16:26:10, Ryan Sturm wrote:
> Since you are adding this for const-ness, can you remove the "bool
> can_create_new" arg from GetDataUseRecorder and GetDataUseRecorderEntry.
Passing
> false to that method does the same thing as this method without the const
> request or the const on the method. DataReductionProxyData has a similar pair
of
> methods (except they are static instead of const methods).
> 
> That will also require removing the const from the return type of this method.

Changed as you suggested.
But I still think being const-function and returning-const would be better.

https://codereview.chromium.org/2583703002/diff/1/chrome/browser/data_use_mea...
chrome/browser/data_use_measurement/chrome_data_use_ascriber.cc:59: return
(user_data && user_data->recorder_entry() != data_use_recorders_.end())
On 2016/12/16 16:26:10, Ryan Sturm wrote:
> You shouldn't need to check user_data->recorder_entry() !=
> data_use_recorders_.end(), as an assumption of the user data class is that a
> valid entry is being passed to the constructor. 
> 

Done.

https://codereview.chromium.org/2583703002/diff/1/chrome/browser/data_use_mea...
chrome/browser/data_use_measurement/chrome_data_use_ascriber.cc:411: void
ChromeDataUseAscriber::RenderFrameHostChanged(int old_render_process_id,
On 2016/12/16 16:26:10, Ryan Sturm wrote:
> You copied this method from above somehow I think.

Oops. Should be due to rebase. Removed.

https://codereview.chromium.org/2583703002/diff/1/chrome/browser/data_use_mea...
File chrome/browser/data_use_measurement/chrome_data_use_ascriber_service.cc
(right):

https://codereview.chromium.org/2583703002/diff/1/chrome/browser/data_use_mea...
chrome/browser/data_use_measurement/chrome_data_use_ascriber_service.cc:222:
void ChromeDataUseAscriberService::RenderFrameHostChanged(
On 2016/12/16 16:26:10, Ryan Sturm wrote:
> You copied this method from above accidentally.

Rmoved.

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

https://codereview.chromium.org/2583703002/diff/1/components/data_use_measure...
components/data_use_measurement/core/data_use_measurement.cc:70:
DataUseAscriber* ascriber,
On 2016/12/16 16:26:10, Ryan Sturm wrote:
> nit: can you order the arguments in the same order as the initialization list;
> i.e., move ascriber to be the last argument.

Done.

https://codereview.chromium.org/2583703002/diff/1/components/data_use_measure...
components/data_use_measurement/core/data_use_measurement.cc:200: const auto
recorder = ascriber_->GetDataUseRecorderIfExists(&request);
On 2016/12/16 16:26:10, Ryan Sturm wrote:
> nit: s/auto/DataUseRecorder*/
> 
> It's a little fuzzy in the style guide whether the method name
Get<type>IfExists
> is enough to allow auto:
> https://google.github.io/styleguide/cppguide.html#auto

Done.

https://codereview.chromium.org/2583703002/diff/1/components/data_use_measure...
components/data_use_measurement/core/data_use_measurement.cc:357:
base::StringPrintf("DataUse.AppTabState.%s.%s",
On 2016/12/16 16:26:10, Ryan Sturm wrote:
> This expression might be a little more readable if you built the histogram
> string in steps.
> 
> std::string histogram_name = "DataUse.AppTabState.";
> histogram_name.append(...)
> if (app_state == brackground)
> {...}
> else
> {...}
> 
> RecordUMAHistogramCount(histogram_name...)

Done.

https://codereview.chromium.org/2583703002/diff/1/components/data_use_measure...
File components/data_use_measurement/core/data_use_measurement.h (right):

https://codereview.chromium.org/2583703002/diff/1/components/data_use_measure...
components/data_use_measurement/core/data_use_measurement.h:79: enum TabState {
FOREGROUND, BACKGROUND, UNKNOWN };
On 2016/12/16 16:26:10, Ryan Sturm wrote:
> Is this enum really necessary, you treat it as boolean wherever you use it, so
> maybe it is better to pass is_visible or is_in_foreground.

Done.

https://codereview.chromium.org/2583703002/diff/1/components/data_use_measure...
components/data_use_measurement/core/data_use_measurement.h:135: // A helper
function used to record the app and tab state when data use
On 2016/12/16 16:26:11, Ryan Sturm wrote:
> How about "Records data use histograms split on TrafficDirection, AppState and
> TabState."

Done.

https://codereview.chromium.org/2583703002/diff/1/components/data_use_measure...
components/data_use_measurement/core/data_use_measurement.h:151: //
DataUseAscriber used to get the attributes of data use. Not owned by
On 2016/12/16 16:26:11, Ryan Sturm wrote:
> I don't think you need to say "Not owned by |this|" since it is a raw pointer.

Done.

https://codereview.chromium.org/2583703002/diff/1/tools/metrics/histograms/hi...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/2583703002/diff/1/tools/metrics/histograms/hi...
tools/metrics/histograms/histograms.xml:109174: +  <affected-histogram
name="DataUse.AppTabState"/>
On 2016/12/16 16:26:11, Ryan Sturm wrote:
> I think it would be better to use an already existing affected-histogram. Does
> DataUse.TrafficSize.User already measure this same thing not broken up by
> direction, and state? If so you should just use that as the affected histogram
> so that these would be "DataUse.TrafficSize.User.Upstream.AppBackground".
> 
> WDYT?

All existing histograms measure datause broken by direction, appstate, cellular.
For the tabstate, cellular is a don't care, which would reduce the number of
histograms. Is it performance/datause intensive to add new affected-histogram ?

[commit-bot: I haz the power, 2016-12-16 19:59:30]

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-12-16 20:30:37]

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

[commit-bot: I haz the power, 2016-12-16 20:30:38]

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-12-16 21:15:32]

Patchset #2 (id:40001) has been deleted

[Raj, 2016-12-16 21:15:38]

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

[commit-bot: I haz the power, 2016-12-16 21:16:23]

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-12-16 23:34:01]

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

[commit-bot: I haz the power, 2016-12-16 23:34:02]

Dry run: Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)

[RyanSturm, 2016-12-19 19:31:50]

couple nits otherwise lgtm.

Maybe let the histograms reviewer decide on the histograms comment I brought up.

https://codereview.chromium.org/2583703002/diff/1/chrome/browser/data_use_mea...
File chrome/browser/data_use_measurement/chrome_data_use_ascriber.cc (right):

https://codereview.chromium.org/2583703002/diff/1/chrome/browser/data_use_mea...
chrome/browser/data_use_measurement/chrome_data_use_ascriber.cc:54: const
DataUseRecorder* ChromeDataUseAscriber::GetDataUseRecorderIfExists(
On 2016/12/16 19:59:10, Raj wrote:
> On 2016/12/16 16:26:10, Ryan Sturm wrote:
> > Since you are adding this for const-ness, can you remove the "bool
> > can_create_new" arg from GetDataUseRecorder and GetDataUseRecorderEntry.
> Passing
> > false to that method does the same thing as this method without the const
> > request or the const on the method. DataReductionProxyData has a similar
pair
> of
> > methods (except they are static instead of const methods).
> > 
> > That will also require removing the const from the return type of this
method.
> 
> Changed as you suggested.
> But I still think being const-function and returning-const would be better.

Sorry I was a little unclear. Const-method is good.

I think returning const is a bad idea, because users will most likely want to
call non-const on the DataUseRecorder.

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

https://codereview.chromium.org/2583703002/diff/1/components/data_use_measure...
components/data_use_measurement/core/data_use_measurement.cc:357:
base::StringPrintf("DataUse.AppTabState.%s.%s",
On 2016/12/16 19:59:10, Raj wrote:
> On 2016/12/16 16:26:10, Ryan Sturm wrote:
> > This expression might be a little more readable if you built the histogram
> > string in steps.
> > 
> > std::string histogram_name = "DataUse.AppTabState.";
> > histogram_name.append(...)
> > if (app_state == brackground)
> > {...}
> > else
> > {...}
> > 
> > RecordUMAHistogramCount(histogram_name...)
> 
> Done.

Thanks.

https://codereview.chromium.org/2583703002/diff/1/tools/metrics/histograms/hi...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/2583703002/diff/1/tools/metrics/histograms/hi...
tools/metrics/histograms/histograms.xml:109174: +  <affected-histogram
name="DataUse.AppTabState"/>
On 2016/12/16 19:59:11, Raj wrote:
> On 2016/12/16 16:26:11, Ryan Sturm wrote:
> > I think it would be better to use an already existing affected-histogram.
Does
> > DataUse.TrafficSize.User already measure this same thing not broken up by
> > direction, and state? If so you should just use that as the affected
histogram
> > so that these would be "DataUse.TrafficSize.User.Upstream.AppBackground".
> > 
> > WDYT?
> 
> All existing histograms measure datause broken by direction, appstate,
cellular.
> For the tabstate, cellular is a don't care, which would reduce the number of
> histograms. Is it performance/datause intensive to add new affected-histogram
?

It is only expensive in the sense that histograms.xml gets bigger and you have
to write descriptions twice AFAIK.

If you add all of these suffixes to <histogram_suffixes
name="DataUse.Dimensions"> will there be a problem?

You should be able to have some suffixes track cellular and some track app state
in the same histogram_suffixes.

https://codereview.chromium.org/2583703002/diff/60001/components/data_use_mea...
File components/data_use_measurement/core/data_use_ascriber.h (right):

https://codereview.chromium.org/2583703002/diff/60001/components/data_use_mea...
components/data_use_measurement/core/data_use_ascriber.h:49: const
net::URLRequest* request) = 0;
const net::URLRequest&

[rkaplow, 2016-12-19 23:18:17]

lgtm

histogram lg

[Raj, 2016-12-20 05:28:18]

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

[commit-bot: I haz the power, 2016-12-20 05:28:28]

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

[Raj, 2016-12-20 05:49:28]

The CQ bit was unchecked by rajendrant@chromium.org

[Raj, 2016-12-20 05:49:28]

The CQ bit was checked by rajendrant@chromium.org

[Raj, 2016-12-20 05:49:29]

The patchset sent to the CQ was uploaded after l-g-t-m from
ryansturm@chromium.org, rkaplow@chromium.org
Link to the patchset: https://codereview.chromium.org/2583703002/#ps100001
(title: "minor fix")

[commit-bot: I haz the power, 2016-12-20 05:49:36]

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-12-20 07:54:43]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1482212968798220,
"parent_rev": "2b66591f6d8f9ad99be698b91278a053aacc9113", "commit_rev":
"7de336e5ff41b598ed1afe78d0e0fd14becccd67"}

[commit-bot: I haz the power, 2016-12-20 07:55:09]

Description was changed from

==========
Record the tab state during data use

DataUse.AppTabState UMA is recorded with the following dimensions.
  Upstream.AppBackground
  Upstream.AppForeground.TabBackground
  Upstream.AppForeground.TabForeground
  Downstream.AppBackground
  Downstream.AppForeground.TabBackground
  Downstream.AppForeground.TabForeground


BUG=673820
==========

to

==========
Record the tab state during data use

DataUse.AppTabState UMA is recorded with the following dimensions.
  Upstream.AppBackground
  Upstream.AppForeground.TabBackground
  Upstream.AppForeground.TabForeground
  Downstream.AppBackground
  Downstream.AppForeground.TabBackground
  Downstream.AppForeground.TabForeground


BUG=673820

Review-Url: https://codereview.chromium.org/2583703002
==========

[commit-bot: I haz the power, 2016-12-20 07:55:10]

Committed patchset #4 (id:100001)

[commit-bot: I haz the power, 2016-12-20 07:57:51]

Description was changed from

==========
Record the tab state during data use

DataUse.AppTabState UMA is recorded with the following dimensions.
  Upstream.AppBackground
  Upstream.AppForeground.TabBackground
  Upstream.AppForeground.TabForeground
  Downstream.AppBackground
  Downstream.AppForeground.TabBackground
  Downstream.AppForeground.TabForeground


BUG=673820

Review-Url: https://codereview.chromium.org/2583703002
==========

to

==========
Record the tab state during data use

DataUse.AppTabState UMA is recorded with the following dimensions.
  Upstream.AppBackground
  Upstream.AppForeground.TabBackground
  Upstream.AppForeground.TabForeground
  Downstream.AppBackground
  Downstream.AppForeground.TabBackground
  Downstream.AppForeground.TabForeground

BUG=673820

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

[commit-bot: I haz the power, 2016-12-20 07:57:52]

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