Update offline pages UMAs

Update offline pages UMAs

1) Mark some UMAs that were removed some time ago, as obsolete
2) Append client namespace suffix to some UMAs
3) Add a new UMA

BUG=610877, 610869
Committed: https://crrev.com/29952fa90b04d34ea92914ee3eeea1daf8e90c55
Cr-Commit-Position: refs/heads/master@{#394322}

[jianli, 2016-05-13 23:51:58]

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

[rkaplow, 2016-05-16 14:37:58]

lgtm

[fgorski, 2016-05-16 16:20:04]

lgtm, but please apply comments.

Can you use crbug/610877?

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

https://codereview.chromium.org/1980623003/diff/1/chrome/browser/android/offl...
chrome/browser/android/offline_pages/offline_page_tab_helper.cc:57:
UMA_HISTOGRAM_BOOLEAN("OfflinePages.ShowOfflinePageWhenOffline",
can we report with suffix here? It would be interesting to learn which URL gets
selected.

This can be done inside of the OfflinePageUtils::GetOfflineURLForOnlineURL, or
as part of the selection process in:
OfflinePageModel::MaybeGetBestPageForOnlineURL

https://codereview.chromium.org/1980623003/diff/1/chrome/browser/android/offl...
chrome/browser/android/offline_pages/offline_page_tab_helper.cc:73:
entry->GetRedirectChain().back() == redirect_url) {
what about this condition. Can you capture both this and invalid redirect?

https://codereview.chromium.org/1980623003/diff/1/chrome/browser/android/offl...
chrome/browser/android/offline_pages/offline_page_tab_helper.cc:123:
base::ThreadTaskRunnerHandle::Get()->PostTask(
Does this redirect count toward other UMA? Do we have it?

https://codereview.chromium.org/1980623003/diff/1/components/offline_pages/of...
File components/offline_pages/offline_page_model.cc (right):

https://codereview.chromium.org/1980623003/diff/1/components/offline_pages/of...
components/offline_pages/offline_page_model.cc:112: std::string
adjusted_histogram_name;
std::string adjusted_histogram_name(histogram_name); ?

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

https://codereview.chromium.org/1980623003/diff/1/tools/metrics/histograms/hi...
tools/metrics/histograms/histograms.xml:34255: +    Deprecated 10/2015, and
replaced by OfflinePages.LoadStatus.
is the date correct?

https://codereview.chromium.org/1980623003/diff/1/tools/metrics/histograms/hi...
tools/metrics/histograms/histograms.xml:92993: +  <suffix name="last-n"
label="Make last N pages offline"/>
label="Offline recent pages"

[fgorski, 2016-05-16 16:25:21]

One more thing.

Please update reporting to SavePageResult in OfflinePAgeBridge.java:395 and
resolve crbug/610869

[jianli, 2016-05-16 20:54:15]

Description was changed from

==========
Update offline pages UMAs

1) Mark some UMAs that were removed some time ago, as obsolete
2) Append client namespace suffix to some UMAs
3) Add a new UMA

BUG=603940
==========

to

==========
Update offline pages UMAs

1) Mark some UMAs that were removed some time ago, as obsolete
2) Append client namespace suffix to some UMAs
3) Add a new UMA

BUG=610877
==========

[jianli, 2016-05-16 22:35:25]

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

https://codereview.chromium.org/1980623003/diff/1/chrome/browser/android/offl...
chrome/browser/android/offline_pages/offline_page_tab_helper.cc:57:
UMA_HISTOGRAM_BOOLEAN("OfflinePages.ShowOfflinePageWhenOffline",
On 2016/05/16 16:20:04, fgorski wrote:
> can we report with suffix here? It would be interesting to learn which URL
gets
> selected.
> 
> This can be done inside of the OfflinePageUtils::GetOfflineURLForOnlineURL, or
> as part of the selection process in:
> OfflinePageModel::MaybeGetBestPageForOnlineURL

I thought about this but we can't do it because no suffix can be used if no
offline URL is found.

https://codereview.chromium.org/1980623003/diff/1/chrome/browser/android/offl...
chrome/browser/android/offline_pages/offline_page_tab_helper.cc:73:
entry->GetRedirectChain().back() == redirect_url) {
On 2016/05/16 16:20:04, fgorski wrote:
> what about this condition. Can you capture both this and invalid redirect?

Also, I moved the above logic to DidFinishNavigation since it is better to
report it after the navigation is done.

https://codereview.chromium.org/1980623003/diff/1/chrome/browser/android/offl...
chrome/browser/android/offline_pages/offline_page_tab_helper.cc:123:
base::ThreadTaskRunnerHandle::Get()->PostTask(
On 2016/05/16 16:20:04, fgorski wrote:
> Does this redirect count toward other UMA? Do we have it?

Yes, it is part of UMA counting in OfflinePageTabHelper::Redirect.

https://codereview.chromium.org/1980623003/diff/1/components/offline_pages/of...
File components/offline_pages/offline_page_model.cc (right):

https://codereview.chromium.org/1980623003/diff/1/components/offline_pages/of...
components/offline_pages/offline_page_model.cc:112: std::string
adjusted_histogram_name;
On 2016/05/16 16:20:04, fgorski wrote:
> std::string adjusted_histogram_name(histogram_name); ?

Done.

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

https://codereview.chromium.org/1980623003/diff/1/tools/metrics/histograms/hi...
tools/metrics/histograms/histograms.xml:34255: +    Deprecated 10/2015, and
replaced by OfflinePages.LoadStatus.
On 2016/05/16 16:20:04, fgorski wrote:
> is the date correct?

Done.

https://codereview.chromium.org/1980623003/diff/1/tools/metrics/histograms/hi...
tools/metrics/histograms/histograms.xml:92993: +  <suffix name="last-n"
label="Make last N pages offline"/>
On 2016/05/16 16:20:04, fgorski wrote:
> label="Offline recent pages"

Done.

[jianli, 2016-05-16 22:36:00]

On 2016/05/16 16:25:21, fgorski wrote:
> One more thing.
> 
> Please update reporting to SavePageResult in OfflinePAgeBridge.java:395 and
> resolve crbug/610869

Good catch. Moved the reporting logic from Java code to C++ code to unify the
UMA reporting logic.

[jianli, 2016-05-16 23:02:49]

Description was changed from

==========
Update offline pages UMAs

1) Mark some UMAs that were removed some time ago, as obsolete
2) Append client namespace suffix to some UMAs
3) Add a new UMA

BUG=610877
==========

to

==========
Update offline pages UMAs

1) Mark some UMAs that were removed some time ago, as obsolete
2) Append client namespace suffix to some UMAs
3) Add a new UMA

BUG=610877,610869
==========

[fgorski, 2016-05-17 03:53:23]

lgtm with a nit

https://codereview.chromium.org/1980623003/diff/20001/chrome/browser/android/...
File chrome/browser/android/offline_pages/offline_page_tab_helper.cc (right):

https://codereview.chromium.org/1980623003/diff/20001/chrome/browser/android/...
chrome/browser/android/offline_pages/offline_page_tab_helper.cc:92:
UMA_HISTOGRAM_BOOLEAN("OfflinePages.ShowOfflinePageWhenOffline", true);
nit: this is a bit forward looking and perhaps we want to handle it differently
once NQE is available, but did you consider a case where we are not exactly
offline? E.g. Because NQE told us to. Will this name still work? Or will we
define a new UMA?

[fgorski, 2016-05-17 16:38:12]

I just went through my large bookmarks patch again.
Could you add this in the histograms?

 <histogram name="OfflinePages.OfflinePageCount">
+  <obsolete>
+    Deprecated 5/2016. This was duplicating OfflinePages.SavedPageCount.
+  </obsolete>

[jianli, 2016-05-17 18:02:53]

On 2016/05/17 03:53:23, fgorski wrote:
> lgtm with a nit
> 
>
https://codereview.chromium.org/1980623003/diff/20001/chrome/browser/android/...
> File chrome/browser/android/offline_pages/offline_page_tab_helper.cc (right):
> 
>
https://codereview.chromium.org/1980623003/diff/20001/chrome/browser/android/...
> chrome/browser/android/offline_pages/offline_page_tab_helper.cc:92:
> UMA_HISTOGRAM_BOOLEAN("OfflinePages.ShowOfflinePageWhenOffline", true);
> nit: this is a bit forward looking and perhaps we want to handle it
differently
> once NQE is available, but did you consider a case where we are not exactly
> offline? E.g. Because NQE told us to. Will this name still work? Or will we
> define a new UMA?

Renamed ShowOfflinePageWhenOffline to ShowOfflinePageOnBadNetwork.

[jianli, 2016-05-17 21:14:50]

The CQ bit was checked by jianli@chromium.org

[jianli, 2016-05-17 21:14:51]

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

[commit-bot: I haz the power, 2016-05-17 21:15:19]

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

[commit-bot: I haz the power, 2016-05-17 21:37:33]

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

[commit-bot: I haz the power, 2016-05-17 21:37:34]

Try jobs failed on following builders:
  ios-simulator-gn on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-gn/...)

[jianli, 2016-05-17 22:10:37]

The CQ bit was checked by jianli@chromium.org

[jianli, 2016-05-17 22:10:38]

The patchset sent to the CQ was uploaded after l-g-t-m from
rkaplow@chromium.org, fgorski@chromium.org
Link to the patchset: https://codereview.chromium.org/1980623003/#ps60001
(title: "Fix test")

[commit-bot: I haz the power, 2016-05-17 22:11:26]

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

[commit-bot: I haz the power, 2016-05-18 00:57:08]

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

[commit-bot: I haz the power, 2016-05-18 00:57:09]

Try jobs failed on following builders:
  win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[jianli, 2016-05-18 01:02:22]

The CQ bit was checked by jianli@chromium.org

[commit-bot: I haz the power, 2016-05-18 01:03:17]

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

[commit-bot: I haz the power, 2016-05-18 03:23:51]

Description was changed from

==========
Update offline pages UMAs

1) Mark some UMAs that were removed some time ago, as obsolete
2) Append client namespace suffix to some UMAs
3) Add a new UMA

BUG=610877,610869
==========

to

==========
Update offline pages UMAs

1) Mark some UMAs that were removed some time ago, as obsolete
2) Append client namespace suffix to some UMAs
3) Add a new UMA

BUG=610877,610869
==========

[commit-bot: I haz the power, 2016-05-18 03:23:52]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-05-18 03:25:34]

Description was changed from

==========
Update offline pages UMAs

1) Mark some UMAs that were removed some time ago, as obsolete
2) Append client namespace suffix to some UMAs
3) Add a new UMA

BUG=610877,610869
==========

to

==========
Update offline pages UMAs

1) Mark some UMAs that were removed some time ago, as obsolete
2) Append client namespace suffix to some UMAs
3) Add a new UMA

BUG=610877,610869

Committed: https://crrev.com/29952fa90b04d34ea92914ee3eeea1daf8e90c55
Cr-Commit-Position: refs/heads/master@{#394322}
==========

[commit-bot: I haz the power, 2016-05-18 03:25:35]

Patchset 4 (id:??) landed as
https://crrev.com/29952fa90b04d34ea92914ee3eeea1daf8e90c55
Cr-Commit-Position: refs/heads/master@{#394322}