790026 - [gcli] screenshot command could use a 'chrome' option

Status: RESOLVED FIXED

(DevTools :: General, defect)

Description

[Girish Sharma [:Optimizer]]

Attachment: Patch v1.0

It will be useful for Firefox developers to easily create screenshot of their patches.

Comment 1

[Girish Sharma [:Optimizer]]

Attachment: Patch v1.1

Changing back to content document in case of a selector option is passed, as anyways gcli does not match chrome selectors in the 'node' type and also, using chrome window to draw creates an offset which the Layout helpers do not take care of and thus wrongly positioned image is captured.

Comment 2

[Girish Sharma [:Optimizer]]

Attachment: Patch v1.2

fixed tests.

Comment 3

[Joe Walker [:jwalker]]

Comment on attachment 660127 [details] [diff] [review]
Patch v1.2

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

r+ with this problem solved.

::: browser/devtools/commandline/CmdScreenshot.jsm
@@ +72,5 @@
>    ],
>    exec: function Command_screenshot(args, context) {
> +    var document = args.chrome && !args.selector
> +                   ? context.environment.chromeDocument
> +                   : context.environment.contentDocument;

Hmmm, so if you do:

>> screenshot --chome --selector X

Then we're going to suddenly ignore the chrome flag? That sounds surprising to me.

I'm not sure why we can't make selector work with a chrome document?

If we really can't get it to work then we should throw an exception, and add a note to bug 659268, so say that this is a place where we could use the features of that bug.

Comment 4

[Girish Sharma [:Optimizer]]

(In reply to Joe Walker [:jwalker] from comment #3)
> Hmmm, so if you do:
> 
> >> screenshot --chome --selector X
> 
> Then we're going to suddenly ignore the chrome flag? That sounds surprising
> to me.

1. The chrome selector was not recognizing the selector #webConsole and many other that I tried.
2. While having the chrome suffix, the selector was still checking for content nodes and while capturing a content node, a offset of the height of the window above the content (mainly the toolbars and title bar) comes into picture and many times the node itself is totally missed.

> I'm not sure why we can't make selector work with a chrome document?

This is a GCLI feature for a 'node' type. I don;t know how to tell the 'node; type to look for a chrome node, or even if it is possible right now.

> If we really can't get it to work then we should throw an exception, and add
> a note to bug 659268, so say that this is a place where we could use the
> features of that bug.

Not really clear what you mean here.

Comment 5

[Girish Sharma [:Optimizer]]

Attachment: Throwing when chrome and selector both are used

Please confirm that I am doing it correctly.
Otherwise ready for a try push.

Comment 6

[Joe Walker [:jwalker]]

Comment on attachment 660470 [details] [diff] [review]
Throwing when chrome and selector both are used

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

::: browser/devtools/commandline/CmdScreenshot.jsm
@@ +74,5 @@
> +    if (args.chrome && args.selector) {
> +      // Node screenshot with chrome option does not work as inteded
> +      // Refer https://bugzilla.mozilla.org/show_bug.cgi?id=659268#c7
> +      // throwing for now.
> +      throw gcli.lookup("screenshotSelectorChromeConflict");

The code above will work (I think), but I was expecting:

throw new Error(gcli.lookup("screenshotSelectorChromeConflict"));

I've got to edit the patch to add r=jwalker to the end, so I'm happy to do this.

Comment 7

[Girish Sharma [:Optimizer]]

(In reply to Joe Walker [:jwalker] from comment #6)
> Comment on attachment 660470 [details] [diff] [review]
> Throwing when chrome and selector both are used
> 
> Review of attachment 660470 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: browser/devtools/commandline/CmdScreenshot.jsm
> @@ +74,5 @@
> > +    if (args.chrome && args.selector) {
> > +      // Node screenshot with chrome option does not work as inteded
> > +      // Refer https://bugzilla.mozilla.org/show_bug.cgi?id=659268#c7
> > +      // throwing for now.
> > +      throw gcli.lookup("screenshotSelectorChromeConflict");
> 
> The code above will work (I think), but I was expecting:
> 
> throw new Error(gcli.lookup("screenshotSelectorChromeConflict"));
> 
> I've got to edit the patch to add r=jwalker to the end, so I'm happy to do
> this.

My, bad :(

Does every reviewer has to edit the patch to add r=<reviewer> ?

Comment 8

[Girish Sharma [:Optimizer]]

Attachment: rebased patch. no changes.

Added the Error("") line myself this time :)

Comment 9

[Girish Sharma [:Optimizer]]

Please follow this order of pushing : 

bug 788890 , then bug 790026 and then bug 790294

Comment 10

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

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

Comment 11

[Tim Taubert [:ttaubert] (inactive)]

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