1121966 - Change DISPLAY_SCALING_<OS> histograms to a single DISPLAY_SCALING

Status: RESOLVED FIXED

(Toolkit :: Telemetry, defect)

Description

[Georg Fritzsche [:gfritzsche]]

Currently we have DISPLAY_SCALING_OSX, DISPLAY_SCALING_MSWIN, DISPLAY_SCALING_LINUX.

We should just use DISPLAY_SCALING and do a proper per-OS breakdown as usual.

Comment 1

[flyingrub]

Attachment: Bug 1121966 - Change DISPLAY_SCALING_<OS> histograms to a single

DISPLAY_SCALING

Replaced the os specific histograms by a single one.

Review commit: https://reviewboard.mozilla.org/r/121376/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/121376/

Comment 2

[flyingrub]

This patch builds fine but when running it I can't find the new histogram in about:telemetry...

Comment 3

[flyingrub]

:Dolske are you still using this histogram ?

Comment 4

[Georg Fritzsche [:gfritzsche]]

It has been a while since this bug was filed.
This is useful, as OS breakdowns can be done server-side in better ways.
However, the users/owners of these histograms should decide if we can just replace them.

Comment 5

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8848479 [details]
Bug 1121966 - Change DISPLAY_SCALING_<OS> histograms to a single

https://reviewboard.mozilla.org/r/121376/#review123866

If these histograms are not used anymore, we should just remove them.
If they are still moved and its ok to change them into one histogram, then we can improve this a bit.

::: toolkit/components/telemetry/histogram-whitelists.json:208
(Diff revision 1)
>      "DEVTOOLS_WEBIDE_REMOTE_CONNECTION_RESULT",
>      "DEVTOOLS_WEBIDE_SIMULATOR_CONNECTION_RESULT",
>      "DEVTOOLS_WEBIDE_TIME_ACTIVE_SECONDS",
>      "DEVTOOLS_WEBIDE_USB_CONNECTION_RESULT",
>      "DEVTOOLS_WEBIDE_WIFI_CONNECTION_RESULT",
> -    "DISPLAY_SCALING_LINUX",
> +    "DISPLAY_SCALING",

We should not add anything to the whitelist here.
Instead we should set proper attributes for the histogram.

Comment 6

[Justin Dolske [:Dolske]]

The data is still useful, and I have no objection to removing the OS-specific probes.

Comment 7

[flyingrub]

Comment on attachment 8848479 [details]
Bug 1121966 - Change DISPLAY_SCALING_<OS> histograms to a single

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/121376/diff/1-2/

Comment 8

[flyingrub]

(In reply to Georg Fritzsche [:gfritzsche] [away March 24 - April 2] from comment #5)
> We should not add anything to the whitelist here.
> Instead we should set proper attributes for the histogram.

I pushed those changes :).

Comment 9

[Georg Fritzsche [:gfritzsche]]

Chris, can you take a look?

Comment 10

[Chris H-C :chutten]

Comment on attachment 8848479 [details]
Bug 1121966 - Change DISPLAY_SCALING_<OS> histograms to a single

https://reviewboard.mozilla.org/r/121376/#review134860

Does it pass lint?

::: browser/components/nsBrowserGlue.js:875
(Diff revision 2)
> -      case "linux":
> -        SCALING_PROBE_NAME = "DISPLAY_SCALING_LINUX";
> -        break;
> -    }
> -    if (SCALING_PROBE_NAME) {
>        let scaling = aWindow.devicePixelRatio * 100;

Isn't the indentation off here?

Comment 11

[flyingrub]

Comment on attachment 8848479 [details]
Bug 1121966 - Change DISPLAY_SCALING_<OS> histograms to a single

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/121376/diff/2-3/

Comment 12

[flyingrub]

Comment on attachment 8848479 [details]
Bug 1121966 - Change DISPLAY_SCALING_<OS> histograms to a single

https://reviewboard.mozilla.org/r/121376/#review134860

```
0 fly@Minotoor ~/workspace/mozilla-central $ ./mach lint browser/components/nsBrowserGlue.js 
✖ 0 problems (0 errors, 0 warnings)
```
But you are right there was an indentation problem there :).

Comment 13

[Chris H-C :chutten]

Comment on attachment 8848479 [details]
Bug 1121966 - Change DISPLAY_SCALING_<OS> histograms to a single

https://reviewboard.mozilla.org/r/121376/#review134910

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/5c16d58e5f23
Change DISPLAY_SCALING_<OS> histograms to a single r=chutten

Comment 15

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/5c16d58e5f23