790294 - GCLI screenshot command should show preview.

Status: RESOLVED FIXED

(DevTools :: General, defect)

Description

[Girish Sharma [:Optimizer]]

Attachment: patch v1.0

It would be nice if the image save to the file would be visible as preview along the text indicating the same.

Comment 1

[Joe Walker [: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.

Comment 2

[Girish Sharma [:Optimizer]]

(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.

Comment 3

[Girish Sharma [:Optimizer]]

Attachment: remove !important

Ready for try.

Comment 4

[Victor Porof [:vporof][:vp]]

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?

Comment 5

[Girish Sharma [:Optimizer]]

Attachment: removing max/min width/height

Ready to land :)

Comment 6

[Girish Sharma [:Optimizer]]

Please follow this order of pushing : 

bug 788890 , then bug 790026 and then bug 790294

Comment 7

[Victor Porof [:vporof][:vp]]

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

Comment 8

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/mozilla-central/rev/dd4d761d670d