776875 - GCLI: Move existing GCLI commands into JSMs

Status: RESOLVED FIXED

(DevTools :: General, defect)

Description

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

GcliCommands.jsm has a number of globals and the more commands we have the more chance of conflicts.

Moving the commands into their own JSMs would solve this issue and the sooner this is done the simpler it will be to do.

Comment 1

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

Attachment: Patch

Some of the commands were becoming fragmented by other commands inserted into the file in the wrong place. This really needed to be done.

All GCLI tests still pass (try = green) so there is not much to check here but if there is anything you don't like then please let me know.

Comment 2

[Mihai Sucan [:msucan]]

Comment on attachment 645739 [details] [diff] [review]
Patch


In GcliCommands.jsm you do:

Cu.import("resource:///modules/devtools/GcliCmdCommands.jsm", this);
Cu.import("resource:///modules/devtools/GcliTiltCommands.jsm", {});
... and more.

I find this weird. We discussed the reasoning on IRC. Maybe you can use loadSubScript()? [1]

I am giving this patch a tentative r+ because it does what you want, but I am not sure if I am the right person to review this kind of changes. I expect Joe has opinions on how he would like GCLI commands reorganized. I suggest asking Dave or wait for Joe to return from vacation.


[1] https://developer.mozilla.org/en/XPCOM_Interface_Reference/mozIJSSubScriptLoader
    Services.scriptloader.loadSubScript()

Comment 3

[Joe Walker [:jwalker]]

plz could thus wait until i get back?

Comment 4

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

Attachment: Patch v2

(In reply to Joe Walker from comment #3)
> plz could thus wait until i get back?

Of course, that was always the plan.

It was really agitating me that so many different commands were sharing the same namespace. Whether you keep it as it is or use require at least it is mostly done.

Also, should we not move the pref and help commands out of gcli.jsm?

Comment 5

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

Attachment: Patch v3

Split test files related to other tools off into their tool's test folders.

Comment 6

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

Attachment: Rebased

Rebased due to Makefile.in changes

Comment 7

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

Attachment: Removed extranneous line

Don't know how that crept in there

Comment 8

[Joe Walker [:jwalker]]

Comment on attachment 649250 [details] [diff] [review]
Rebased

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

This looks good, thanks.

Some things I think we need to do:
- Move tilt/webconsole commands into their 'parent' areas
- Check to see if we're not slowing things down by having too many small JSMs. Lets discuss in team meeting (and report back).
- Have less of the 'Gcli' in filenames for 2 reasons; in Firefox we're just calling it the command line, also the pattern of commands/GcliFooCommands.jsm tells you the same thing 3 times.
- Have one directory for GCLI commands that don't have another home - we currently have GcliCookieCommands.jsm, but commands/GcliEchoCommands.jsm. I wonder if we need the commands directory?

I suspect you could make these changes very easily by editing the patch file.

Comment 9

[Joe Walker [:jwalker]]

So posting reviews after someone has obsoleted the file unobsoletes it. Interesting. My comments are good for the new version though.

Comment 10

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

(In reply to Joe Walker from comment #8)
> Comment on attachment 649250 [details] [diff] [review]
> Rebased
> 
> Review of attachment 649250 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This looks good, thanks.
> 
> Some things I think we need to do:
> - Move tilt/webconsole commands into their 'parent' areas

They already are.

> - Check to see if we're not slowing things down by having too many small
> JSMs. Lets discuss in team meeting (and report back).

I guess we could be, I don't think we have any maintainable alternatives though. I do suspect that importing JSMs is super-optimized. Maybe we have an async alternative?

> - Have less of the 'Gcli' in filenames for 2 reasons; in Firefox we're just
> calling it the command line, also the pattern of
> commands/GcliFooCommands.jsm tells you the same thing 3 times.

Okay, will do.

> - Have one directory for GCLI commands that don't have another home - we
> currently have GcliCookieCommands.jsm, but commands/GcliEchoCommands.jsm. I
> wonder if we need the commands directory?
> 

Something must have been wrong when you applied the patch. Both of those files are in the commands directory, all commands that don't have another home are. If you want I can move them up a level. and rename them to e.g. cmdCookie.jsm

> I suspect you could make these changes very easily by editing the patch file.
Yup, that is my usual trick.

Comment 11

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

Attachment: Addressed reviewers comments

> > - Check to see if we're not slowing things down by having too many small
> > JSMs. Lets discuss in team meeting (and report back).

Discussed

> > - Have less of the 'Gcli' in filenames for 2 reasons; in Firefox we're just
> > calling it the command line, also the pattern of
> > commands/GcliFooCommands.jsm tells you the same thing 3 times.
> 

I have removed Gcli from all filenames wherever possible (including tests) without touching anything on the git side.
I have moved the commands to their parent directory and called them CmdBlah.jsm.
The Cmd prefix is useful to identify them as gcli commands. Bare in mind that they are imported using e.g. Cu.import("resource:///modules/devtools/CmdDbg.jsm"); Without the Cmd prefix these imports would be confusing.

> 
> > - Have one directory for GCLI commands that don't have another home - we
> > currently have GcliCookieCommands.jsm, but commands/GcliEchoCommands.jsm. I
> > wonder if we need the commands directory?
> > 

I have moved the commands to their parent directory and called them CmdBlah.jsm.

> Something must have been wrong when you applied the patch. Both of those
> files were in the commands directory, all commands that don't have another
> home are. If you want I can move them up a level. and rename them to e.g.
> cmdCookie.jsm
> 

I have moved and tidied these files.

Comment 12

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

Attachment: Corrected symbol name from CmdCmd.jsm

Comment 13

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

Attachment: Rebased

Comment 14

[Joe Walker [:jwalker]]

Comment on attachment 651313 [details] [diff] [review]
Rebased

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

::: browser/devtools/debugger/test/Makefile.in
@@ +10,5 @@
>  
>  include $(DEPTH)/config/autoconf.mk
>  
>  MOCHITEST_BROWSER_TESTS = \
> +  browser_dbg_cmd.js \

I suspect this isn't indented with a tab like the other lines, and given how touchy makefiles can be, i wonder if we should make sure?

@@ +71,4 @@
>  	$(NULL)
>  
>  MOCHITEST_BROWSER_PAGES = \
> +  browser_dbg_cmd.html \

as above

::: browser/devtools/debugger/test/head.js
@@ +16,5 @@
>  
> +// This file is designed to choose an appropriate head.js file in the case of
> +// gcli tests. If you are considering making changes to this file then you
> +// probably need to change either browser/devtools/debugger/test/head2.js or
> +// browser/devtools/commandline/test/head.js

I wonder if we could have a function:

function loadCommandLineHelper() {
 Services.scriptloader.loadSubScript("resource:///modules/devtools/gcli/head.js", this);
};

That we copied into other versions of head.js? That way people could test commands in a test environment that they were familiar with.

Comment 15

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

Attachment: removed spaces from makefile

(In reply to Joe Walker from comment #14)
> Comment on attachment 651313 [details] [diff] [review]
> Rebased
> 
> Review of attachment 651313 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: browser/devtools/debugger/test/Makefile.in
> @@ +10,5 @@
> >  
> >  include $(DEPTH)/config/autoconf.mk
> >  
> >  MOCHITEST_BROWSER_TESTS = \
> > +  browser_dbg_cmd.js \
> 
> I suspect this isn't indented with a tab like the other lines, and given how
> touchy makefiles can be, i wonder if we should make sure?
> 
> @@ +71,4 @@
> >  	$(NULL)
> >  
> >  MOCHITEST_BROWSER_PAGES = \
> > +  browser_dbg_cmd.html \
> 
> as above
> 

Changed

> ::: browser/devtools/debugger/test/head.js
> @@ +16,5 @@
> >  
> > +// This file is designed to choose an appropriate head.js file in the case of
> > +// gcli tests. If you are considering making changes to this file then you
> > +// probably need to change either browser/devtools/debugger/test/head2.js or
> > +// browser/devtools/commandline/test/head.js
> 
> I wonder if we could have a function:
> 
> function loadCommandLineHelper() {
>  Services.scriptloader.loadSubScript("resource:///modules/devtools/gcli/head.
> js", this);
> };
> 
> That we copied into other versions of head.js? That way people could test
> commands in a test environment that they were familiar with.

If only it was that easy ... if we did that then both head.js files would be loaded. That was my original approach but it turned out to be a disaster because we end up with teardown functions that error out, memory leaks because things are half initialized and need I mention naming conflicts ...

FWIW: I did discuss the method that I used with the team before I did it.

Loading *only* one file or the other is the only way that this can work as far as I can tell.

Comment 16

[Joe Walker [:jwalker]]

(In reply to Michael Ratcliffe [:miker] [:mratcliffe] from comment #15)
> > ::: browser/devtools/debugger/test/head.js
> > @@ +16,5 @@
> > >  
> > > +// This file is designed to choose an appropriate head.js file in the case of
> > > +// gcli tests. If you are considering making changes to this file then you
> > > +// probably need to change either browser/devtools/debugger/test/head2.js or
> > > +// browser/devtools/commandline/test/head.js
> > 
> > I wonder if we could have a function:
> > 
> > function loadCommandLineHelper() {
> >  Services.scriptloader.loadSubScript("resource:///modules/devtools/gcli/head.
> > js", this);
> > };
> > 
> > That we copied into other versions of head.js? That way people could test
> > commands in a test environment that they were familiar with.
> 
> If only it was that easy ... if we did that then both head.js files would be
> loaded. That was my original approach but it turned out to be a disaster
> because we end up with teardown functions that error out, memory leaks
> because things are half initialized and need I mention naming conflicts ...
> 
> FWIW: I did discuss the method that I used with the team before I did it.
> 
> Loading *only* one file or the other is the only way that this can work as
> far as I can tell.

So what if we had gclitesthelpers.js which contained just the definition of DeveloperToolbarTest? I guess we'd need to copy addTab into DeveloperToolbarTest, but would that work?

Sorry to be a pain, but otherwise people writing tests would not have access to all the bits of testing infrastructure that they were familiar with. It would be just like writing commandline tests but in a different directory.

Comment 17

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

Let me try to describe the problem. For this example let us use the debugger tests.

The debugger test's head.js contains the following:
if (!DebuggerServer.initialized) {
  DebuggerServer.init(function () { return true; });
  DebuggerServer.addBrowserActors();
}

It seems that this is not cleaned up by their teardown function, this is possibly a bug but may be a feature of relying on events used in the tests themselves in which case it is not a bug.

Also, methods like addTab() can be overwritten and teardown functions can be trying (and failing) to clean up stuff that was never initialized.

(In reply to Joe Walker [:jwalker] from comment #16)
> So what if we had gclitesthelpers.js which contained just the definition of
> DeveloperToolbarTest? I guess we'd need to copy addTab into
> DeveloperToolbarTest, but would that work?
> 

For the reasons mentioned above, not really.

> Sorry to be a pain, but otherwise people writing tests would not have access
> to all the bits of testing infrastructure that they were familiar with. It
> would be just like writing commandline tests but in a different directory.

From head2.js they have just as much access as they did from head.js, nothing changes there.

At the same time I recognize that this is not the ideal solution as it changes the structure of something that we are all familiar with.

Comment 18

[Joe Walker [:jwalker]]

Attachment: for past to have a look at

memleaks in the debugger

Comment 19

[Joe Walker [:jwalker]]

Attachment: memleak-free

I made the following changes:

- renamed browser_cmd_web.js back to browser_gcli_web.js because it's generated by GCLI, and because there is a bug to split it up, so I can do the rename then. Which is a long way of saying because I'm lazy, but not very lazy ;-)

- Adopt a new head2.js mechanism, i.e.
-- Moved the definition of DeveloperToolbarTest to helper.js and copied that to each place that needed it. (Again lazy but not very lazy)
-- Removed the near duplicate from shared/head.js
-- Added entries in the Makefiles to reference the new helper.js
-- Remove the special entry that copied gclis head.js into the browser
-- Moved the head2.js files back to head.js
-- Import helper.js from each place that needs it

- Moved the memory leak detector functions to leakhunt.js. When we work out how to share test code, then we can have this available anywhere too

- Added a mechanism to have DeveloperToolbarTest know about outstanding tests and to be able to clean up magically at the end.
-- You can see this in use in the addon tests where we pass a list of the test functions to test() and then
--- Don't call finish at the end
--- Wrap async test functions in a checkCalled() wrapper so we don't exit before they're done.
-- I also tweaked several of the tests to use this

What's up with the setTimeout in debugger/test/browser/dbg_cmd_break.js?
Did we add it? Either way, I don't think we need it now.

Comment 20

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

(In reply to Joe Walker [:jwalker] from comment #19)
> Created attachment 653348 [details] [diff] [review]
> memleak-free
> 
> What's up with the setTimeout in debugger/test/browser/dbg_cmd_break.js?
> Did we add it? Either way, I don't think we need it now.

When running the test on it's own it was leaking, the timeout fixed it.

Comment 21

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

Comment on attachment 653348 [details] [diff] [review]
memleak-free

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

Great job ... just a couple of nits and one bug:

r+ with the following addressed, no.3 is optional.

::: browser/devtools/commandline/test/helper.js
@@ +449,5 @@
> +  }
> +};
> +
> +/**
> + * 

Trailing space

::: browser/devtools/shared/test/leakhunt.js
@@ +1,1 @@
> +/* This Source Code Form is subject to the terms of the Mozilla Public

This file is missing from the makefile

::: browser/devtools/styleeditor/test/helper.js
@@ +25,5 @@
> + *
> + *
> + *
> + *
> + *  FOR A LONG TIME.

Should read "FOR A VERY LONG TIME"

Comment 22

[Joe Walker [:jwalker]]

Attachment: v12

Comment 23

[Joe Walker [:jwalker]]

(Would like a green try before we land)

https://tbpl.mozilla.org/?tree=Try&rev=d99dfc81bbab

Comment 24

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

(In reply to Joe Walker [:jwalker] from comment #23)
> (Would like a green try before we land)
> 
> https://tbpl.mozilla.org/?tree=Try&rev=d99dfc81bbab

I already had it green on try, otherwise I wouldn't have marked it.

https://tbpl.mozilla.org/?tree=Try&rev=cb07300f939d

I will leave it to you though

Comment 25

[Joe Walker [:jwalker]]

(In reply to Michael Ratcliffe [:miker] [:mratcliffe] from comment #24)
> (In reply to Joe Walker [:jwalker] from comment #23)
> > (Would like a green try before we land)
> > 
> > https://tbpl.mozilla.org/?tree=Try&rev=d99dfc81bbab
> 
> I already had it green on try, otherwise I wouldn't have marked it.
> 
> https://tbpl.mozilla.org/?tree=Try&rev=cb07300f939d
> 
> I will leave it to you though

You're right, but I broke it in many ways - just wanted to be sure.

Comment 26

[Joe Walker [:jwalker]]

Attachment: v13

rebase

Comment 27

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

Backed out:

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

Comment 28

[Joe Walker [:jwalker]]

Re-land
https://tbpl.mozilla.org/?tree=Fx-Team&rev=c80d0e010be3

Comment 29

[Dave Camp (:dcamp)]

https://hg.mozilla.org/mozilla-central/rev/828067f7c340

Comment 30

[Dave Camp (:dcamp)]

Oops, https://hg.mozilla.org/mozilla-central/rev/ebb525cd2e70 was the relanded changeset.