[Offline Pages] Add UMA to record final offline request status and loading failed error code.

[Offline Pages] Add UMA to record final offline request status and loading failed error code.

BUG=704999
Review-Url: https://codereview.chromium.org/2751253004
Cr-Commit-Position: refs/heads/master@{#458001}
Committed: https://chromium.googlesource.com/chromium/src/+/642d1848697b731de396d6df1577443e33e17ad0

[chili, 2017-03-17 19:06:33]

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

[commit-bot: I haz the power, 2017-03-17 19:07:10]

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, 2017-03-17 19:11:45]

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

[commit-bot: I haz the power, 2017-03-17 19:11:46]

Dry run: Try jobs failed on following builders:
  android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)
  chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...)
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[chili, 2017-03-17 19:29:52]

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

[chili, 2017-03-17 19:30:19]

chili@chromium.org changed reviewers:
+ petewil@chromium.org

[commit-bot: I haz the power, 2017-03-17 19:30:42]

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, 2017-03-17 20:31:43]

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

[commit-bot: I haz the power, 2017-03-17 20:31:44]

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

[Pete Williamson, 2017-03-17 20:50:56]

Mostly looks good, a few questions.

https://codereview.chromium.org/2751253004/diff/40001/components/offline_page...
File components/offline_pages/core/background/request_coordinator_unittest.cc
(right):

https://codereview.chromium.org/2751253004/diff/40001/components/offline_page...
components/offline_pages/core/background/request_coordinator_unittest.cc:674:
"OfflinePages.Background.FinalSavePageResult.bookmark", 0, /* SUCCESS */
Are there any tests where we could check for a failed UMA?  If so, it would be
nice to check just to make sure we aren't always writing "success".

https://codereview.chromium.org/2751253004/diff/40001/tools/metrics/histogram...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/2751253004/diff/40001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:103368: +<enum
name="OfflinePagesBackgroundSavePageResult" type="int">
Why add this, wasn't it already present, or was it just to make sure the values
match our expectations?

[chili, 2017-03-17 21:17:19]

https://codereview.chromium.org/2751253004/diff/40001/components/offline_page...
File components/offline_pages/core/background/request_coordinator_unittest.cc
(right):

https://codereview.chromium.org/2751253004/diff/40001/components/offline_page...
components/offline_pages/core/background/request_coordinator_unittest.cc:674:
"OfflinePages.Background.FinalSavePageResult.bookmark", 0, /* SUCCESS */
On 2017/03/17 20:50:56, Pete Williamson wrote:
> Are there any tests where we could check for a failed UMA?  If so, it would be
> nice to check just to make sure we aren't always writing "success". 

Done.

https://codereview.chromium.org/2751253004/diff/40001/tools/metrics/histogram...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/2751253004/diff/40001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:103368: +<enum
name="OfflinePagesBackgroundSavePageResult" type="int">
On 2017/03/17 20:50:56, Pete Williamson wrote:
> Why add this, wasn't it already present, or was it just to make sure the
values
> match our expectations?

I actually misread. This is the request_notifier::BackgroundSavePageResult. The
other is offliner::RequestStatus...

[Pete Williamson, 2017-03-17 21:29:04]

lgtm

[chili, 2017-03-17 23:20:02]

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

[chili, 2017-03-17 23:20:03]

+holte for histogram change

[Steven Holte, 2017-03-17 23:34:03]

lgtm

[chili, 2017-03-19 01:05:55]

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

[commit-bot: I haz the power, 2017-03-19 01:06:03]

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, 2017-03-19 03:59:22]

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

[commit-bot: I haz the power, 2017-03-19 03:59:23]

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

[chili, 2017-03-19 19:43:22]

The CQ bit was checked by chili@chromium.org

[chili, 2017-03-19 19:43:22]

The patchset sent to the CQ was uploaded after l-g-t-m from
petewil@chromium.org, holte@chromium.org
Link to the patchset: https://codereview.chromium.org/2751253004/#ps80001
(title: "fix unit test so DidFinishNavigation is only called once")

[commit-bot: I haz the power, 2017-03-19 19:43:27]

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-03-19 20:40:07]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1489952602510430,
"parent_rev": "b56e7169125edf091bf4f5b6a4e830218653f880", "commit_rev":
"642d1848697b731de396d6df1577443e33e17ad0"}

[commit-bot: I haz the power, 2017-03-19 20:41:12]

Description was changed from

==========
[Offline Pages] Add UMA to record final offline request status and loading
failed error code.

BUG=
==========

to

==========
[Offline Pages] Add UMA to record final offline request status and loading
failed error code.

BUG=

Review-Url: https://codereview.chromium.org/2751253004
Cr-Commit-Position: refs/heads/master@{#458001}
Committed:
https://chromium.googlesource.com/chromium/src/+/642d1848697b731de396d6df1577...
==========

[commit-bot: I haz the power, 2017-03-19 20:41:13]

Committed patchset #5 (id:80001) as
https://chromium.googlesource.com/chromium/src/+/642d1848697b731de396d6df1577...

[chili, 2017-03-24 18:38:15]

Description was changed from

==========
[Offline Pages] Add UMA to record final offline request status and loading
failed error code.

BUG=

Review-Url: https://codereview.chromium.org/2751253004
Cr-Commit-Position: refs/heads/master@{#458001}
Committed:
https://chromium.googlesource.com/chromium/src/+/642d1848697b731de396d6df1577...
==========

to

==========
[Offline Pages] Add UMA to record final offline request status and loading
failed error code.

BUG=704999

Review-Url: https://codereview.chromium.org/2751253004
Cr-Commit-Position: refs/heads/master@{#458001}
Committed:
https://chromium.googlesource.com/chromium/src/+/642d1848697b731de396d6df1577...
==========