Closed Bug 725717 Opened 10 years ago Closed 9 years ago

GCLI needs a 'tilt' command

Categories

(DevTools :: Console, defect, P2)

defect

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 13

People

(Reporter: jwalker, Assigned: vporof)

References

Details

(Keywords: dev-doc-complete, Whiteboard: [tilt])

Attachments

(1 file, 3 obsolete files)

I'm not sure what parameters we might need.
We could have a param for the target node. If there's no target node specified, we're basically also fixing (or providing an alternative to 718058).
Whiteboard: [tilt]
Worth reading:
  https://github.com/mozilla/gcli/blob/master/docs/writing-commands.md

I think you're going to need something like this:

/**
 * 'tilt' command
 */
gcli.addCommand({
  name: "tilt",
  description: gcli.lookup("tiltDesc"),
  manual: gcli.lookup("tiltManual"),
  params: [
    {
      name: "node",
      type: "node",
      defaultValue: null,
      description: gcli.lookup("tiltNodeDesc"),
      manual: gcli.lookup("tiltNodeManual")
    }
  ],
  exec: function Command_inspect(args, context) {
    if (args.node == null) {
      // Open tilt without a highlighted node
    }
    else {
      // Open tilt with a highlighted node
      // args.node will be a DOM element
    }
  }
});

It's possible that we might want a set of commands though ...

> tilt open div#id
> tilt close
> tilt rotate 90
> tilt zoom 2

But maybe that's going too far. I guess it depends on what tilt can do.
(In reply to Joe Walker from comment #2)
> 
> But maybe that's going too far. I guess it depends on what tilt can do.

Tilt can basically do everything the inspector does + the 3D extravaganza. I think having commands for rotate/translate/zoom/transform is an excellent idea! We could also add support for reset, focus, delete etc. Basically, the way I see it, everything you can do with Tilt using the mouse or keyboard could be done using a command. The implementation should be extremely straight forward, as Tilt provides an API for all of this.

Is writing these commands in GcliCommands.jsm a good idea, or can/should these properties be added somewhere else? I would prefer the former.
Assignee: nobody → vporof
Status: NEW → ASSIGNED
(In reply to Victor Porof from comment #3)
> (In reply to Joe Walker from comment #2)
> > 
> > But maybe that's going too far. I guess it depends on what tilt can do.
> 
> Tilt can basically do everything the inspector does + the 3D extravaganza. I
> think having commands for rotate/translate/zoom/transform is an excellent
> idea! We could also add support for reset, focus, delete etc. Basically, the
> way I see it, everything you can do with Tilt using the mouse or keyboard
> could be done using a command. The implementation should be extremely
> straight forward, as Tilt provides an API for all of this.
> 
> Is writing these commands in GcliCommands.jsm a good idea, or can/should
> these properties be added somewhere else? I would prefer the former.

The down side of putting all the files in one place is that you need frequent rebasing as other people add to GcliCommands.jsm.

Generally I add simple commands to GcliCommands.jsm (which is everything committed so far) but 'pref' which is 3 commands and a new type, went into its own file. See here for work in progress:

https://bugzilla.mozilla.org/attachment.cgi?id=595861
Attached patch v1 (obsolete) — Splinter Review
I'll need to close the console when starting Tilt. The reason for it is: when resizing from a smaller to larger dimension, the canvas looses detail and Tilt doesn't assign new resolutions for this because it would cause flickering; also, I bet most users will close the webconsole after using the Tilt command, and since Tilt is immune to reflows and repaints, they'll be left with a smaller inaccurate version of what they actually want to visualize.

Is this something that's ok to do?
Attachment #597356 - Flags: feedback?(jwalker)
(In reply to Victor Porof from comment #5)
> Created attachment 597356 [details] [diff] [review]
> v1
> 
> I'll need to close the console when starting Tilt.

However, this isn't necessary when the webconsole is in it's own window. Is there an easy way to determine that?
Depends on: 726634
Attached patch v2 (obsolete) — Splinter Review
Added the remaining commands.
Attachment #597410 - Flags: review?(jwalker)
Comment on attachment 597410 [details] [diff] [review]
v2

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

::: browser/devtools/webconsole/GcliTiltCommands.jsm
@@ +18,5 @@
> + * Portions created by the Initial Developer are Copyright (C) 2011
> + * the Initial Developer. All Rights Reserved.
> + *
> + * Contributor(s):
> + *   Joe Walker <jwalker@mozilla.com> (original author)

I think you need to go here

@@ +35,5 @@
> + *
> + * ***** END LICENSE BLOCK ***** */
> +
> +
> +let EXPORTED_SYMBOLS = [ "GcliCommands" ];

I don't think you're actually exporting anything here, so probably [ ] is better.

@@ +127,5 @@
> +  manual: gcli.lookup("tiltRotateManual"),
> +  params: [
> +    {
> +      name: "x",
> +      type: { name: 'number', min: -360, max: 360 },

It might be worth adding step:10, so up/down arrow move to the right place faster

@@ +168,5 @@
> +  manual: gcli.lookup("tiltZoomManual"),
> +  params: [
> +    {
> +      name: "zoom",
> +      type: { name: 'number', min: -Number.MAX_VALUE },

Hmm - I'm not sure if this value for min is helpful (unless I missed something), and could make things downright confusing if we ever use min as a default or a slider boundary or something
(In reply to Joe Walker from comment #8)
> Comment on attachment 597410 [details] [diff] [review]
> v2
> 
> Review of attachment 597410 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: browser/devtools/webconsole/GcliTiltCommands.jsm
> @@ +18,5 @@
> > + * Portions created by the Initial Developer are Copyright (C) 2011
> > + * the Initial Developer. All Rights Reserved.
> > + *
> > + * Contributor(s):
> > + *   Joe Walker <jwalker@mozilla.com> (original author)
> 
> I think you need to go here
> 

Hello copy paste :) Fixed.

> @@ +35,5 @@
> > + *
> > + * ***** END LICENSE BLOCK ***** */
> > +
> > +
> > +let EXPORTED_SYMBOLS = [ "GcliCommands" ];
> 
> I don't think you're actually exporting anything here, so probably [ ] is
> better.
> 

Agreed. But then, why is there a EXPORTED_SYMBOLS = [ "GcliCommands" ]; in GcliCommands.jsm?

> @@ +127,5 @@
> > +  manual: gcli.lookup("tiltRotateManual"),
> > +  params: [
> > +    {
> > +      name: "x",
> > +      type: { name: 'number', min: -360, max: 360 },
> 
> It might be worth adding step:10, so up/down arrow move to the right place
> faster
> 

Agreed.

> @@ +168,5 @@
> > +  manual: gcli.lookup("tiltZoomManual"),
> > +  params: [
> > +    {
> > +      name: "zoom",
> > +      type: { name: 'number', min: -Number.MAX_VALUE },
> 
> Hmm - I'm not sure if this value for min is helpful (unless I missed
> something), and could make things downright confusing if we ever use min as
> a default or a slider boundary or something

In that case, I can't input a negative value for zoom, which is needed for this command to work properly. Is there a way to assure that 'number' accepts negative values?
Also, is there an easy answer to comment #6?
(In reply to Victor Porof from comment #6)
> (In reply to Victor Porof from comment #5)
> > I'll need to close the console when starting Tilt.
> 
> However, this isn't necessary when the webconsole is in it's own window. Is
> there an easy way to determine that?

I don't know, off the top of my head. IIRC there is a way, but I'd need to look through the code. Mihai actually wrote it, if you have trouble finding it.
(In reply to Victor Porof from comment #9)
> (In reply to Joe Walker from comment #8)
> > Comment on attachment 597410 [details] [diff] [review]
...
> > @@ +168,5 @@
> > > +  manual: gcli.lookup("tiltZoomManual"),
> > > +  params: [
> > > +    {
> > > +      name: "zoom",
> > > +      type: { name: 'number', min: -Number.MAX_VALUE },
> > 
> > Hmm - I'm not sure if this value for min is helpful (unless I missed
> > something), and could make things downright confusing if we ever use min as
> > a default or a slider boundary or something
> 
> In that case, I can't input a negative value for zoom, which is needed for
> this command to work properly. Is there a way to assure that 'number'
> accepts negative values?

We need to fix bug 727535 that I just raised.
I suggest taking min: -Number.MAX_VALUE out and pointing anyone that complains to this bug. It should be quick to fix.
Attached patch v3 (obsolete) — Splinter Review
Fixed stuff. Also talked with msucan and made the necessary changes for comment #6.
Attachment #597356 - Attachment is obsolete: true
Attachment #597410 - Attachment is obsolete: true
Attachment #597356 - Flags: feedback?(jwalker)
Attachment #597410 - Flags: review?(jwalker)
Attachment #597516 - Flags: review?(jwalker)
Blocks: 727529
I seem to get errors when testing:

TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/devtools/webconsole/test/browser_gcli_web.js |
Stack trace:
    JS frame :: chrome://mochitests/content/browser/browser/devtools/webconsole/test/browser_gcli_web.js :: <TOP_LEVEL> :: line 1106
    JS frame :: chrome://mochitests/content/browser/browser/devtools/webconsole/test/browser_gcli_web.js :: <TOP_LEVEL> :: line 367
    JS frame :: chrome://mochitests/content/browser/browser/devtools/webconsole/test/browser_gcli_web.js :: <TOP_LEVEL> :: line 286
    JS frame :: chrome://mochitests/content/browser/browser/devtools/webconsole/test/browser_gcli_web.js :: <TOP_LEVEL> :: line 287
    JS frame :: chrome://mochitests/content/browser/browser/devtools/webconsole/test/browser_gcli_web.js :: <TOP_LEVEL> :: line 149
    JS frame :: chrome://mochitests/content/browser/browser/devtools/webconsole/test/browser_gcli_web.js :: <TOP_LEVEL> :: line 150
    JS frame :: chrome://mochitests/content/browser/browser/devtools/webconsole/test/browser_gcli_web.js :: <TOP_LEVEL> :: line 66
    JS frame :: chrome://mochitests/content/browser/browser/devtools/webconsole/test/browser_gcli_web.js :: onLoad :: line 2204

Which is on the line
test.ok(assignC.getPredictions().length < 20); // could break ...

So at least it was predictable :) Joe?
Attached patch v3.1Splinter Review
Removed the troublesome getPredictions().length test line. Tests pass.
Attachment #597516 - Attachment is obsolete: true
Attachment #597516 - Flags: review?(jwalker)
Attachment #599652 - Flags: review?(jwalker)
Attachment #599652 - Flags: review?(jwalker) → review+
Whiteboard: [tilt] → [tilt][land-in-fx-team]
https://hg.mozilla.org/integration/fx-team/rev/83a2116800d5
Whiteboard: [tilt][land-in-fx-team] → [tilt][fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/83a2116800d5
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Whiteboard: [tilt][fixed-in-fx-team] → [tilt]
Target Milestone: --- → Firefox 13
I'd say that "ammount" is a typo (used several times). No need to change entity names to fix it.
(In reply to Francesco Lodolo [:flod] from comment #18)
> I'd say that "ammount" is a typo (used several times). No need to change
> entity names to fix it.

Yes, typo. Follow-up bug?
I think it would be nice if the MDN doc would list these, since they're pretty fun to play with.
Keywords: dev-doc-needed
(In reply to Victor Porof from comment #19)
> (In reply to Francesco Lodolo [:flod] from comment #18)
> > I'd say that "ammount" is a typo (used several times). No need to change
> > entity names to fix it.
> 
> Yes, typo. Follow-up bug?

Bug 731523 (not sure how dependencies should be set)
(In reply to Francesco Lodolo [:flod] from comment #21)
> Bug 731523 (not sure how dependencies should be set)

I think you did the right thing in leaving out the dependency btw. While that bug does depend on this bug in that you can't apply that patch without this patch, we don't generally list all such dependencies (which would clearly be a very very long list) - so we can assume dependence on landed patches.
The existence of this command is documented here:

https://developer.mozilla.org/en/Tools/GCLI

I will leave it to "help tilt" to be more specific.
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.