890531 - Please include the value of window.devicePixelRatio in a section of about:support

Status: RESOLVED FIXED

(Toolkit :: General, task)

Description

[Mardeg]

I think about:support is the easiest place to ask people to look for the device pixel ratio who don't know if their system has enlarged their screen or not (whether by default or they forgot they messed with it).

This is in order to best advise them what value to set layout.css.devPixelsPerPx to that is closest to what they saw before Firefox 22.

The current way I'm getting them to reveal the value is by either a site that reports it or via a data: URI (see URL field)

I'm not sure which section it should be listed in, maybe "Graphics"?

Comment 1

[Abhishek Potnis [:avp]]

Attachment: WIP Patch 1

I have added the key "Device Pixel Ratio" in aboutSupport.properties and Troubleshoot.jsm; but could not find a way to set its value. I have assumed that the property would go under Graphics as mentioned in the previous comment.

I tried building with the changes that I have made, but am getting errors. <http://pastebin.mozilla.org/8573389>

Comment 2

[Drew Willcoxon :adw]

Comment on attachment 8560251 [details] [diff] [review]
WIP Patch 1

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

Thanks for taking this bug, Abhishek.

About your build errors -- it looks like there was a bug related to nsISecurityWarningDialogs.idl that requires you to clobber your build, meaning to remove your object directory and build again from scratch: http://mxr.mozilla.org/mozilla-central/search?string=nsISecurityWarningDialogs  I think you can do `./mach clobber` to remove your build directory, and then after that you'll need to build all of Firefox again.

I agree that device pixel ratio fits best under the graphics section.  Device pixel ratio is actually a property of individual windows.  In your patch, you added devicePixelRatio to the gfxInfoProps object, which isn't right because gfxInfoProps is used to look up into the Cc["@mozilla.org/gfx/info;1"].getService(Ci.nsIGfxInfo) service.  You'll need to look up devicePixelRatio on all the user's windows instead.  (You can see that devicePixelRatio is defined on the nsIDOMWindow interface here: http://mxr.mozilla.org/mozilla-central/source/dom/interfaces/base/nsIDOMWindow.idl?rev=0ec7a1f9aa47#388)

Fortunately the graphics data provider in Troubleshoot.jsm already enumerates all the user's windows, so you can do your work inside that loop, too: http://mxr.mozilla.org/mozilla-central/source/toolkit/modules/Troubleshoot.jsm?rev=14abbfddd5c2#322

Right now that loop is storing each enumerated object in winUtils: http://mxr.mozilla.org/mozilla-central/source/toolkit/modules/Troubleshoot.jsm?rev=14abbfddd5c2#325

You'll need to change that line to store each object as the raw enumerated object instead because you will need it later:

  let win = winEnumer.getNext();
  let winUtils = win.QueryInterface(Ci.nsIInterfaceRequestor).
                 getInterface(Ci.nsIDOMWindowUtils);

And then at the end of the loop, you want to QI (QueryInterface) the win object to an nsIDOMWindow, and from there you can get the devicePixelRatio:

  let dpr = win.QueryInterface(Ci.nsIDOMWindow).devicePixelRatio;

Whew.

So you'll want to save up those dpr's somehow, one per window, and include them in the `data` object.  I suggest simply using an array, where each item is the dpr of a window.

Now you have to modify the about:support page to show your new data.  about:support is implemented by these two files:

http://mxr.mozilla.org/mozilla-central/source/toolkit/content/aboutSupport.xhtml
http://mxr.mozilla.org/mozilla-central/source/toolkit/content/aboutSupport.js

You should only have to modify the JS file.  You can see there's a big object called snapshotFormatters that corresponds to all the dataProviders in Troubleshoot.jsm.  You want to modify the graphics one.  Inside the graphics one, you can see that it converts the raw data to an `out` object, and then finally a localizedOut object that's used to update the page's DOM.  You need to convert your new data to a new property on the `out` object.

I think we should display the data in a way that's not too hard to understand.  We could just list the array of dpr's right from the data, so if you have five windows open, it would say, "1, 2, 2, 2", but it's not clear what that means.  Instead I'd like to see something like "1 window: 1 pixel per point, 3 windows: 2 pixels per point".

That's going to be a little complicated to do.  You're right to add your strings to the aboutSupport.properties file.  We're going to need several strings to do this.  I suggest:

devicePixelRatio: The title of the table row, just like your patch already has.

devicePixelRatioWindows: A PluralForm.jsm-compatible string, a semi-colon-separated list of plural forms.  See https://developer.mozilla.org/en-US/docs/Mozilla/Localization/Localization_and_Plurals and http://mxr.mozilla.org/mozilla-central/source/intl/locale/PluralForm.jsm.  In this case, we want (in English):

  #1 window: #2 #3;#1 windows: #2 #3

devicePixelRatioPixelsPerPoint: Another PluralForm string:

  pixel per point;pixels per point

devicePixelRatioConcat: The rule for concatenating strings in a list.  e.g., in English, we write: "foo, bar, baz".

  %1$S, %2$S

You'll need to combine all these strings to get a final string like "1 window: 1 pixel per point, 3 windows: 2 pixels per point".  You can look at how aboutSupport.js already uses PluralForm in other places.  You can reference the strings in the properties file by using the `strings` object that the graphics section already creates by calling stringBundle().  `strings` implements this interface: http://mxr.mozilla.org/mozilla-central/source/intl/strres/nsIStringBundle.idl  The two methods on that interface that you'll need are GetStringFromName and formatStringFromName.  You can see how other places in aboutSupport.js use those methods.  Here's some documentation: https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XPCOM/Reference/Interface/nsIStringBundle

I'll let you figure out how to make the final string.  Let me know when you have questions. :-)

Comment 3

[Abhishek Potnis [:avp]]

Attachment: WIP v2

Hi Drew,

Thanks a lot for the detailed comment on fixing the bug. I have yet to add the  string "devicePixelRatioWindows" in about.properties and onwards.

I will upload the completed patch in a couple of days.

Thanks.

Comment 4

[Drew Willcoxon :adw]

Comment on attachment 8567496 [details] [diff] [review]
WIP v2

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

Hi Abhishek, this looks like a good start, thanks.

I re-read my comment above, and I think we can make this a little easier if we show the text as something like "window 1: 1, window 2: 1, window 3: 2".  That is, just enumerate the windows, referring to them by index.  On second thought, I don't think we should say "pixels per point" because that's not actually right.  If the DPR is 2, then there are actually 2^2=4 pixels per point.

But if you would like to implement it, I still think it would be fine to say "2 windows: 1, 1 window: 2", as I described above -- it's just trickier.

So either "window 1: 1, window 2: 1, window 3: 2", which is easier, or "2 windows: 1, 1 window: 2", which is a little harder -- whichever one you like.

::: toolkit/modules/Troubleshoot.jsm
@@ +334,5 @@
>        }
>        if (data.windowLayerManagerType != "Basic")
>          data.numAcceleratedWindows++;
> +    let dpr = win.QueryInterface(Ci.nsIDOMWindow).devicePixelRatio;
> +    data.dprs[numTotalWindows] = dpr;

You wouldn't have to move the numTotalWindows definition if you did data.dprs.push(dpr) instead.

And please call it data.devicePixelRatios instead of data.dprs.

Comment 5

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Hi avp, can you address the feedback from adw in comment 4 and finish the patch?

Comment 6

[Abhishek Potnis [:avp]]

Hey Sebastian,

I am sorry but I will not be able to work ahead on this bug due to my academic commitments.

Comment 7

[hilal.dib]

Hi, I would like to take this bug on as my first bug. Could I be assigned to it?

Comment 8

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Hi hilal.dib, here you go. If you have any questions, just ask here.

Comment 9

[hilal.dib]

https://github.com/mozilla/gecko-dev/compare/master...hilaldib9:master

These are my changes as of right now. I'm having trouble enumerating more than one display, for some reason I only get one element in the devicePixelRatio. Any ideas?

Comment 10

[:Gijs (he/him)]

(In reply to hilal.dib from comment #9)
> https://github.com/mozilla/gecko-dev/compare/master...hilaldib9:master
> 
> These are my changes as of right now. I'm having trouble enumerating more
> than one display, for some reason I only get one element in the
> devicePixelRatio. Any ideas?

You're enumerating windows, not screens. So you'd need to open multiple windows before this code would show multiple windows.

Comment 11

[hilal.dib]

Ahh, thank you! I was a bit confused, since here on http://developer.mozilla.org/en-US/docs/Web/API/Window, it says "In a tabbed browser, such as Firefox, each tab contains its own window object.", and so I was expecting every tab to have its own entry.

Good to know that the code works though, because I was scratching my head on that one. The next step is to clean up the way we output it then. Hopefully will be done soon.

Comment 12

[BugBot [:suhaib / :marco/ :calixte]]

This good-first-bug hasn't had any activity for 6 months, it is automatically unassigned.
For more information, please visit auto_nag documentation.

Comment 13

[Karthik Sundar]

is this issue still open??

Comment 14

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Yes, it is still available.

Comment 15

[manila]

Hello, If Karthik doesn't take this bug may I be assigned to it?

Comment 16

[Dhanesh]

hi. i would like to take this . can anyone helping me to fix this?

Comment 17

[Dhanesh]

(In reply to Dhanesh from comment #16)

hi. i would like to take this . help me to fix this.

Thanks

Comment 18

[onyemakonorambrose]

Hello, I would like to work on this bug if it is still available. Do assign it to me please.

Comment 19

[rohaidb]

If this bug is still available, I would like to be assigned to it. Thank you

Comment 20

[rohaidb]

(In reply to rohaidb from comment #19)

If this bug is still available, I would like to be assigned to it. Thank you

I forgot to add that I'm an Outreachy applicant.

Comment 21

[Alan Shirk]

Attachment: Bug 890531 - Added Window Device Pixel Ratios to Graphics section of about:support. r?sfoster

-Created an array of device pixel ratios and looped through it to properly display each window's ratio

Comment 22

[Alan Shirk]

Attachment: WIP: Bug 890531 - Displaying raw array data

Depends on D162771

Comment 23

[Alan Shirk]

Attachment: WIP: Bug 890531 - Changed display name to Window Device Pixel Ratios

Depends on D162787

Comment 24

[Iulian Moraru]

https://hg.mozilla.org/mozilla-central/rev/79123d1214a0