1143497 - Offer a way to extend WebConsole commands

Status: RESOLVED FIXED

(DevTools :: Console, defect)

Description

[Florent Fayolle]

User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:37.0) Gecko/20100101 Firefox/37.0
Build ID: 20150312193711

Steps to reproduce:

There is currently no API to allow Devtools extension developers to extend the commands of the evaluated expressions in the WebConsole.

Florent

Comment 1

[Florent Fayolle]

I have not looked much at that issue, but I would probably raise an event at the end of |JSTermHelpers| with the default bindings, so extensions developers can reuse them.
https://dxr.mozilla.org/mozilla-central/source/toolkit/devtools/webconsole/utils.js#1524

I don't know yet if that's a good idea to allow them wrap/redefine them (that can allow them extend a behavior but also create a mess if they all do that in a dirty manner).

Florent

Comment 2

[Jan Honza Odvarko [:Honza] (always need-info? me)]

@Brian, what do you think?

Honza

Comment 3

[Brian Grinstead [:bgrins]]

These are defined in JSTermHelpers: https://dxr.mozilla.org/mozilla-central/source/toolkit/devtools/webconsole/utils.js#1524.

That object is created in evalWithDebuger: https://dxr.mozilla.org/mozilla-central/source/toolkit/devtools/server/actors/webconsole.js#1114.  There is another instance created once and cached for autocompletion in the prompt: https://dxr.mozilla.org/mozilla-central/source/toolkit/devtools/server/actors/webconsole.js#874.

If you look at the constructor of JSTermHelpers, all it really does is define properties on the sandbox object passed in.  These come in different forms - functions like '$$' are easy since they can just return an object from the owner's window.  Some are more complicated, like 'inspect', which returns a helper result so that the frontend can do some custom processing with it: https://dxr.mozilla.org/mozilla-central/source/browser/devtools/webconsole/webconsole.js#3337.  It depends on what you need these to do: 

1) Functions that return a result to be displayed in the webconsole should be relatively easy to extend.. we could provide some mechanism to register a JSTermHelper extension that would be called at the end of the constructor.  It could look something like:

  JSTermHelpers.extend(aOwner => {
    aOwner.sandbox.foobar = arg1 => { return arg1 + "foobar"; }

    Object.defineProperty(aOwner.sandbox, "$1", {
      get: function() {
        return aOwner.makeDebuggeeValue(aOwner.selectedNode)
      }
    });
  });

2) Functions that need to some custom processing on the frontend (like add a button to a result, change what messages are displayed, copy text to the clipboard, etc) we would need an alternate plan.  Is this a requirement?

Comment 4

[Florent Fayolle]

Attachment: 1143497.patch (WIP)

I have started implementing it in the backend. I still have to implement it in the Front-End.

> It depends on what you need these to do: 

I think of being the most generic possible, so it can be reused by any extension (and through the Firebug SDK for example).

> 2) Functions that need to some custom processing on the frontend (like add a button to a result, change what messages are displayed, copy text to the clipboard, etc) we would need an alternate plan.  Is this a requirement?

Probably yes. In our case, we would like to reimplement Firebug command lines (https://github.com/firebug/firebug.next/issues/361), such as the include() command (which displays a throbber and notice back when the script is loaded).

For now, I have nothing clear in mind on how to handle RDP for that.

Florent

Comment 5

[Florent Fayolle]

Attachment: 1143497.patch

Finally, as Honza suggested, I think we should only focus on server-side APIs.

I just removed the two commands I created to test, and run the webconsole tests (all of them passed).

Brian, could you review please?

Florent

Comment 6

[Brian Grinstead [:bgrins]]

Comment on attachment 8582705 [details] [diff] [review]
1143497.patch

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

Good start - see comments below.  Plus, we need a test in toolkit/devtools/webconsole/tests/unit -- see test_consoleID.js as a starting template to base it off of

::: toolkit/devtools/server/actors/webconsole.js
@@ +31,5 @@
>    return require("sdk/event/core");
>  });
>  
>  for (let name of ["WebConsoleUtils", "ConsoleServiceListener",
> +                  "ConsoleAPIListener", "WebConsoleCommands", "JSPropertyProvider",

Nit: > 80 characters on this line

@@ +875,1 @@
>          this._jstermHelpersCache = Object.getOwnPropertyNames(helpers.sandbox);

The _jstermHelpersCache variable should be renamed

::: toolkit/devtools/webconsole/utils.js
@@ +911,5 @@
>      if (!prop) {
>        return null;
>      }
>  
> +    if (/\[\d+\]$/.test(prop)) {

I can't tell what changed on this line, seems unnecessary

@@ +1515,2 @@
>   *
>   * A list of helper functions used by Firebug can be found here:

The diff becomes a lot better if you move this comment to before the call to WebConsoleCommands.register("$")

@@ +1584,3 @@
>  
>    /**
> +   * (Internal only) Add the bindings to |owner.sandbox|.

Just a thought - we could export a function from this module called addWebConsoleCommands that does the same thing instead of making it a private method.  Since webconsole.js only uses WebConsoleCommands to call _addBindings, that file could be updated to only import this new function and we wouldn't have to worry about making this 'private'.  What do you think?

@@ +1600,5 @@
> +          // WebConsoleActor can reconfigure the property).
> +          enumerable: true,
> +          configurable: true
> +        });
> +        for (let funcName of ["get", "set"]) {

Nit: I think something like this is more straightforward instead of this funcName loop:

if (typeof command.get === "function") {
	command.get = command.get.bind(undefined, owner);
}
if (typeof command.set === "function") {
	command.set = command.set.bind(undefined, owner);
}

@@ +1611,2 @@
>        }
>        else {

This should check to make sure the command is a function.  And nit: please handle if (typeof command === "function") first since it's a one-liner.

if (typeof command === "function") {
  owner.sandbox[name] = command.bind(undefined, owner);
} else if (typeof command === "object") {
  ....
}

@@ +1638,5 @@
> + *        A string that is passed to window.document.querySelectorAll.
> + * @return nsIDOMNodeList
> + *         Returns the result of document.querySelectorAll(aSelector).
> + */
> +WebConsoleCommands.register("$$", function JSTH_$$(aOwner, aSelector)

To me, it seems undesirable to let someone override a built in command (like '$$' or 'help').  Thoughts?  If so, we could store all of these builtins in a Map local to this file and just extend the this._registeredCommands map with it in _addBindings so that the built in commands always win.

Comment 7

[Florent Fayolle]

Attachment: 1143497.patch (TODO Tests)

Thanks for your feedback!

> Nit: > 80 characters on this line

Done.

> The _jstermHelpersCache variable should be renamed

Done.

> I can't tell what changed on this line, seems unnecessary

I replaced an irregular whitespace. 

> The diff becomes a lot better if you move this comment to before 
> the call to WebConsoleCommands.register("$")

Done.

> Just a thought - we could export a function from this module called addWebConsoleCommands that does 
> the same thing instead of making it a private method.  Since webconsole.js only uses 
> WebConsoleCommands to call _addBindings, that file could be updated to only import this new function 
> and we wouldn't have to worry about making this 'private'.  What do you think?

Sounds good. Done.

> Nit: I think something like this is more straightforward instead of this funcName loop:

Done.

> This should check to make sure the command is a function.  
> And nit: please handle if (typeof command === "function") first since it's a one-liner.

Done.

> To me, it seems undesirable to let someone override a built in command (like '$$' or 'help'). 

As we exchanged through IRC: even if that's a danger:
- Addon developers can already hack as they want (they can hack by changing _jstermHelpersCache/_webConsoleCommandsCache). We have to trust them to be good citizens :).
- There might be use-cases where an addon developer wants to extend a command (wrap it). Typically: if they want to customize the string being copies with copy.

That makes me think that a good Firebug.SDK API could be |WebConsoleCommands.wrapExisting("copy", function (orig, ...args) { ... });|.

-----

TODO : tests.

Florent

Comment 8

[Florent Fayolle]

Attachment: 1143497.patch

Test done. While running all the test of the DevTools:
- Tests of toolkit/devtools are OK
- Tests of browser/devtools have lots of KOs

I'll see what causes them (maybe running tests individually will work).

Florent

Comment 9

[Florent Fayolle]

Attachment: 1143497.patch

OK, I tested again with only browser/devtools/webconsole and a few of browser/devtools/inspector, and they look to be run finely.

I am ready for your review Brian :).

Florent

Comment 10

[Florent Fayolle]

Attachment: logs_webconsole.txt

(the logs of the tests I run, for traceability)

Florent

Comment 11

[Brian Grinstead [:bgrins]]

Comment on attachment 8586999 [details] [diff] [review]
1143497.patch

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

This needs rebasing after Bug 792063

::: toolkit/devtools/webconsole/test/test_commands_registration.html
@@ +24,5 @@
> +function evaluateJS(input, callback, evaluatingSync) {
> +  gState.client.evaluateJS(input, callback);
> +}
> +
> +function startTest()

Since you started this patch, I've landed a test in this folder (bug 792063) that does some similar things using newer style testing conventions.  See https://dxr.mozilla.org/mozilla-central/source/toolkit/devtools/webconsole/test/test_jsterm_last_result.html, specifically the evaluateJS function which returns a promise and the test functions which use Task.async.  This makes the tests easier to read and write -- can you update this to match that?

@@ +35,5 @@
> +function onAttach(aState, aResponse)
> +{
> +  gState = aState;
> +
> +  let tests = Object.keys(window).filter(x => x.startsWith("test_"))

I'd prefer to see the tests listed here in a hardcoded array.  I know it's repeating things and ugly, but if for some reason a test name got changed it could all of a sudden not run anymore without any indication.  At least with it duplicated we would get an exception in that case.

@@ +84,5 @@
> +  evaluateJS("cd('>o_/')", aResponse => {
> +    checkObject(aResponse, {
> +      from: gState.actor,
> +      result: "bang!"
> +    });

Can you also add an assertion that calls cd() without the special arg and make sure that the default behavior happens?  May be easier to do that assertion if you wrap a different function than cd.

@@ +138,5 @@
> +  // Otherwise, end the test.
> +  closeDebugger(gState, function() {
> +    gState = null;
> +    if (evaluatingSync) {
> +      evaluatingSync = false;

We don't need to worry about running this in sync/async modes.  You can just copy the testEnd from test_jsterm_last_result.html

::: toolkit/devtools/webconsole/utils.js
@@ +1588,4 @@
>   *
>   * A list of helper functions used by Firebug can be found here:
>   *   http://getfirebug.com/wiki/index.php/Command_Line_API
> + */

I wish this diff was less noisy - I guess because the indentation of each function call is changing it's just not able to figure out what is going on.  Oh well

Comment 12

[Florent Fayolle]

> This makes the tests easier to read and write -- can you update this to match that?

Sounds a good idea.

> I'd prefer to see the tests listed here in a hardcoded array.

The reason why I made this way was that I don't have to matter about managing a list of functions. I am relatively scatterbrained and can miss adding the function to a list.

Anyway, that's not that important, and I give you the last word if you still think that an explicit list is a better idea.

> Can you also add an assertion that calls cd() without the special arg and make sure that the default 
> behavior happens?  May be easier to do that assertion if you wrap a different function than cd.

Yeah, I was thinking the same. You're right.

> We don't need to worry about running this in sync/async modes.  You can just copy the testEnd from test_jsterm_last_result.html

Sure

> I wish this diff was less noisy - I guess because the indentation of each function call 
> is changing it's just not able to figure out what is going on.  Oh well

Have you taken a look at these diff (hg diff / git diff) options:

 -w --ignore-all-space    ignore white space when comparing lines
 -b --ignore-space-change ignore changes in the amount of white space

Florent

Comment 13

[Florent Fayolle]

PS: I'll take your remarks into account this evening (UTC+1 ; means in ~12 hours)

Florent

Comment 14

[Florent Fayolle]

Attachment: 1143497.patch

I have updated the patch with your remarks and merged with the latest changes.

A little difference with what you asked: I made a global |test| array of functions in my tests, which I think is a good compromise. Is that OK for you?

Note that all tests of toolkit/devtools/webconsole/test and toolkit/devtools/tests pass but I have lots of errors when running those of browser/devtools/webconsole/ (even without the patch). Do you think that's fine enough?

Florent

Comment 15

[Brian Grinstead [:bgrins]]

Comment on attachment 8587668 [details] [diff] [review]
1143497.patch

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

Looks good, here is a try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=a5bd3ad557e1

::: toolkit/devtools/webconsole/test/test_commands_registration.html
@@ +18,5 @@
> +let gState;
> +let tests;
> +
> +let {devtools} = Cu.import("resource://gre/modules/devtools/Loader.jsm", {});
> +let { WebConsoleCommands } = devtools.require("devtools/toolkit/webconsole/utils");

Nit: Use consistent whitespace (no spaces between brackets and imported function)

@@ +20,5 @@
> +
> +let {devtools} = Cu.import("resource://gre/modules/devtools/Loader.jsm", {});
> +let { WebConsoleCommands } = devtools.require("devtools/toolkit/webconsole/utils");
> +
> +let evaluatingSync = true;

Nit: remove unused evaluatingSync variable

Comment 16

[Florent Fayolle]

Attachment: 1143497.patch

Nits fixed. Thanks :bgrins.

Florent

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/fx-team/rev/79e51bf97839

Comment 18

[Ekanan Ketunuti]

https://hg.mozilla.org/mozilla-central/rev/79e51bf97839