Closed Bug 988335 Opened 9 years ago Closed 9 years ago

Convert to Promise.jsm in the devtools server actors

Categories

(DevTools :: Debugger, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 31

People

(Reporter: Paolo, Assigned: Paolo)

References

Details

Attachments

(1 file)

Attached patch The patchSplinter Review
This conversion for bug 943517 also doesn't seem problematic:

https://tbpl.mozilla.org/?tree=Try&rev=ced97d6ee3b5
Attachment #8397091 - Flags: review?(nfitzgerald)
Comment on attachment 8397091 [details] [diff] [review]
The patch

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

Looks great!
Attachment #8397091 - Flags: review?(nfitzgerald) → review+
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.
(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?
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).
Blocks: 988997
(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).
https://hg.mozilla.org/mozilla-central/rev/98ac1097f8e2
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 31
Blocks: 996671
QA Whiteboard: [qa-]
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.