Closed Bug 984365 Opened 6 years ago Closed 6 years ago

Break up BuiltinCommands.jsm

Categories

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

defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 31

People

(Reporter: jwalker, Assigned: jwalker)

References

Details

Attachments

(1 file, 4 obsolete files)

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.
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?
(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.
Attached patch breakup-984365.patch (obsolete) — Splinter Review
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
Attachment #8399329 - Flags: review?(rcampbell)
Attachment #8399329 - Flags: review?(pbrosset)
Attachment #8399329 - Flags: review?(past)
Attachment #8399329 - Flags: review?(mratcliffe)
Attachment #8399329 - Flags: review?(fayearthur)
Attachment #8399329 - Flags: feedback?(scrapmachines)
Attachment #8399329 - Flags: feedback?(philipp)
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 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?
Attachment #8399329 - Flags: feedback?(philipp) → feedback-
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.
Attachment #8399329 - Flags: feedback?(scrapmachines) → feedback+
Attached patch breakup-984365.patch (obsolete) — Splinter Review
Oops, thanks for spotting that Philipp - this is the right patch.
Assignee: nobody → jwalker
Attachment #8399329 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #8399329 - Flags: review?(rcampbell)
Attachment #8399329 - Flags: review?(pbrosset)
Attachment #8399329 - Flags: review?(past)
Attachment #8399329 - Flags: review?(mratcliffe)
Attachment #8399329 - Flags: review?(fayearthur)
Attachment #8399370 - Flags: review?(rcampbell)
Attachment #8399370 - Flags: review?(pbrosset)
Attachment #8399370 - Flags: review?(past)
Attachment #8399370 - Flags: review?(mratcliffe)
Attachment #8399370 - Flags: review?(fayearthur)
Attachment #8399370 - Flags: feedback?(scrapmachines)
Attachment #8399370 - Flags: feedback?(philipp)
(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.
(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.
So "git push-to-hg" from moz-git-tools creates a new HG patch queue called "patches-git-temp".

Sorry for the bug churn.
Attachment #8399370 - Attachment is obsolete: true
Attachment #8399370 - Flags: review?(rcampbell)
Attachment #8399370 - Flags: review?(pbrosset)
Attachment #8399370 - Flags: review?(past)
Attachment #8399370 - Flags: review?(mratcliffe)
Attachment #8399370 - Flags: review?(fayearthur)
Attachment #8399370 - Flags: feedback?(scrapmachines)
Attachment #8399370 - Flags: feedback?(philipp)
Attachment #8399376 - Flags: review?(rcampbell)
Attachment #8399376 - Flags: review?(pbrosset)
Attachment #8399376 - Flags: review?(past)
Attachment #8399376 - Flags: review?(mratcliffe)
Attachment #8399376 - Flags: review?(fayearthur)
Attachment #8399376 - Flags: feedback?(philipp)
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
Attachment #8399376 - Flags: feedback?(philipp) → feedback+
(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.
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 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
Attachment #8399376 - Flags: review?(fayearthur) → review+
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
Attachment #8399376 - Flags: review?(pbrosset) → review+
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.
(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 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");
Attachment #8399376 - Flags: review?(past) → review+
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.
Attachment #8399376 - Flags: review?(rcampbell) → review+
(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.
(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 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+
Attachment #8399376 - Flags: review?(mratcliffe) → review+
(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.
(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.
(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.
Attached patch breakup-984365.patch v4 (obsolete) — Splinter Review
Greg: please could you take a look at the build file changes in this patch to make sure I've done what's needed?
Attachment #8399376 - Attachment is obsolete: true
Attachment #8402636 - Flags: review+
Attachment #8402636 - Flags: feedback?(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.
Attachment #8402636 - Flags: feedback?(gps) → feedback+
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.
Attachment #8405314 - Flags: review?(gps) → review+
https://hg.mozilla.org/mozilla-central/rev/7b4367158864
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 31
QA Whiteboard: [qa-]
Product: Firefox → DevTools
Product: DevTools → DevTools Graveyard
You need to log in before you can comment on or make changes to this bug.