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

Status: RESOLVED FIXED

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

Description

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

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.

Comment 1

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Johan, are you interested in this one?

Comment 2

[Johan K. Jensen :johankj]

Attachment: bug1207544_rename_dpi_to_dpr.patch

Something like this?

Comment 3

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Comment on attachment 8664871 [details] [diff] [review]
bug1207544_rename_dpi_to_dpr.patch

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

Great, thanks!

Comment 4

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Johan, could you push this to try?  Or I can if you don't get to it first.

Comment 5

[Johan K. Jensen :johankj]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=052134c763c4

Comment 6

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Hmm, it seems like there are many failures, especially in browser_cmd_screenshot.js.

Comment 7

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

Attachment: patch, v2

Issue was that in the localization file, "screenshotDPIManual" hadn't yet been changed to "screenshotDPRManual".

Comment 8

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

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.

Comment 9

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

Pushed to Try with Johan's Try config: https://treeherder.mozilla.org/#/jobs?repo=try&revision=47dcb82e71b2

Comment 10

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/4359a7f15627

Comment 11

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

https://hg.mozilla.org/mozilla-central/rev/4359a7f15627