Open Bug 890531 Opened 8 years ago Updated 6 months ago
Please include the value of window
.device Pixel Ratio in a section of about:support
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"?
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>
Assignee: nobody → abhishekp.bugzilla
Status: NEW → ASSIGNED
Attachment #8560251 - Flags: feedback?(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. :-)
Attachment #8560251 - Flags: feedback?(adw) → feedback+
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 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.
Attachment #8567496 - Flags: feedback?(adw) → feedback+
Hi avp, can you address the feedback from adw in comment 4 and finish the patch?
Assignee: abhishekp.bugzilla → nobody
Status: ASSIGNED → NEW
Hey Sebastian, I am sorry but I will not be able to work ahead on this bug due to my academic commitments.
Hi, I would like to take this bug on as my first bug. Could I be assigned to it?
Hi hilal.dib, here you go. If you have any questions, just ask here.
Assignee: nobody → hilal.dib
Status: NEW → ASSIGNED
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?
(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.
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.
Whiteboard: [good first bug][lang=js] → [lang=js]
Assignee: hilal.dib → nobody
Status: ASSIGNED → NEW
You need to log in before you can comment on or make changes to this bug.