Change DISPLAY_SCALING_<OS> histograms to a single DISPLAY_SCALING

RESOLVED FIXED in Firefox 55

Status

()

RESOLVED FIXED
4 years ago
a year ago

People

(Reporter: gfritzsche, Assigned: flyingrub)

Tracking

(Blocks: 1 bug)

Trunk
mozilla55
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox55 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment)

(Reporter)

Description

4 years ago
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.
(Assignee)

Updated

a year ago
Assignee: nobody → flyinggrub
Comment hidden (mozreview-request)
(Assignee)

Comment 2

a year ago
This patch builds fine but when running it I can't find the new histogram in about:telemetry...
(Assignee)

Comment 3

a year ago
:Dolske are you still using this histogram ?
Flags: needinfo?(dolske)
(Reporter)

Comment 4

a year ago
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.
(Reporter)

Comment 5

a year ago
mozreview-review
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.
The data is still useful, and I have no objection to removing the OS-specific probes.
Flags: needinfo?(dolske)
Comment hidden (mozreview-request)
(Assignee)

Comment 8

a year ago
(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 :).
(Assignee)

Updated

a year ago
Attachment #8848479 - Flags: review?(gfritzsche)
(Reporter)

Updated

a year ago
Attachment #8848479 - Flags: review?(chutten)
(Reporter)

Comment 9

a year ago
Chris, can you take a look?

Comment 10

a year ago
mozreview-review
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?
Attachment #8848479 - Flags: review?(chutten) → review-
Comment hidden (mozreview-request)
(Assignee)

Comment 12

a year ago
mozreview-review-reply
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

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

https://reviewboard.mozilla.org/r/121376/#review134910
Attachment #8848479 - Flags: review?(chutten) → review+
(Assignee)

Updated

a year ago
Keywords: checkin-needed

Comment 14

a year ago
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/5c16d58e5f23
Change DISPLAY_SCALING_<OS> histograms to a single r=chutten
Keywords: checkin-needed

Comment 15

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/5c16d58e5f23
Status: NEW → RESOLVED
Last Resolved: a year ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in before you can comment on or make changes to this bug.