Screenshot command should allow you to choose Retina or non-Retina resolution

RESOLVED FIXED in Firefox 43

Status

()

Firefox
Developer Tools: Graphic Commandline and Toolbar
--
enhancement
RESOLVED FIXED
4 years ago
2 years ago

People

(Reporter: mconley, Assigned: Johan K. Jensen)

Tracking

Trunk
Firefox 43
x86
Mac OS X
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox43 fixed, relnote-firefox 44+)

Details

Attachments

(1 attachment, 1 obsolete attachment)

I just filed bug 970618, and on a related note, it might be handy to be able to choose whether or not I want Retina or a non-Retina resolution.

Not sure how much of a wrench that throws into things, but thought I'd file.
(Assignee)

Comment 1

3 years ago
Created attachment 8650136 [details] [diff] [review]
bug970625_add_dpi_option_to_screenshot_gcli.patch

Added option to define the DPI of the image.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=3f78127328af
Assignee: nobody → jj
Status: NEW → ASSIGNED
Attachment #8650136 - Flags: review?(jryans)
Comment on attachment 8650136 [details] [diff] [review]
bug970625_add_dpi_option_to_screenshot_gcli.patch

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

::: toolkit/devtools/gcli/commands/screenshot.js
@@ +293,5 @@
>    height -= scrollbarHeight.value;
>  
>    const canvas = document.createElementNS("http://www.w3.org/1999/xhtml", "canvas");
>    const ctx = canvas.getContext("2d");
> +  const ratio = args.dpi && args.dpi > 0 ? args.dpi : window.devicePixelRatio;

I think this could be reduced to just:

    const ratio = args.dpi ? args.dpi : window.devicePixelRatio;
Attachment #8650136 - Flags: review?(jryans) → review+
(Assignee)

Comment 3

3 years ago
(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #2)
> I think this could be reduced to just:
> 
>     const ratio = args.dpi ? args.dpi : window.devicePixelRatio;

I thought so too, but it still allows for negative numbers to be passed to the dpi-parameter.

Dove a little into https://dxr.mozilla.org/mozilla-central/source/toolkit/devtools/gcli/source/lib/gcli/types/number.js#71-81
and found this line in the getMin-function
>    if (this.min) {
which will always evaluate to false given 0, and then fallback to an undefined value for getMin.
(That is: having a min-value of 0 is the same as not having a min-value at all)

So either the getMin-function should be changed to check against null rather than a false-like value, or I should pass a function to the min-property of the dpi param:
>    type: { name: "number", min: () => 0, allowFloat: true },

(Worth noting that the delay-parameter also uses 0 as a min-value, but checks that args.delay > 0 in the captureScreenshot-function)
Flags: needinfo?(jryans)
Hmm, let's ask Joe about this GCLI internal issue.  Joe, see comment 3.

Also, what's the process for modifying GCLI internals?  Is it okay to land fixes inside the `source` on m-c?
Flags: needinfo?(jryans) → needinfo?(jwalker)
r? me, land it in central, and I'll sort the rest.
Thanks for asking
Flags: needinfo?(jwalker)
Looks like the min fix just landed, so this patch can be updated whenever you've got time.
Flags: needinfo?(jj)
(Assignee)

Comment 7

3 years ago
Created attachment 8653415 [details] [diff] [review]
bug970625_add_dpi_option_to_screenshot_gcli.patch

https://treeherder.mozilla.org/#/jobs?repo=try&revision=89021ff79502
Attachment #8650136 - Attachment is obsolete: true
Flags: needinfo?(jj)
Attachment #8653415 - Flags: review?(jryans)
Comment on attachment 8653415 [details] [diff] [review]
bug970625_add_dpi_option_to_screenshot_gcli.patch

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

Great, thank you for working on this!
Attachment #8653415 - Flags: review?(jryans) → review+
(Assignee)

Updated

3 years ago
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/bfc9825d4ac7
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
status-firefox43: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 43
Release Note Request (optional, but appreciated)
[Why is this notable]: suggested by dev team
[Suggested wording]: Screenshot commands allow user choice of screen resolutions in Developer Tools
[Links (documentation, blog post, etc)]:

If you have improvements for the release note wording, please comment and needinfo me. Thanks!
relnote-firefox: --- → 43+
The command treats the provided --dpi argument as device pixel ration, so it doesn't do what it claims. It should be only relnoted once that has been fixed.
(In reply to Liz Henry (:lizzard) (needinfo? me) from comment #11)
> Release Note Request (optional, but appreciated)
> [Why is this notable]: suggested by dev team
> [Suggested wording]: Screenshot commands allow user choice of screen
> resolutions in Developer Tools
> [Links (documentation, blog post, etc)]:
> 
> If you have improvements for the release note wording, please comment and
> needinfo me. Thanks!

:lizzard, could we move the relnote to 44 because of bug 1207544?

Also, I think "Screenshot commands allow user choice of pixel ratio in Developer Tools" might be a slightly more precise wording.
Flags: needinfo?(lhenry)
Ritu, want this for 44 relnotes?
Flags: needinfo?(lhenry) → needinfo?(rkothari)
Added to Fx44 release notes.
relnote-firefox: 43+ → 44+
Flags: needinfo?(rkothari)
You need to log in before you can comment on or make changes to this bug.