GCLI: Move existing GCLI commands into JSMs

RESOLVED FIXED in Firefox 17

Status

()

Firefox
Developer Tools
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: miker, Assigned: jwalker)

Tracking

unspecified
Firefox 17
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [gclicommands][fixed-in-fx-team])

Attachments

(1 attachment, 12 obsolete attachments)

v13
227.42 KB, patch
Details | Diff | Splinter Review
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.
Status: NEW → ASSIGNED
Depends on: 776518, 771169
Blocks: 771526
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.
Attachment #645739 - Flags: review?(mihai.sucan)
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()
Attachment #645739 - Flags: review?(mihai.sucan) → review+
plz could thus wait until i get back?
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?
Attachment #645739 - Attachment is obsolete: true
Created attachment 649014 [details] [diff] [review]
Patch v3

Split test files related to other tools off into their tool's test folders.
Attachment #649014 - Flags: review?(jwalker)
Attachment #646185 - Attachment is obsolete: true
Created attachment 649250 [details] [diff] [review]
Rebased

Rebased due to Makefile.in changes
Attachment #649014 - Attachment is obsolete: true
Attachment #649014 - Flags: review?(jwalker)
Attachment #649250 - Flags: review?(jwalker)
Created attachment 649258 [details] [diff] [review]
Removed extranneous line

Don't know how that crept in there
Attachment #649250 - Attachment is obsolete: true
Attachment #649250 - Flags: review?(jwalker)
Attachment #649258 - Flags: review?(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.
Attachment #649250 - Attachment is obsolete: false
So posting reviews after someone has obsoleted the file unobsoletes it. Interesting. My comments are good for the new version though.
Attachment #649250 - Attachment is obsolete: true
(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.
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.
Attachment #649258 - Attachment is obsolete: true
Attachment #649258 - Flags: review?(jwalker)
Attachment #650027 - Flags: review?(jwalker)
Created attachment 650906 [details] [diff] [review]
Corrected symbol name from CmdCmd.jsm
Attachment #650027 - Attachment is obsolete: true
Attachment #650027 - Flags: review?(jwalker)
Attachment #650906 - Flags: review?(jwalker)
Created attachment 651313 [details] [diff] [review]
Rebased
Attachment #650906 - Attachment is obsolete: true
Attachment #650906 - Flags: review?(jwalker)
Attachment #651313 - Flags: review?(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.
Attachment #651313 - Flags: review?(jwalker)
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.
Attachment #651313 - Attachment is obsolete: true
Attachment #651767 - Flags: review?(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.
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.
Created attachment 653314 [details] [diff] [review]
for past to have a look at

memleaks in the debugger
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.
Attachment #651767 - Attachment is obsolete: true
Attachment #653314 - Attachment is obsolete: true
Attachment #651767 - Flags: review?(jwalker)
Attachment #653348 - Flags: review?(mratcliffe)
(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 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"
Attachment #653348 - Flags: review?(mratcliffe) → review+
Created attachment 653442 [details] [diff] [review]
v12
Assignee: mratcliffe → jwalker
Attachment #653348 - Attachment is obsolete: true
Whiteboard: [gclicommands] → [gclicommands][land-in-fx-team]
(Would like a green try before we land)

https://tbpl.mozilla.org/?tree=Try&rev=d99dfc81bbab
Whiteboard: [gclicommands][land-in-fx-team] → [gclicommands]
(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
(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.
Whiteboard: [gclicommands] → [gclicommands][land-in-fx-team]
Created attachment 654967 [details] [diff] [review]
v13

rebase
Attachment #653442 - Attachment is obsolete: true
Backed out:

https://hg.mozilla.org/integration/fx-team/rev/f4596ef17eed
Re-land
https://tbpl.mozilla.org/?tree=Fx-Team&rev=c80d0e010be3
Whiteboard: [gclicommands][land-in-fx-team] → [gclicommands][fixed-in-fx-team]

Comment 29

5 years ago
https://hg.mozilla.org/mozilla-central/rev/828067f7c340
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 17

Comment 30

5 years ago
Oops, https://hg.mozilla.org/mozilla-central/rev/ebb525cd2e70 was the relanded changeset.
You need to log in before you can comment on or make changes to this bug.