Add UMA metric to track parallel download requests stats.

Add UMA metric to track parallel download requests stats.

Add requests count to track the actual requests sent and special
requests(offset smaller than initial request offset) sent in parallel
download.

Add a boolean metric to track if the file sink is gone when we try to
push a byte stream reader to the sink.


BUG=644352
Review-Url: https://codereview.chromium.org/2789623005
Cr-Commit-Position: refs/heads/master@{#461556}
Committed: https://chromium.googlesource.com/chromium/src/+/ec174ac638779aa4db3573285a40b513e98c5e93

[xingliu, 2017-03-30 22:34:22]

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

[commit-bot: I haz the power, 2017-03-30 22:35:25]

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

[xingliu, 2017-03-30 22:45:37]

xingliu@chromium.org changed reviewers:
+ dtrainor@chromium.org, isherman@chromium.org, qinmin@chromium.org

[xingliu, 2017-03-30 22:45:37]

Hi, PTAL, thanks.

[commit-bot: I haz the power, 2017-03-30 23:48:28]

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

[commit-bot: I haz the power, 2017-03-30 23:48:29]

Dry run: This issue passed the CQ dry run.

[Ilya Sherman, 2017-03-31 00:33:04]

Metrics LGTM % nits:

https://codereview.chromium.org/2789623005/diff/1/content/browser/download/do...
File content/browser/download/download_stats.cc (right):

https://codereview.chromium.org/2789623005/diff/1/content/browser/download/do...
content/browser/download/download_stats.cc:778: request_count, 1, 10, 10);
nit: Did you check that this gives you the desired bucket layout?  I suspect you
might want the last argument to be 11, to make space for the underflow bucket. 
In any case, I'd recommend double-checking locally via chrome://histograms that
this gives you the desired layout.

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

https://codereview.chromium.org/2789623005/diff/1/tools/metrics/histograms/hi...
tools/metrics/histograms/histograms.xml:13773: +<histogram
name="Download.ParallelDownloadEarlyRequestCount">
nit: Please add a units attribute.

https://codereview.chromium.org/2789623005/diff/1/tools/metrics/histograms/hi...
tools/metrics/histograms/histograms.xml:13782: +<histogram
name="Download.ParallelDownloadRequestCount">
nit: Please add a units attribute.

[xingliu, 2017-03-31 21:47:15]

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

[commit-bot: I haz the power, 2017-03-31 21:48:06]

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

[xingliu, 2017-03-31 21:48:27]

Thanks for the review, PTAL.

https://codereview.chromium.org/2789623005/diff/1/content/browser/download/do...
File content/browser/download/download_stats.cc (right):

https://codereview.chromium.org/2789623005/diff/1/content/browser/download/do...
content/browser/download/download_stats.cc:778: request_count, 1, 10, 10);
On 2017/03/31 00:33:04, Ilya Sherman wrote:
> nit: Did you check that this gives you the desired bucket layout?  I suspect
you
> might want the last argument to be 11, to make space for the underflow bucket.

> In any case, I'd recommend double-checking locally via chrome://histograms
that
> this gives you the desired layout.

Done, thanks.

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

https://codereview.chromium.org/2789623005/diff/1/tools/metrics/histograms/hi...
tools/metrics/histograms/histograms.xml:13773: +<histogram
name="Download.ParallelDownloadEarlyRequestCount">
On 2017/03/31 00:33:04, Ilya Sherman wrote:
> nit: Please add a units attribute.

Done.

https://codereview.chromium.org/2789623005/diff/1/tools/metrics/histograms/hi...
tools/metrics/histograms/histograms.xml:13782: +<histogram
name="Download.ParallelDownloadRequestCount">
On 2017/03/31 00:33:04, Ilya Sherman wrote:
> nit: Please add a units attribute.

Done.

[Ilya Sherman, 2017-03-31 22:03:10]

Metrics LGTM, thanks.  (It looks like you removed one of the metrics?  Was that
intentional?)

[xingliu, 2017-03-31 22:24:48]

On 2017/03/31 22:03:10, Ilya Sherman wrote:
> Metrics LGTM, thanks.  (It looks like you removed one of the metrics?  Was
that
> intentional?)

Yes.

[commit-bot: I haz the power, 2017-03-31 22:30:42]

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

[commit-bot: I haz the power, 2017-03-31 22:30:43]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[David Trainor- moved to gerrit, 2017-04-03 17:59:57]

lgtm

https://codereview.chromium.org/2789623005/diff/80001/content/browser/downloa...
File content/browser/download/parallel_download_job.cc (right):

https://codereview.chromium.org/2789623005/diff/80001/content/browser/downloa...
content/browser/download/parallel_download_job.cc:175: DCHECK(it->offset >=
initial_request_offset_);
DCHECK_GE?

[qinmin, 2017-04-03 18:36:20]

lgtm % nit

https://codereview.chromium.org/2789623005/diff/80001/content/browser/downloa...
File content/browser/download/parallel_download_job.cc (right):

https://codereview.chromium.org/2789623005/diff/80001/content/browser/downloa...
content/browser/download/parallel_download_job.cc:173: for (auto it =
slices_to_download.begin() + 1; it != slices_to_download.end();
nit: const auto& it

[xingliu, 2017-04-03 20:37:07]

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

[commit-bot: I haz the power, 2017-04-03 20:37:44]

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

[xingliu, 2017-04-03 20:38:05]

Thanks for the review.

https://codereview.chromium.org/2789623005/diff/80001/content/browser/downloa...
File content/browser/download/parallel_download_job.cc (right):

https://codereview.chromium.org/2789623005/diff/80001/content/browser/downloa...
content/browser/download/parallel_download_job.cc:173: for (auto it =
slices_to_download.begin() + 1; it != slices_to_download.end();
On 2017/04/03 18:36:19, qinmin wrote:
> nit: const auto& it

This will yield compiling error, so does auto& it.

https://codereview.chromium.org/2789623005/diff/80001/content/browser/downloa...
content/browser/download/parallel_download_job.cc:175: DCHECK(it->offset >=
initial_request_offset_);
On 2017/04/03 17:59:57, David Trainor-ping if over 24h wrote:
> DCHECK_GE?

Done.

Also replaced a DCHECK with DCHECK_LE in another line.

[qinmin, 2017-04-03 20:46:49]

https://codereview.chromium.org/2789623005/diff/80001/content/browser/downloa...
File content/browser/download/parallel_download_job.cc (right):

https://codereview.chromium.org/2789623005/diff/80001/content/browser/downloa...
content/browser/download/parallel_download_job.cc:173: for (auto it =
slices_to_download.begin() + 1; it != slices_to_download.end();
On 2017/04/03 20:38:05, xingliu wrote:
> On 2017/04/03 18:36:19, qinmin wrote:
> > nit: const auto& it
> 
> This will yield compiling error, so does auto& it.

Really? "for (const auto& slice : slices_to_download)" doesn't compile?

[xingliu, 2017-04-03 21:12:24]

On 2017/04/03 20:46:49, qinmin wrote:
>
https://codereview.chromium.org/2789623005/diff/80001/content/browser/downloa...
> File content/browser/download/parallel_download_job.cc (right):
> 
>
https://codereview.chromium.org/2789623005/diff/80001/content/browser/downloa...
> content/browser/download/parallel_download_job.cc:173: for (auto it =
> slices_to_download.begin() + 1; it != slices_to_download.end();
> On 2017/04/03 20:38:05, xingliu wrote:
> > On 2017/04/03 18:36:19, qinmin wrote:
> > > nit: const auto& it
> > 
> > This will yield compiling error, so does auto& it.
> 
> Really? "for (const auto& slice : slices_to_download)" doesn't compile?

Ah, I see.. Thanks. 

But here we iterate from the second element, const auto& will introduce another
bool or something
to skip the first element.

[commit-bot: I haz the power, 2017-04-03 22:00:54]

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

[commit-bot: I haz the power, 2017-04-03 22:00:56]

Dry run: This issue passed the CQ dry run.

[xingliu, 2017-04-03 22:13:27]

The CQ bit was checked by xingliu@chromium.org

[xingliu, 2017-04-03 22:13:27]

The patchset sent to the CQ was uploaded after l-g-t-m from
isherman@chromium.org, dtrainor@chromium.org, qinmin@chromium.org
Link to the patchset: https://codereview.chromium.org/2789623005/#ps100001
(title: "Work on feedback.")

[commit-bot: I haz the power, 2017-04-03 22:13: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-04-03 22:20:09]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1491257607444200,
"parent_rev": "278b26a717794d67f361145e0a4a72a758839d8a", "commit_rev":
"ec174ac638779aa4db3573285a40b513e98c5e93"}

[commit-bot: I haz the power, 2017-04-03 22:21:22]

Description was changed from

==========
Add UMA metric to track parallel download requests stats.

Add requests count to track the actual requests sent and special
requests(offset smaller than initial request offset) sent in parallel
download.

Add a boolean metric to track if the file sink is gone when we try to
push a byte stream reader to the sink.


BUG=644352
==========

to

==========
Add UMA metric to track parallel download requests stats.

Add requests count to track the actual requests sent and special
requests(offset smaller than initial request offset) sent in parallel
download.

Add a boolean metric to track if the file sink is gone when we try to
push a byte stream reader to the sink.


BUG=644352

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

[commit-bot: I haz the power, 2017-04-03 22:21:23]

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