Closed Bug 1440320 Opened 6 years ago Closed 6 years ago

Convert devtools/shared from Task.jsm/yield to async/await

Categories

(DevTools :: General, enhancement)

Product:
DevTools
Component:
General
Type:
enhancement
Priority:
Not set
Severity:
normal

Tracking

(firefox60 fixed)

Status:
RESOLVED FIXED
Milestone:
Firefox 60
Tracking Flags:
Tracking Status
firefox60 --- fixed

People

(Reporter: ochameau, Assigned: ochameau)

References

Details

Attachments

(2 files, 5 obsolete files)

Bug 1440320 - Removed unused methods from async-utils module.
59 bytes, text/x-review-board-request
jryans
: review+
Details
Bug 1440320 - Convert Task.jsm to async/await in devtools/shared.
59 bytes, text/x-review-board-request
jryans
: review+
Details 
Similar to bug 1437828 but this time focused on devtools/shared folder.
Depends on: 1440322
Assignee: nobody → poirot.alex
As for the server, I split the work with the same kind of intermediate patches to ease the review. But I plan to merge them all before merging.
Also attached the xpcshell script to have it reviewed, but I'll remove it.

Most of the patches at fully automated and boring to review. I read them completely to review them.
The most interesting part to review are:
* the xpcshell script
 - kIgnorePaths list all the files that shouldn't be converted to async
  I ignored:
  * gcli for now. We may try to convert it if we don't remove it soon enough.
  * task.js, as there is no point in modifying it if we want to remove it.
  * css-properties front, because of bug 1440322. This is going to require more analysis.
 - generatorWhitelist and generatorWhitelistPrefixes
  List all the files that uses generators outside of Task usage, i.e. real generators.
  This is one of the biggest task to do while crafting these refactoring patches.
* the "manual fixes" patches
  Address a couple of naive eslint fixes, but also real errors introduced by the xpcshell script.
  It sometimes fails on complex setup.

If you are concerned about converting the client at the same time, I'm making progress on bug 1440321.
I may have it ready next week if I don't hit any big roadblock.
Also, a quick note about the "promise issues regarding client codebase", which also impact shared, by extension.
This is specific to Promise and not Task. This is related to the switch from privileged Promise, that are always alive and never destroyed, to Promise coming from panel's document. These second promises lifecycle is linked to their document. Whenever the document is destroyed, its promises are destroyed and stops resolving. You still get valid JS references to them, still can call resolve/reject callbacks, but they will stop resolving. Thus breaking many test and also introducing real races during toolbox closing.

So Task isn't introducing the same issues AFAIK. This is why I'm trying to do Task refactoring before Promises in shared and client folders.
>  - kIgnorePaths list all the files that shouldn't be converted to async

I also ignore devtools/shared/heapsnapshot/tests/unit/test_HeapAnalyses_takeCensus_04.js, I think, for the same reason as bug 1438121.
Comment on attachment 8953250 [details]
Bug 1440320 - Removed unused methods from async-utils module.

https://reviewboard.mozilla.org/r/222534/#review228914

Thanks for this cleanup! :)
Attachment #8953250 - Flags: review?(jryans) → review+
Comment on attachment 8953251 [details]
Bug 1440320 - Convert Task.jsm to async/await in devtools/shared.

https://reviewboard.mozilla.org/r/222536/#review228916

Looks great to me! :)
Attachment #8953251 - Flags: review?(jryans) → review+
Comment on attachment 8953252 [details]
Bug 1440320 - Put spaces before parentheses in "async function()" pattern.

https://reviewboard.mozilla.org/r/222538/#review228918
Attachment #8953252 - Flags: review?(jryans) → review+
Comment on attachment 8953253 [details]
Bug 1440320 - Get rid of the now useless require("devtools/shared/task").

https://reviewboard.mozilla.org/r/222540/#review228920

Looks good overall! :)

::: devtools/shared/async-utils.js:16
(Diff revision 1)
>   *
>   * See Task documentation at https://developer.mozilla.org/en-US/docs/Mozilla/JavaScript_code_modules/Task.jsm.
>   */
>  
> -var {Task} = require("devtools/shared/task");
>  

Nit: remove extra blank line

::: devtools/shared/security/socket.js:55
(Diff revision 1)
>    return Cc["@mozilla.org/nss_errors_service;1"]
>           .getService(Ci.nsINSSErrorsService);
>  });
>  
> -const { Task } = require("devtools/shared/task");
>  

Nit: remove extra blank line

::: devtools/shared/task.js
(Diff revision 1)
>   * fulfilled.  "Task.spawn" returns a promise that is resolved when the task
>   * completes successfully, or is rejected if an exception occurs.
>   *
>   * -----------------------------------------------------------------------------
>   *
> - * const {Task} = require("devtools/shared/task");

I think we should keep this line.  These are docs for the task lib itself.
Attachment #8953253 - Flags: review?(jryans) → review+
Comment on attachment 8953254 [details]
Bug 1440320 - Convert generators to async functions in devtools/shared

https://reviewboard.mozilla.org/r/222542/#review228922
Attachment #8953254 - Flags: review?(jryans) → review+
Comment on attachment 8953255 [details]
Bug 1440320 - manual fixes

https://reviewboard.mozilla.org/r/222544/#review228926

Looks good! :)

::: commit-message-96e50:1
(Diff revision 1)
> +Bug 1440320 - manual fixes r=jryans

I think the summary should be more detailed, like "Manual cleanup of Task conversion in devtools/shared" or something.
Attachment #8953255 - Flags: review?(jryans) → review+
Great work, thanks for doing this! :D
Attachment #8953249 - Attachment is obsolete: true
Attachment #8953252 - Attachment is obsolete: true
Attachment #8953253 - Attachment is obsolete: true
Attachment #8953254 - Attachment is obsolete: true
Attachment #8953255 - Attachment is obsolete: true
Pushed by apoirot@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/a32e89eb6712
Removed unused methods from async-utils module. r=jryans
https://hg.mozilla.org/integration/autoland/rev/c40bf199f612
Convert Task.jsm to async/await in devtools/shared. r=jryans
https://hg.mozilla.org/mozilla-central/rev/a32e89eb6712
https://hg.mozilla.org/mozilla-central/rev/c40bf199f612
Status: NEW → RESOLVED
Closed: 6 years ago
status-firefox60: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 60
Blocks: 1365607
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: