Closed Bug 995184 Opened 6 years ago Closed 6 years ago

Copy the legacy "promise.js" implementation from the Add-on SDK to devtools

Categories

(DevTools :: General, defect)

defect
Not set

Tracking

(Not tracked)

VERIFIED FIXED
Firefox 31

People

(Reporter: Paolo, Assigned: Paolo)

References

Details

(Whiteboard: p=2 s=it-31c-30a-29b.3 [qa-])

Attachments

(1 file, 1 obsolete file)

Attached patch The patch (obsolete) — Splinter Review
The Add-on SDK that will be released in Firefox 31 (the next ESR version) will replace its legacy Promise implementation with a new one that is compliant with the latest DOM Promises specification.

This new implementation will cause some issues with devtools automated testing, because of the difference in the exact order of callback invocations and events.

Most of the work for updating tests has already been done in dependencies of bug 881050, but the remaining cases are difficult enough that they're unlikely to be completed in the required timeframe.

Since devtools is the last remaining consumer of the legacy Promise implementation, it might make sense to copy the implementation to a private devtools module for now, to unblock the Add-on SDK from switching in bug 881047.

The long-term work that Patrick is doing to refactor existing browser-chrome tests for various components will definitely facilitate the final removal of this legacy implementation from devtools in the future.
Attachment #8405352 - Flags: review?(dcamp)
Flags: firefox-backlog?
This was green on the tryserver:

https://tbpl.mozilla.org/?tree=Try&rev=b1e529efdc6c
Flags: firefox-backlog? → firefox-backlog+
Whiteboard: p=2
Comment on attachment 8405352 [details] [diff] [review]
The patch

I guess there are several people who worked on Promises that are qualified to review this patch, anyone wanting to take this?

We need to land this for the ESR and this means only this week remains.
Attachment #8405352 - Flags: review?(past)
Attachment #8405352 - Flags: review?(bbenvie)
Comment on attachment 8405352 [details] [diff] [review]
The patch

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

(In reply to :Paolo Amadini from comment #1)
> This was green on the tryserver:
> 
> https://tbpl.mozilla.org/?tree=Try&rev=b1e529efdc6c

Actually, the B2G errors are very much related to this patch. Pay attention to the M1 B2G desktop test:

07:12:28     INFO -  11741 INFO TEST-START | /tests/toolkit/devtools/apps/tests/test_webapps_actor.html
07:12:28     INFO -  System JS : ERROR (null):0 - uncaught exception: SpecialPowersException: "Error while executing chrome script 'http://mochi.test:8888/tests/toolkit/devtools/apps/tests/debugger-protocol-helper.js':
07:12:28     INFO -  TypeError: this.Components is undefined
07:12:28     INFO -  resource://gre/modules/devtools/deprecated-sync-thenables.js:1"

I'm quite confident that the "DebuggerServer is not defined" errors in gaia-integration tests are related, too.
Attachment #8405352 - Flags: review?(past) → review-
Attached patch Updated patchSplinter Review
(In reply to Panos Astithas [:past] from comment #3)
> Actually, the B2G errors are very much related to this patch. Pay attention
> to the M1 B2G desktop test:

Ah, I may easily have overlooked the results, believing they matched the intermittent failure signature, or I spoke before all the results came in.

Just removing the unneeded line causing the error may improve things:

https://tbpl.mozilla.org/?tree=Try&rev=b20caa6da1bf
Attachment #8405352 - Attachment is obsolete: true
Attachment #8405352 - Flags: review?(dcamp)
Attachment #8405352 - Flags: review?(bbenvie)
Attachment #8410182 - Flags: review?(past)
Comment on attachment 8410182 [details] [diff] [review]
Updated patch

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

I'm glad it was that easy!
Attachment #8410182 - Flags: review?(past) → review+
https://hg.mozilla.org/mozilla-central/rev/1de96b48593e
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 31
Whiteboard: p=2 → p=2 s=it-31c-30a-29b.3 [qa?]
Flags: in-testsuite?
Whiteboard: p=2 s=it-31c-30a-29b.3 [qa?] → p=2 s=it-31c-30a-29b.3 [qa-]
Status: RESOLVED → VERIFIED
Setting in-testsuite+ since the affected tests were updated to support this change.
Flags: in-testsuite? → in-testsuite+
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.