Now that bug 810490 landed, we can look into switching the Developer Tools to our new "Promises/A+" compliant implementation. This may be tried out on top of "Promise.jsm", so that we can fix the cases where switching to a delayed implementation of the "then" function would break some code. Once the code of the Developer Tools is updated, we can switch the Add-on SDK module "promise.js" to use "Promise.jsm" as its back-end, and continue to import the SDK module to leverage its helper functions. Fixing this requires some patience while going through all the failing tests. For example, the JavaScript Debugger is tricky to convert because it uses promises while enqueueing client-server packets, and delayed resolution of "then" may make those packets go out of order. This would be easy to fix by making each outgoing packet wait on the completion of the previous one, except that an outgoing packet handler may trigger a nested event loop, thus blocking the queue. So, another solution should be found here.

Generally, the steps are the following: - in directory browser/devtools replace all occurrences of "resource://gre/modules/commonjs/sdk/core/promise.js" (the deprecated module) by "resource://gre/modules/Promise.jsm" (the new module); - run the tests, see what breaks, fix the issues with the help of irc channels #introduction and #devtools.

I'll work on this.

How are things going, Panos?

Due to vacations and work weeks I haven't started working on this yet, but it's high on my priority list.

Panos, I'm not sure if you're working on this, but if not I'd be willing to do it.

I've already started and I'm currently going through the test failures. There is still the problem on the server where we want to load the promise library using loadSubscript, which promise.js supported, but Promise.jsm doesn't. That's next on my list.

There are large chunks of GCLI that will break if we switch to fully async promises. The best solution would be to make GCLI use another promise implementation (maybe continue using the addon-sdk one). This should be simple to do. Why does GCLI do this? Originally argument resolution in GCLI was synchronous, but became async about a year ago. Command resolution is implemented using special kind of 'command' argument. Before async arguments, this made lots of sense. However there is nothing inherently async about command resolution. Making command resolution fully async is very painful, so I made use of the addon-sdk's ability to have sync promises, and used a trick to rip the value out of a promise while staying in the same tick of the event loop. I think that the best solution will probably be to stop command resolution from using the argument system, but I need time to investigate.

Here is a patch which contains the mechanical change from sdk's promise.js to Promise.jsm across devtools. A large number of tests fail with it.

Trying to fix the failed tests, I decided to concentrate on a smaller number of tools first, so in this patch I am undoing some of the conversions of the previous patch. This is just a WIP, which makes at least xpcshell and chrome mochitests work by reverting the debugger server changes, as well as gcli changes for the reasons Joe mentioned.

This is my attempt to fix the debugger server issues properly, by splitting Promise.jsm in two files: one small Promise.jsm stub that just loads promise.js, which itself can be loaded using loadSubscript. This is the same approach we use for DevToolsUtils.jsm, which also needs to be loaded in the debugger server. The changes to promise.js (after a straight copy from Promise.jsm) are to remove the EXPORTED_SYMBOLS declaration, convert the methods of the Promise object to properties of 'this' and use an arrow function for all() in order to close over 'this'. With this patch xpcshell tests pass again, as do most of the debugger mochitests (about a dozen still fail). I haven't investigated yet why these failures occur, but I've temporarily disabled the chrome debugging tests, because they caused the test runner to fail spectacularly.

As discussed with Brandon, he is going to pick up this work, so that I can focus on other things.

Brandon, are you working on this?

This patch is mainly just promise.js and Promise.jsm stub.

This patch is an in-progress conversion of the devtools to use Promise.jsm. browser/devtools folders confirmed to work: * app-manager * commandline * fontinspector * layoutview * markupview * profiler * responseivedesign * scratchpad * styleeditor * styleinspector * sourceeditor * tilt * webconsole still have issues: * debugger * framework * inspector * netmonitor * shadereditor The process I've been going through is converting one file at a time to Promise.jsm, running all tests for that tool, and fixing any failures that arise. So some files in the above folders have been converted successfully, but issues still remain with one or more. For example, almost all of framework has been converted except for toolbox-hosts.js. Because of the interconnectedness, it's quite often that the offending code is found somewhere far from where the error arises or the test fails, or that it's some subtle timing dependency between two disparate pieces of code.

(In reply to David Rajchenbach Teller [:Yoric] <needinfo? me> from comment #12) > Brandon, are you working on this? Indeed, I have been working on it. I'll keep this bug more up to date on my progress.

The commits just added to https://github.com/joewalker/gcli/pull/16 (scroll to the bottom) fix bug 934965 and bug 830184 which prevented GCLI using the Promise.jsm form toolkit. I hope this will land fairly soon.

Good to see progress on this conversion, I think it's important both to get the new Promise.jsm features and to reduce code complexity. I see that the new "promise.js" file is loaded with "loadSubScript", and then it calls (apparently successfully) Components.utils.import for "XPCOMUtils.jsm" and "Services.jsm". So, what exactly is preventing us from using Components.utils.import in the first place? If the issue is that you need two distinct "Promise.jsm" instances because the debugger would otherwise stop thinking it's client code (bug 834756), can't we just create a one-line devtools-specific module (say "DebuggerServerPromise.jsm") that only calls loadSubScript on Toolkit's original "Promise.jsm" file, and import the former? It's also not clear to me why the same problem doesn't occur with the imported "Services.jsm" and "XPCOMUtils.jsm" modules.

Fixes the framework failures.

(In reply to :Paolo Amadini from comment #17) > I see that the new "promise.js" file is loaded with "loadSubScript", and > then it calls (apparently successfully) Components.utils.import for > "XPCOMUtils.jsm" and "Services.jsm". So, what exactly is preventing us from > using Components.utils.import in the first place? > > If the issue is that you need two distinct "Promise.jsm" instances because > the debugger would > otherwise stop thinking it's client code (bug 834756), can't we just create > a one-line devtools-specific module (say "DebuggerServerPromise.jsm") that > only calls loadSubScript on Toolkit's original "Promise.jsm" file, and > import the former? > > It's also not clear to me why the same problem doesn't occur with the > imported "Services.jsm" and "XPCOMUtils.jsm" modules. Panos would be able to answer this better than me.

The problem is precisely the one fixed in bug 834756 (which if you recall I had anticipated in bug 834756 comment 16), that the debugger needs to load the promises library in the same compartment as itself, so that it doesn't try to debug it. Unfortunately, the way jsm modules work is at odds with that, as every jsm is loaded in a separate compartment. So the only way that I know of to load a script in the same compartment is loadSubscript, which is why I made this arrangement of a Promise.jsm shell that wraps the promise.js core for all other consumers, while leaving the debugger with the option of just loading the core itself. I think the reason we don't have issues with other jsm modules imported by the debugger is that these modules are only used in the startup phase of the server, before the actual debugging begins, whereas we use promises every time a remote protocol request is being handled.

I'm going to break off the more problematic component conversions into their own bugs and get the bulk of the conversions landed in this bug.

Breaks off the stuff moved into different patches and also some of this into subpieces.

The changes to styleeditor and styleinspector.

The changes for framework. Changing any of gDevTools.jsm, target.js, or toolbox.js causes tests to fail for inspector o_O. This patch includes fixes for one of those tests but I'm still trying to figure out the others. Failures are in browser_inspector_breadcrumbs.js and browser_inspector_pseudoclass_lock.js. I don't know yet what the commonality is between these two. I believe once these two tests are fixed then these patches will be ready for review.

Fixes test failures in inspector except for browser_inspector_pseudoclass-lock.js timing out.

Since this touches so many different areas, I'm splitting everything off into their own bugs so as not to drive reviewers insane.

Brandon, are you still working on this bug?

Yeah, although this is a meta bug now. Progress for each component is tracked in individual bugs.

not a mentorable bug.

Close as WONTFIX since bug 1368456 (and possibly bug 1283869).

Actually, there are still instances of "deprecated-sync-thenables" in the tree that should be converted to Promise. If some of the dependent bugs refer to code that has already been fixed, they can be closed individually.