1205038 - Add missing BROWSER_SET_DEFAULT_ALWAYS_CHECK histogram to Histograms.json, and fix BROWSER_SET_DEFAULT_TIME_TO_COMPLETION_SECONDS

Status: RESOLVED FIXED

(Firefox :: Shell Integration, defect)

Description

[Jared Wein [:jaws] (please needinfo? me)]

Bug 1191242 was supposed to add the BROWSER_SET_DEFAULT_ALWAYS_CHECK histogram but it wasn't part of the Histograms.json changes that landed.

Because it is missing it is also causing the BROWSER_SET_DEFAULT_DIALOG_PROMPT_RAWCOUNT histogram to not get recorded.

The Histogram for this is,
  "BROWSER_SET_DEFAULT_ALWAYS_CHECK": {
    "expires_in_version": "never",
    "kind": "boolean",
    "releaseChannelCollection": "opt-out",
    "description": "True if the profile has `browser.shell.checkDefaultBrowser` set to true."
  },

and was reviewed in bug 1191242.

Comment 1

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: Patch

Comment 2

[Justin Dolske [:Dolske]]

(In reply to (Mostly away 9/11-9/23) Jared Wein [:jaws] (please needinfo? me) from comment #0)

> Because it is missing it is also causing the
> BROWSER_SET_DEFAULT_DIALOG_PROMPT_RAWCOUNT histogram to not get recorded.

To clarify for the record, this is because the getHistogramById("...ALWAYS_CHECK").add() call throws, and so the _RAWCOUNT probe is never run.

We also noticed the BROWSER_SET_DEFAULT_TIME_TO_COMPLETION_SECONDS probe wasn't being recorded. This ends up being because the |this| scope on the timer callback was wrong, and so |this._setAsDefaultTimer.cancel()| was throwing before the probe was recorded. 

That's a trivial fix with an arrow function. Seems to work now on OS X, with a workaround for bug 1205066, which is causing another exception to be thrown elsewhere :|.

Comment 3

[Justin Dolske [:Dolske]]

Attachment: Patch v.2

r+ from me on Jared's bit, so need a second reviewer for my bit. ;-)

Comment 4

[:Gijs (he/him)]

Comment on attachment 8661482 [details] [diff] [review]
Patch v.2

Review of attachment 8661482 [details] [diff] [review]:
-----------------------------------------------------------------

rs=me

Comment 5

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/306ee3d02ef8

Comment 6

[Justin Dolske [:Dolske]]

Comment on attachment 8661482 [details] [diff] [review]
Patch v.2

Approval Request Comment
[Feature/regressing bug #]: bug 1191242 implemented this
[User impact if declined]: none, but we lose highly desired telemetry
[Describe test coverage new/current, TreeHerder]: n/a
[Risks and why]: low-risk, fixes two spots that are currently throwing (harmlessly, due to a catch-block, other than not recording the telemetry data)
[String/UUID change made/needed]: n/a

Bug 1191242 was uplifted to beta/41. At this point it looks like uplift/and release/41 RC builds are done, so I'm not sure if this is actually feasible to get into 41 unless there's a 41.0.1? (I probably wouldn't respin/chemspill for just this, but I'd like it to be considered for a ridealong.)

Comment 7

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8661482 [details] [diff] [review]
Patch v.2

During RC week, only bugs which are sec-crit + easy to exploit, severe crash/hangs and major regressions that make the release unusable are being considered. This bug does not seem to be a release blocker. Wontfix for 41.

Comment 8

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8661482 [details] [diff] [review]
Patch v.2

Same as the Beta uplift comment.

Comment 9

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/306ee3d02ef8

Comment 10

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8661482 [details] [diff] [review]
Patch v.2

We can take it in 42. Plenty of time!

Comment 11

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/f188a8e59381

Comment 12

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/f188a8e59381