Last Comment Bug 776875 - GCLI: Move existing GCLI commands into JSMs
: GCLI: Move existing GCLI commands into JSMs
Status: RESOLVED FIXED
[gclicommands][fixed-in-fx-team]
:
Product: Firefox
Classification: Client Software
Component: Developer Tools (show other bugs)
: unspecified
: All All
: -- normal (vote)
: Firefox 17
Assigned To: Joe Walker [:jwalker] (needinfo me or ping on irc)
:
Mentors:
Depends on: 771169 776518
Blocks: GCLICMD 771526
  Show dependency treegraph
 
Reported: 2012-07-24 04:46 PDT by Michael Ratcliffe [:miker] [:mratcliffe]
Modified: 2012-08-25 17:01 PDT (History)
5 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch (98.81 KB, patch)
2012-07-25 06:12 PDT, Michael Ratcliffe [:miker] [:mratcliffe]
mihai.sucan: review+
Details | Diff | Splinter Review
Patch v2 (100.52 KB, patch)
2012-07-26 09:48 PDT, Michael Ratcliffe [:miker] [:mratcliffe]
no flags Details | Diff | Splinter Review
Patch v3 (143.02 KB, patch)
2012-08-04 10:38 PDT, Michael Ratcliffe [:miker] [:mratcliffe]
no flags Details | Diff | Splinter Review
Rebased (142.96 KB, patch)
2012-08-06 06:11 PDT, Michael Ratcliffe [:miker] [:mratcliffe]
no flags Details | Diff | Splinter Review
Removed extranneous line (142.96 KB, patch)
2012-08-06 06:55 PDT, Michael Ratcliffe [:miker] [:mratcliffe]
no flags Details | Diff | Splinter Review
Addressed reviewers comments (162.53 KB, text/plain)
2012-08-08 04:39 PDT, Michael Ratcliffe [:miker] [:mratcliffe]
no flags Details
Corrected symbol name from CmdCmd.jsm (162.53 KB, patch)
2012-08-10 09:19 PDT, Michael Ratcliffe [:miker] [:mratcliffe]
no flags Details | Diff | Splinter Review
Rebased (162.52 KB, patch)
2012-08-13 03:53 PDT, Michael Ratcliffe [:miker] [:mratcliffe]
no flags Details | Diff | Splinter Review
removed spaces from makefile (162.52 KB, patch)
2012-08-14 07:55 PDT, Michael Ratcliffe [:miker] [:mratcliffe]
no flags Details | Diff | Splinter Review
for past to have a look at (218.53 KB, patch)
2012-08-20 02:35 PDT, Joe Walker [:jwalker] (needinfo me or ping on irc)
no flags Details | Diff | Splinter Review
memleak-free (227.45 KB, patch)
2012-08-20 06:33 PDT, Joe Walker [:jwalker] (needinfo me or ping on irc)
mratcliffe: review+
Details | Diff | Splinter Review
v12 (227.46 KB, patch)
2012-08-20 11:11 PDT, Joe Walker [:jwalker] (needinfo me or ping on irc)
no flags Details | Diff | Splinter Review
v13 (227.42 KB, patch)
2012-08-24 03:10 PDT, Joe Walker [:jwalker] (needinfo me or ping on irc)
no flags Details | Diff | Splinter Review

Description Michael Ratcliffe [:miker] [:mratcliffe] 2012-07-24 04:46:08 PDT
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 Michael Ratcliffe [:miker] [:mratcliffe] 2012-07-25 06:12:00 PDT
Created attachment 645739 [details] [diff] [review]
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] 2012-07-25 09:59:51 PDT
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] (needinfo me or ping on irc) 2012-07-26 08:37:20 PDT
plz could thus wait until i get back?
Comment 4 Michael Ratcliffe [:miker] [:mratcliffe] 2012-07-26 09:48:27 PDT
Created attachment 646185 [details] [diff] [review]
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 Michael Ratcliffe [:miker] [:mratcliffe] 2012-08-04 10:38:45 PDT
Created attachment 649014 [details] [diff] [review]
Patch v3

Split test files related to other tools off into their tool's test folders.
Comment 6 Michael Ratcliffe [:miker] [:mratcliffe] 2012-08-06 06:11:46 PDT
Created attachment 649250 [details] [diff] [review]
Rebased

Rebased due to Makefile.in changes
Comment 7 Michael Ratcliffe [:miker] [:mratcliffe] 2012-08-06 06:55:44 PDT
Created attachment 649258 [details] [diff] [review]
Removed extranneous line

Don't know how that crept in there
Comment 8 Joe Walker [:jwalker] (needinfo me or ping on irc) 2012-08-07 02:09:47 PDT
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] (needinfo me or ping on irc) 2012-08-07 02:11:50 PDT
So posting reviews after someone has obsoleted the file unobsoletes it. Interesting. My comments are good for the new version though.
Comment 10 Michael Ratcliffe [:miker] [:mratcliffe] 2012-08-07 05:14:58 PDT
(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 Michael Ratcliffe [:miker] [:mratcliffe] 2012-08-08 04:39:11 PDT
Created attachment 650027 [details]
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 Michael Ratcliffe [:miker] [:mratcliffe] 2012-08-10 09:19:00 PDT
Created attachment 650906 [details] [diff] [review]
Corrected symbol name from CmdCmd.jsm
Comment 13 Michael Ratcliffe [:miker] [:mratcliffe] 2012-08-13 03:53:20 PDT
Created attachment 651313 [details] [diff] [review]
Rebased
Comment 14 Joe Walker [:jwalker] (needinfo me or ping on irc) 2012-08-14 05:50:12 PDT
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 Michael Ratcliffe [:miker] [:mratcliffe] 2012-08-14 07:55:10 PDT
Created attachment 651767 [details] [diff] [review]
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] (needinfo me or ping on irc) 2012-08-14 09:05:27 PDT
(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 Michael Ratcliffe [:miker] [:mratcliffe] 2012-08-15 04:08:08 PDT
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] (needinfo me or ping on irc) 2012-08-20 02:35:47 PDT
Created attachment 653314 [details] [diff] [review]
for past to have a look at

memleaks in the debugger
Comment 19 Joe Walker [:jwalker] (needinfo me or ping on irc) 2012-08-20 06:33:08 PDT
Created attachment 653348 [details] [diff] [review]
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 Michael Ratcliffe [:miker] [:mratcliffe] 2012-08-20 06:49:05 PDT
(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 Michael Ratcliffe [:miker] [:mratcliffe] 2012-08-20 07:31:00 PDT
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] (needinfo me or ping on irc) 2012-08-20 11:11:34 PDT
Created attachment 653442 [details] [diff] [review]
v12
Comment 23 Joe Walker [:jwalker] (needinfo me or ping on irc) 2012-08-20 14:05:38 PDT
(Would like a green try before we land)

https://tbpl.mozilla.org/?tree=Try&rev=d99dfc81bbab
Comment 24 Michael Ratcliffe [:miker] [:mratcliffe] 2012-08-20 14:57:24 PDT
(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] (needinfo me or ping on irc) 2012-08-20 15:16:36 PDT
(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] (needinfo me or ping on irc) 2012-08-24 03:10:10 PDT
Created attachment 654967 [details] [diff] [review]
v13

rebase
Comment 27 Rob Campbell [:rc] (:robcee) 2012-08-24 06:29:00 PDT
Backed out:

https://hg.mozilla.org/integration/fx-team/rev/f4596ef17eed
Comment 28 Joe Walker [:jwalker] (needinfo me or ping on irc) 2012-08-24 11:16:14 PDT
Re-land
https://tbpl.mozilla.org/?tree=Fx-Team&rev=c80d0e010be3
Comment 29 Dave Camp (:dcamp) 2012-08-25 16:58:20 PDT
https://hg.mozilla.org/mozilla-central/rev/828067f7c340
Comment 30 Dave Camp (:dcamp) 2012-08-25 17:01:00 PDT
Oops, https://hg.mozilla.org/mozilla-central/rev/ebb525cd2e70 was the relanded changeset.

Note You need to log in before you can comment on or make changes to this bug.