Closed Bug 790026 Opened 7 years ago Closed 7 years ago

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

Categories

(DevTools :: General, defect)

defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 18

People

(Reporter: Optimizer, Assigned: Optimizer)

References

Details

Attachments

(1 file, 4 obsolete files)

Attached patch Patch v1.0 (obsolete) — Splinter Review
It will be useful for Firefox developers to easily create screenshot of their patches.
Attachment #659819 - Flags: review?(jwalker)
Attached patch Patch v1.1 (obsolete) — Splinter Review
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.
Attachment #659819 - Attachment is obsolete: true
Attachment #659819 - Flags: review?(jwalker)
Attachment #659843 - Flags: review?(jwalker)
Attached patch Patch v1.2 (obsolete) — Splinter Review
fixed tests.
Attachment #659843 - Attachment is obsolete: true
Attachment #659843 - Flags: review?(jwalker)
Attachment #660127 - Flags: review?(jwalker)
Blocks: 790294
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.
Attachment #660127 - Flags: review?(jwalker) → review+
(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.
Please confirm that I am doing it correctly.
Otherwise ready for a try push.
Attachment #660127 - Attachment is obsolete: true
Attachment #660470 - Flags: review?(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.
Attachment #660470 - Flags: review?(jwalker) → review+
(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> ?
Added the Error("") line myself this time :)
Attachment #660470 - Attachment is obsolete: true
Attachment #660985 - 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/f736813af10c
Whiteboard: [land-in-fx-team] → [fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/f736813af10c
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.