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

RESOLVED FIXED in Firefox 61

Status

enhancement
RESOLVED FIXED
2 years ago
Last year

People

(Reporter: ochameau, Assigned: ochameau)

Tracking

(Blocks 1 bug)

unspecified
Firefox 61
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox61 fixed)

Details

Attachments

(4 attachments, 11 obsolete attachments)

25.05 KB, patch
Details | Diff | Splinter Review
2.01 MB, patch
ochameau
: review+
Details | Diff | Splinter Review
3.07 KB, patch
ochameau
: review+
Details | Diff | Splinter Review
1.10 KB, patch
ochameau
: review+
Details | Diff | Splinter Review
Similar to bug 1437828 but this time focused on devtools/client folder.
Assignee: nobody → poirot.alex
Depends on: 1441527
Depends on: 1441528
Depends on: 1441531
Making some slow progress on client, but it appears to be more challenging than shared and server.
Still orange try:
  https://treeherder.mozilla.org/#/jobs?repo=try&revision=62f50c070466f320e70018cf8779e5ed64ee34fd&selectedJob=164618488
Depends on: 1441792
Depends on: 1442153
Depends on: 1442159
Depends on: 1442160
Depends on: 1443184
Depends on: 1443319
Depends on: 1443470
Depends on: 1443480
I'm heading forward ignoring the following components: debugger, webconsole old tests, gcli, animation inspector (bug 1443480).
And I'm aiming at refactoring right away netmonitor in bug 1443470.
Also, various tests aren't refactored and will be addressed independantly, see all the blocking bugs.
Multi plaforms try:
  https://treeherder.mozilla.org/#/jobs?repo=try&revision=9e319afd533d0621245576316d18a263db14fe51
  Reports no particular error.

And linux one with rebuild 6 to spot intermittents:
  https://treeherder.mozilla.org/#/jobs?repo=try&revision=abd139aa2709a5e071e4b5cafc8209a07700e18e
  Seems to highlight an increase of this existing intermittent:
    bug 1440059 - devtools/client/webconsole/new-console-output/test/mochitest/browser_console_context_menu_entries.js | The context menu is displayed on a network message
  But also seems to introduce a new one:
    devtools/client/inspector/markup/test/browser_markup_toggle_03.js | Container for node EM is expanded

Given these results, I'm wondering if the inspector should be refactored independantly. There is already a lot of blocking bugs related to the inspector...
Green try without inspector refactoring:
  https://treeherder.mozilla.org/#/jobs?repo=try&revision=a7a7b632b21d80ddc0ca790348c1cb6bef5a85a9

So I'm heading to a patch also ignoring the inspector folder. To be fixed in a followup.
Depends on: 1444033
As previous bugs, this is fully automated with:

$ ./obj-firefox-artifact/dist/bin/run-mozilla.sh ./obj-firefox-artifact/dist/bin/xpcshell xpc devtools/client
Attachment #8957362 - Flags: review?(jryans)
xpcshell script can't be configured to whitelist only one particular line when replacing task.jsm usage,
so I'm manually reverting an expected usage of generators:
  * test_middleware-task-03.js
(note that the second usage of the xpcshell script, with --replace-generators, supports ignoring one line...)
Attachment #8957363 - Flags: review?(jryans)
The xpcshell script also warns about possible issues in some automatic replace.
I'm addressing all the reported issues in this changeset.
Attachment #8957364 - Flags: review?(jryans)
This is the second fully automated step, using the xpcshell script, this time to replace all generators to async:
$ ./obj-firefox-artifact/dist/bin/run-mozilla.sh ./obj-firefox-artifact/dist/bin/xpcshell xpc devtools/client --replace-generators

See the first changeset to see which file I'm whitelisting.
Attachment #8957365 - Flags: review?(jryans)
Third fully automated step, as previous bugs, to accomodate eslint:
$ find devtools/client/ -type f -exec sed -i 's/async function(/async function (/g' {} \;
$ git checkout devtools/client/debugger/
$ git checkout devtools/client/commandline/
$ git checkout devtools/client/webconsole/test/
$ git checkout devtools/client/animationinspector/
$ git checkout devtools/client/inspector/
(I'm resetting all the folders I don't convert in client)
Attachment #8957366 - Flags: review?(jryans)
Yet another automated step, there is now a bunch of useless require that would introduce errors in eslint:
$ find devtools/client/ -type f -exec sed -i '/devtools\/shared\/task/d' {} \;
$ git checkout devtools/client/commandline/
$ git checkout devtools/client/debugger/
$ git checkout devtools/client/shared/redux/middleware/task.js
$ git checkout devtools/client/inspector/
I'm also resettings locations where I don't convert to async/await yet.
Now, because of shared-head's Task.jsm require removal,
this would be now missing in tests for tools that aren't converted yet.
So require Task wherever it is still necessary.
Attachment #8957368 - Flags: review?(jryans)
It isn't clear why the xpcshell script didn't replaced these generators,
but this is necessary to get a green try!

I should propably go over the whole codebase and review all the generators that still exists after the Task refactoring...
Attachment #8957369 - Flags: review?(jryans)
This test was racy:
* it wasn't waiting correctly for debugger panel opening (getPanelWhenReady)
* nor was it waiting correctly for debugger to select the expected source (waitUntil)

Some other webconsole test have similar test, but are written differently,
with a test-only event sent from production code...
Attachment #8957371 - Flags: review?(jryans)
Posted patch eslint autofix. (obsolete) — Splinter Review
A final automated change, with ./mach eslint --fix.
Mostly fixes whitespace missing before '(' in "function()".
Attachment #8957373 - Flags: review?(jryans)
Posted patch fix eslint errors manually. (obsolete) — Splinter Review
Finally, a manual patch to address all eslint issues that aren't fixed automatically.
Attachment #8957374 - Flags: review?(jryans)
Attachment #8957367 - Flags: review?(jryans)
Here is a equivalent view of these patches on github:
  https://github.com/ochameau/mozilla-central/commits/task-client
Please feel free to do comments there instead of splinter.
I will only ask you to r+ the final patch in bz in order to be able to land it from here with checkin-needed.
I plan to merge all changeset into a single one once I got your r+, except:
  "Fix race within browser_net_view-source-debugger.js"
and may be "Manually convert generators from devtools/client/memory/test/browser/."
But feel free to tell me to do differently!
Comment on attachment 8957362 [details] [diff] [review]
Convert Task.jsm to async/await in devtools/client.

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

Thanks for working on this! :) I scanned quickly, but it seems reasonable.
Attachment #8957362 - Flags: review?(jryans) → review+
Attachment #8957363 - Flags: review?(jryans) → review+
Comment on attachment 8957364 [details] [diff] [review]
review files reported as "to review" by xpcshell script

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

Added some notes on GH, but looks good overall.
Attachment #8957364 - Flags: review?(jryans) → review+
Comment on attachment 8957365 [details] [diff] [review]
Convert generators to async functions in devtools/client

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

Looks great! :)
Attachment #8957365 - Flags: review?(jryans) → review+
Attachment #8957366 - Flags: review?(jryans) → review+
Attachment #8957367 - Flags: review?(jryans) → review+
Attachment #8957368 - Flags: review?(jryans) → review+
Attachment #8957369 - Flags: review?(jryans) → review+
Attachment #8957371 - Flags: review?(jryans) → review+
Attachment #8957373 - Flags: review?(jryans) → review+
Comment on attachment 8957374 [details] [diff] [review]
fix eslint errors manually.

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

Looks great, some comments on GH.
Attachment #8957374 - Flags: review?(jryans) → review+
Wow, that was quite a monster effort.  Thanks for working on this! :D The GH links were especially helpful since Splinter doesn't show which characters changed in a line.
Try after the fix made to storage test failure:
  https://treeherder.mozilla.org/#/jobs?repo=try&revision=5bfc95070af623f6403e3b3e805fcf9a21e760d9&selectedJob=167187615
It highlights another failure I've not seen before, specific to OSX:
  https://treeherder.mozilla.org/logviewer.html#?job_id=167187615&repo=try
  TEST-UNEXPECTED-FAIL | devtools/client/shadereditor/test/browser_se_aaa_run_first_leaktest.js | Test timed out - 
Error I did not had in try run from comment 3.
Merged patch containing all the previous ones.
I had to revert on Task.jsm refactoring from shadereditor to get green tests.
Attachment #8957954 - Flags: review+
Attachment #8957362 - Attachment is obsolete: true
Attachment #8957363 - Attachment is obsolete: true
Attachment #8957364 - Attachment is obsolete: true
Attachment #8957365 - Attachment is obsolete: true
Attachment #8957366 - Attachment is obsolete: true
Attachment #8957367 - Attachment is obsolete: true
Attachment #8957368 - Attachment is obsolete: true
Attachment #8957373 - Attachment is obsolete: true
Attachment #8957374 - Attachment is obsolete: true
Attachment #8957369 - Attachment is obsolete: true
Attachment #8957371 - Attachment is obsolete: true
Depends on: 1444755
Pushed by btara@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ee2c71862489
Convert Task.jsm to async/await in devtools/client. r=jryans
https://hg.mozilla.org/integration/mozilla-inbound/rev/f3baa6a3bedc
Manually convert generators from devtools/client/memory/test/browser/. r=jryans
https://hg.mozilla.org/integration/mozilla-inbound/rev/746fce6799db
Fix race within browser_net_view-source-debugger.js r=jryans
Keywords: checkin-needed
Backout by ebalazs@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/4931f6f5cebd
Backed out 3 changesets for merge conflicts on a CLOSED TREE
Backout by ebalazs@mozilla.com:
https://hg.mozilla.org/mozilla-central/rev/6f6280b2e169
Backed out 3 changesets for merge conflicts
Oh no...  I guess it's my spacing change. :( I just assumed this would merge before mine, since it was many hours ahead...
I'm trying to repair this series locally...
Pushed by jryans@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/8e5e21375673
Convert Task.jsm to async/await in devtools/client. r=jryans
https://hg.mozilla.org/integration/autoland/rev/f79b38183eba
Manually convert generators from devtools/client/memory/test/browser/. r=jryans
https://hg.mozilla.org/integration/autoland/rev/896864ff931d
Fix race within browser_net_view-source-debugger.js r=jryans
Pushed by jryans@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/9788b85f2fb2
Follow up linting fix in Memory tool tests. r=me
This bug cleaned many devtools/client dirs, but other still remain under the open dependencies.  I'll move them to the parent bug 1365607, so that it's more apparent that there is still work to do here.
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.