Record histograms for on-demand update check results.

Record histograms for on-demand update check results.

And stop mis-using user metrics action.

BUG=424689
Committed: https://crrev.com/0fed98512bbbcd0c3583babfbdde1e72f2013c5a
Cr-Commit-Position: refs/heads/master@{#300217}

[grt (UTC plus 2), 2014-10-17 18:55:54]

grt@chromium.org changed reviewers:
+ asvitkine@chromium.org

[grt (UTC plus 2), 2014-10-17 18:56:48]

Hi Alexei. Could you take a look at this? Thanks.

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

https://codereview.chromium.org/659333003/diff/1/tools/metrics/histograms/his...
tools/metrics/histograms/histograms.xml:36222: +<histogram
name="UpdateCheckWin.ErrorHresult">
should i specify units or type or anything here?

[Alexei Svitkine (slow), 2014-10-17 19:00:56]

https://codereview.chromium.org/659333003/diff/1/chrome/browser/google/google...
File chrome/browser/google/google_update_win.cc (right):

https://codereview.chromium.org/659333003/diff/1/chrome/browser/google/google...
chrome/browser/google/google_update_win.cc:397: "UpdateCheckWin.UpgradeResult",
results, NUM_UPGRADE_RESULTS);
Can you use the existing GoogleUpdate. prefix instead of introducing a new one
top-level one?

https://codereview.chromium.org/659333003/diff/1/chrome/browser/ui/webui/help...
File chrome/browser/ui/webui/help/version_updater_win.cc (left):

https://codereview.chromium.org/659333003/diff/1/chrome/browser/ui/webui/help...
chrome/browser/ui/webui/help/version_updater_win.cc:211:
content::RecordAction(UserMetricsAction("UpgradeCheck_Upgraded"));
Can you mark all these actions as obsolete in actions.xml?

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

https://codereview.chromium.org/659333003/diff/1/tools/metrics/histograms/his...
tools/metrics/histograms/histograms.xml:36222: +<histogram
name="UpdateCheckWin.ErrorHresult">
On 2014/10/17 18:56:48, grt wrote:
> should i specify units or type or anything here?

No, though you could make an enum for hresult values if there isn't one already.
If not, you'll just see the numeric codes, which might be OK for your use.

[grt (UTC plus 2), 2014-10-18 02:36:34]

grt@chromium.org changed reviewers:
+ pkasting@chromium.org

[grt (UTC plus 2), 2014-10-18 02:37:35]

Thanks, Alexei. Please take another look.

pkasting: OWNERS review for chrome/*. Thanks.

https://codereview.chromium.org/659333003/diff/1/chrome/browser/google/google...
File chrome/browser/google/google_update_win.cc (right):

https://codereview.chromium.org/659333003/diff/1/chrome/browser/google/google...
chrome/browser/google/google_update_win.cc:397: "UpdateCheckWin.UpgradeResult",
results, NUM_UPGRADE_RESULTS);
On 2014/10/17 19:00:55, Alexei Svitkine wrote:
> Can you use the existing GoogleUpdate. prefix instead of introducing a new one
> top-level one?

Done.

https://codereview.chromium.org/659333003/diff/1/chrome/browser/ui/webui/help...
File chrome/browser/ui/webui/help/version_updater_win.cc (left):

https://codereview.chromium.org/659333003/diff/1/chrome/browser/ui/webui/help...
chrome/browser/ui/webui/help/version_updater_win.cc:211:
content::RecordAction(UserMetricsAction("UpgradeCheck_Upgraded"));
On 2014/10/17 19:00:55, Alexei Svitkine wrote:
> Can you mark all these actions as obsolete in actions.xml?

Done for these and for two that don't seem to have been present in the code for
some time.

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

https://codereview.chromium.org/659333003/diff/1/tools/metrics/histograms/his...
tools/metrics/histograms/histograms.xml:36222: +<histogram
name="UpdateCheckWin.ErrorHresult">
On 2014/10/17 19:00:55, Alexei Svitkine wrote:
> On 2014/10/17 18:56:48, grt wrote:
> > should i specify units or type or anything here?
> 
> No, though you could make an enum for hresult values if there isn't one
already.
> If not, you'll just see the numeric codes, which might be OK for your use.

Numeric codes are good for now since the potential space for HRESULTs is large.
After collecting some data, perhaps it will make sense to make an enum with
pretty names for the popular values.  Thanks.

[Peter Kasting, 2014-10-18 03:01:11]

LGTM

https://codereview.chromium.org/659333003/diff/20001/chrome/browser/ui/webui/...
File chrome/browser/ui/webui/help/version_updater_win.cc (right):

https://codereview.chromium.org/659333003/diff/20001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/help/version_updater_win.cc:251: : NEARLY_UPDATED;
Nit: I think this might be slightly clearer reversed:

  callback_.Run(
      (version.IsValid() && version.CompareTo(running_version) > 0) ?
          NEARLY_UPDATED : UPDATED, 0, base::string16());

[grt (UTC plus 2), 2014-10-18 03:28:52]

Thanks, Peter.

https://codereview.chromium.org/659333003/diff/20001/chrome/browser/ui/webui/...
File chrome/browser/ui/webui/help/version_updater_win.cc (right):

https://codereview.chromium.org/659333003/diff/20001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/help/version_updater_win.cc:251: : NEARLY_UPDATED;
On 2014/10/18 03:01:11, Peter Kasting wrote:
> Nit: I think this might be slightly clearer reversed:
> 
>   callback_.Run(
>       (version.IsValid() && version.CompareTo(running_version) > 0) ?
>           NEARLY_UPDATED : UPDATED, 0, base::string16());

Done with formatting courtesy of "git cl format".

[Alexei Svitkine (slow), 2014-10-18 05:36:45]

LGTM after you fix the names to match histograms.xml, thanks.

https://codereview.chromium.org/659333003/diff/40001/chrome/browser/google/go...
File chrome/browser/google/google_update_win.cc (right):

https://codereview.chromium.org/659333003/diff/40001/chrome/browser/google/go...
chrome/browser/google/google_update_win.cc:397: "UpdateCheckWin.UpgradeResult",
results, NUM_UPGRADE_RESULTS);
Looks like you forgot to rename these in the code to use the GoogleUpdate.
prefix.

[grt (UTC plus 2), 2014-10-18 15:20:57]

Thanks

https://codereview.chromium.org/659333003/diff/40001/chrome/browser/google/go...
File chrome/browser/google/google_update_win.cc (right):

https://codereview.chromium.org/659333003/diff/40001/chrome/browser/google/go...
chrome/browser/google/google_update_win.cc:397: "UpdateCheckWin.UpgradeResult",
results, NUM_UPGRADE_RESULTS);
On 2014/10/18 05:36:44, Alexei Svitkine wrote:
> Looks like you forgot to rename these in the code to use the GoogleUpdate.
> prefix.

!

[grt (UTC plus 2), 2014-10-18 15:21:03]

The CQ bit was checked by grt@chromium.org

[commit-bot: I haz the power, 2014-10-18 15:21:13]

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

[commit-bot: I haz the power, 2014-10-18 16:17:01]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2014-10-18 16:17:50]

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