Closed
Bug 1003109
Opened 10 years ago
Closed 10 years ago
Replace `TaskUtils.spawn` with `Task.spawn`
Categories
(Toolkit :: General, defect)
Toolkit
General
Tracking
()
RESOLVED
FIXED
mozilla32
People
(Reporter: poiru, Assigned: poiru)
Details
Attachments
(1 file, 3 obsolete files)
16.46 KB,
patch
|
poiru
:
review+
|
Details | Diff | Splinter Review |
Similar to bug 956678.
Assignee | ||
Comment 1•10 years ago
|
||
Try push: https://tbpl.mozilla.org/?tree=Try&rev=97257c6c46fc
Attachment #8414420 -
Flags: review?(dteller)
Comment 2•10 years ago
|
||
Comment on attachment 8414420 [details] [diff] [review] Replace `TaskUtils.spawn` with `Task.spawn` Review of attachment 8414420 [details] [diff] [review]: ----------------------------------------------------------------- Good initiative and thanks for the patch. ::: toolkit/components/search/nsSearchService.js @@ -339,5 @@ > - * > - * @return {Promise} A promise, resolved successfully if |statement.execute| > - * succeeds, rejected if it fails. > - */ > - executeStatement: function executeStatement(statement, onResult) { Are you sure that TaskUtils.executeStatement is not used? @@ +4466,5 @@ > LOG("metadata writeCommit: done"); > } > ); > // Use our error logging instead of the default one. > + return promise.then(null, () => {}); Just `return promise` here and remove the comment above.
Attachment #8414420 -
Flags: review?(dteller) → feedback+
Assignee | ||
Comment 3•10 years ago
|
||
(In reply to David Rajchenbach Teller [:Yoric] from comment #2) > Are you sure that TaskUtils.executeStatement is not used? Yep, at least based on: http://mxr.mozilla.org/mozilla-central/search?string=executeStatement&find=toolkit/components/search > > // Use our error logging instead of the default one. > > + return promise.then(null, () => {}); > > Just `return promise` here and remove the comment above. Done.
Attachment #8414420 -
Attachment is obsolete: true
Attachment #8414428 -
Flags: review?(dteller)
Comment 4•10 years ago
|
||
Comment on attachment 8414428 [details] [diff] [review] Replace `TaskUtils.spawn` with `Task.spawn` Review of attachment 8414428 [details] [diff] [review]: ----------------------------------------------------------------- Looks good. Could you take the opportunity to convert all the uses of Task.spawn(function() { ... }) to Task.spawn(function*() { ... }) in these files? That's the new and official syntax for generators.
Attachment #8414428 -
Flags: review?(dteller) → review+
Assignee | ||
Comment 5•10 years ago
|
||
(In reply to David Rajchenbach Teller [:Yoric] from comment #4) > Looks good. Could you take the opportunity to convert all the uses of > ... > in these files? That's the new and official syntax for generators. Done. Thanks for the quick reviews!
Attachment #8414428 -
Attachment is obsolete: true
Attachment #8414429 -
Flags: review+
Assignee | ||
Updated•10 years ago
|
Keywords: checkin-needed
Comment 6•10 years ago
|
||
https://hg.mozilla.org/integration/fx-team/rev/77abda00ae6e
Keywords: checkin-needed
Whiteboard: [fixed-in-fx-team]
Comment 7•10 years ago
|
||
Backed out for failures like: https://tbpl.mozilla.org/php/getParsedLog.php?id=38714009&tree=Fx-Team There may also be more, see the rest at: https://tbpl.mozilla.org/?tree=Fx-Team&rev=7de28e99eb68 remote: https://hg.mozilla.org/integration/fx-team/rev/7bb33e71b2a0
Assignee | ||
Comment 8•10 years ago
|
||
(In reply to David Rajchenbach Teller [:Yoric] from comment #4) > Looks good. Could you take the opportunity to convert all the uses of > Task.spawn(function() { > ... > }) > > to > > Task.spawn(function*() { > ... > }) > > in these files? That's the new and official syntax for generators. This was the cause of the bustage so reverting this change.
Attachment #8414429 -
Attachment is obsolete: true
Attachment #8415326 -
Flags: review+
Assignee | ||
Updated•10 years ago
|
Keywords: checkin-needed
Whiteboard: [fixed-in-fx-team]
Comment 9•10 years ago
|
||
When converting function to function*, you should also convert "throw new Task.Result(something)" to "return something"
Comment 10•10 years ago
|
||
We're now requesting (fairly-minimal, but appropriate) Try pushes for checkin-neededs (regardless of the diff) - removing keyword since there isn't one pasted in the bug for the latest version of the patch.
Keywords: checkin-needed
Assignee | ||
Comment 11•10 years ago
|
||
(In reply to Ed Morley [:edmorley UTC+0] from comment #10) > We're now requesting (fairly-minimal, but appropriate) Try pushes for > checkin-neededs (regardless of the diff). Makes sense, try push: https://tbpl.mozilla.org/?tree=Try&rev=755956728cdc
Keywords: checkin-needed
Assignee | ||
Comment 12•10 years ago
|
||
(In reply to Marco Bonardo [:mak] from comment #9) > When converting function to function*, you should also convert "throw new > Task.Result(something)" to "return something" Ah, thanks. I'll file a follow-up to take care of this in other files as well.
Comment 13•10 years ago
|
||
(In reply to Birunthan Mohanathas [:poiru] from comment #12) > (In reply to Marco Bonardo [:mak] from comment #9) > > When converting function to function*, you should also convert "throw new > > Task.Result(something)" to "return something" > > Ah, thanks. I'll file a follow-up to take care of this in other files as > well. sounds good! Thanks.
Comment 14•10 years ago
|
||
https://hg.mozilla.org/integration/fx-team/rev/9dedf7c1b648
Keywords: checkin-needed
Whiteboard: [fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/9dedf7c1b648
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → mozilla32
You need to log in
before you can comment on or make changes to this bug.
Description
•