725717 - GCLI needs a 'tilt' command

Status: RESOLVED FIXED

(DevTools :: Console, defect, P2)

Description

[Joe Walker [:jwalker]]

I'm not sure what parameters we might need.

Comment 1

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

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

Comment 2

[Joe Walker [:jwalker]]

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.

Comment 3

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

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

Comment 4

[Joe Walker [:jwalker]]

(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

Comment 5

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

Attachment: v1

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?

Comment 6

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

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

Comment 7

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

Attachment: v2

Added the remaining commands.

Comment 8

[Joe Walker [: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 <[e-mail]> (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

Comment 9

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

(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 <[e-mail]> (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?

Comment 10

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

Also, is there an easy answer to comment #6?

Comment 11

[Joe Walker [:jwalker]]

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

Comment 12

[Joe Walker [:jwalker]]

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

Comment 13

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

Attachment: v3

Fixed stuff. Also talked with msucan and made the necessary changes for comment #6.

Comment 14

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

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?

Comment 15

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

Attachment: v3.1

Removed the troublesome getPredictions().length test line. Tests pass.

Comment 16

[Rob Campbell [:rc] (:robcee)]

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

Comment 17

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/mozilla-central/rev/83a2116800d5

Comment 18

[Francesco Lodolo [:flod]]

I'd say that "ammount" is a typo (used several times). No need to change entity names to fix it.

Comment 19

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

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

Comment 20

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

I think it would be nice if the MDN doc would list these, since they're pretty fun to play with.

Comment 21

[Francesco Lodolo [:flod]]

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

Comment 22

[Joe Walker [:jwalker]]

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

Comment 23

[Eric Shepherd [:sheppy]]

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.