Closed Bug 1003109 Opened 6 years ago Closed 6 years ago

Replace `TaskUtils.spawn` with `Task.spawn`

Categories

(Toolkit :: General, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla32

People

(Reporter: poiru, Assigned: poiru)

Details

Attachments

(1 file, 3 obsolete files)

Similar to bug 956678.
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+
(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 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+
(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+
Keywords: checkin-needed
(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+
Keywords: checkin-needed
Whiteboard: [fixed-in-fx-team]
When converting function to function*, you should also convert "throw new Task.Result(something)" to "return something"
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
(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
(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.
(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.
https://hg.mozilla.org/mozilla-central/rev/9dedf7c1b648
Status: ASSIGNED → RESOLVED
Closed: 6 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.