Support for identifying traffic type in data use ascriber

Support for identifying traffic type in data use ascriber

Traffic type will be used in data saver site-breakdown code to ignore
non user initiated traffic.

BUG=718656
Review-Url: https://codereview.chromium.org/2865913002
Cr-Commit-Position: refs/heads/master@{#472293}
Committed: https://chromium.googlesource.com/chromium/src/+/c9b7a5a0d2cf04ecbf06f763b39ba6e12a1c4235

[Raj, 2017-05-08 05:40:55]

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

[commit-bot: I haz the power, 2017-05-08 05:41:11]

Dry run: CQ is trying da patch.

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

[Raj, 2017-05-08 05:49:15]

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

[commit-bot: I haz the power, 2017-05-08 05:49: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, 2017-05-08 07:38:43]

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

[commit-bot: I haz the power, 2017-05-08 07:38:44]

Dry run: This issue passed the CQ dry run.

[Raj, 2017-05-08 17:24:53]

rajendrant@chromium.org changed reviewers:
+ tbansal@chromium.org

[tbansal1, 2017-05-09 00:59:08]

https://codereview.chromium.org/2865913002/diff/20001/chrome/browser/data_use...
File chrome/browser/data_use_measurement/chrome_data_use_ascriber.cc (right):

https://codereview.chromium.org/2865913002/diff/20001/chrome/browser/data_use...
chrome/browser/data_use_measurement/chrome_data_use_ascriber.cc:88:
DataUseRecorderEntry entry = CreateNewDataUseRecorder(request);
May be just provide traffic type as an argument to  CreateNewDataUseRecorder()?
That way you would not have to repeat same code every where 
CreateNewDataUseRecorder() is called. Also, it would provide *some* guarantee
that the traffic type is set in all cases.

https://codereview.chromium.org/2865913002/diff/20001/components/data_use_mea...
File components/data_use_measurement/core/data_use.cc (right):

https://codereview.chromium.org/2865913002/diff/20001/components/data_use_mea...
components/data_use_measurement/core/data_use.cc:14:
total_bytes_received_(other.total_bytes_received_) {}
Add |traffic_type_| here.

https://codereview.chromium.org/2865913002/diff/20001/components/data_use_mea...
components/data_use_measurement/core/data_use.cc:20: total_bytes_received_ +=
other.total_bytes_received_;
May be DCHECK that |traffic_type_| matches |other.traffic_type_|?

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

https://codereview.chromium.org/2865913002/diff/20001/components/data_use_mea...
components/data_use_measurement/core/data_use.h:67: TrafficType traffic_type_;
Is it possible to make this a const field and make sure that it is set in the
ctor? Then, you can also remove UNKNOWN as one of the TrafficType enums? Second
advantage is that it is guaranteed that it will be set everytime a DataUse
object is created which will make the code more robust.

[megjablon, 2017-05-10 20:41:38]

Description was changed from

==========
Support for identifying traffic type in data use ascriber

Traffic type will be used in data saver site-breakdown code to ignore
non user initiated traffic.

BUG=718656
==========

to

==========
Support for identifying traffic type in data use ascriber

Traffic type will be used in data saver site-breakdown code to ignore
non user initiated traffic.

BUG=718656
==========

[megjablon, 2017-05-10 20:41:38]

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

[megjablon, 2017-05-10 20:42:04]

megjablon@chromium.org changed reviewers:
- megjablon@chromium.org

[rajendrant, 2017-05-11 21:16:07]

rajendrant@google.com changed reviewers:
+ rajendrant@google.com

[rajendrant, 2017-05-11 21:16:08]

https://codereview.chromium.org/2865913002/diff/20001/chrome/browser/data_use...
File chrome/browser/data_use_measurement/chrome_data_use_ascriber.cc (right):

https://codereview.chromium.org/2865913002/diff/20001/chrome/browser/data_use...
chrome/browser/data_use_measurement/chrome_data_use_ascriber.cc:88:
DataUseRecorderEntry entry = CreateNewDataUseRecorder(request);
On 2017/05/09 00:59:08, tbansal1 wrote:
> May be just provide traffic type as an argument to 
CreateNewDataUseRecorder()?
> That way you would not have to repeat same code every where 
> CreateNewDataUseRecorder() is called. Also, it would provide *some* guarantee
> that the traffic type is set in all cases.

Done.
But still UNKNOWN type is needed.

https://codereview.chromium.org/2865913002/diff/20001/components/data_use_mea...
File components/data_use_measurement/core/data_use.cc (right):

https://codereview.chromium.org/2865913002/diff/20001/components/data_use_mea...
components/data_use_measurement/core/data_use.cc:14:
total_bytes_received_(other.total_bytes_received_) {}
On 2017/05/09 00:59:08, tbansal1 wrote:
> Add |traffic_type_| here.

Done.

https://codereview.chromium.org/2865913002/diff/20001/components/data_use_mea...
components/data_use_measurement/core/data_use.cc:20: total_bytes_received_ +=
other.total_bytes_received_;
On 2017/05/09 00:59:08, tbansal1 wrote:
> May be DCHECK that |traffic_type_| matches |other.traffic_type_|?

Need not be same while merging. When render frame is created before its main
frame request is sent, type will be UNKNOWN and later merged.

https://cs.chromium.org/chromium/src/chrome/browser/data_use_measurement/chro...

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

https://codereview.chromium.org/2865913002/diff/20001/components/data_use_mea...
components/data_use_measurement/core/data_use.h:67: TrafficType traffic_type_;
On 2017/05/09 00:59:08, tbansal1 wrote:
> Is it possible to make this a const field and make sure that it is set in the
> ctor? Then, you can also remove UNKNOWN as one of the TrafficType enums?
Second
> advantage is that it is guaranteed that it will be set everytime a DataUse
> object is created which will make the code more robust.

Made it as const field.
But still UNKNOWN type is needed.

[tbansal1, 2017-05-12 15:59:58]

Bunch of nits below. I will TAnL after tests are added. While 
you are going through these woefully undocumented classes,
may I ask you to add comments to even the existing code 
(based on your understanding and debugging), so we have
better understanding of it. Thank you so much for debugging 
and cleaning this up.

https://codereview.chromium.org/2865913002/diff/20001/components/data_use_mea...
File components/data_use_measurement/core/data_use.cc (right):

https://codereview.chromium.org/2865913002/diff/20001/components/data_use_mea...
components/data_use_measurement/core/data_use.cc:20: total_bytes_received_ +=
other.total_bytes_received_;
On 2017/05/11 21:16:08, rajendrant wrote:
> On 2017/05/09 00:59:08, tbansal1 wrote:
> > May be DCHECK that |traffic_type_| matches |other.traffic_type_|?
> 
> Need not be same while merging. When render frame is created before its main
> frame request is sent, type will be UNKNOWN and later merged.
> 
>
https://cs.chromium.org/chromium/src/chrome/browser/data_use_measurement/chro...

May be add that as a comment here?

https://codereview.chromium.org/2865913002/diff/40001/components/data_use_mea...
File components/data_use_measurement/core/data_use.h (right):

https://codereview.chromium.org/2865913002/diff/40001/components/data_use_mea...
components/data_use_measurement/core/data_use.h:21: enum TrafficType {
can this be enum class?
https://groups.google.com/a/chromium.org/forum/#!topic/chromium-dev/Q5WmkAImanc

https://codereview.chromium.org/2865913002/diff/40001/components/data_use_mea...
components/data_use_measurement/core/data_use.h:23: UNKNOWN,
May be add a TODO to record metrics on the distribution of traffic type. This
would be useful to figure out if there are lot of samples in UNKNOWN or
SERVICE_WORKER buckets (which are both bad).

https://codereview.chromium.org/2865913002/diff/40001/components/data_use_mea...
components/data_use_measurement/core/data_use.h:35: DataUse(TrafficType
traffic_type);
explicit

https://codereview.chromium.org/2865913002/diff/40001/components/data_use_mea...
components/data_use_measurement/core/data_use.h:56: TrafficType traffic_type() {
return traffic_type_; }
const function

https://codereview.chromium.org/2865913002/diff/40001/components/data_use_mea...
components/data_use_measurement/core/data_use.h:59: friend class
DataUseRecorder;
TODO to remove this friend.
This probably means that DataUseRecorder is writing to the private members of
this class. This makes it difficult to understand how this class interacts with
other classes.

https://codereview.chromium.org/2865913002/diff/40001/components/data_use_mea...
components/data_use_measurement/core/data_use.h:66: int64_t
total_bytes_received_;
DISALLOW_COPY_AND_ASSIGN
or override the assignment operator.

https://codereview.chromium.org/2865913002/diff/40001/components/data_use_mea...
components/data_use_measurement/core/data_use.h:66: int64_t
total_bytes_received_;
May be add a TODO to add thread checker.

https://codereview.chromium.org/2865913002/diff/40001/components/data_use_mea...
File components/data_use_measurement/core/data_use_recorder.h (right):

https://codereview.chromium.org/2865913002/diff/40001/components/data_use_mea...
components/data_use_measurement/core/data_use_recorder.h:28:
DataUseRecorder(DataUse::TrafficType traffic_type);
explicit

[rajendrant, 2017-05-16 00:50:08]

ptal.
Added test.

Yes. will add more comments in other CLs too where the logic is changing.

https://codereview.chromium.org/2865913002/diff/20001/components/data_use_mea...
File components/data_use_measurement/core/data_use.cc (right):

https://codereview.chromium.org/2865913002/diff/20001/components/data_use_mea...
components/data_use_measurement/core/data_use.cc:20: total_bytes_received_ +=
other.total_bytes_received_;
On 2017/05/12 15:59:58, tbansal1 wrote:
> On 2017/05/11 21:16:08, rajendrant wrote:
> > On 2017/05/09 00:59:08, tbansal1 wrote:
> > > May be DCHECK that |traffic_type_| matches |other.traffic_type_|?
> > 
> > Need not be same while merging. When render frame is created before its main
> > frame request is sent, type will be UNKNOWN and later merged.
> > 
> >
>
https://cs.chromium.org/chromium/src/chrome/browser/data_use_measurement/chro...
> 
> May be add that as a comment here?

Done.

https://codereview.chromium.org/2865913002/diff/40001/components/data_use_mea...
File components/data_use_measurement/core/data_use.h (right):

https://codereview.chromium.org/2865913002/diff/40001/components/data_use_mea...
components/data_use_measurement/core/data_use.h:21: enum TrafficType {
On 2017/05/12 15:59:58, tbansal1 wrote:
> can this be enum class?
>
https://groups.google.com/a/chromium.org/forum/#!topic/chromium-dev/Q5WmkAImanc

Done.

https://codereview.chromium.org/2865913002/diff/40001/components/data_use_mea...
components/data_use_measurement/core/data_use.h:23: UNKNOWN,
On 2017/05/12 15:59:58, tbansal1 wrote:
> May be add a TODO to record metrics on the distribution of traffic type. This
> would be useful to figure out if there are lot of samples in UNKNOWN or
> SERVICE_WORKER buckets (which are both bad).

Done.

https://codereview.chromium.org/2865913002/diff/40001/components/data_use_mea...
components/data_use_measurement/core/data_use.h:35: DataUse(TrafficType
traffic_type);
On 2017/05/12 15:59:58, tbansal1 wrote:
> explicit

Done.

https://codereview.chromium.org/2865913002/diff/40001/components/data_use_mea...
components/data_use_measurement/core/data_use.h:56: TrafficType traffic_type() {
return traffic_type_; }
On 2017/05/12 15:59:58, tbansal1 wrote:
> const function

Done.

https://codereview.chromium.org/2865913002/diff/40001/components/data_use_mea...
components/data_use_measurement/core/data_use.h:59: friend class
DataUseRecorder;
On 2017/05/12 15:59:58, tbansal1 wrote:
> TODO to remove this friend.
> This probably means that DataUseRecorder is writing to the private members of
> this class. This makes it difficult to understand how this class interacts
with
> other classes.

Done.

https://codereview.chromium.org/2865913002/diff/40001/components/data_use_mea...
components/data_use_measurement/core/data_use.h:66: int64_t
total_bytes_received_;
On 2017/05/12 15:59:58, tbansal1 wrote:
> May be add a TODO to add thread checker.

Thread checker could be too much for this class. This is supposed to be light
object sent across data use observers.

In future this class could derive from base::SupportsUserData so that observers
can annotate arbitrary information to this. For example, Freighter could
annotate this with amortized bytes.

WDYT

https://codereview.chromium.org/2865913002/diff/40001/components/data_use_mea...
components/data_use_measurement/core/data_use.h:66: int64_t
total_bytes_received_;
On 2017/05/12 15:59:58, tbansal1 wrote:
> DISALLOW_COPY_AND_ASSIGN
> or override the assignment operator.

Done.

https://codereview.chromium.org/2865913002/diff/40001/components/data_use_mea...
File components/data_use_measurement/core/data_use_recorder.h (right):

https://codereview.chromium.org/2865913002/diff/40001/components/data_use_mea...
components/data_use_measurement/core/data_use_recorder.h:28:
DataUseRecorder(DataUse::TrafficType traffic_type);
On 2017/05/12 15:59:58, tbansal1 wrote:
> explicit

Done.

[tbansal1, 2017-05-16 13:32:37]

lgtm

[rajendrant, 2017-05-16 17:57:10]

The CQ bit was checked by rajendrant@google.com

[commit-bot: I haz the power, 2017-05-16 17:58: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, 2017-05-16 18:06:59]

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

[commit-bot: I haz the power, 2017-05-16 18:07:00]

Try jobs failed on following builders:
  ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)

[Raj, 2017-05-16 22:01:51]

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

[commit-bot: I haz the power, 2017-05-16 22:04: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, 2017-05-17 00:05:56]

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

[commit-bot: I haz the power, 2017-05-17 00:05:57]

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

[rajendrant, 2017-05-17 00:49:18]

The CQ bit was checked by rajendrant@google.com

[rajendrant, 2017-05-17 00:49:18]

The patchset sent to the CQ was uploaded after l-g-t-m from tbansal@chromium.org
Link to the patchset: https://codereview.chromium.org/2865913002/#ps100001
(title: "compile fix")

[commit-bot: I haz the power, 2017-05-17 00:51:49]

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, 2017-05-17 02:40:41]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1494982158423090,
"parent_rev": "7f18c51a5a46888bedd8034eb154c69ae07e7cba", "commit_rev":
"c9b7a5a0d2cf04ecbf06f763b39ba6e12a1c4235"}

[commit-bot: I haz the power, 2017-05-17 02:40:57]

Description was changed from

==========
Support for identifying traffic type in data use ascriber

Traffic type will be used in data saver site-breakdown code to ignore
non user initiated traffic.

BUG=718656
==========

to

==========
Support for identifying traffic type in data use ascriber

Traffic type will be used in data saver site-breakdown code to ignore
non user initiated traffic.

BUG=718656

Review-Url: https://codereview.chromium.org/2865913002
Cr-Commit-Position: refs/heads/master@{#472293}
Committed:
https://chromium.googlesource.com/chromium/src/+/c9b7a5a0d2cf04ecbf06f763b39b...
==========

[commit-bot: I haz the power, 2017-05-17 02:40:58]

Committed patchset #6 (id:100001) as
https://chromium.googlesource.com/chromium/src/+/c9b7a5a0d2cf04ecbf06f763b39b...