Closed Bug 1143497 Opened 5 years ago Closed 5 years ago

Offer a way to extend WebConsole commands

Categories

(DevTools :: Console, defect)

39 Branch
defect
Not set

Tracking

(firefox40 fixed)

RESOLVED FIXED
Firefox 40
Tracking Status
firefox40 --- fixed

People

(Reporter: fayolle-florent, Assigned: fayolle-florent)

References

Details

Attachments

(2 files, 6 obsolete files)

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
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
Component: Untriaged → Developer Tools: Console
OS: Linux → All
Hardware: x86_64 → All
@Brian, what do you think?

Honza
Flags: needinfo?(bgrinstead)
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?
Flags: needinfo?(bgrinstead)
Attached patch 1143497.patch (WIP) (obsolete) — Splinter Review
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
Attached patch 1143497.patch (obsolete) — Splinter Review
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
Attachment #8581298 - Attachment is obsolete: true
Attachment #8582705 - Flags: review?(bgrinstead)
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.
Attachment #8582705 - Flags: review?(bgrinstead)
Assignee: nobody → fayolle-florent
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Attached patch 1143497.patch (TODO Tests) (obsolete) — Splinter Review
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
Attachment #8582705 - Attachment is obsolete: true
Attached patch 1143497.patch (obsolete) — Splinter Review
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
Attachment #8583289 - Attachment is obsolete: true
Attached patch 1143497.patch (obsolete) — Splinter Review
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
Attachment #8586981 - Attachment is obsolete: true
Attachment #8586999 - Flags: review?(bgrinstead)
Attached file logs_webconsole.txt
(the logs of the tests I run, for traceability)

Florent
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
Attachment #8586999 - Flags: review?(bgrinstead) → feedback+
> 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
PS: I'll take your remarks into account this evening (UTC+1 ; means in ~12 hours)

Florent
Attached patch 1143497.patch (obsolete) — Splinter Review
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
Attachment #8586999 - Attachment is obsolete: true
Attachment #8587668 - Flags: review?(bgrinstead)
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
Attachment #8587668 - Flags: review?(bgrinstead) → review+
Attached patch 1143497.patchSplinter Review
Nits fixed. Thanks :bgrins.

Florent
Attachment #8587668 - Attachment is obsolete: true
Attachment #8588630 - Flags: review+
Blocks: 1151610
https://hg.mozilla.org/integration/fx-team/rev/79e51bf97839
Flags: in-testsuite+
Keywords: checkin-needed
Whiteboard: [fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/79e51bf97839
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 40
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.