Include tab IDs when reporting data use accounting.

Include tab IDs when reporting data use accounting.

This adds an asynchronous hop to the UI thread and a hop back to the IO
thread to the data use accounting reporting path in order to find and
include the associated tab ID in the report.

BUG=547228
Committed: https://crrev.com/a133de07109e54152949b811da287f6c0c0f34a5
Cr-Commit-Position: refs/heads/master@{#358963}

[sclittle, 2015-10-24 06:59:30]

sclittle@chromium.org changed reviewers:
+ bengr@chromium.org, tbansal@chromium.org

[sclittle, 2015-10-24 06:59:31]

PTAL for design sanity check.

I still need to polish it and add tests, but this seems to work right now, at
least on desktop. I'll test this more extensively on Android later.

[tbansal1, 2015-10-26 22:24:30]

looks good.

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

https://codereview.chromium.org/1421983002/diff/1/chrome/browser/net/chrome_n...
chrome/browser/net/chrome_network_delegate_unittest.cc:108: void
ReportDataUse(int64_t tx_bytes,
Why not keep the request object? Makes it easier if we need to pull something
else out of request.

https://codereview.chromium.org/1421983002/diff/1/components/data_usage/core/...
File components/data_usage/core/data_use_aggregator.cc (right):

https://codereview.chromium.org/1421983002/diff/1/components/data_usage/core/...
components/data_usage/core/data_use_aggregator.cc:7: #include <sstream>
why is sstream required?

[sclittle, 2015-10-26 23:56:02]

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

https://codereview.chromium.org/1421983002/diff/1/chrome/browser/net/chrome_n...
chrome/browser/net/chrome_network_delegate_unittest.cc:108: void
ReportDataUse(int64_t tx_bytes,
On 2015/10/26 22:24:29, tbansal1 wrote:
> Why not keep the request object? Makes it easier if we need to pull something
> else out of request.

You're right, but the request might no longer exist after the hop to the UI
thread, so we have to pull everything we need out of it before that hop.

https://codereview.chromium.org/1421983002/diff/1/components/data_usage/core/...
File components/data_usage/core/data_use_aggregator.cc (right):

https://codereview.chromium.org/1421983002/diff/1/components/data_usage/core/...
components/data_usage/core/data_use_aggregator.cc:7: #include <sstream>
On 2015/10/26 22:24:30, tbansal1 wrote:
> why is sstream required?

<sstream> and <string> are just included right now for debug logging, they'll be
removed in a later patch on this CL.

[bengr, 2015-10-27 00:01:47]

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

https://codereview.chromium.org/1421983002/diff/1/chrome/browser/net/chrome_n...
chrome/browser/net/chrome_network_delegate.cc:291: // the original tab that
initiated the prerender.
Great! Please do.

https://codereview.chromium.org/1421983002/diff/1/chrome/browser/net/chrome_n...
chrome/browser/net/chrome_network_delegate.cc:785: // Hop to the UI thread and
back to get the tab ID for the render frame.
Can you use PostTaskAndReply?

https://codereview.chromium.org/1421983002/diff/1/components/data_usage/core/...
File components/data_usage/core/data_use_aggregator.cc (right):

https://codereview.chromium.org/1421983002/diff/1/components/data_usage/core/...
components/data_usage/core/data_use_aggregator.cc:23: // TODO in this CL: Remove
this before landing.
Please do remove it.

https://codereview.chromium.org/1421983002/diff/1/components/data_usage/core/...
components/data_usage/core/data_use_aggregator.cc:119: // TODO in this CL:
Remove this debug logging observer.
And this too.

[sclittle, 2015-10-28 22:30:45]

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

https://codereview.chromium.org/1421983002/diff/1/chrome/browser/net/chrome_n...
chrome/browser/net/chrome_network_delegate.cc:291: // the original tab that
initiated the prerender.
On 2015/10/27 00:01:47, bengr wrote:
> Great! Please do.

Maybe later. I think this might be impossible without mucking about in the guts
of prerendering.

https://codereview.chromium.org/1421983002/diff/1/chrome/browser/net/chrome_n...
chrome/browser/net/chrome_network_delegate.cc:785: // Hop to the UI thread and
back to get the tab ID for the render frame.
On 2015/10/27 00:01:47, bengr wrote:
> Can you use PostTaskAndReply?

Not cleanly. We need to pass back the tab ID, and doing that with
PostTaskAndReply would require passing in a scoped_ptr<int32_t> for the tab ID
and modifying it in place, which would be much uglier.

https://codereview.chromium.org/1421983002/diff/1/components/data_usage/core/...
File components/data_usage/core/data_use_aggregator.cc (right):

https://codereview.chromium.org/1421983002/diff/1/components/data_usage/core/...
components/data_usage/core/data_use_aggregator.cc:23: // TODO in this CL: Remove
this before landing.
On 2015/10/27 00:01:47, bengr wrote:
> Please do remove it.

Done.

https://codereview.chromium.org/1421983002/diff/1/components/data_usage/core/...
components/data_usage/core/data_use_aggregator.cc:119: // TODO in this CL:
Remove this debug logging observer.
On 2015/10/27 00:01:47, bengr wrote:
> And this too.

Done.

[sclittle, 2015-10-28 22:31:27]

sclittle@chromium.org changed reviewers:
+ mmenke@chromium.org

[sclittle, 2015-10-28 22:31:27]

mmenke: chrome_network_delegate*
bengr, tbansal: *

Thanks in advance!

[mmenke, 2015-10-28 22:43:13]

https://codereview.chromium.org/1421983002/diff/20001/chrome/browser/net/chro...
File chrome/browser/net/chrome_network_delegate.cc (right):

https://codereview.chromium.org/1421983002/diff/20001/chrome/browser/net/chro...
chrome/browser/net/chrome_network_delegate.cc:790:
report_data_use_with_tab_id_callback));
I don't think we want post two tasks every time a request receives data.  This
seems way too intrusive.  I'd be less concerned if you just did this when a
request completes or is redirected.

We also want to avoid more chrome/ dependencies on these IDs - long term, we
want to completely hide them from chrome/, and have content run the callbacks
that care on the IO thread.

[sclittle, 2015-10-28 23:04:56]

https://codereview.chromium.org/1421983002/diff/20001/chrome/browser/net/chro...
File chrome/browser/net/chrome_network_delegate.cc (right):

https://codereview.chromium.org/1421983002/diff/20001/chrome/browser/net/chro...
chrome/browser/net/chrome_network_delegate.cc:790:
report_data_use_with_tab_id_callback));
On 2015/10/28 22:43:12, mmenke wrote:
> I don't think we want post two tasks every time a request receives data.  This
> seems way too intrusive.  I'd be less concerned if you just did this when a
> request completes or is redirected.
> 
> We also want to avoid more chrome/ dependencies on these IDs - long term, we
> want to completely hide them from chrome/, and have content run the callbacks
> that care on the IO thread.

Which IDs are to be avoided, e.g. Tab IDs or render_process/frame_id? Do you
know of any other way of getting the tab ID for a URLRequest?

For the performance hit, would it be acceptable if the IO->UI->IO hop was only
done for the first chunk of data received for a URLRequest, then re-used for
later chunks for the same URLRequest?

[mmenke, 2015-10-28 23:18:42]

https://codereview.chromium.org/1421983002/diff/20001/chrome/browser/net/chro...
File chrome/browser/net/chrome_network_delegate.cc (right):

https://codereview.chromium.org/1421983002/diff/20001/chrome/browser/net/chro...
chrome/browser/net/chrome_network_delegate.cc:790:
report_data_use_with_tab_id_callback));
On 2015/10/28 23:04:56, sclittle wrote:
> On 2015/10/28 22:43:12, mmenke wrote:
> > I don't think we want post two tasks every time a request receives data. 
This
> > seems way too intrusive.  I'd be less concerned if you just did this when a
> > request completes or is redirected.
> > 
> > We also want to avoid more chrome/ dependencies on these IDs - long term, we
> > want to completely hide them from chrome/, and have content run the
callbacks
> > that care on the IO thread.
> 
> Which IDs are to be avoided, e.g. Tab IDs or render_process/frame_id? Do you
> know of any other way of getting the tab ID for a URLRequest?

Apologies, I meant render process / frame IDs.  The plan is to pass the tab ID
to ResourceThrottle stack, which will ideally be moved to the UI thread (So
always incurring two extra thread hops, but only on start, redirect, headers
received, and auth challenges).  I'd be less concerned if we plugged this in as
a ResourceThrottle, but throttles don't currently get notified of completion,
which is a problem.

> For the performance hit, would it be acceptable if the IO->UI->IO hop was only
> done for the first chunk of data received for a URLRequest, then re-used for
> later chunks for the same URLRequest?

Yes, I'd be happy with that - unfortunately, unless we delay the request on the
first hop, not sure how we'd get it.  Let me think about this a bit tomorrow.

[bengr, 2015-10-30 15:56:07]

https://codereview.chromium.org/1421983002/diff/20001/chrome/browser/net/chro...
File chrome/browser/net/chrome_network_delegate.cc (right):

https://codereview.chromium.org/1421983002/diff/20001/chrome/browser/net/chro...
chrome/browser/net/chrome_network_delegate.cc:790:
report_data_use_with_tab_id_callback));
On 2015/10/28 23:18:42, mmenke wrote:
> On 2015/10/28 23:04:56, sclittle wrote:
> > On 2015/10/28 22:43:12, mmenke wrote:
> > > I don't think we want post two tasks every time a request receives data. 
> This
> > > seems way too intrusive.  I'd be less concerned if you just did this when
a
> > > request completes or is redirected.
> > > 
> > > We also want to avoid more chrome/ dependencies on these IDs - long term,
we
> > > want to completely hide them from chrome/, and have content run the
> callbacks
> > > that care on the IO thread.
> > 
> > Which IDs are to be avoided, e.g. Tab IDs or render_process/frame_id? Do you
> > know of any other way of getting the tab ID for a URLRequest?
> 
> Apologies, I meant render process / frame IDs.  The plan is to pass the tab ID
> to ResourceThrottle stack, which will ideally be moved to the UI thread (So
> always incurring two extra thread hops, but only on start, redirect, headers
> received, and auth challenges).  I'd be less concerned if we plugged this in
as
> a ResourceThrottle, but throttles don't currently get notified of completion,
> which is a problem.
> 
> > For the performance hit, would it be acceptable if the IO->UI->IO hop was
only
> > done for the first chunk of data received for a URLRequest, then re-used for
> > later chunks for the same URLRequest?
> 
> Yes, I'd be happy with that - unfortunately, unless we delay the request on
the
> first hop, not sure how we'd get it.  Let me think about this a bit tomorrow.

Is your concern that the request might not outlive the thread hops?

[mmenke, 2015-10-30 16:49:23]

On 2015/10/30 15:56:07, bengr wrote:
>
https://codereview.chromium.org/1421983002/diff/20001/chrome/browser/net/chro...
> File chrome/browser/net/chrome_network_delegate.cc (right):
> 
>
https://codereview.chromium.org/1421983002/diff/20001/chrome/browser/net/chro...
> chrome/browser/net/chrome_network_delegate.cc:790:
> report_data_use_with_tab_id_callback));
> On 2015/10/28 23:18:42, mmenke wrote:
> > On 2015/10/28 23:04:56, sclittle wrote:
> > > On 2015/10/28 22:43:12, mmenke wrote:
> > > > I don't think we want post two tasks every time a request receives data.

> > This
> > > > seems way too intrusive.  I'd be less concerned if you just did this
when
> a
> > > > request completes or is redirected.
> > > > 
> > > > We also want to avoid more chrome/ dependencies on these IDs - long
term,
> we
> > > > want to completely hide them from chrome/, and have content run the
> > callbacks
> > > > that care on the IO thread.
> > > 
> > > Which IDs are to be avoided, e.g. Tab IDs or render_process/frame_id? Do
you
> > > know of any other way of getting the tab ID for a URLRequest?
> > 
> > Apologies, I meant render process / frame IDs.  The plan is to pass the tab
ID
> > to ResourceThrottle stack, which will ideally be moved to the UI thread (So
> > always incurring two extra thread hops, but only on start, redirect, headers
> > received, and auth challenges).  I'd be less concerned if we plugged this in
> as
> > a ResourceThrottle, but throttles don't currently get notified of
completion,
> > which is a problem.
> > 
> > > For the performance hit, would it be acceptable if the IO->UI->IO hop was
> only
> > > done for the first chunk of data received for a URLRequest, then re-used
for
> > > later chunks for the same URLRequest?
> > 
> > Yes, I'd be happy with that - unfortunately, unless we delay the request on
> the
> > first hop, not sure how we'd get it.  Let me think about this a bit
tomorrow.
> 
> Is your concern that the request might not outlive the thread hops?

More along the lines that we'd receive more data while the hop is occurring.

We'd have 3 options:
1)  Pause the request during the hop (I'm not a huge fan of this, though longer
term, if David's always hope to the IO thread on redirects, start, and headers
received idea pans out, we'll basically be doing this anyways...And we can
probably grab a tab ID from that, too...Which makes this entire discussion a
non-issue, but that may be quite a ways out).
2)  If we don't know the tab ID, also hop (Allows a lot of unnecessary hops)
3)  Store unrecorded data with the request, and then just dump it all when the
first hop completes.

You're right that there's also the issue of what happens when the request is
cancelled / completed too quickly.  I suppose we'd need to attach an object to
the request and grab a weak_ptr to it.  As more data is received, we can just
tell it about any recent data received (Or perhaps an object with a reference to
it, that's actually owned by the base::Callback busy getting the tab ID)

I think if we go with approach 3, this is actually all fairly tractable.  This
works fine, just wondering about putting it in ChromeNetworkDelegate, given how
much a catch-all that class already is.

There's also the possibility the tab or RenderFrameHost doesn't exist by the
time we get to the UI thread, in which case, I think we have to just give up, or
claim the data is not associated with any tab.  And there's the case the
ProfileIOData has been destroyed by the time you get back to the IO thread, in
which case we have to just give up (Rare case, obviously).

Also, a question...What is the DataUseAggregator going to do with the tab ID? 
May be another way to get at what you want.

Sorry for not getting back to this yesterday - a lot of stuff on my plate this
week.

[mmenke, 2015-10-30 16:55:19]

On 2015/10/30 16:49:23, mmenke wrote:
> On 2015/10/30 15:56:07, bengr wrote:
> >
>
https://codereview.chromium.org/1421983002/diff/20001/chrome/browser/net/chro...
> > File chrome/browser/net/chrome_network_delegate.cc (right):
> > 
> >
>
https://codereview.chromium.org/1421983002/diff/20001/chrome/browser/net/chro...
> > chrome/browser/net/chrome_network_delegate.cc:790:
> > report_data_use_with_tab_id_callback));
> > On 2015/10/28 23:18:42, mmenke wrote:
> > > On 2015/10/28 23:04:56, sclittle wrote:
> > > > On 2015/10/28 22:43:12, mmenke wrote:
> > > > > I don't think we want post two tasks every time a request receives
data.
> 
> > > This
> > > > > seems way too intrusive.  I'd be less concerned if you just did this
> when
> > a
> > > > > request completes or is redirected.
> > > > > 
> > > > > We also want to avoid more chrome/ dependencies on these IDs - long
> term,
> > we
> > > > > want to completely hide them from chrome/, and have content run the
> > > callbacks
> > > > > that care on the IO thread.
> > > > 
> > > > Which IDs are to be avoided, e.g. Tab IDs or render_process/frame_id? Do
> you
> > > > know of any other way of getting the tab ID for a URLRequest?
> > > 
> > > Apologies, I meant render process / frame IDs.  The plan is to pass the
tab
> ID
> > > to ResourceThrottle stack, which will ideally be moved to the UI thread
(So
> > > always incurring two extra thread hops, but only on start, redirect,
headers
> > > received, and auth challenges).  I'd be less concerned if we plugged this
in
> > as
> > > a ResourceThrottle, but throttles don't currently get notified of
> completion,
> > > which is a problem.
> > > 
> > > > For the performance hit, would it be acceptable if the IO->UI->IO hop
was
> > only
> > > > done for the first chunk of data received for a URLRequest, then re-used
> for
> > > > later chunks for the same URLRequest?
> > > 
> > > Yes, I'd be happy with that - unfortunately, unless we delay the request
on
> > the
> > > first hop, not sure how we'd get it.  Let me think about this a bit
> tomorrow.
> > 
> > Is your concern that the request might not outlive the thread hops?
> 
> More along the lines that we'd receive more data while the hop is occurring.
> 
> We'd have 3 options:
> 1)  Pause the request during the hop (I'm not a huge fan of this, though
longer
> term, if David's always hope to the IO thread on redirects, start, and headers
> received idea pans out, we'll basically be doing this anyways...And we can
> probably grab a tab ID from that, too...Which makes this entire discussion a
> non-issue, but that may be quite a ways out).
> 2)  If we don't know the tab ID, also hop (Allows a lot of unnecessary hops)
> 3)  Store unrecorded data with the request, and then just dump it all when the
> first hop completes.
> 
> You're right that there's also the issue of what happens when the request is
> cancelled / completed too quickly.  I suppose we'd need to attach an object to
> the request and grab a weak_ptr to it.  As more data is received, we can just
> tell it about any recent data received (Or perhaps an object with a reference
to
> it, that's actually owned by the base::Callback busy getting the tab ID)
> 
> I think if we go with approach 3, this is actually all fairly tractable.  This
> works fine, just wondering about putting it in ChromeNetworkDelegate, given
how
> much a catch-all that class already is.
> 
> There's also the possibility the tab or RenderFrameHost doesn't exist by the
> time we get to the UI thread, in which case, I think we have to just give up,
or
> claim the data is not associated with any tab.  And there's the case the
> ProfileIOData has been destroyed by the time you get back to the IO thread, in
> which case we have to just give up (Rare case, obviously).
> 
> Also, a question...What is the DataUseAggregator going to do with the tab ID? 
> May be another way to get at what you want.
> 
> Sorry for not getting back to this yesterday - a lot of stuff on my plate this
> week.

One other option:  Wait until the request is complete, and we have accumulated
all data, and only then do the hop.  We won't need the URLRequest any more at
that point.

Potential problems there:
* That doesn't give you real time information.  I'm not sure if you need this or
not.  If this is just for histograms, maybe not?
* That's more likely to lose tab information when requests are cancelled on tab
close, RenderFrame close, or cross-process navigation.

[mmenke, 2015-11-02 18:41:56]

On 2015/10/30 16:55:19, mmenke wrote:
> On 2015/10/30 16:49:23, mmenke wrote:
> > On 2015/10/30 15:56:07, bengr wrote:
> > >
> >
>
https://codereview.chromium.org/1421983002/diff/20001/chrome/browser/net/chro...
> > > File chrome/browser/net/chrome_network_delegate.cc (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/1421983002/diff/20001/chrome/browser/net/chro...
> > > chrome/browser/net/chrome_network_delegate.cc:790:
> > > report_data_use_with_tab_id_callback));
> > > On 2015/10/28 23:18:42, mmenke wrote:
> > > > On 2015/10/28 23:04:56, sclittle wrote:
> > > > > On 2015/10/28 22:43:12, mmenke wrote:
> > > > > > I don't think we want post two tasks every time a request receives
> data.
> > 
> > > > This
> > > > > > seems way too intrusive.  I'd be less concerned if you just did this
> > when
> > > a
> > > > > > request completes or is redirected.
> > > > > > 
> > > > > > We also want to avoid more chrome/ dependencies on these IDs - long
> > term,
> > > we
> > > > > > want to completely hide them from chrome/, and have content run the
> > > > callbacks
> > > > > > that care on the IO thread.
> > > > > 
> > > > > Which IDs are to be avoided, e.g. Tab IDs or render_process/frame_id?
Do
> > you
> > > > > know of any other way of getting the tab ID for a URLRequest?
> > > > 
> > > > Apologies, I meant render process / frame IDs.  The plan is to pass the
> tab
> > ID
> > > > to ResourceThrottle stack, which will ideally be moved to the UI thread
> (So
> > > > always incurring two extra thread hops, but only on start, redirect,
> headers
> > > > received, and auth challenges).  I'd be less concerned if we plugged
this
> in
> > > as
> > > > a ResourceThrottle, but throttles don't currently get notified of
> > completion,
> > > > which is a problem.
> > > > 
> > > > > For the performance hit, would it be acceptable if the IO->UI->IO hop
> was
> > > only
> > > > > done for the first chunk of data received for a URLRequest, then
re-used
> > for
> > > > > later chunks for the same URLRequest?
> > > > 
> > > > Yes, I'd be happy with that - unfortunately, unless we delay the request
> on
> > > the
> > > > first hop, not sure how we'd get it.  Let me think about this a bit
> > tomorrow.
> > > 
> > > Is your concern that the request might not outlive the thread hops?
> > 
> > More along the lines that we'd receive more data while the hop is occurring.
> > 
> > We'd have 3 options:
> > 1)  Pause the request during the hop (I'm not a huge fan of this, though
> longer
> > term, if David's always hope to the IO thread on redirects, start, and
headers
> > received idea pans out, we'll basically be doing this anyways...And we can
> > probably grab a tab ID from that, too...Which makes this entire discussion a
> > non-issue, but that may be quite a ways out).
> > 2)  If we don't know the tab ID, also hop (Allows a lot of unnecessary hops)
> > 3)  Store unrecorded data with the request, and then just dump it all when
the
> > first hop completes.
> > 
> > You're right that there's also the issue of what happens when the request is
> > cancelled / completed too quickly.  I suppose we'd need to attach an object
to
> > the request and grab a weak_ptr to it.  As more data is received, we can
just
> > tell it about any recent data received (Or perhaps an object with a
reference
> to
> > it, that's actually owned by the base::Callback busy getting the tab ID)
> > 
> > I think if we go with approach 3, this is actually all fairly tractable. 
This
> > works fine, just wondering about putting it in ChromeNetworkDelegate, given
> how
> > much a catch-all that class already is.
> > 
> > There's also the possibility the tab or RenderFrameHost doesn't exist by the
> > time we get to the UI thread, in which case, I think we have to just give
up,
> or
> > claim the data is not associated with any tab.  And there's the case the
> > ProfileIOData has been destroyed by the time you get back to the IO thread,
in
> > which case we have to just give up (Rare case, obviously).
> > 
> > Also, a question...What is the DataUseAggregator going to do with the tab
ID? 
> > May be another way to get at what you want.
> > 
> > Sorry for not getting back to this yesterday - a lot of stuff on my plate
this
> > week.
> 
> One other option:  Wait until the request is complete, and we have accumulated
> all data, and only then do the hop.  We won't need the URLRequest any more at
> that point.
> 
> Potential problems there:
> * That doesn't give you real time information.  I'm not sure if you need this
or
> not.  If this is just for histograms, maybe not?
> * That's more likely to lose tab information when requests are cancelled on
tab
> close, RenderFrame close, or cross-process navigation.

Another option:  Accumulate byte transfer information on a per RenderView basis,
and periodically send it to the UI thread to get tab info.  This is much like
what the task manager does (Except it consumers the data on the UI thread, too).

https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/tas...

[sclittle, 2015-11-04 19:55:21]

PTAL, I've batched tab ID hops for the same URLRequest together, and moved the
logic for this out of ChromeNetworkDelegate and into a separate data_usage
directory.

[sclittle, 2015-11-04 19:56:36]

sclittle@chromium.org changed reviewers:
+ jochen@chromium.org

[sclittle, 2015-11-04 19:56:37]

+jochen for newly added chrome/browser/data_usage directory.

[bengr, 2015-11-05 00:00:38]

This CL is pretty large, but quite clear. Thanks for the clean code. Here's a
first round of comments.

https://codereview.chromium.org/1421983002/diff/40001/chrome/browser/data_usa...
File chrome/browser/data_usage/tab_id_annotator.cc (right):

https://codereview.chromium.org/1421983002/diff/40001/chrome/browser/data_usa...
chrome/browser/data_usage/tab_id_annotator.cc:30: // Attempt to get the
associated tab ID for a given render frame. Returns -1 if
"Attempt" -> "Attempts"

https://codereview.chromium.org/1421983002/diff/40001/chrome/browser/data_usa...
chrome/browser/data_usage/tab_id_annotator.cc:45: void
AnnotateDataUse(scoped_ptr<DataUse> data_use,
Why not just have the caller call the callback? Why does this method need to
take the scoped_ptr?

https://codereview.chromium.org/1421983002/diff/40001/chrome/browser/data_usa...
chrome/browser/data_usage/tab_id_annotator.cc:46: const
base::Callback<void(scoped_ptr<DataUse>)>& callback,
Consider using a typedef.

https://codereview.chromium.org/1421983002/diff/40001/chrome/browser/data_usa...
chrome/browser/data_usage/tab_id_annotator.cc:49: data_use->tab_id = tab_id;
I'd prefer having setters on DataUse.

https://codereview.chromium.org/1421983002/diff/40001/chrome/browser/data_usa...
chrome/browser/data_usage/tab_id_annotator.cc:63: DCHECK(request);
I've heard and agree with the argument that there's no point in adding a DCHECK
to check that a pointer is non-null, because the crash you'll get a few lines
down will betray the culprit in the stack trace. Please remove here and
elsewhere.

https://codereview.chromium.org/1421983002/diff/40001/chrome/browser/data_usa...
File chrome/browser/data_usage/tab_id_annotator.h (right):

https://codereview.chromium.org/1421983002/diff/40001/chrome/browser/data_usa...
chrome/browser/data_usage/tab_id_annotator.h:1: // Copyright (c) 2015 The
Chromium Authors. All rights reserved.
Remove the "(c)" as per the latest hotness.

https://codereview.chromium.org/1421983002/diff/40001/chrome/browser/data_usa...
File chrome/browser/data_usage/tab_id_annotator_unittest.cc (right):

https://codereview.chromium.org/1421983002/diff/40001/chrome/browser/data_usa...
chrome/browser/data_usage/tab_id_annotator_unittest.cc:1: // Copyright (c) 2015
The Chromium Authors. All rights reserved.
Remove "(c)"

https://codereview.chromium.org/1421983002/diff/40001/chrome/browser/data_usa...
chrome/browser/data_usage/tab_id_annotator_unittest.cc:53: // Creates a fake
DataUse object with the given |tab_id|.
Creates a fake -> Synthesizes

https://codereview.chromium.org/1421983002/diff/40001/chrome/browser/data_usa...
chrome/browser/data_usage/tab_id_annotator_unittest.cc:82: // Test that for a
sample URLRequest, associated with the given
Test -> Tests

https://codereview.chromium.org/1421983002/diff/40001/chrome/browser/data_usa...
chrome/browser/data_usage/tab_id_annotator_unittest.cc:104: render_process_id,
-1, render_frame_id, true, false, true, true, false);
Mind adding some comments about what all those booleans are?

https://codereview.chromium.org/1421983002/diff/40001/chrome/browser/data_usa...
chrome/browser/data_usage/tab_id_annotator_unittest.cc:111: request.get(),
CreateDataUse(-2),
What's the meaning of -2?

https://codereview.chromium.org/1421983002/diff/40001/chrome/browser/data_usa...
chrome/browser/data_usage/tab_id_annotator_unittest.cc:121:
TEST_F(TabIdAnnotatorTest, AnnotateWithNoRenderFrame) {
Does this also test for a render_process_id/render_frame_id pair that has no
associated tab? Can you add tests for having one but not the other?

https://codereview.chromium.org/1421983002/diff/40001/chrome/browser/data_usa...
File chrome/browser/data_usage/tab_id_provider.cc (right):

https://codereview.chromium.org/1421983002/diff/40001/chrome/browser/data_usa...
chrome/browser/data_usage/tab_id_provider.cc:1: // Copyright (c) 2015 The
Chromium Authors. All rights reserved.
"(c)" -> ""

https://codereview.chromium.org/1421983002/diff/40001/chrome/browser/data_usa...
chrome/browser/data_usage/tab_id_provider.cc:29: // can be added before it
starts running the callbacks. Callbacks added will
can -> can only?
will -> will each

https://codereview.chromium.org/1421983002/diff/40001/chrome/browser/data_usa...
File chrome/browser/data_usage/tab_id_provider.h (right):

https://codereview.chromium.org/1421983002/diff/40001/chrome/browser/data_usa...
chrome/browser/data_usage/tab_id_provider.h:1: // Copyright (c) 2015 The
Chromium Authors. All rights reserved.
"(c)" -> ""

https://codereview.chromium.org/1421983002/diff/40001/chrome/browser/data_usa...
chrome/browser/data_usage/tab_id_provider.h:55: bool is_tab_id_ready_;
Why do you need both a tab_id and this? Couldn't initializing tab_id to -1
suffice? Please add a comment to explain.

https://codereview.chromium.org/1421983002/diff/40001/chrome/browser/data_usa...
File chrome/browser/data_usage/tab_id_provider_unittest.cc (right):

https://codereview.chromium.org/1421983002/diff/40001/chrome/browser/data_usa...
chrome/browser/data_usage/tab_id_provider_unittest.cc:1: // Copyright (c) 2015
The Chromium Authors. All rights reserved.
"(c)" -> ""

https://codereview.chromium.org/1421983002/diff/40001/components/data_usage/c...
File components/data_usage/core/data_use.h (right):

https://codereview.chromium.org/1421983002/diff/40001/components/data_usage/c...
components/data_usage/core/data_use.h:30: bool operator==(const DataUse& other)
const;
One of your tests had a method to check if two DataUse objects were equal, but
didn't use this operator.

https://codereview.chromium.org/1421983002/diff/40001/components/data_usage/c...
File components/data_usage/core/data_use_aggregator.h (right):

https://codereview.chromium.org/1421983002/diff/40001/components/data_usage/c...
components/data_usage/core/data_use_aggregator.h:44: // Construct a new
DataUseAggregator with the given |annotator|. A NULL
"Constructs"

https://codereview.chromium.org/1421983002/diff/40001/components/data_usage/c...
components/data_usage/core/data_use_aggregator.h:76: // Append |data_use| to the
buffer of unreported data use and prepare to
"Appends", "prepares"

https://codereview.chromium.org/1421983002/diff/40001/components/data_usage/c...
File components/data_usage/core/data_use_annotator.h (right):

https://codereview.chromium.org/1421983002/diff/40001/components/data_usage/c...
components/data_usage/core/data_use_annotator.h:8: #include
"base/callback_forward.h"
Why do you need callback_forward.h and not callback.h?

[sclittle, 2015-11-05 01:22:49]

https://codereview.chromium.org/1421983002/diff/40001/chrome/browser/data_usa...
File chrome/browser/data_usage/tab_id_annotator.cc (right):

https://codereview.chromium.org/1421983002/diff/40001/chrome/browser/data_usa...
chrome/browser/data_usage/tab_id_annotator.cc:30: // Attempt to get the
associated tab ID for a given render frame. Returns -1 if
On 2015/11/05 00:00:37, bengr wrote:
> "Attempt" -> "Attempts"

Done.

https://codereview.chromium.org/1421983002/diff/40001/chrome/browser/data_usa...
chrome/browser/data_usage/tab_id_annotator.cc:45: void
AnnotateDataUse(scoped_ptr<DataUse> data_use,
On 2015/11/05 00:00:37, bengr wrote:
> Why not just have the caller call the callback? Why does this method need to
> take the scoped_ptr?

For simplicity and ease of testing, the caller (which is TabIdProvider) doesn't
know about DataUse objects, it only knows about tab IDs.

The AnnotateDataUse function here is the bridge between receiving the tab ID
from the TabIdProvider, and passing the DataUse with that tab ID set to the
DataUseAggregator.

https://codereview.chromium.org/1421983002/diff/40001/chrome/browser/data_usa...
chrome/browser/data_usage/tab_id_annotator.cc:46: const
base::Callback<void(scoped_ptr<DataUse>)>& callback,
On 2015/11/05 00:00:37, bengr wrote:
> Consider using a typedef.

Done.

https://codereview.chromium.org/1421983002/diff/40001/chrome/browser/data_usa...
chrome/browser/data_usage/tab_id_annotator.cc:49: data_use->tab_id = tab_id;
On 2015/11/05 00:00:38, bengr wrote:
> I'd prefer having setters on DataUse.

That would be for a separate CL, although I disagree - if DataUse were a class
instead of a struct, we'd just have a getter and setter for every single data
object, which seems a little ridiculous. Also, AIUI conceptually DataUse objects
shouldn't do stuff themselves, other things should do stuff to DataUse objects.

https://codereview.chromium.org/1421983002/diff/40001/chrome/browser/data_usa...
chrome/browser/data_usage/tab_id_annotator.cc:63: DCHECK(request);
On 2015/11/05 00:00:38, bengr wrote:
> I've heard and agree with the argument that there's no point in adding a
DCHECK
> to check that a pointer is non-null, because the crash you'll get a few lines
> down will betray the culprit in the stack trace. Please remove here and
> elsewhere. 

Sure. The argument for having these pointer DCHECKs is that you get better crash
messages and easier to read code (e.g. here, it's clear at a glance that
|request| can't be NULL), but I don't feel strongly about it.

I'm keeping the |data_use| DCHECKs though, since data_use.Pass() would still
work for a null scoped_ptr, and debugging segfaults across posted tasks to other
threads is not fun.

https://codereview.chromium.org/1421983002/diff/40001/chrome/browser/data_usa...
File chrome/browser/data_usage/tab_id_annotator.h (right):

https://codereview.chromium.org/1421983002/diff/40001/chrome/browser/data_usa...
chrome/browser/data_usage/tab_id_annotator.h:1: // Copyright (c) 2015 The
Chromium Authors. All rights reserved.
On 2015/11/05 00:00:38, bengr wrote:
> Remove the "(c)" as per the latest hotness.

Done.

https://codereview.chromium.org/1421983002/diff/40001/chrome/browser/data_usa...
File chrome/browser/data_usage/tab_id_annotator_unittest.cc (right):

https://codereview.chromium.org/1421983002/diff/40001/chrome/browser/data_usa...
chrome/browser/data_usage/tab_id_annotator_unittest.cc:1: // Copyright (c) 2015
The Chromium Authors. All rights reserved.
On 2015/11/05 00:00:38, bengr wrote:
> Remove "(c)"

Done.

https://codereview.chromium.org/1421983002/diff/40001/chrome/browser/data_usa...
chrome/browser/data_usage/tab_id_annotator_unittest.cc:53: // Creates a fake
DataUse object with the given |tab_id|.
On 2015/11/05 00:00:38, bengr wrote:
> Creates a fake -> Synthesizes

Done.

https://codereview.chromium.org/1421983002/diff/40001/chrome/browser/data_usa...
chrome/browser/data_usage/tab_id_annotator_unittest.cc:82: // Test that for a
sample URLRequest, associated with the given
On 2015/11/05 00:00:38, bengr wrote:
> Test -> Tests

Done.

https://codereview.chromium.org/1421983002/diff/40001/chrome/browser/data_usa...
chrome/browser/data_usage/tab_id_annotator_unittest.cc:104: render_process_id,
-1, render_frame_id, true, false, true, true, false);
On 2015/11/05 00:00:38, bengr wrote:
> Mind adding some comments about what all those booleans are?

Done.

https://codereview.chromium.org/1421983002/diff/40001/chrome/browser/data_usa...
chrome/browser/data_usage/tab_id_annotator_unittest.cc:111: request.get(),
CreateDataUse(-2),
On 2015/11/05 00:00:38, bengr wrote:
> What's the meaning of -2?

It's just an invalid tab ID that won't be confused with -1. Pulled it out into a
constant and added comment.

https://codereview.chromium.org/1421983002/diff/40001/chrome/browser/data_usa...
chrome/browser/data_usage/tab_id_annotator_unittest.cc:121:
TEST_F(TabIdAnnotatorTest, AnnotateWithNoRenderFrame) {
On 2015/11/05 00:00:38, bengr wrote:
> Does this also test for a render_process_id/render_frame_id pair that has no
> associated tab? Can you add tests for having one but not the other?

Done.

https://codereview.chromium.org/1421983002/diff/40001/chrome/browser/data_usa...
File chrome/browser/data_usage/tab_id_provider.cc (right):

https://codereview.chromium.org/1421983002/diff/40001/chrome/browser/data_usa...
chrome/browser/data_usage/tab_id_provider.cc:1: // Copyright (c) 2015 The
Chromium Authors. All rights reserved.
On 2015/11/05 00:00:38, bengr wrote:
> "(c)" -> ""

Done.

https://codereview.chromium.org/1421983002/diff/40001/chrome/browser/data_usa...
chrome/browser/data_usage/tab_id_provider.cc:29: // can be added before it
starts running the callbacks. Callbacks added will
On 2015/11/05 00:00:38, bengr wrote:
> can -> can only?
> will -> will each

Done.

https://codereview.chromium.org/1421983002/diff/40001/chrome/browser/data_usa...
File chrome/browser/data_usage/tab_id_provider.h (right):

https://codereview.chromium.org/1421983002/diff/40001/chrome/browser/data_usa...
chrome/browser/data_usage/tab_id_provider.h:1: // Copyright (c) 2015 The
Chromium Authors. All rights reserved.
On 2015/11/05 00:00:38, bengr wrote:
> "(c)" -> ""

Done.

https://codereview.chromium.org/1421983002/diff/40001/chrome/browser/data_usa...
chrome/browser/data_usage/tab_id_provider.h:55: bool is_tab_id_ready_;
On 2015/11/05 00:00:38, bengr wrote:
> Why do you need both a tab_id and this? Couldn't initializing tab_id to -1
> suffice? Please add a comment to explain.

-1 is special, it means that no tab ID was found, it doesn't mean that we're
still waiting for a tab ID. It's possible that the tab ID getter could return -1
("no tab ID was found"), and we should cache that -1 here and provide that to
whoever wants it.

https://codereview.chromium.org/1421983002/diff/40001/chrome/browser/data_usa...
File chrome/browser/data_usage/tab_id_provider_unittest.cc (right):

https://codereview.chromium.org/1421983002/diff/40001/chrome/browser/data_usa...
chrome/browser/data_usage/tab_id_provider_unittest.cc:1: // Copyright (c) 2015
The Chromium Authors. All rights reserved.
On 2015/11/05 00:00:38, bengr wrote:
> "(c)" -> ""

Done.

https://codereview.chromium.org/1421983002/diff/40001/components/data_usage/c...
File components/data_usage/core/data_use.h (right):

https://codereview.chromium.org/1421983002/diff/40001/components/data_usage/c...
components/data_usage/core/data_use.h:30: bool operator==(const DataUse& other)
const;
On 2015/11/05 00:00:38, bengr wrote:
> One of your tests had a method to check if two DataUse objects were equal, but
> didn't use this operator.

I think you mean the ExpectDataUse function in tab_id_annotator_unittest.cc,
which does actually use the "==" operator. It calls EXPECT_EQ(*expected,
*actual).

https://codereview.chromium.org/1421983002/diff/40001/components/data_usage/c...
File components/data_usage/core/data_use_aggregator.h (right):

https://codereview.chromium.org/1421983002/diff/40001/components/data_usage/c...
components/data_usage/core/data_use_aggregator.h:44: // Construct a new
DataUseAggregator with the given |annotator|. A NULL
On 2015/11/05 00:00:38, bengr wrote:
> "Constructs"

Done.

https://codereview.chromium.org/1421983002/diff/40001/components/data_usage/c...
components/data_usage/core/data_use_aggregator.h:76: // Append |data_use| to the
buffer of unreported data use and prepare to
On 2015/11/05 00:00:38, bengr wrote:
> "Appends", "prepares"

Done.

https://codereview.chromium.org/1421983002/diff/40001/components/data_usage/c...
File components/data_usage/core/data_use_annotator.h (right):

https://codereview.chromium.org/1421983002/diff/40001/components/data_usage/c...
components/data_usage/core/data_use_annotator.h:8: #include
"base/callback_forward.h"
On 2015/11/05 00:00:38, bengr wrote:
> Why do you need callback_forward.h and not callback.h?

We do need it now, because of the new typedef here. We didn't need it before
because we were just using a const ref of the callback type, and callback.h is
pretty big. From
https://code.google.com/p/chromium/codesearch#chromium/src/base/callback.h&l=12:

"NOTE: Header files that do not require the full definition of Callback or
Closure should #include "base/callback_forward.h" instead of this file."

[bengr, 2015-11-05 16:40:22]

components/data_usage/* LGTM

[mmenke, 2015-11-05 18:25:03]

On 2015/11/05 16:40:22, bengr wrote:
> components/data_usage/* LGTM

I'll try to get to this today.  If not, definitely tomorrow.

[sclittle, 2015-11-07 00:44:45]

Friendly ping.

[jochen (gone - plz use gerrit), 2015-11-07 06:27:39]

jochen@chromium.org changed reviewers:
+ sky@chromium.org

[jochen (gone - plz use gerrit), 2015-11-07 06:27:39]

that's a lot of code just to get the tab id. Is all this general tab id getter
callback machinery really needed here, or would a simple postTaskAndReply in the
network delegate be enough?

Also, I'm not sure whether the session tab id is what you need?

+sky who knows that code better

e.g. when a web contents is prerendered, it doesn't necessarily have a tab id
yet.

[sky, 2015-11-08 16:22:18]

On 2015/11/07 06:27:39, jochen (slow - traveling) wrote:
> that's a lot of code just to get the tab id. Is all this general tab id getter
> callback machinery really needed here, or would a simple postTaskAndReply in
the
> network delegate be enough?
> 
> Also, I'm not sure whether the session tab id is what you need?
> 
> +sky who knows that code better
> 
> e.g. when a web contents is prerendered, it doesn't necessarily have a tab id
> yet.

The session id is assigned by TabHelpers::AttachTabHelpers, which is also called
by the prerender code. Not necessarily all code calls into
TabHelpers::AttachTabHelpers though.

[Avi (use Gerrit), 2015-11-08 17:40:41]

On 2015/11/08 16:22:18, sky wrote:
> Not necessarily all code calls into TabHelpers::AttachTabHelpers though.

Every time someone talks about AttachTabHelpers, they always seem to make it
sound magical and arbitrary, so let me clarify:

If a WebContents is a tab in a browser window it *will* have
TabHelpers::AttachTabHelpers called on it.

If a WebContents is *not* a tab in a browser window, it will *not* have
TabHelpers::AttachTabHelpers called on it.

As for prerendering, prerendered pages are destined to be tabs in a browser
window, so it will have AttachTabHelpers called on it. That call happens in
PrerenderContents::StartPrerendering. So once prerendering starts, you will have
the tab helpers you seek.

[mmenke, 2015-11-09 19:34:50]

On 2015/11/08 17:40:41, Avi wrote:
> On 2015/11/08 16:22:18, sky wrote:
> > Not necessarily all code calls into TabHelpers::AttachTabHelpers though.
> 
> Every time someone talks about AttachTabHelpers, they always seem to make it
> sound magical and arbitrary, so let me clarify:
> 
> If a WebContents is a tab in a browser window it *will* have
> TabHelpers::AttachTabHelpers called on it.
> 
> If a WebContents is *not* a tab in a browser window, it will *not* have
> TabHelpers::AttachTabHelpers called on it.
> 
> As for prerendering, prerendered pages are destined to be tabs in a browser
> window, so it will have AttachTabHelpers called on it. That call happens in
> PrerenderContents::StartPrerendering. So once prerendering starts, you will
have
> the tab helpers you seek.

Sorry for extreme slowness on this one...  Looking at a dev blocker now,
probably taking tomorrow off.  Travelling Wed...  I'll plan to bring a copy of
this CL on my flight, if I don't get to it by then.  :(

[sclittle, 2015-11-09 19:45:58]

On 2015/11/07 06:27:39, jochen (slow - traveling) wrote:
> that's a lot of code just to get the tab id. Is all this general tab id getter
> callback machinery really needed here, or would a simple postTaskAndReply in
the
> network delegate be enough?
> 
> Also, I'm not sure whether the session tab id is what you need?
> 
> +sky who knows that code better
> 
> e.g. when a web contents is prerendered, it doesn't necessarily have a tab id
> yet.

In patch set #2, this was just a PostTaskAndReply in the network delegate, but
IIUC mmenke expressed concern about the performance hit of hopping to the UI
thread and back after every chunk of data read for a URLRequest, and the code
for grouping tab ID callbacks for the same URLRequest together was too big to
just add to network delegate itself, so I separated them into different files
and classes.

The actual tab ID getter callback machinery is only a couple hundred lines + C++
boilerplate + lots of unittests. It's only general like that to make it easier
to test.

[sclittle, 2015-11-09 19:47:27]

On 2015/11/09 19:34:50, mmenke wrote:
> Sorry for extreme slowness on this one...  Looking at a dev blocker now,
> probably taking tomorrow off.  Travelling Wed...  I'll plan to bring a copy of
> this CL on my flight, if I don't get to it by then.  :(

@mmenke: OK, I really want to get this into 48, is there someone else who could
review the net/ and chrome/browser/net changes that you'd suggest?

[mmenke, 2015-11-09 19:58:05]

On 2015/11/09 19:47:27, sclittle wrote:
> On 2015/11/09 19:34:50, mmenke wrote:
> > Sorry for extreme slowness on this one...  Looking at a dev blocker now,
> > probably taking tomorrow off.  Travelling Wed...  I'll plan to bring a copy
of
> > this CL on my flight, if I don't get to it by then.  :(
> 
> @mmenke: OK, I really want to get this into 48, is there someone else who
could
> review the net/ and chrome/browser/net changes that you'd suggest?

Davidben, perhaps.  He knows navigation and tabs better than I do, too.  But
he's also slow and heavily loaded with reviews.  And he's already out of town.

[mmenke, 2015-11-09 21:32:18]

On 2015/11/09 19:58:05, mmenke wrote:
> On 2015/11/09 19:47:27, sclittle wrote:
> > On 2015/11/09 19:34:50, mmenke wrote:
> > > Sorry for extreme slowness on this one...  Looking at a dev blocker now,
> > > probably taking tomorrow off.  Travelling Wed...  I'll plan to bring a
copy
> of
> > > this CL on my flight, if I don't get to it by then.  :(
> > 
> > @mmenke: OK, I really want to get this into 48, is there someone else who
> could
> > review the net/ and chrome/browser/net changes that you'd suggest?
> 
> Davidben, perhaps.  He knows navigation and tabs better than I do, too.  But
> he's also slow and heavily loaded with reviews.  And he's already out of town.

Oh, and I'm happy with the net/, browser/net, and io_thread changes, which all
LGTM.  Just the other changes I'm not sure I'll have time for.  Happy to defer
to others for chrome/browser/data_usage.

[jochen (gone - plz use gerrit), 2015-11-10 20:07:15]

lgtm

[sclittle, 2015-11-10 20:43:47]

The CQ bit was checked by sclittle@chromium.org

[commit-bot: I haz the power, 2015-11-10 20:44:45]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1421983002/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1421983002/60001

[commit-bot: I haz the power, 2015-11-10 20:57:51]

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

[commit-bot: I haz the power, 2015-11-10 20:57:52]

Try jobs failed on following builders:
  android_chromium_gn_compile_rel on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromiu...)
  linux_chromium_compile_dbg_32_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_gn_chromeos_rel on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[sclittle, 2015-11-10 21:48:01]

The CQ bit was checked by sclittle@chromium.org

[sclittle, 2015-11-10 21:48:02]

The patchset sent to the CQ was uploaded after l-g-t-m from bengr@chromium.org,
jochen@chromium.org, mmenke@chromium.org
Link to the patchset: https://codereview.chromium.org/1421983002/#ps80001
(title: "Fixed broken test")

[commit-bot: I haz the power, 2015-11-10 21:48:52]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1421983002/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1421983002/80001

[commit-bot: I haz the power, 2015-11-10 22:08:13]

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

[commit-bot: I haz the power, 2015-11-10 22:08:14]

Try jobs failed on following builders:
  android_chromium_gn_compile_rel on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromiu...)
  android_compile_dbg on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/android_compile...)

[sclittle, 2015-11-10 22:16:04]

The CQ bit was checked by sclittle@chromium.org

[sclittle, 2015-11-10 22:16:04]

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

[commit-bot: I haz the power, 2015-11-10 22:17:24]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1421983002/100001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1421983002/100001

[commit-bot: I haz the power, 2015-11-10 23:54:28]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2015-11-10 23:55:45]

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