767900 - GCLI needs a command to enable execution of XUL Commands

Status: RESOLVED INCOMPLETE

(DevTools Graveyard :: Graphic Commandline and Toolbar, defect)

Description

[Joe Walker [:jwalker]]

i.e. https://developer.mozilla.org/en/XUL_Tutorial/Commands

Comment 1

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Filter on teabags

Comment 2

[Joe Walker [:jwalker]]

https://gist.github.com/3048788

Comment 3

[Joe Walker [:jwalker]]

Triage, filter on TEABAGS

Comment 4

[Joe Walker [:jwalker]]

Triage: Filter on the TRIAGE keyword.

Comment 5

[Joe Walker [:jwalker]]

Triage

Comment 6

[saran]

Hi Joe.. I would like to help with this bug... Could you tell me what I am supposed to do ?

Comment 7

[Joe Walker [:jwalker]]

Step 1: Create a new command that looks something like this:
>> xulcmd <name>

<name> will be a selection type, and the options are xul commands. You should be able to get a list of the xul commands using querySelectorAll on a chrome document.

The exec function then just execs the selected command. You'll need to lookup the mdn docs on a xul command for this.

Thanks.

Comment 8

[saran]

Hi.. I tired writing the command. I used lookup function and document.querySelectorAll to get all the xul commands. In the exec() I just print "Hi" for now... But I get an error 


/*
Exception: option.name is undefined
@resource:///modules/devtools/gcli.jsm:1855
@resource:///modules/devtools/gcli.jsm:1922
@resource:///modules/devtools/gcli.jsm:993
Parameter@resource:///modules/devtools/gcli.jsm:2483
@resource:///modules/devtools/gcli.jsm:2398
Command@resource:///modules/devtools/gcli.jsm:2418
addCommand@resource:///modules/devtools/gcli.jsm:2607
@Scratchpad:42
*/

This is the command defn : 

Components.utils.import("resource:///modules/devtools/gcli.jsm");

gcli.addCommand({
  name: 'xulcommand',
  description: 'Run a XUL Command',
  params: [
    {

      name: 'name',
      
      type: {
        name: 'selection',
        lookup: function() {
          var forEach = Array.prototype.forEach;
          var xulcmds = document.querySelectorAll('command');
          var arr=[];
          forEach.call(xulcmds, function(xulcmds) {
              arr.push(xulcmds);
          });
          
          return arr;


        }
      },
	  description: 'The XUL command' ,
      

    }
  ],
  exec: function() {
   return "Hi";
  }
});

Comment 9

[Joe Walker [:jwalker]]

So the initial problem is that lookup should return an array of objects containing name and value properties. i.e.:

          forEach.call(xulcmds, function(xulcmd) {
              arr.push({ name: xulcmd.id, value: xulcmd });
          });

Comment 10

[saran]

Yes.I did as you told .. :)  Now I am able to see the commands in the selection list . But when I select any command and click enter , I get the following error :

TypeError: args.xulcmd is undefined

This is my exec function.. I used doCommand() to execute the command.I referred to this link https://developer.mozilla.org/en-US/docs/XUL_Tutorial/Updating_Commands

Comment 11

[saran]

Components.utils.import("resource:///modules/devtools/gcli.jsm");

gcli.addCommand({
  name: 'xulcommand',
  description: 'Run a XUL Command',
  params: [
    {

      name: 'name',
      
      type: {
        name: 'selection',
        lookup: function() {
          var forEach = Array.prototype.forEach;
          var xulcmds = document.querySelectorAll('command');
          var arr=[];
           forEach.call(xulcmds, function(xulcmd) {
              arr.push({ name: xulcmd.id, value: xulcmd });
          });
          
          return arr;

        }
      },
	  description: 'The XUL command' ,

    }
  ],
  exec: function(args,context) {
   return args.xulcmd.doCommand();
  }
});

Comment 12

[Joe Walker [:jwalker]]

That's because you called the param 'name' in the param declaration, and the referred to it by 'xulcmd' later. i.e. "name: 'xulcmd',"

Comment 13

[saran]

oh yes.. when i replaced param name with xulcmd , it seems to work.. I could choose from the list of commands and execute them. Thanks.
So what is the next step ?

Comment 14

[Joe Walker [:jwalker]]

Tasks remaining:
- Convert to JSM
- Format properly
- Make English strings localizable
- Include some basic testing
- Hide the command when we're not 'chrome' enabled

I suggest taking a look at CmdCalllogChrome.jsm for an example.

Comment 15

[saran]

Attachment: Patch containing the localised strings

Comment 16

[saran]

Attachment: jsm file for the new GCLI command

Comment 17

[Joe Walker [:jwalker]]

Comment on attachment 678325 [details] [diff] [review]
jsm file for the new GCLI command

Thanks for this. 2 things that we need to do:

We should join these 2 patches together, making the JSM part of the tree properly, also there is a naming convention for l10n properties which would see the runCommandDesc property renamed.

I suggest that we call the xulcmd parameter "id" since that's what we're looking up by. That would mean that runCommandDesc would be called xulCommandIdDesc.

Does that make sense?

Comment 18

[saran]

Thanks for the feedback. About the joining of the 2 patches. I cannot seem to include the .jsm file in the patch.The patch contains only changes pertaining to the modifying of existing files. I created the jsm file using the vim command. Is there some other setting that needs to be done ? I am working on Ubuntu.

Comment 19

[Girish Sharma [:Optimizer]]

(In reply to saran from comment #18)
> Thanks for the feedback. About the joining of the 2 patches. I cannot seem
> to include the .jsm file in the patch.The patch contains only changes
> pertaining to the modifying of existing files. I created the jsm file using
> the vim command. Is there some other setting that needs to be done ? I am
> working on Ubuntu.

You need to do

hg add /path/to/the/file.jsm

and then refresh the patch, then export the patch.

If you ar eworking on Mercurial queues then refer : https://developer.mozilla.org/en-US/docs/Mercurial_Queues

Comment 20

[saran]

Attachment: Patch containing all the changes

In Commands.jsm included the new command as well.

Comment 21

[saran]

@girish : thanks for the tip..

Comment 22

[Joe Walker [:jwalker]]

Comment on attachment 679042 [details] [diff] [review]
Patch containing all the changes

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

Excellent, thanks. So some more things from the hit list in comment 14:

Tasks remaining:
- Convert to JSM
  Done!

- Make English strings localizable
  Done!

- Format properly
I can see a couple of minor things to get right; we should add a standard header comment. See CmdRestart.jsm for an example. Also we should remove trailing spaces (line 23 of CmdXULCmd.jsm fails) This sounds trivial but it helps avoid unintended changes becoming distracting the diffs.

- Hide the command when we're not 'chrome' enabled
There's an example of this in CmdCalllogChrome.jsm - see "get hidden() gcli.hiddenByChromePref(),"

Comment 23

[saran]

hi joe... about the hiding when not chrome enabled ... I have included the code in my patch already . Do I need to add something more ?

Comment 24

[Joe Walker [:jwalker]]

(In reply to saran from comment #23)
> hi joe... about the hiding when not chrome enabled ... I have included the
> code in my patch already . Do I need to add something more ?

I'm sorry, I didn't see that - you've done it correctly already.

Comment 25

[saran]

Attachment: Patch with a few more changes

Comment 26

[Joe Walker [:jwalker]]

Comment on attachment 679177 [details] [diff] [review]
Patch with a few more changes

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

Excellent thanks. So the one final thing is some tests. I think a good place to start is to look at the tests in browser_cmd_cookie.js. If you'd like to know more about what
helpers.check and DeveloperToolbarTest.exec do, then see the doc-comments in helpers.js.

Comment 27

[Joe Walker [:jwalker]]

New component triage. Filter on "Lobster Thermidor aux crevettes with a Mornay sauce"

Comment 28

[Tiziana Sellitto [:tiziana]]

saran, are you still working on this bug? I'm going through older "good first bugs" that have been inactive for a while. 
I think Joe could help you in case you need info about how to procede.

Comment 29

[Joe Walker [:jwalker]]

Resetting priorities because these P* levels are very out of date.
Sorry for the bug spam. Filter on Lobster Thermidor

Comment 30

[Pablo Almenar]

If this bug isn't fixed yet, please **** it to me and I'll try to finish it.

Comment 31

[Joe Walker [:jwalker]]

Consider it assigned to you Pablo (ignore the assignee flag).
Thanks for offering to help.

Comment 32

[Pablo Almenar]

Attachment: New GCLI command that executes the xul command passed as parameter

Comment 33

[Joe Walker [:jwalker]]

Comment on attachment 8679415 [details] [diff] [review]
New GCLI command that executes the xul command passed as parameter

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

Looks good, thanks for working on it.

I think we should have at least some basic tests. browser_cmd_listen.js [1] would be a decent template for how to write a test, and there's documentation on writing GCLI tests [2].

We should also hide the command unless the user has opted into chrome debugging. There's an example of how to do that in the calllog command [3].

[1]: https://dxr.mozilla.org/mozilla-central/source/devtools/client/commandline/test/browser_cmd_listen.js
[2]: https://dxr.mozilla.org/mozilla-central/source/devtools/client/commandline/test/helpers.js#1187
[3]: https://dxr.mozilla.org/mozilla-central/source/devtools/shared/gcli/commands/calllog.js#103

::: devtools/shared/gcli/commands/xulcommand.js
@@ +40,5 @@
> +      try {
> +        return args.command.doCommand();
> +      }
> +      catch (ex) {
> +        throw new Error("An error occurred executing " + args.command.id + " (" + ex + ")");

I think we need to localize this. If you need an example of localization strings with parameters - grep for 'injectFailed'.

Comment 34

[Pablo Almenar]

Attachment: Updated as per Joe's feedback

Thank you a lot! I attached a new patch with those updates, please tell me what you think.

Comment 35

[Pablo Almenar]

Attachment: Updated as per Joe's feedback*

Updating due to a couple typos.

Comment 36

[Joe Walker [:jwalker]]

Comment on attachment 8679486 [details] [diff] [review]
Updated as per Joe's feedback*

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

I'm overloaded, and on PTO: Mike while you're looking at the addons command, could you also help get this over the finish line please?

Comment 37

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Comment on attachment 8679486 [details] [diff] [review]
Updated as per Joe's feedback*

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

Great job! Sorry it took a while to get around to this but we have been busy.

There are a couple of issues though:
1. Open about:home
2. Open the Developer Toolbar.
3. xulcommand Browser:NewUserContextTab

This throws:
chrome://browser/content/browser.js, line 14955: TypeError: event is null

Which is caused by the following code:
function openNewUserContextTab(event)
{
  openUILinkIn(BROWSER_NEW_TAB_URL, "tab", {
    userContextId: event.target.getAttribute('usercontextid'),
  });
}

Some commands need an event object containing the click event from the button that was clicked. As far as I can see, the try / catch in exec() should catch this so I would argue that this is an GCLI issue.

We could add a parameter containing the id of a browser button and use that to create a click event to pass but that would be up to jwalker.

Anyhow, there are a few things that need to be addressed, mostly just nits:

::: devtools/shared/gcli/commands/xulcommand.js
@@ +4,5 @@
> +
> +"use strict";
> +
> +const l10n = require("gcli/l10n");
> +

You need:
const gcli = require("gcli/index");

So that return gcli.hiddenByChromePref(); will work.

@@ +18,5 @@
> +    params: [{
> +      name: "command",
> +      type: {
> +        name: "selection",
> +        lookup: function (context) {

Nit: Remove the extra space:
lookup: function(context) {

@@ +22,5 @@
> +        lookup: function (context) {
> +          let browserWindow = context.environment.chromeWindow;
> +          let commands = browserWindow.document.querySelectorAll('command');
> +          let enabledCommands = [];
> +          Array.prototype.forEach.call(commands, function (command) {

Nit: Remove the extra space:
Array.prototype.forEach.call(commands, function(command) {

@@ +30,5 @@
> +                value: command
> +              });
> +            }
> +          });
> +          enabledCommands.sort(function (cmd1, cmd2){

Nit: Remove the extra space:
enabledCommands.sort(function(cmd1, cmd2){

@@ +43,5 @@
> +      try {
> +        return args.command.doCommand();
> +      }
> +      catch (ex) {
> +        throw new Error ( l10n.lookupFormat("xulcommandFailed", [name]) );

Remove extra spaces and name is not defined. Use:
        throw new Error(l10n.lookupFormat("xulcommandFailed",
                        [args.command.id]));

Comment 38

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

@jwalker: Some xul commands throw because no event object is passed (event is almost always supposed to be a click event to identify a button).

What do you think of the idea of adding an extra parameter containing the id of a button to create a click event for.

Also, the try / catch in exec() should catch the exception as far as I can see but it doesn't... maybe you have an idea why?

Comment 39

[Joe Walker [:jwalker]]

(In reply to Michael Ratcliffe [:miker] [:mratcliffe] from comment #38)
> @jwalker: Some xul commands throw because no event object is passed (event
> is almost always supposed to be a click event to identify a button).
> 
> What do you think of the idea of adding an extra parameter containing the id
> of a button to create a click event for.

Sounds good. We have a node type (see the screenshot command [1] for an example)

> Also, the try / catch in exec() should catch the exception as far as I can
> see but it doesn't... maybe you have an idea why?

No idea at all. You're saying that the try/catch in this file is needed because a try/catch further up the stack fails to catch? That seems like a bug in spidermonkey to me.

[1]: https://dxr.mozilla.org/mozilla-central/source/devtools/shared/gcli/commands/screenshot.js#83

Comment 40

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

@Pablo: Can you address my comments in Comment 37 and then I can help with adding a parameter that can send a click event to provide context to commands that need it.

Comment 41

[Pablo Almenar]

Attachment: xulcommand.patch

Hey, here is a solution I attempted, applying your comments.

I ended up not adding a new parameter since the Commands themselves are nodes already and can dispatch an event that properly executes the command. Please let me know if this is not the design we want or if it's not the best way to do it (which it probably isn't) as well as any other feedback you have.

Thanks a lot!

Comment 42

[Pablo Almenar]

Attachment: xulcommand.patch

Sorry, there was a typo on the other patch

Comment 43

[Pablo Almenar]

Attachment: xulcommand.patch

Forgot to ask for feedback...

Comment 44

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Comment on attachment 8690335 [details] [diff] [review]
xulcommand.patch

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

r+ on the assumption that try is green:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=4a1164fe345e

Comment 45

[Firefox Bug Husbandry Bot]

Per bug 1491875, this component has been closed, and the affected code is being removed from Firefox. Closing this bug as incomplete.