Report redirect UMA in offline interceptor

Report redirect UMA in offline interceptor

BUG=666963
Committed: https://crrev.com/48ea10d8835e13336cf03c243b938bcb133dba49
Cr-Commit-Position: refs/heads/master@{#434029}

[jianli, 2016-11-18 23:44:38]

jianli@chromium.org changed reviewers:
+ fgorski@chromium.org

[jianli, 2016-11-18 23:44:47]

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

[commit-bot: I haz the power, 2016-11-18 23:45:52]

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

[fgorski, 2016-11-18 23:52:22]

lgtm with comments (mostly naming and docs)

https://codereview.chromium.org/2507293006/diff/1/chrome/browser/android/offl...
File chrome/browser/android/offline_pages/offline_page_request_job.cc (right):

https://codereview.chromium.org/2507293006/diff/1/chrome/browser/android/offl...
chrome/browser/android/offline_pages/offline_page_request_job.cc:62: REDIRECTED,
add a comment someplace whether this indicates successful showing of an offline
page, please.

https://codereview.chromium.org/2507293006/diff/1/chrome/browser/android/offl...
File chrome/browser/android/offline_pages/offline_page_request_job_unittest.cc
(right):

https://codereview.chromium.org/2507293006/diff/1/chrome/browser/android/offl...
chrome/browser/android/offline_pages/offline_page_request_job_unittest.cc:252:
// Expect one count of|result| UMA reported and no more.
nit: space before |

Expect exactly one...  drop " and no more."

First explain what "and no more" means. I s it opposite of explanation in 258?
In that case capture it more explicitly.

https://codereview.chromium.org/2507293006/diff/1/chrome/browser/android/offl...
chrome/browser/android/offline_pages/offline_page_request_job_unittest.cc:255:
// Expect multiple |count| of |result| UMA reported and no more.
Expect exactly |count|... and drop "and no more"

https://codereview.chromium.org/2507293006/diff/1/chrome/browser/android/offl...
chrome/browser/android/offline_pages/offline_page_request_job_unittest.cc:260:
void ExpectAggregatedRequestResultHistogramOnMultiBuckets(
This is meant to be exactly one in specific bucket, and ignore other buckets?
Probably we could rename that to.

[jianli, 2016-11-19 00:54:01]

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

[jianli, 2016-11-19 00:54:05]

https://codereview.chromium.org/2507293006/diff/1/chrome/browser/android/offl...
File chrome/browser/android/offline_pages/offline_page_request_job.cc (right):

https://codereview.chromium.org/2507293006/diff/1/chrome/browser/android/offl...
chrome/browser/android/offline_pages/offline_page_request_job.cc:62: REDIRECTED,
On 2016/11/18 23:52:22, fgorski wrote:
> add a comment someplace whether this indicates successful showing of an
offline
> page, please.

Done.

https://codereview.chromium.org/2507293006/diff/1/chrome/browser/android/offl...
File chrome/browser/android/offline_pages/offline_page_request_job_unittest.cc
(right):

https://codereview.chromium.org/2507293006/diff/1/chrome/browser/android/offl...
chrome/browser/android/offline_pages/offline_page_request_job_unittest.cc:252:
// Expect one count of|result| UMA reported and no more.
On 2016/11/18 23:52:22, fgorski wrote:
> nit: space before |
> 
> Expect exactly one...  drop " and no more."

Done.

> 
> First explain what "and no more" means. I s it opposite of explanation in 258?
> In that case capture it more explicitly. 

Yes.

https://codereview.chromium.org/2507293006/diff/1/chrome/browser/android/offl...
chrome/browser/android/offline_pages/offline_page_request_job_unittest.cc:255:
// Expect multiple |count| of |result| UMA reported and no more.
On 2016/11/18 23:52:22, fgorski wrote:
> Expect exactly |count|... and drop "and no more"

Done.

https://codereview.chromium.org/2507293006/diff/1/chrome/browser/android/offl...
chrome/browser/android/offline_pages/offline_page_request_job_unittest.cc:260:
void ExpectAggregatedRequestResultHistogramOnMultiBuckets(
On 2016/11/18 23:52:22, fgorski wrote:
> This is meant to be exactly one in specific bucket, and ignore other buckets?
> Probably we could rename that to.

Eexactly one in specific bucket and other buckets could also have data.

[jianli, 2016-11-19 00:54:21]

jianli@chromium.org changed reviewers:
+ holte@chromium.org

[jianli, 2016-11-19 00:54:21]

holte for histograms

[commit-bot: I haz the power, 2016-11-19 00:54:38]

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-11-19 01:53:10]

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

[commit-bot: I haz the power, 2016-11-19 01:53:14]

Dry run: This issue passed the CQ dry run.

[jianli, 2016-11-22 22:00:15]

jianli@chromium.org changed reviewers:
+ mpearson@chromium.org
- holte@chromium.org

[jianli, 2016-11-22 22:00:42]

mpearson for histograms

[Mark P, 2016-11-22 22:35:25]

histograms.xml lgtm

[jianli, 2016-11-22 22:39:15]

The CQ bit was checked by jianli@chromium.org

[jianli, 2016-11-22 22:39:16]

The patchset sent to the CQ was uploaded after l-g-t-m from fgorski@chromium.org
Link to the patchset: https://codereview.chromium.org/2507293006/#ps20001
(title: "Address feedback")

[commit-bot: I haz the power, 2016-11-22 22:39:56]

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-11-22 23:56:21]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1479854355605000,
"parent_rev": "048e97a9647f2b6444111a171271d500e11483d4", "commit_rev":
"4ff9450829d7ab25b6bb2127b8b996fd2bd2f6ef"}

[commit-bot: I haz the power, 2016-11-22 23:56:46]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-11-23 00:00:40]

Description was changed from

==========
Report redirect UMA in offline interceptor

BUG=666963
==========

to

==========
Report redirect UMA in offline interceptor

BUG=666963

Committed: https://crrev.com/48ea10d8835e13336cf03c243b938bcb133dba49
Cr-Commit-Position: refs/heads/master@{#434029}
==========

[commit-bot: I haz the power, 2016-11-23 00:00:42]

Patchset 2 (id:??) landed as
https://crrev.com/48ea10d8835e13336cf03c243b938bcb133dba49
Cr-Commit-Position: refs/heads/master@{#434029}