Closed Bug 1207544 Opened 6 years ago Closed 6 years ago

screenshot --dpi asks for pixels per inch and uses it as devicePixelRatio


(DevTools Graveyard :: Graphic Commandline and Toolbar, defect)

Not set


(firefox42 unaffected, firefox43 affected, firefox44 fixed)

Firefox 44
Tracking Status
firefox42 --- unaffected
firefox43 --- affected
firefox44 --- fixed


(Reporter: aryx, Assigned: jj)



(Whiteboard: [polish-backlog][difficulty=easy])


(2 files)

Bug 970625 added the parameter --dpi to the graphical command line's commadn 'screenshot'.

The strings are:
# LOCALIZATION NOTE (screenshotDPIDesc) A very short string to describe
# the 'dpi' parameter to the 'screenshot' command, which is displayed in
# a dialog when the user is using this command.
screenshotDPIDesc=Dots per inch

# LOCALIZATION NOTE (screenshotDPIManual) A fuller description of the
# 'dpi' parameter to the 'screenshot' command, displayed when the user
# asks for help on what it does.
screenshotDPIManual=The number of dots per inch in the screenshot

But the code change contains:
> -  const ratio = window.devicePixelRatio;
> +  const ratio = args.dpi ? args.dpi : window.devicePixelRatio;

DPI and devicePixelRatio are two different things. The latter one is the count of physical pixels which can be put into one CSS pixel.

So running |screenshot --dpi 150| fails because the image gets too big.

The strings are already on Aurora, so if it needs to get fixed there it has to use dpi and not dpr.
Johan, are you interested in this one?
Flags: needinfo?(jj)
Something like this?
Assignee: nobody → jj
Flags: needinfo?(jj)
Attachment #8664871 - Flags: review?(jryans)
Comment on attachment 8664871 [details] [diff] [review]

Review of attachment 8664871 [details] [diff] [review]:

Great, thanks!
Attachment #8664871 - Flags: review?(jryans) → review+
Johan, could you push this to try?  Or I can if you don't get to it first.
Flags: needinfo?(jj)
Hmm, it seems like there are many failures, especially in browser_cmd_screenshot.js.
Whiteboard: [polish-backlog] → [polish-backlog][difficulty=easy]
Attached patch patch, v2Splinter Review
Issue was that in the localization file, "screenshotDPIManual" hadn't yet been changed to "screenshotDPRManual".
Attachment #8670912 - Flags: review?(jryans)
Comment on attachment 8670912 [details] [diff] [review]
patch, v2

Review of attachment 8670912 [details] [diff] [review]:

Ah, of course.

Do you have try access to do another run?  If not, ni? me so I can take care of it.
Attachment #8670912 - Flags: review?(jryans) → review+
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 44
Product: Firefox → DevTools
Product: DevTools → DevTools Graveyard
You need to log in before you can comment on or make changes to this bug.