984365 - Break up BuiltinCommands.jsm

Status: RESOLVED FIXED

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

Description

[Joe Walker [:jwalker]]

What we need to do:

* Allow things to register modules that contain a number of commands, which can be loaded and unloaded as needed
* Rather than importing BuiltinCommands as part of starting GCLI or the toolbox, we should just register the modules to be loaded, and then load them as needed.
* Actually split BuiltinCommands up.

Comment 1

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

BuiltinCommands is in browser/devtools/commandline but contains a few commands essential for the gcli to run and/or related to core mozilla components like the addons manager. Maybe this bug could also be used to move those commands somewhere in toolkit/devtools?

Comment 2

[Joe Walker [:jwalker]]

(In reply to Philipp Kewisch [:Fallen] from comment #1)
> ... Maybe this bug could also be used to
> move those commands somewhere in toolkit/devtools?

Seems reasonable to me.

Comment 3

[Joe Walker [:jwalker]]

Attachment: breakup-984365.patch

This patch breaks up BuiltinCommands.jsm, and updates the various CmdFoo.jsm to be *.js files. This is churn that patch fails to represent, and several people need to review it, so I've broken it up into a set of easy to follow commits in a pull request [1].

There is a nice side effect of these changes that GCLI "pulls" commands from around our code base rather than things pushing commands into GCLI, which helps us lazy loading.

To review, go here: https://github.com/joewalker/gecko-dev/pull/1
Each commit has "r=someone" at the end, find yours, look at the description of what happened in that commit and review. Comments on bugzilla are preferred if possible.

[1]: https://github.com/joewalker/gecko-dev/pull/1

Comment 4

[Joe Walker [:jwalker]]

This patch passes a full devtools mochitest on my machine, but fails at the 'mach package' step on try.

See https://tbpl.mozilla.org/?tree=Try&rev=43c695349475 for details.

The quick summary is:

$ ./mach package
....
resource://app/modules/devtools/DeveloperToolbar.jsm
[58326] ###!!! ASSERTION: Existing entry in StartupCache.: 'entry == nullptr', file /builds/slave/try-l64-d-00000000000000000000/build/startupcache/StartupCache.cpp, line 364
UNKNOWN [/builds/slave/try-l64-d-00000000000000000000/build/obj-firefox/dist/bin/libxul.so +0x0142C8FF]
UNKNOWN [/builds/slave/try-l64-d-00000000000000000000/build/obj-firefox/dist/bin/libxul.so +0x0142DD64]
UNKNOWN [/builds/slave/try-l64-d-00000000000000000000/build/obj-firefox/dist/bin/libxul.so +0x0142DF97]
NS_InvokeByIndex+0x0000019F [/builds/slave/try-l64-d-00000000000000000000/build/obj-firefox/dist/bin/libxul.so +0x0094A9A0]
UNKNOWN [/builds/slave/try-l64-d-00000000000000000000/build/obj-firefox/dist/bin/libxul.so +0x0140E3F6]
UNKNOWN [/builds/slave/try-l64-d-00000000000000000000/build/obj-firefox/dist/bin/libxul.so +0x0140F1C8]
UNKNOWN [/builds/slave/try-l64-d-00000000000000000000/build/obj-firefox/dist/bin/libxul.so +0x0140FFDA]
UNKNOWN 0x7f10711db734
[58326] ###!!! ASSERTION: Existing entry in StartupCache.: 'entry == nullptr', file /builds/slave/try-l64-d-00000000000000000000/build/startupcache/StartupCache.cpp, line 364
Hit MOZ_CRASH() at /builds/slave/try-l64-d-00000000000000000000/build/memory/mozalloc/mozalloc_abort.cpp:30
Traceback (most recent call last):
  File "/builds/slave/try-l64-d-00000000000000000000/build/toolkit/mozapps/installer/packager.py", line 385, in <module>
    main()
  File "/builds/slave/try-l64-d-00000000000000000000/build/toolkit/mozapps/installer/packager.py", line 377, in main
    args.source, gre_path, base)
  File "/builds/slave/try-l64-d-00000000000000000000/build/toolkit/mozapps/installer/packager.py", line 158, in precompile_cache
    errors.fatal('Error while running startup cache precompilation')
  File "/builds/slave/try-l64-d-00000000000000000000/build/python/mozbuild/mozpack/errors.py", line 101, in fatal
    self._handle(self.FATAL, msg)
  File "/builds/slave/try-l64-d-00000000000000000000/build/python/mozbuild/mozpack/errors.py", line 96, in _handle
    raise ErrorMessage(msg)
mozpack.errors.ErrorMessage: Error: Error while running startup cache precompilation
make[3]: Leaving directory `/builds/slave/try-l64-d-00000000000000000000/build/obj-firefox/browser/installer'
make[2]: Leaving directory `/builds/slave/try-l64-d-00000000000000000000/build/obj-firefox/browser/installer'
make[1]: Leaving directory `/builds/slave/try-l64-d-00000000000000000000/build/obj-firefox/browser/installer'
make[3]: *** [stage-package] Error 1
make[2]: *** [make-package] Error 2
make[1]: *** [default] Error 2
make: *** [package] Error 2

My best guess right now is some sort of import loop, but if you've seen this before please shout.
I don't expect that the fix will invalidate any reviews.

Comment 5

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Comment on attachment 8399329 [details] [diff] [review]
breakup-984365.patch

This patch still leaves files like BuiltinCommands.jsm in browser/devtools. It contains commands related to addons, cookies, screenshots, which is not specific to browser/ but really any product and should therefore be in toolkit/devtools to my mind. There are likely a few other commands too. Sorry if I was unclear about this, do you think its reasonable to move these commands into toolkit/devtools in this bug?

Comment 6

[Girish Sharma [:Optimizer]]

Comment on attachment 8399329 [details] [diff] [review]
breakup-984365.patch

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

I just have one comment, which applies to the overall patch too.

Why not use Task.async along with new syntax for generator functions instead of Task.spawn and old generator function syntax. This will convert the code from:

foo: function(args) {
  return Task.spawn(function() {
    // do stuff.
    throw new Task.result(returnVal);
  });
}

to

foo: Task.async(function*(args) {
  // do stuff.
  return returnVal;
})

Less indentation, no ugly throw calls to return from the method.

Comment 7

[Joe Walker [:jwalker]]

Attachment: breakup-984365.patch

Oops, thanks for spotting that Philipp - this is the right patch.

Comment 8

[Joe Walker [:jwalker]]

(In reply to Girish Sharma [:Optimizer] from comment #6)
> Comment on attachment 8399329 [details] [diff] [review]
> breakup-984365.patch
> 
> Review of attachment 8399329 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I just have one comment, which applies to the overall patch too.
> 
> Why not use Task.async along with new syntax for generator functions instead
> of Task.spawn and old generator function syntax. This will convert the code
> from:
> 
> foo: function(args) {
>   return Task.spawn(function() {
>     // do stuff.
>     throw new Task.result(returnVal);
>   });
> }
> 
> to
> 
> foo: Task.async(function*(args) {
>   // do stuff.
>   return returnVal;
> })
> 
> Less indentation, no ugly throw calls to return from the method.

Agreed Task.async is much better, it's just inertia and wanting to avoid changing too much at the same time. I'd like to fix this in a follow-up.

Comment 9

[Joe Walker [:jwalker]]

(In reply to Joe Walker [:jwalker] from comment #7)
> Created attachment 8399370 [details] [diff] [review]
> breakup-984365.patch
> 
> Oops, thanks for spotting that Philipp - this is the right patch.

Huh - it's not. There's something bizarre going on with moz-git-tools. I'm looking into it.

Comment 10

[Joe Walker [:jwalker]]

Attachment: 0001-Bug-984365-Refactor-and-split-out-BuiltinCommands.js.patch

So "git push-to-hg" from moz-git-tools creates a new HG patch queue called "patches-git-temp".

Sorry for the bug churn.

Comment 11

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Comment on attachment 8399376 [details] [diff] [review]
0001-Bug-984365-Refactor-and-split-out-BuiltinCommands.js.patch

This looks much better, thanks! As a drive-by review comment, you could probably make use of Services.dirsvc and Preferences.jsm in toolkit/devtools/gcli/commands/cmd.js

Comment 12

[Joe Walker [:jwalker]]

(In reply to Philipp Kewisch [:Fallen] from comment #11)
> Comment on attachment 8399376 [details] [diff] [review]
> 0001-Bug-984365-Refactor-and-split-out-BuiltinCommands.js.patch
> 
> This looks much better, thanks! As a drive-by review comment, you could
> probably make use of Services.dirsvc and Preferences.jsm in
> toolkit/devtools/gcli/commands/cmd.js

Yes, but I want to avoid this bug growing and growing. It contains minor refactoring (like using " rather than ') - the kind that's unlikely to break anything, but I've removed larger changes already to avoid this becoming a too-big thing. Happy for follow-up bugs though.

Comment 13

[Joe Walker [:jwalker]]

I have a solution to the problem in comment 4. The problem is that we've got an include loop, as suspected.

I've solved this by wrapping the init of GCLI in a lazy getter in DeveloperToolbar. This doesn't actually delay initialization in a meaningful way but it does allow the startup cache to get out of the knot that it's in.

Comment 14

[Heather Arthur [:harth]]

Comment on attachment 8399376 [details] [diff] [review]
0001-Bug-984365-Refactor-and-split-out-BuiltinCommands.js.patch

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

style editor r=me

Comment 15

[Patrick Brosset <:pbro>]

Comment on attachment 8399376 [details] [diff] [review]
0001-Bug-984365-Refactor-and-split-out-BuiltinCommands.js.patch

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

R+ for these 2 parts:
- https://github.com/joewalker/gecko-dev/commit/d612bfef2e7eadecfd387151542260ca48f50a02
- https://github.com/joewalker/gecko-dev/commit/677ed95ac344dfd301be5f83049cbbbb1ca34a44

Comment 16

[Mihai Sucan [:msucan]]

Comment on attachment 8399376 [details] [diff] [review]
0001-Bug-984365-Refactor-and-split-out-BuiltinCommands.js.patch

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

Just a quick note...

::: browser/devtools/webconsole/console-commands.js
@@ +69,5 @@
> +    name: "console clear",
> +    description: gcli.lookup("consoleclearDesc"),
> +    exec: function Command_consoleClear(args, context) {
> +      let HUDService = require("devtools/webconsole/hudservice");
> +      let hud = HUDService.getHudByWindow(context.environment.window);

This doesnt work with remote targets, eg. e10s. You can update this code to use getToolbox(target), then hud = getPanel("webconsole").hud. getPanel() can return null if the webconsole was not yet opened.

Comment 17

[Joe Walker [:jwalker]]

(In reply to Mihai Sucan [:msucan] from comment #16)
> Comment on attachment 8399376 [details] [diff] [review]
> 0001-Bug-984365-Refactor-and-split-out-BuiltinCommands.js.patch
> 
> Review of attachment 8399376 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Just a quick note...
> 
> ::: browser/devtools/webconsole/console-commands.js
> @@ +69,5 @@
> > +    name: "console clear",
> > +    description: gcli.lookup("consoleclearDesc"),
> > +    exec: function Command_consoleClear(args, context) {
> > +      let HUDService = require("devtools/webconsole/hudservice");
> > +      let hud = HUDService.getHudByWindow(context.environment.window);
> 
> This doesnt work with remote targets, eg. e10s. You can update this code to
> use getToolbox(target), then hud = getPanel("webconsole").hud. getPanel()
> can return null if the webconsole was not yet opened.

Also, before too long you should also be able to do this:

{
  name: 'somecommand',
  runon: 'client|server',
  exec: function() { ... }
}

(Or something like that)
In one place you get access to the DOM, in the other you get a target. Everything else should be transparent.

But perhaps this would be a fix worth making in the mean time.

Comment 18

[Panos Astithas (he/him) [:past] (please ni?)]

Comment on attachment 8399376 [details] [diff] [review]
0001-Bug-984365-Refactor-and-split-out-BuiltinCommands.js.patch

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

::: toolkit/devtools/gcli/commands/listen.js
@@ +4,5 @@
> +
> +"use strict";
> +
> +const { Cc, Ci, Cu } = require("chrome");
> +const { Services } = Cu.import("resource://gre/modules/Services.jsm", {});

If listen.js is loaded by the devtools loader, then Services is preloaded and you can just do:

const Services = require("Services");

Comment 19

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

Comment on attachment 8399376 [details] [diff] [review]
0001-Bug-984365-Refactor-and-split-out-BuiltinCommands.js.patch

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

from: 

https://github.com/joewalker/gecko-dev/commit/df49ac5b42d99d9226afd704ece73d74de3cd55d = r+
https://github.com/joewalker/gecko-dev/commit/a64d949365f524af0e3558018ed415db326844aa = r+
https://github.com/joewalker/gecko-dev/commit/7de19cb0c50427d637060f8fa94ae54591feac77 = r+
https://github.com/joewalker/gecko-dev/commit/5a8589d1ec08cde11e011298ca6cb546bac015a8 = r+
https://github.com/joewalker/gecko-dev/commit/d345940b9fd3cf23eb12d6a976e971032dbc45e8 = r+
https://github.com/joewalker/gecko-dev/commit/349242a719cffb37476c9bb4eb90cee0e007ea5f = r+

r+ for all of these. Should address mihai's comments either here or in a follow-up patch.

Would like to get the below follow-ups on file too.

::: browser/devtools/commandline/commands-index.js
@@ +20,5 @@
> +  "gcli/commands/paintflashing",
> +  "gcli/commands/restart",
> +  "gcli/commands/screenshot",
> +  "gcli/commands/tools",
> +];

istr in the distant past we talked about moving commands into the tools' directories they were referring to. Is that still something we'd like to do? I think there'd still be value in doing that as it gets the commands closer to the things they're interacting with.

::: browser/devtools/webconsole/console-commands.js
@@ +69,5 @@
> +    name: "console clear",
> +    description: gcli.lookup("consoleclearDesc"),
> +    exec: function Command_consoleClear(args, context) {
> +      let HUDService = require("devtools/webconsole/hudservice");
> +      let hud = HUDService.getHudByWindow(context.environment.window);

can we update this in https://github.com/joewalker/gecko-dev/commit/349242a719cffb37476c9bb4eb90cee0e007ea5f ?

::: toolkit/devtools/gcli/commands/cmd.js
@@ +5,5 @@
> +"use strict";
> +
> +const { Cc, Ci, Cu } = require("chrome");
> +
> +const { Promise: promise } = Cu.import("resource://gre/modules/commonjs/sdk/core/promise.js", {});

have you looked at changing this to Promise.jsm? I realize that would probably inflate this patch and break tests, but we should try to move to it eventually.

Comment 20

[Joe Walker [:jwalker]]

(In reply to Panos Astithas [:past] from comment #18)
> Comment on attachment 8399376 [details] [diff] [review]
> 0001-Bug-984365-Refactor-and-split-out-BuiltinCommands.js.patch
> 
> Review of attachment 8399376 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: toolkit/devtools/gcli/commands/listen.js
> @@ +4,5 @@
> > +
> > +"use strict";
> > +
> > +const { Cc, Ci, Cu } = require("chrome");
> > +const { Services } = Cu.import("resource://gre/modules/Services.jsm", {});
> 
> If listen.js is loaded by the devtools loader, then Services is preloaded
> and you can just do:
> 
> const Services = require("Services");

Ah, thanks Panos. Fixed here and in the restart / tools code too.

Comment 21

[Joe Walker [:jwalker]]

(In reply to Rob Campbell [:rc] (:robcee) from comment #19)
> Comment on attachment 8399376 [details] [diff] [review]
> 0001-Bug-984365-Refactor-and-split-out-BuiltinCommands.js.patch
> 
> Review of attachment 8399376 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> from: 
> 
> https://github.com/joewalker/gecko-dev/commit/
> df49ac5b42d99d9226afd704ece73d74de3cd55d = r+
> https://github.com/joewalker/gecko-dev/commit/
> a64d949365f524af0e3558018ed415db326844aa = r+
> https://github.com/joewalker/gecko-dev/commit/
> 7de19cb0c50427d637060f8fa94ae54591feac77 = r+
> https://github.com/joewalker/gecko-dev/commit/
> 5a8589d1ec08cde11e011298ca6cb546bac015a8 = r+
> https://github.com/joewalker/gecko-dev/commit/
> d345940b9fd3cf23eb12d6a976e971032dbc45e8 = r+
> https://github.com/joewalker/gecko-dev/commit/
> 349242a719cffb37476c9bb4eb90cee0e007ea5f = r+
> 
> r+ for all of these. Should address mihai's comments either here or in a
> follow-up patch.
> 
> Would like to get the below follow-ups on file too.
> 
> ::: browser/devtools/commandline/commands-index.js
> @@ +20,5 @@
> > +  "gcli/commands/paintflashing",
> > +  "gcli/commands/restart",
> > +  "gcli/commands/screenshot",
> > +  "gcli/commands/tools",
> > +];
> 
> istr in the distant past we talked about moving commands into the tools'
> directories they were referring to. Is that still something we'd like to do?
> I think there'd still be value in doing that as it gets the commands closer
> to the things they're interacting with.

This patch does that for all cases where there is a natural home, so the console commands now live in browser/devtools/webconsole/console-commands.js and so on.

The list you've highlighted has no natural home (as far as I can see) so I put them all together.

> ::: browser/devtools/webconsole/console-commands.js
> @@ +69,5 @@
> > +    name: "console clear",
> > +    description: gcli.lookup("consoleclearDesc"),
> > +    exec: function Command_consoleClear(args, context) {
> > +      let HUDService = require("devtools/webconsole/hudservice");
> > +      let hud = HUDService.getHudByWindow(context.environment.window);
> 
> can we update this in
> https://github.com/joewalker/gecko-dev/commit/
> 349242a719cffb37476c9bb4eb90cee0e007ea5f ?

Done.

> ::: toolkit/devtools/gcli/commands/cmd.js
> @@ +5,5 @@
> > +"use strict";
> > +
> > +const { Cc, Ci, Cu } = require("chrome");
> > +
> > +const { Promise: promise } = Cu.import("resource://gre/modules/commonjs/sdk/core/promise.js", {});
> 
> have you looked at changing this to Promise.jsm? I realize that would
> probably inflate this patch and break tests, but we should try to move to it
> eventually.

I'm trying to avoid getting sucked into making a big patch huge but I'm just testing out replacing the 2 remaining uses of SDK promises and we'll see what breaks.

Comment 22

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

Comment on attachment 8399376 [details] [diff] [review]
0001-Bug-984365-Refactor-and-split-out-BuiltinCommands.js.patch

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

Reviewed:
https://github.com/joewalker/gecko-dev/commit/086bde73884a30c1c6933cb481fd2a9c284e8210 = r+
https://github.com/joewalker/gecko-dev/commit/e6e90c249897d3a9a38f873e2c4b4efbf928d2c9 = r+
https://github.com/joewalker/gecko-dev/commit/3b4773e924896cae16f3842213befe98cf90e13d = r+
https://github.com/joewalker/gecko-dev/commit/eff2e4f2008926958a82ecabdc84219da0887d28 = r+
https://github.com/joewalker/gecko-dev/commit/4a4f1c514078e0d3fba4ce6a22c59d06b8698cc6 = r+
https://github.com/joewalker/gecko-dev/commit/0e3fccdcf7fe071ce92e07795ff6e2f85c788592 = r+
https://github.com/joewalker/gecko-dev/commit/06f415f5bf8b6044fbf09be6e068aa4fd5430b11 = r+
https://github.com/joewalker/gecko-dev/commit/baa6b8050d3171e5be639929dcaa4c3b0b90e892 = r+
https://github.com/joewalker/gecko-dev/commit/58b5cb68413650ea9ab3f96467d343d249cbc4c9 = r+
https://github.com/joewalker/gecko-dev/commit/58b5cb68413650ea9ab3f96467d343d249cbc4c9 = r+
https://github.com/joewalker/gecko-dev/commit/d63051ec3b6b41deebdf1def3e54b7333cc993f2 = r+
https://github.com/joewalker/gecko-dev/commit/656b15533ff4db7cb12ce75d685364d1a4c9ebd6 = r+
https://github.com/joewalker/gecko-dev/commit/e2a7bb8fb5c0c5341a36f1fe08896e2198e31bd6 = r+
https://github.com/joewalker/gecko-dev/commit/4d1a141b50fac9cf6db5f1df79e9c306d9231e54 = r+
https://github.com/joewalker/gecko-dev/commit/b5c9f626451e5a0d3dd971b29b361e0aa291019e = r+
https://github.com/joewalker/gecko-dev/commit/8633468c1ca54d22e7403dfcfc17a0deba82b586 = r+
https://github.com/joewalker/gecko-dev/commit/34b7131322b200b71b4a465b9e6c537865354a25 = r+
https://github.com/joewalker/gecko-dev/commit/a139568ab7af00c3bf8561a7e8414b9289baea5f = r+
https://github.com/joewalker/gecko-dev/commit/451da87e2afe15ae0e6a05dde822ef3083325338 = r+
https://github.com/joewalker/gecko-dev/commit/48117da5397674edacd175ad9734f692e41833d0 = r+

I also looked at the following that had no r= asignments:
https://github.com/joewalker/gecko-dev/commit/430384e35b4202bc700c3c7d79cbb68b076f48f0 = r+
https://github.com/joewalker/gecko-dev/commit/bf047c32712c209a9abff42e456cb4a421e29c62 = r+

Comment 23

[Joe Walker [:jwalker]]

(In reply to Rob Campbell [:rc] (:robcee) from comment #19)
> ...
>
> ::: toolkit/devtools/gcli/commands/cmd.js
> @@ +5,5 @@
> > +"use strict";
> > +
> > +const { Cc, Ci, Cu } = require("chrome");
> > +
> > +const { Promise: promise } = Cu.import("resource://gre/modules/commonjs/sdk/core/promise.js", {});
> 
> have you looked at changing this to Promise.jsm? I realize that would
> probably inflate this patch and break tests, but we should try to move to it
> eventually.

I have. The one you highlighted, I've fixed, but GCLI itself still uses SDK promises, but not because of an issue in GCLI tests, but in one of the debugger tests.

Here's the error message from making GCLI use toolkit promises:

 0:32.93 TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/devtools/debugger/test/browser_dbg_searchbox-help-popup-02.js | The editor selection appears to be correct after gaining focus.
 0:32.93 Stack trace:
 0:32.93     JS frame :: chrome://mochitests/content/browser/browser/devtools/debugger/test/browser_dbg_searchbox-help-popup-02.js :: testFocusLost :: line 70

The test does this:

    blah
      .then(tryShowPopup)     <- this is OK
      .then(focusEditor)
      .then(testFocusLost)    <- these tests fail

This could be explained if focusEditor() / gEditor.focus() took more than one tick to complete as a result of GCLI switching to async promises:

    function focusEditor() {
      let deferred = promise.defer();
      // Focusing the editor takes a tick to update the caret and selection.
      gEditor.focus();
      executeSoon(deferred.resolve);
      return deferred.promise;
    }

Except that gEditor.focus is just codemirror.focus, so I'm not immediately clear how GCLI using async promises has anything to do with that.

TL;DR: It's non-trivial. It can get fixed in a follow-up.

Comment 24

[Joe Walker [:jwalker]]

(In reply to Joe Walker [:jwalker] from comment #23)
> TL;DR: It's non-trivial. It can get fixed in a follow-up.

Chatted about this in the 'scrum' earlier, and I'm fairly sure we know what's up. The debugger head.js imports helpers.js which uses promises from GCLI which switched promises. Even though the debugger head.js imports sdk promises itself, this later gets over-written by helpers.js

There are several solutions to this problem, but I think we should still leave them to a follow-up.

Comment 25

[Joe Walker [:jwalker]]

(In reply to Joe Walker [:jwalker] from comment #24)
> There are several solutions to this problem, but I think we should still
> leave them to a follow-up.

Bug 992309.

Comment 26

[Joe Walker [:jwalker]]

Attachment: breakup-984365.patch v4

Greg: please could you take a look at the build file changes in this patch to make sure I've done what's needed?

Comment 27

[Joe Walker [:jwalker]]

Attachment 8402636 [details] [diff] fixes the review comments above.

Comment 28

[Gregory Szorc [:gps]]

Comment on attachment 8402636 [details] [diff] [review]
breakup-984365.patch v4

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

This won't get build r+ until INSTALL_TARGETS is used.

::: browser/devtools/commandline/Makefile.in
@@ +6,5 @@
>  include $(topsrcdir)/config/rules.mk
>  
>  libs::
>  	$(NSINSTALL) $(srcdir)/*.jsm $(FINAL_TARGET)/modules/devtools
> +	$(NSINSTALL) $(srcdir)/*.js $(FINAL_TARGET)/modules/devtools/commandline

So, devtools Makefiles do not conform to build standards.

The libs:: $(NSINSTALL) foo is an anti-pattern.

You want to see INSTALL_TARGETS instead. See https://hg.mozilla.org/mozilla-central/file/83ae54e18689/services/sync/Makefile.in for an example.

We prefer each .js and .jsm file be enumerated. But I'll allow you to use $(wildcard $(srcdir)/*.jsm) instead.

::: browser/devtools/responsivedesign/Makefile.in
@@ +5,5 @@
>  include $(topsrcdir)/config/rules.mk
>  
>  libs::
>  	$(NSINSTALL) $(srcdir)/*.jsm $(FINAL_TARGET)/modules/devtools
> +	$(NSINSTALL) $(srcdir)/*.js $(FINAL_TARGET)/modules/devtools

Ditto.

::: toolkit/devtools/gcli/Makefile.in
@@ +45,5 @@
>  include $(topsrcdir)/config/rules.mk
>  
>  libs::
>  	$(INSTALL) $(IFLAGS1) $(srcdir)/*.jsm $(FINAL_TARGET)/modules/devtools
> +	$(INSTALL) $(IFLAGS1) $(srcdir)/commands/*.js $(FINAL_TARGET)/modules/devtools/gcli/commands

Ditto.

Comment 29

[Joe Walker [:jwalker]]

Attachment: breakup-984365.patch v5

Changes since the last patch:
https://github.com/joewalker/gecko-dev/commit/c9e8865b4d2b78ddef6efca59b87cda3f054a106

Comment 30

[Joe Walker [:jwalker]]

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

Comment 31

[Gregory Szorc [:gps]]

Comment on attachment 8405314 [details] [diff] [review]
breakup-984365.patch v5

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

The build changes look guide. I didn't verify you added entries for every single .js and .jsm file present in the source directory, however. I trust things will fail loudly if you didn't.

Comment 32

[Joe Walker [:jwalker]]

https://tbpl.mozilla.org/?tree=Fx-Team&rev=c17fcb94177b
https://hg.mozilla.org/integration/fx-team/rev/7b4367158864

Comment 33

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/7b4367158864