Propagate network delegate events to ChromeDataUseAscriber.

Propagate network delegate events to ChromeDataUseAscriber.

ChromeDataUseAscriber ascribes all data use to logical entities
such as page loads. Propagating pertinent network events to
the ascriber provides it with the hooks to track data usage.
The actual tracking will be done in a future cl.

BUG=645188
Committed: https://crrev.com/c6135966d9e76c2f69f21f1f6a24be7609309b0d
Cr-Commit-Position: refs/heads/master@{#420926}

[Not at Google. Contact bengr, 2016-09-21 16:17:27]

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

[Not at Google. Contact bengr, 2016-09-21 16:17:48]

Description was changed from

==========
Propagate network delegate events to ChromeDataUseAscriber.

ChromeDataUseAscriber ascribes all data use to logical entities
such as page loads. Propagating pertinent network events to
the ascriber provides it with the hooks to track data usage.
The actual tracking will be done in a future cl.

BUG=645188
==========

to

==========
Propagate network delegate events to ChromeDataUseAscriber.

ChromeDataUseAscriber ascribes all data use to logical entities
such as page loads. Propagating pertinent network events to
the ascriber provides it with the hooks to track data usage.
The actual tracking will be done in a future cl.

BUG=645188
==========

[Not at Google. Contact bengr, 2016-09-21 16:17:48]

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

[commit-bot: I haz the power, 2016-09-21 16:18:02]

Dry run: 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-21 16:19:14]

bengr:
  components/data_use_measurement/*

mmenke:
  chrome/browser/*

[Not at Google. Contact bengr, 2016-09-21 16:23:17]

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

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

Dry run: 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-21 16:35:59]

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

[commit-bot: I haz the power, 2016-09-21 16:36:26]

Dry run: 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-21 16:54:00]

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

[commit-bot: I haz the power, 2016-09-21 16:54:08]

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-21 16:58:07]

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

[commit-bot: I haz the power, 2016-09-21 16:58:07]

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_...)
  chromeos_x86-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_x86-ge...)
  linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[mmenke, 2016-09-21 17:57:27]

On 2016/09/21 16:19:14, kundaji wrote:
> bengr:
>   components/data_use_measurement/*
> 
> mmenke:
>   chrome/browser/*

Should we use a LayeredNetworkDelegate for this instead?

[Not at Google. Contact bengr, 2016-09-21 18:39:28]

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

[commit-bot: I haz the power, 2016-09-21 18:39:55]

Dry run: 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-21 19:04:26]

On 2016/09/21 17:57:27, mmenke wrote:
> On 2016/09/21 16:19:14, kundaji wrote:
> > bengr:
> >   components/data_use_measurement/*
> > 
> > mmenke:
> >   chrome/browser/*
> 
> Should we use a LayeredNetworkDelegate for this instead?

Using LayeredNetworkDelegate was my first instinct. It is doable, but is a
little more complicated. 

Specifically, will need an extra class that will subclass LayeredNetworkDelegate
and propagate the events to ChromeDataUseAscriber. This is because there should
be only one instance of ChromeDataUseAscriber, whereas there are multiple
NetworkDelegates. 

What do you think?

[mmenke, 2016-09-21 19:06:05]

On 2016/09/21 19:04:26, kundaji wrote:
> On 2016/09/21 17:57:27, mmenke wrote:
> > On 2016/09/21 16:19:14, kundaji wrote:
> > > bengr:
> > >   components/data_use_measurement/*
> > > 
> > > mmenke:
> > >   chrome/browser/*
> > 
> > Should we use a LayeredNetworkDelegate for this instead?
> 
> Using LayeredNetworkDelegate was my first instinct. It is doable, but is a
> little more complicated. 
> 
> Specifically, will need an extra class that will subclass
LayeredNetworkDelegate
> and propagate the events to ChromeDataUseAscriber. This is because there
should
> be only one instance of ChromeDataUseAscriber, whereas there are multiple
> NetworkDelegates. 
> 
> What do you think?

I'm fine with that - I think the layering is much simpler, and it's much more
easily pluggable into the network stack.  I'm fine with the extra class, seems
worth it.

[commit-bot: I haz the power, 2016-09-21 20:11:53]

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

[commit-bot: I haz the power, 2016-09-21 20:11:54]

Dry run: This issue passed the CQ dry run.

[Not at Google. Contact bengr, 2016-09-21 20:32:13]

On 2016/09/21 19:06:05, mmenke wrote:
> On 2016/09/21 19:04:26, kundaji wrote:
> > On 2016/09/21 17:57:27, mmenke wrote:
> > > On 2016/09/21 16:19:14, kundaji wrote:
> > > > bengr:
> > > >   components/data_use_measurement/*
> > > > 
> > > > mmenke:
> > > >   chrome/browser/*
> > > 
> > > Should we use a LayeredNetworkDelegate for this instead?
> > 
> > Using LayeredNetworkDelegate was my first instinct. It is doable, but is a
> > little more complicated. 
> > 
> > Specifically, will need an extra class that will subclass
> LayeredNetworkDelegate
> > and propagate the events to ChromeDataUseAscriber. This is because there
> should
> > be only one instance of ChromeDataUseAscriber, whereas there are multiple
> > NetworkDelegates. 
> > 
> > What do you think?
> 
> I'm fine with that - I think the layering is much simpler, and it's much more
> easily pluggable into the network stack.  I'm fine with the extra class, seems
> worth it.

Sounds good. I am changing the cl to use a LayeredNetworkDelegate. I have 2
questions:

1) If a delegate changes new_url in OnBeforeURLRequest():
      a) is OnBeforeURLRequest() called again with the new URL for all the
delegates?
      b) is OnBeforeRedirect() called for all the delegates with the new URL?
2) If a delegate returns anything other than a OK or ERR_IO_PENDING in
OnBeforeURLRequest() will the OnCompleted() method still be called for the
URLRequest?

Thanks!

[mmenke, 2016-09-21 20:34:51]

On 2016/09/21 20:32:13, kundaji wrote:
> On 2016/09/21 19:06:05, mmenke wrote:
> > On 2016/09/21 19:04:26, kundaji wrote:
> > > On 2016/09/21 17:57:27, mmenke wrote:
> > > > On 2016/09/21 16:19:14, kundaji wrote:
> > > > > bengr:
> > > > >   components/data_use_measurement/*
> > > > > 
> > > > > mmenke:
> > > > >   chrome/browser/*
> > > > 
> > > > Should we use a LayeredNetworkDelegate for this instead?
> > > 
> > > Using LayeredNetworkDelegate was my first instinct. It is doable, but is a
> > > little more complicated. 
> > > 
> > > Specifically, will need an extra class that will subclass
> > LayeredNetworkDelegate
> > > and propagate the events to ChromeDataUseAscriber. This is because there
> > should
> > > be only one instance of ChromeDataUseAscriber, whereas there are multiple
> > > NetworkDelegates. 
> > > 
> > > What do you think?
> > 
> > I'm fine with that - I think the layering is much simpler, and it's much
more
> > easily pluggable into the network stack.  I'm fine with the extra class,
seems
> > worth it.
> 
> Sounds good. I am changing the cl to use a LayeredNetworkDelegate. I have 2
> questions:
> 
> 1) If a delegate changes new_url in OnBeforeURLRequest():
>       a) is OnBeforeURLRequest() called again with the new URL for all the
> delegates?

No.

>       b) is OnBeforeRedirect() called for all the delegates with the new URL?

Yes, it looks just like a normal redirect (Well, mostly like a normal redirect).

> 2) If a delegate returns anything other than a OK or ERR_IO_PENDING in
> OnBeforeURLRequest() will the OnCompleted() method still be called for the
> URLRequest?

Yes.

> Thanks!

[Not at Google. Contact bengr, 2016-09-21 23:18:11]

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

[commit-bot: I haz the power, 2016-09-21 23:18:52]

Dry run: 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-21 23:24:12]

On 2016/09/21 20:34:51, mmenke wrote:
> On 2016/09/21 20:32:13, kundaji wrote:
> > On 2016/09/21 19:06:05, mmenke wrote:
> > > On 2016/09/21 19:04:26, kundaji wrote:
> > > > On 2016/09/21 17:57:27, mmenke wrote:
> > > > > On 2016/09/21 16:19:14, kundaji wrote:
> > > > > > bengr:
> > > > > >   components/data_use_measurement/*
> > > > > > 
> > > > > > mmenke:
> > > > > >   chrome/browser/*
> > > > > 
> > > > > Should we use a LayeredNetworkDelegate for this instead?
> > > > 
> > > > Using LayeredNetworkDelegate was my first instinct. It is doable, but is
a
> > > > little more complicated. 
> > > > 
> > > > Specifically, will need an extra class that will subclass
> > > LayeredNetworkDelegate
> > > > and propagate the events to ChromeDataUseAscriber. This is because there
> > > should
> > > > be only one instance of ChromeDataUseAscriber, whereas there are
multiple
> > > > NetworkDelegates. 
> > > > 
> > > > What do you think?
> > > 
> > > I'm fine with that - I think the layering is much simpler, and it's much
> more
> > > easily pluggable into the network stack.  I'm fine with the extra class,
> seems
> > > worth it.
> > 
> > Sounds good. I am changing the cl to use a LayeredNetworkDelegate. I have 2
> > questions:
> > 
> > 1) If a delegate changes new_url in OnBeforeURLRequest():
> >       a) is OnBeforeURLRequest() called again with the new URL for all the
> > delegates?
> 
> No.
> 
> >       b) is OnBeforeRedirect() called for all the delegates with the new
URL?
> 
> Yes, it looks just like a normal redirect (Well, mostly like a normal
redirect).
> 
> > 2) If a delegate returns anything other than a OK or ERR_IO_PENDING in
> > OnBeforeURLRequest() will the OnCompleted() method still be called for the
> > URLRequest?
> 
> Yes.
> 
> > Thanks!

Switched to using a LayeredNetworkDelegate. PTAL.

[Not at Google. Contact bengr, 2016-09-21 23:33:20]

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

[commit-bot: I haz the power, 2016-09-21 23:34: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-22 00:28:18]

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

[commit-bot: I haz the power, 2016-09-22 00:28:19]

Dry run: This issue passed the CQ dry run.

[mmenke, 2016-09-22 14:36:45]

ProfileImpIOData, IOData LGTM.  I assume it's deliberate that you're not hooking
up incognito mode.

https://codereview.chromium.org/2354323002/diff/120001/chrome/browser/io_thre...
File chrome/browser/io_thread.cc (right):

https://codereview.chromium.org/2354323002/diff/120001/chrome/browser/io_thre...
chrome/browser/io_thread.cc:52: #include
"components/data_use_measurement/core/data_use_ascriber.h"
Not needed, since you never derefernce one of these, see other comment, though. 
Both comments apply to both files.

https://codereview.chromium.org/2354323002/diff/120001/chrome/browser/io_thre...
chrome/browser/io_thread.cc:536: globals_->data_use_ascriber.get());
optional:  I'd suggest making this method:

globals_->data_use_ascriber->CreateNetworkDelegate(
    std::move(chrome_network_delegate));

Instead.  Then we only have to know about one DataUse class, and the ascriber
will (hopefully) be responsible for any changes in the API.  I only have a weak
preference for this, so if you prefer this approach, that's fine.

[Not at Google. Contact bengr, 2016-09-22 16:32:08]

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

[commit-bot: I haz the power, 2016-09-22 16:32:25]

Dry run: 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-22 16:35:56]

Yes, deliberately not hooking up incognito. Thanks for confirming.

https://codereview.chromium.org/2354323002/diff/120001/chrome/browser/io_thre...
File chrome/browser/io_thread.cc (right):

https://codereview.chromium.org/2354323002/diff/120001/chrome/browser/io_thre...
chrome/browser/io_thread.cc:52: #include
"components/data_use_measurement/core/data_use_ascriber.h"
On 2016/09/22 14:36:45, mmenke wrote:
> Not needed, since you never derefernce one of these, see other comment,
though. 
> Both comments apply to both files.

Done.

https://codereview.chromium.org/2354323002/diff/120001/chrome/browser/io_thre...
chrome/browser/io_thread.cc:536: globals_->data_use_ascriber.get());
On 2016/09/22 14:36:45, mmenke wrote:
> optional:  I'd suggest making this method:
> 
> globals_->data_use_ascriber->CreateNetworkDelegate(
>     std::move(chrome_network_delegate));
> 
> Instead.  Then we only have to know about one DataUse class, and the ascriber
> will (hopefully) be responsible for any changes in the API.  I only have a
weak
> preference for this, so if you prefer this approach, that's fine.

Done. I like this suggestion since it makes the interface of the
data_use_measurement component cleaner as well.

[commit-bot: I haz the power, 2016-09-22 17:24:00]

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

[commit-bot: I haz the power, 2016-09-22 17:24:01]

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

[Not at Google. Contact bengr, 2016-09-22 18:17:55]

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

[Raj, 2016-09-22 20:45:10]

lgtm

[Not at Google. Contact bengr, 2016-09-22 20:45:36]

kundaji@chromium.org changed reviewers:
- bengr@chromium.org

[Not at Google. Contact bengr, 2016-09-22 20:47:12]

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

[bengr, 2016-09-23 21:21:13]

lgtm with nits.

https://codereview.chromium.org/2354323002/diff/140001/components/data_use_me...
File components/data_use_measurement/core/data_use_ascriber.cc (right):

https://codereview.chromium.org/2354323002/diff/140001/components/data_use_me...
components/data_use_measurement/core/data_use_ascriber.cc:20: void
DataUseAscriber::OnBeforeUrlRequest(net::URLRequest* request) {}
It's hard to tell if theses are the right prototypes without implementations.
I'm ok with you landing as is if you're ok with me pushing back on this API once
you have code.

https://codereview.chromium.org/2354323002/diff/140001/components/data_use_me...
File components/data_use_measurement/core/data_use_ascriber.h (right):

https://codereview.chromium.org/2354323002/diff/140001/components/data_use_me...
components/data_use_measurement/core/data_use_ascriber.h:9: #include <memory>
Add a blank line above.

https://codereview.chromium.org/2354323002/diff/140001/components/data_use_me...
File components/data_use_measurement/core/data_use_network_delegate.h (right):

https://codereview.chromium.org/2354323002/diff/140001/components/data_use_me...
components/data_use_measurement/core/data_use_network_delegate.h:9: #include
<memory>
Insert blank line above.

[Not at Google. Contact bengr, 2016-09-23 23:18:10]

https://codereview.chromium.org/2354323002/diff/140001/components/data_use_me...
File components/data_use_measurement/core/data_use_ascriber.cc (right):

https://codereview.chromium.org/2354323002/diff/140001/components/data_use_me...
components/data_use_measurement/core/data_use_ascriber.cc:20: void
DataUseAscriber::OnBeforeUrlRequest(net::URLRequest* request) {}
On 2016/09/23 21:21:13, bengr wrote:
> It's hard to tell if theses are the right prototypes without implementations.
> I'm ok with you landing as is if you're ok with me pushing back on this API
once
> you have code.

These calls come from the network delegate, so not sure what you mean by
changing this API. Do you have any alternative hooks to get network events in
mind?

On a separate note, I just realized that since only DataUseNetworkDelegate calls
these methods, they could be made protected with DataUseNetworkDelegate being
made a friend class.

[Not at Google. Contact bengr, 2016-09-26 16:29:00]

https://codereview.chromium.org/2354323002/diff/140001/components/data_use_me...
File components/data_use_measurement/core/data_use_ascriber.h (right):

https://codereview.chromium.org/2354323002/diff/140001/components/data_use_me...
components/data_use_measurement/core/data_use_ascriber.h:9: #include <memory>
On 2016/09/23 21:21:13, bengr wrote:
> Add a blank line above.

Done.

https://codereview.chromium.org/2354323002/diff/140001/components/data_use_me...
File components/data_use_measurement/core/data_use_network_delegate.h (right):

https://codereview.chromium.org/2354323002/diff/140001/components/data_use_me...
components/data_use_measurement/core/data_use_network_delegate.h:9: #include
<memory>
On 2016/09/23 21:21:13, bengr wrote:
> Insert blank line above.

Done.

[Not at Google. Contact bengr, 2016-09-26 16:29:07]

The CQ bit was checked by kundaji@chromium.org

[Not at Google. Contact bengr, 2016-09-26 16:29:07]

The patchset sent to the CQ was uploaded after l-g-t-m from mmenke@chromium.org,
bengr@chromium.org, rajendrant@chromium.org
Link to the patchset: https://codereview.chromium.org/2354323002/#ps160001
(title: "new lines")

[commit-bot: I haz the power, 2016-09-26 16:29:31]

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 17:54:49]

Description was changed from

==========
Propagate network delegate events to ChromeDataUseAscriber.

ChromeDataUseAscriber ascribes all data use to logical entities
such as page loads. Propagating pertinent network events to
the ascriber provides it with the hooks to track data usage.
The actual tracking will be done in a future cl.

BUG=645188
==========

to

==========
Propagate network delegate events to ChromeDataUseAscriber.

ChromeDataUseAscriber ascribes all data use to logical entities
such as page loads. Propagating pertinent network events to
the ascriber provides it with the hooks to track data usage.
The actual tracking will be done in a future cl.

BUG=645188
==========

[commit-bot: I haz the power, 2016-09-26 17:54:51]

Committed patchset #9 (id:160001)

[commit-bot: I haz the power, 2016-09-26 17:57:25]

Description was changed from

==========
Propagate network delegate events to ChromeDataUseAscriber.

ChromeDataUseAscriber ascribes all data use to logical entities
such as page loads. Propagating pertinent network events to
the ascriber provides it with the hooks to track data usage.
The actual tracking will be done in a future cl.

BUG=645188
==========

to

==========
Propagate network delegate events to ChromeDataUseAscriber.

ChromeDataUseAscriber ascribes all data use to logical entities
such as page loads. Propagating pertinent network events to
the ascriber provides it with the hooks to track data usage.
The actual tracking will be done in a future cl.

BUG=645188

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

[commit-bot: I haz the power, 2016-09-26 17:57:26]

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