Last Comment Bug 725717 - GCLI needs a 'tilt' command
: GCLI needs a 'tilt' command
Status: RESOLVED FIXED
[tilt]
: dev-doc-complete
Product: Firefox
Classification: Client Software
Component: Developer Tools: Console (show other bugs)
: unspecified
: All All
: P2 normal (vote)
: Firefox 13
Assigned To: Victor Porof [:vporof][:vp]
:
: Brian Grinstead [:bgrins]
Mentors:
Depends on: 726634
Blocks: 727529 GCLICMD
  Show dependency treegraph
 
Reported: 2012-02-09 10:12 PST by Joe Walker [:jwalker] (needinfo me or ping on irc)
Modified: 2012-06-27 11:34 PDT (History)
5 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
v1 (4.93 KB, patch)
2012-02-15 03:06 PST, Victor Porof [:vporof][:vp]
no flags Details | Diff | Splinter Review
v2 (16.70 KB, patch)
2012-02-15 07:56 PST, Victor Porof [:vporof][:vp]
no flags Details | Diff | Splinter Review
v3 (17.47 KB, patch)
2012-02-15 12:21 PST, Victor Porof [:vporof][:vp]
no flags Details | Diff | Splinter Review
v3.1 (18.08 KB, patch)
2012-02-22 09:24 PST, Victor Porof [:vporof][:vp]
jwalker: review+
Details | Diff | Splinter Review

Description Joe Walker [:jwalker] (needinfo me or ping on irc) 2012-02-09 10:12:50 PST
I'm not sure what parameters we might need.
Comment 1 Victor Porof [:vporof][:vp] 2012-02-09 10:32:12 PST
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] (needinfo me or ping on irc) 2012-02-09 11:36:19 PST
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] 2012-02-09 12:01:34 PST
(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] (needinfo me or ping on irc) 2012-02-09 13:11:22 PST
(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] 2012-02-15 03:06:08 PST
Created attachment 597356 [details] [diff] [review]
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] 2012-02-15 04:50:30 PST
(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] 2012-02-15 07:56:26 PST
Created attachment 597410 [details] [diff] [review]
v2

Added the remaining commands.
Comment 8 Joe Walker [:jwalker] (needinfo me or ping on irc) 2012-02-15 09:02:13 PST
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
Comment 9 Victor Porof [:vporof][:vp] 2012-02-15 10:26:54 PST
(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?
Comment 10 Victor Porof [:vporof][:vp] 2012-02-15 10:27:56 PST
Also, is there an easy answer to comment #6?
Comment 11 Joe Walker [:jwalker] (needinfo me or ping on irc) 2012-02-15 11:02:51 PST
(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] (needinfo me or ping on irc) 2012-02-15 11:08:38 PST
(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] 2012-02-15 12:21:39 PST
Created attachment 597516 [details] [diff] [review]
v3

Fixed stuff. Also talked with msucan and made the necessary changes for comment #6.
Comment 14 Victor Porof [:vporof][:vp] 2012-02-15 14:43:35 PST
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] 2012-02-22 09:24:31 PST
Created attachment 599652 [details] [diff] [review]
v3.1

Removed the troublesome getPredictions().length test line. Tests pass.
Comment 16 Rob Campbell [:rc] (:robcee) 2012-02-24 06:50:09 PST
https://hg.mozilla.org/integration/fx-team/rev/83a2116800d5
Comment 17 Tim Taubert [:ttaubert] 2012-02-27 01:12:42 PST
https://hg.mozilla.org/mozilla-central/rev/83a2116800d5
Comment 18 Francesco Lodolo [:flod] 2012-02-28 23:24:38 PST
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] 2012-02-28 23:39:34 PST
(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] 2012-02-28 23:40:31 PST
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] 2012-02-28 23:53:55 PST
(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] (needinfo me or ping on irc) 2012-02-29 02:06:14 PST
(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] 2012-04-28 14:35:39 PDT
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.

Note You need to log in before you can comment on or make changes to this bug.