575789 - JSTerm: helper functions

Status: RESOLVED FIXED

(DevTools :: General, defect)

Description

[David Dahl :ddahl]

We should start implementing various helper functions inside of the console/jsterm that will be helpful to developers.

Let's use this bug to enumerate these ideas. (We can file new bugs if needed for patches.)

The first thing I can think of is a way to find a property or value inside of an object via a keyword:

console.helpers.find("wiggleWaggle", obj)
outputs:
obj.foo.wiggleWaggle = 17
or:
no property named "wiggleWaggle" found in foo

Comment 1

[David Dahl :ddahl]

Another bit of functionality I would like to see:

console.helpers.pprint(myObject)

which will pretty print this object in the console, we can start with text-only output, which would be a huge helper.

See also the code in the "JSShell" as packaged in the Extension Developer Extension: https://addons.mozilla.org/en-US/firefox/addon/7434/

i wrote a function called "filterProps" in a JSShell fork, which quickly allowed me to find properties in Cc and Ci when working in chrome code.

http://code.google.com/p/moz-shell/source/browse/trunk/chrome/content/mozshell.js#373

Comment 2

[David Dahl :ddahl]

As Johnath said, we should treat this work as a way to inform our approach on bug 559748: "Create an API for injecting commands and code libraries into the Console input and JS Workspaces"

Comment 3

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

another list of methods we should include:

http://getfirebug.com/wiki/index.php/Command_Line_API

Comment 4

[David Dahl :ddahl]

and also, instead of using javascriptFunction() style execution, we can use more of a shell style:

find wiggleWaggle myObject

pprint myObject

which would make this more 'ergonomic'

Comment 5

[Julian Viereck]

(In reply to comment #4)
> and also, instead of using javascriptFunction() style execution, we can use
> more of a shell style:
> 
> find wiggleWaggle myObject
> 
> pprint myObject

I like this style. This also solves the problem of having a function find() that might override the current window's find() function.

(In reply to comment #3)
> another list of methods we should include:
> 
> http://getfirebug.com/wiki/index.php/Command_Line_API

Some of the functions return a value, e.g. the $(id) returns a DOMNode. Using the proposed style this could be

    b = $ id

which looks more strange and requires a parsing of the input string to do things right (you have to determ the $, pass the `id` to id and then assign the evaluated result of $(id) to b). That doesn't look that easy anymore.

Instead you could write

   b = @$(id)

and substitue @$ then by console.helpers.$. The problem with this is, you have to figure out if the @$ is inside of an string or an RegExp, where you don't want to make a substitution. 

As a start, we could supporting the `find`, `inspect` and `pprint` things, that only kick in, if they are at the beginning of the evaluated string.

Comment 6

[David Dahl :ddahl]

(In reply to comment #5)

> As a start, we could supporting the `find`, `inspect` and `pprint` things, that
> only kick in, if they are at the beginning of the evaluated string.

Yeah. simple starts are good. Of course we will need to think about how to make this extendable, perhaps with jetpack (Firefox SDK). We should propose a JEP for a "ExtendConsole" API.

Comment 7

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

(In reply to comment #4)
> and also, instead of using javascriptFunction() style execution, we can use
> more of a shell style:
> 
> find wiggleWaggle myObject
> 
> pprint myObject
> 
> which would make this more 'ergonomic'

is that preferable to $(querySelectorString)? or $$(id)? I'd still like to have these supported as they're something of a standard for console interaction in browser consoles.

Comment 8

[Johnathan Nightingale [:johnath]]

Yeah - I don't feel a need to be more ergonomic than the rest of javascript that devs are interacting with - () notation around functions and such is fine with me - preferable, even, since it avoids having to invent new syntax.

Comment 9

[Julian Viereck]

Seems like more people prefer the JS way. I've done a little bit of research on how the $ is added to the page. 

For firebug, when the panel is brought up the first time, a loop is done over all firebug commands ($, $$...). If they are already on the page, then they are skipped, otherwise they are added to the page scope.

(Chromes) WebInspector is a little bit better. As long as the $ is not defined on the page, it is only available from the command line. It isn't mixed into the page's window scope.

The WebKit way seems cleaner to me, but I'm not sure if that is easy to implement or not.

Comment 10

[David Dahl :ddahl]

(In reply to comment #8)
> Yeah - I don't feel a need to be more ergonomic than the rest of javascript
> that devs are interacting with - () notation around functions and such is fine
> with me - preferable, even, since it avoids having to invent new syntax.

Its not so much of inventing syntax as implementing a GetOptArgs-like evaluator. This idea might be better experimented with in a Jetpack.

Comment 11

[David Dahl :ddahl]

(In reply to comment #9)
> Seems like more people prefer the JS way. I've done a little bit of research on
> how the $ is added to the page. 
> 
> For firebug, when the panel is brought up the first time, a loop is done over
> all firebug commands ($, $$...). If they are already on the page, then they are
> skipped, otherwise they are added to the page scope.
> 
> (Chromes) WebInspector is a little bit better. As long as the $ is not defined
> on the page, it is only available from the command line. It isn't mixed into
> the page's window scope.
> 
> The WebKit way seems cleaner to me, but I'm not sure if that is easy to
> implement or not.

I'm sure this is doable as far as the distinct scopes are concerned. CCing mrbkap...

Comment 12

[Julian Viereck]

Attachment: First bit of iteration

IMPORTANT: This is far fetched from beeing finished but it should give an idea of the direction I'm heading. Ergo: DON'T PUSH THIS!

I took a look at how Firebug puts in the $, $$, keys functions. They do it by using an object and use evalInSandbox("with(object){" + <EVAL_STRING> + "}"). I'm using this approach in the patch but also ensure that the helper $ etc. is removed again in case the user defined a $ on the page in the meantime (Firebug doesn't check for this...).

To get the helper $ etc. in the file `helperInjection.js`'s content is read and executed in the documents sandbox. This should be quite secure as nothing touches chrome code then (the $ etc. functions are defined inside of the window and do not call anything on a chrome object).

Could anyone please give feedback if that's the right way to continue?

Comment 13

[David Dahl :ddahl]

Comment on attachment 455468 [details] [diff] [review]
First bit of iteration

After talking to mrbkap, this approach is a bit heavy handed. 

1. If we do something like: 

sandbox.$ = function JST_$ (){ // this is a closure
  var window = self.sandbox.window.wrappedJSObject;
  function _$(aSelector) {
      return window.document.querySelectorAll.apply(document, aSelector); // or whatever:)
  }
  return _$;
};

This $ will use the scope of the contentWindow, but will be wrapped in such a fashion that the contentWindow will not have any access to it, only the console itself.

The approach I think is better is that if the contentDocument has a $,  the developer can just use that, if not we add our $. (and $$). I think that the developer will expect this. btw, if we did add our $ to each document will not overwrite their $ - it can still be accessed as window.$

Cool huh? this will make this patch very short. and you can ditch loading the extra jsm via your stream method.

Just add the new code to the JSTerm prototype.

>+  // Taken from http://forums.mozillazine.org/viewtopic.php?p=921150#921150
>+  // TODO: This is a sync operation - replace with an asyn one!!!
>+  getContents: function JST_getContents(aURL){
>+    var ioService=Components.classes["@mozilla.org/network/io-service;1"]
>+      .getService(Components.interfaces.nsIIOService);
>+    var scriptableStream=Components
>+      .classes["@mozilla.org/scriptableinputstream;1"]
>+      .getService(Components.interfaces.nsIScriptableInputStream);
>+
>+    var channel=ioService.newChannel(aURL,null,null);
>+    var input=channel.open();
>+    scriptableStream.init(input);
>+    var str=scriptableStream.read(input.available());
>+    scriptableStream.close();
>+    input.close();
>+    return str;
>+  },
>+
all of this is unneeded. btw: use the Services module to get the ioService: "Services.io"

>+var _FirefoxCommandLine = {
>+  __CHECK_SCOPE_HAS_PROP_ITSELF__: function() {
>+    for (var prop in _FirefoxCommandLine) {
>+      if (typeof window[prop] != 'undefined') {
>+        delete _FirefoxCommandLine[prop];
>+      }
>+    }
>+  }
>+};
>+
>+(function() {
>+  var helper = {
>+    $: function (aId) 
>+    { 
>+      return document.getElementById.apply(document, aId);
>+    },
>+    
>+    $$: function(aSelector) {
>+      return document.querySelectorAll.apply(document, aSelector);
>+    },
>+    
>+    keys: function(aObject) {
>+      var arrKeys = [];
>+      try {
>+        for (let prop in aObject) {
>+          arrKeys.push(prop);
>+        }        
>+      } catch(e) { }
>+      return arrKeys;
>+    }
>+  };
>+  
>+  for (var prop in helper) {
>+    if (typeof window[prop] == 'undefined') {
>+      _FirefoxCommandLine[prop] = helper[prop];
>+    }
>+  }
>+})();
>

looks groovy.

Comment 14

[Julian Viereck]

(In reply to comment #13)

> sandbox.$ = function JST_$ (){ // this is a closure
>   var window = self.sandbox.window.wrappedJSObject;
>   function _$(aSelector) {
>       return window.document.querySelectorAll.apply(document, aSelector); // or
> whatever:)
>   }
>   return _$;
> };

The working version looks like this:

    this.sandbox.$ = (function() {
      let window = self._window.wrappedJSObject;
      return function () {
        window.document.getElementById.apply(window.document, arguments);
      }
    })();
    
> The approach I think is better is that if the contentDocument has a $,  the
> developer can just use that, if not we add our $.

This is true and false. The $ is not on the page, which is a big win, but if there is a $ defined on the page (jQuery, PrototypeJS), then you can't access the $ from the command line. It will always execute the $ defined above. To get around this, you have to check inside of the sandbox.$ if there is a $ defined in the window and use that one then:

    this.sandbox.$ = (function() {
      let window = self._window.wrappedJSObject;
      if (typeof window.$ != 'undefined') {
        return window.$;
      } else {
        return function () {
          window.document.getElementById.apply(window.document, arguments);
        }
      }
    })();

Nearly perfect, but if you execute `$` in the command line to get the toString() of $, this will not return the window.$.toString(). To fix this, the sandbox.$.toString function has to been overridden:

    this.sandbox.$.toString = function() {
      let window = self._window.wrappedJSObject;
      if (typeof window.$ != 'undefined') {
        return window.$.toString();
      } else {
        return (function () {
          window.document.getElementById.apply(window.document, arguments);
        }).toString();
      }
    }

The code should look much simpler when I start using a for(help in helper) loop. Patch to come ;)

Comment 15

[Julian Viereck]

Attachment: Implements helper $, $$, clear, keys, values, inspect and pprint.

Comment 16

[Julian Viereck]

Comment on attachment 455673 [details] [diff] [review]
Implements helper $, $$, clear, keys, values, inspect and pprint.

This patch is not working as the line 

    this.sandbox[help].toString

is not executed. This means that executing only the helper functions on the console ($, $$...) doesn't show up the $.toString() result of the $-function on the page.

Comment 17

[David Dahl :ddahl]

Comment on attachment 455673 [details] [diff] [review]
Implements helper $, $$, clear, keys, values, inspect and pprint.

can you re-request feedback on this for me?

Comment 18

[David Dahl :ddahl]

Comment on attachment 455673 [details] [diff] [review]
Implements helper $, $$, clear, keys, values, inspect and pprint.


>+    let helper = {
>+      $: function(aId)
please name all of these functions

>+      {
>+        return window.document.getElementById.(aId);
>+      },
>+
>+      $$: function(aSelector)
>+      {
>+        return window.document.querySelectorAll(aSelector);
>+      },
>+
>+      // Taken from WebInspector. Is this okay???
>+      $x: function(xpath, context)
>+      {
>+        var nodes = [];
>+        try {
>+            var doc = context || window.document;
spacing here is not 2 chars

Please rebase this against trunk and ask for feedback. It is so close to +

exciting stuff!

Comment 19

[Julian Viereck]

Attachment: Patch 2

Improved patch:
- fixed some bugs in the functions
- added unit tests (expect for the inspect() helper - any idea how to test this?)
- added JavaDoc
- named functions

Also:
- changed the output of the pprint function a little bit
- the helper functions are now available to the WebConsole by using a with(){} statement

Note: This patch requires the patch from bug 573102 to apply cleanly.

Comment 20

[David Dahl :ddahl]

Comment on attachment 464384 [details] [diff] [review]
Patch 2

+      try {
+        let results = doc.evaluate(aXPath, aContext, null,
+                                    Ci.nsIDOMXPathResult.ANY_TYPE, null);
+        let node;
+        while (node = results.iterateNext()) nodes.push(node);
Please use { } with all while loops - explicit bracing is better.
+      } catch (ex) {
nit: newline before catch
+        throw ex;
not sure if you want to re-throw here or Cu.reportError(ex)? and ex.stack? What do you think?
+      }
+      return nodes;
+    },

+    values: function JSTH_values(aObject)
+    {
+      var arrValues = [];
+      try {
+        for (var prop in aObject) {
+          arrValues.push(aObject[prop]);
+        }
+      } catch(e) { }
use 'ex' for exception
nit: newline before catch
perhaps we do another Cu.reportError(ex)
+      return arrValues;
+    },

r+ with changes - we can discuss the report error calls. i think that is better than allowing the console to throw.

Comment 21

[Julian Viereck]

Attachment: Patch 3

Patch is based on ddahl's feedback. I have removed the throws. This means, when an error takes place within the helper functions, these are thrown in place causing them to be displayed on the WebConsole so that the developer knows what's going on.

Also improved the pprint function to use the namesAndValuesOf function from the PropertyPanel.jsm.

Comment 22

[David Dahl :ddahl]

Comment on attachment 464791 [details] [diff] [review]
Patch 3


>+XPCOMUtils.defineLazyGetter(this, "namesAndValuesOf", function () {
>+  var obj = {};
>+  try {
>+    Cu.import("resource://gre/modules/PropertyPanel.jsm", obj);
>+  } catch (err) {

catch on newline throughout this patch:)

use 'catch (ex) {' please 

>+    Cu.reportError(err);
>+  }
>+  return obj.namesAndValuesOf;
>+});
>+
> XPCOMUtils.defineLazyGetter(this, "PropertyPanel", function () {
>   var obj = {};
>   try {
>     Cu.import("resource://gre/modules/PropertyPanel.jsm", obj);
>   } catch (err) {
ditto

nice work

Comment 23

[Julian Viereck]

Attachment: Patch v3.1

Rebased patch. Added a XPCNativeWrapper.unwrap to the JSTH_values function and added some checks for the input value to the JSTH_pprint function.

Comment 24

[Julian Viereck]

Attachment: Patch v3.2

Moved a string to the i10n property file.

Comment 25

[Kevin Dangoor]

This feature has strings, so it either needs to block beta5/6 or not get in at all.

Comment 26

[Shawn Wilsher :sdwilsh]

Comment on attachment 469461 [details] [diff] [review]
Patch v3.2

>+XPCOMUtils.defineLazyGetter(this, "namesAndValuesOf", function () {
>+  var obj = {};
>+  try {
>+    Cu.import("resource://gre/modules/PropertyPanel.jsm", obj);
>+  }
>+  catch (err) {
>+    Cu.reportError(err);
>+  }
>+  return obj.namesAndValuesOf;
>+});
lose the try-catch since your tests should catch this.

>+
> XPCOMUtils.defineLazyGetter(this, "PropertyPanel", function () {
>   var obj = {};
>   try {
>     Cu.import("resource://gre/modules/PropertyPanel.jsm", obj);
>-  } catch (err) {
>+  }
>+  catch (err) {
>     Cu.reportError(err);
>   }
>   return obj.PropertyPanel;
> });
ditto

>+function JSTermHelper(aJSTerm)
>+{
>+  return {
>+    /**
>+     *
[global] please add descriptions for all of these

>+     * @param string aId
>+     *        A string that is passed to window.document.getElementById.
>+     * @returns nsIDOMNode | null
please use English instead of pseudocode for this

>+    /**
>+     *
>+     * @param string aXPath
>+     *        xPath search query to execute.
>+     * @param nsIDOMNode [aContext]
use [optional] and not brackets around the variable name (per previous discussion about this)

r=sdwilsh

Comment 27

[Julian Viereck]

Attachment: Patch v3.3

Rebased patch improved based on Shawn's latest comments. This is ready for checking but needs approval or blocking status.

Comment 28

[Kevin Dangoor]

Note that this is a nice feature to have and it adds a string.

Comment 29

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

Comment on attachment 470446 [details] [diff] [review]
Patch v3.3

http://hg.mozilla.org/mozilla-central/rev/b80f3d724eb3

Comment 30

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

Comment on attachment 470446 [details] [diff] [review]
Patch v3.3

backed-out in:
http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1283281747.1283283872.25722.gz&fulltext=1

Comment 31

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

see:

http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1283281747.1283283872.25722.gz&fulltext=1

for details

Comment 32

[Julian Viereck]

(In reply to comment #31)
> see:
> 
> http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1283281747.1283283872.25722.gz&fulltext=1
> 
> for details

Made a try run (8a84609a2db1) and this patch doesn't seem to cause any problems. Check it back in again?

Comment 33

[David Dahl :ddahl]

http://hg.mozilla.org/mozilla-central/file/6160fe29dd37