988335 - Convert to Promise.jsm in the devtools server actors

Status: RESOLVED FIXED

(DevTools :: Debugger, defect)

Description

[:Paolo Amadini]

Attachment: The patch

This conversion for bug 943517 also doesn't seem problematic:

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

Comment 1

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Comment on attachment 8397091 [details] [diff] [review]
The patch

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

Looks great!

Comment 2

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

Bug 986481 added a |promise| global in every protocol.js-based actor, so just removing the imports should be enough, with the possible exception of webbrowser.js.

Comment 3

[:Paolo Amadini]

(In reply to Panos Astithas [:past] from comment #2)
> Bug 986481 added a |promise| global in every protocol.js-based actor, so
> just removing the imports should be enough, with the possible exception of
> webbrowser.js.

The protocol.js file itself imports the old promises, can that affect which module is actually used here?

Why is webbrowser.js different?

Comment 4

[:Paolo Amadini]

Alternate approach: https://tbpl.mozilla.org/?tree=Try&rev=02ace51e03a9

Comment 5

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

webbrowser.js (and now that I look at it, string.js too) is from the era before protocol.js, when we loaded every actor in the server's scope using loadSubscript. Therefore I'm not sure whether its scope will already contain the promise constructor or not, you could do a quick local test for that.

You are right that protocol.js currently imports the sdk promises, but since it is always loaded with the devtools loader it should already have the promise constructor in scope. Just removing that import from protocol.js should work, unless it implicitly depends in sync promises somewhere (and I don't know that it does).

Comment 6

[:Paolo Amadini]

(In reply to Panos Astithas [:past] from comment #5)
> Just removing that import from protocol.js
> should work, unless it implicitly depends in sync promises somewhere (and I
> don't know that it does).

This is actually one of the things we're trying to figure out in bug 943517. To get the actors variable out of the way, I'll go ahead with the original version of the patch for now, and I filed bug 988997 to take care of the cleanup once protocol.js has been migrated as well (or sooner if that makes sense).

Comment 7

[:Paolo Amadini]

https://hg.mozilla.org/integration/fx-team/rev/98ac1097f8e2

Comment 8

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/98ac1097f8e2