Closed Bug 790294 Opened 7 years ago Closed 7 years ago

GCLI screenshot command should show preview.

Categories

(DevTools :: General, defect)

x86_64
Windows 7
defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 18

People

(Reporter: Optimizer, Assigned: Optimizer)

References

Details

Attachments

(1 file, 2 obsolete files)

Attached patch patch v1.0 (obsolete) — Splinter Review
It would be nice if the image save to the file would be visible as preview along the text indicating the same.
Attachment #660128 - Flags: review?(jwalker)
Comment on attachment 660128 [details] [diff] [review]
patch v1.0

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

r+ with one change.
Thanks for this patch - it's a nice touch that will make people go 'oooh'.

::: browser/devtools/commandline/CmdScreenshot.jsm
@@ +220,5 @@
> +                       "min-height:" + previewHeight + "px !important;" +
> +                       "height:" + previewHeight + "px !important;" +
> +                       "background-image: url('" + data + "') !important;" +
> +                       "background-size: 256px " + previewHeight + "px !important;" +
> +                       "margin: 4px !important; display: block !important");

I don't think we should need all the '!important's. The specificity of an element's style node should make it win against all.
Attachment #660128 - Flags: review?(jwalker) → review+
(In reply to Joe Walker [:jwalker] from comment #1)
> I don't think we should need all the '!important's. The specificity of an
> element's style node should make it win against all.

Actually, without the !important s, the style was not getting applied on the image. I also don't want so much !important s. I will see what all can I get rid of.
Attached patch remove !important (obsolete) — Splinter Review
Ready for try.
Attachment #660128 - Attachment is obsolete: true
Attachment #660469 - Flags: review+
Comment on attachment 660469 [details] [diff] [review]
remove !important

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

::: browser/devtools/commandline/CmdScreenshot.jsm
@@ +219,5 @@
>      div.style.cursor = "pointer";
> +    let image = document.createElement("div");
> +    let previewHeight = parseInt(256*height/width);
> +    image.setAttribute("style",
> +                       "max-width:256px; min-width:256px;" +

Just curious here, why set max-width and min-width and not just width?

@@ +222,5 @@
> +    image.setAttribute("style",
> +                       "max-width:256px; min-width:256px;" +
> +                       "max-height:" + previewHeight + "px;" +
> +                       "min-height:" + previewHeight + "px;" +
> +                       "height:" + previewHeight + "px;" +

max-height and min-height and height?
Ready to land :)
Attachment #660469 - Attachment is obsolete: true
Attachment #660986 - Flags: review+
Please follow this order of pushing : 

bug 788890 , then bug 790026 and then bug 790294
Whiteboard: [land-in-fx-team]
https://hg.mozilla.org/integration/fx-team/rev/dd4d761d670d
Whiteboard: [land-in-fx-team] → [fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/dd4d761d670d
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 18
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.