convert uses of "defer" to "new Promise" in client/shared (except in test/, vendor/ and widgets/ subdirs)

RESOLVED FIXED in Firefox 57

Status

P3
enhancement
RESOLVED FIXED
a year ago
7 months ago

People

(Reporter: tera_1225, Assigned: tera_1225)

Tracking

(Blocks: 1 bug)

57 Branch
Firefox 57

Firefox Tracking Flags

(firefox57 fixed)

Details

Attachments

(3 attachments, 1 obsolete attachment)

(Assignee)

Description

a year ago
User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.12; rv:57.0) Gecko/20100101 Firefox/57.0
Build ID: 20170907220212
(Assignee)

Updated

a year ago
Blocks: 1283869
Severity: normal → enhancement
Component: Untriaged → Developer Tools: Shared Components
Priority: -- → P3
(Assignee)

Comment 1

a year ago
As a segment of https://bugzilla.mozilla.org/show_bug.cgi?id=1283869
File list:
$ grep -R "defer()" . | grep -v "/test/" | grep -v "/vendor/" | grep -v "/widgets" | sed s/:.*// | uniq
./AppCacheUtils.jsm
./developer-toolbar.js
./doorhanger.js
./frame-script-utils.js
./getjson.js
./poller.js
./redux/middleware/promise.js

There are quite a few in there so I thought it best to leave /test/ /vendor/ and /widgets/ for a later bug.
(Assignee)

Comment 2

a year ago
Created attachment 8906138 [details] [diff] [review]
Proposed patch
Attachment #8906138 - Flags: review?(odvarko)
Comment on attachment 8906138 [details] [diff] [review]
Proposed patch

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

Thanks for the patch, looks good to me!

R+ assuming my comment is resolved and try is green!

Try push:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=5d82b099c6e3327b1983d65e623ccd95b86cd5dc

Honza

::: devtools/client/shared/AppCacheUtils.jsm
@@ +35,3 @@
>  var { gDevTools } = require("devtools/client/framework/devtools");
>  var Services = require("Services");
>  var promise = require("promise");

`promise` is never used, can be removed.
Attachment #8906138 - Flags: review?(odvarko) → review+
(Assignee)

Comment 4

a year ago
Created attachment 8907302 [details] [diff] [review]
Address issue raised by [:Honza] about previous patch, and extra lines caught by ES linter.

I'm sorry this is in a separate patch... Couldn't quite get on top of combining it into the previous patch (I'm new to mercurial).
Attachment #8907302 - Flags: review?(odvarko)
Comment on attachment 8907302 [details] [diff] [review]
Address issue raised by [:Honza] about previous patch, and extra lines caught by ES linter.

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

Thanks for the update!

Honza
Attachment #8907302 - Flags: review?(odvarko) → review+
Please push to try yet before landing

Honza
(Assignee)

Comment 7

a year ago
Hi Jan. Glad everything is ok for you. Can you possible perform the push to TRY for me as I don't have access myself...
(Assignee)

Comment 8

a year ago
Hi Honza, even (sorry...)
(Assignee)

Comment 9

a year ago
Created attachment 8907853 [details] [diff] [review]
Another fix for errors caught by ES lint: a missing empty line at end of file, and logic error causing promise chain rejection.
Attachment #8907853 - Flags: review?(odvarko)
Comment on attachment 8907853 [details] [diff] [review]
Another fix for errors caught by ES lint: a missing empty line at end of file, and logic error causing promise chain rejection.

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

Thanks for the update!

Honza
Attachment #8907853 - Flags: review?(odvarko) → review+
(Assignee)

Comment 11

a year ago
Created attachment 8908192 [details] [diff] [review]
Bug1398329-4.patch

Woops, wrong file... in last attachment 8907853 [details] [diff] [review]
Attachment #8907853 - Attachment is obsolete: true
Attachment #8908192 - Flags: review?(nchevobbe)

Updated

a year ago
Assignee: nobody → tera_1225
Status: NEW → ASSIGNED
(Assignee)

Comment 12

a year ago
Seems like the tests are ok, aside from a few that are intermittent failures that already have bugs.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=644bf92233d13d2378aecdc5a5629e9d695772db&selectedJob=131105247
Comment on attachment 8908192 [details] [diff] [review]
Bug1398329-4.patch

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

Looks good to me. Thanks !
Attachment #8908192 - Flags: review?(nchevobbe) → review+

Updated

a year ago
Keywords: checkin-needed
In the future, please fold your follow-up patches into the main patch before requesting checkin.

Comment 15

a year ago
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e5d3009b7d0a
Convert uses of defer to new Promise in client/shared (except in test/, vendor/ and widgets/ subdirs). r=Honza, r=nchevobbe
Keywords: checkin-needed
(Assignee)

Comment 16

a year ago
[:RyanVM] Sorry about the multiple patches... I'm new to mercurial and trying to get the hang of the patch system. Have been setting up MozReview also so should be less trouble in the future.
Thanks for the checkin.
Regards,
Mark.
https://hg.mozilla.org/mozilla-central/rev/e5d3009b7d0a
Status: ASSIGNED → RESOLVED
Last Resolved: a year ago
status-firefox57: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 57

Updated

7 months ago
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.