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

Status: RESOLVED FIXED

(DevTools :: General, enhancement)

Description

[Alexandre Poirot [:ochameau]]

Similar to bug 1437828 but this time focused on devtools/client folder.

Comment 1

[Alexandre Poirot [:ochameau]]

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

Comment 2

[Alexandre Poirot [:ochameau]]

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.

Comment 3

[Alexandre Poirot [:ochameau]]

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...

Comment 4

[Alexandre Poirot [:ochameau]]

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.

Comment 5

[Alexandre Poirot [:ochameau]]

Attachment: xpcshell script used to convert files, coming from bug 1353542

Comment 6

[Alexandre Poirot [:ochameau]]

Attachment: Convert Task.jsm to async/await in devtools/client.

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

Comment 7

[Alexandre Poirot [:ochameau]]

Attachment: Manually revert generator that xpcshell script can't have whitelist for.

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...)

Comment 8

[Alexandre Poirot [:ochameau]]

Attachment: review files reported as "to review" by xpcshell script

The xpcshell script also warns about possible issues in some automatic replace.
I'm addressing all the reported issues in this changeset.

Comment 9

[Alexandre Poirot [:ochameau]]

Attachment: Convert generators to async functions in devtools/client

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.

Comment 10

[Alexandre Poirot [:ochameau]]

Attachment: Put spaces before parentheses in "async function()" pattern in devtools/client.

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)

Comment 11

[Alexandre Poirot [:ochameau]]

Attachment: Get rid of the now useless require("devtools/shared/task").

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.

Comment 12

[Alexandre Poirot [:ochameau]]

Attachment: Import Task.jsm from tests that still use it.

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.

Comment 13

[Alexandre Poirot [:ochameau]]

Attachment: Manually convert generators from devtools/client/memory/test/browser/.

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...

Comment 14

[Alexandre Poirot [:ochameau]]

Attachment: Fix race within browser_net_view-source-debugger.js

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...

Comment 15

[Alexandre Poirot [:ochameau]]

Attachment: eslint autofix.

A final automated change, with ./mach eslint --fix.
Mostly fixes whitespace missing before '(' in "function()".

Comment 16

[Alexandre Poirot [:ochameau]]

Attachment: fix eslint errors manually.

Finally, a manual patch to address all eslint issues that aren't fixed automatically.

Comment 17

[Alexandre Poirot [:ochameau]]

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 18

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

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.

Comment 19

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

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.

Comment 20

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Comment on attachment 8957365 [details] [diff] [review]
Convert generators to async functions in devtools/client

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

Looks great! :)

Comment 21

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Comment on attachment 8957374 [details] [diff] [review]
fix eslint errors manually.

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

Looks great, some comments on GH.

Comment 22

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

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.

Comment 23

[Alexandre Poirot [:ochameau]]

One last try push:
  https://treeherder.mozilla.org/#/jobs?repo=try&revision=6f122715c428e329431ec789fc50449c3fb575d0

Comment 24

[Alexandre Poirot [:ochameau]]

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.

Comment 25

[Alexandre Poirot [:ochameau]]

Attachment: Convert Task.jsm to async/await in devtools/client.

Merged patch containing all the previous ones.
I had to revert on Task.jsm refactoring from shadereditor to get green tests.

Comment 26

[Alexandre Poirot [:ochameau]]

Attachment: Manually convert generators from devtools/client/memory/test/browser/

Comment 27

[Alexandre Poirot [:ochameau]]

Attachment: Fix race within browser_net_view-source-debugger.js

Comment 28

[Alexandre Poirot [:ochameau]]

Finally, a multi platform *green* try:
  https://treeherder.mozilla.org/#/jobs?repo=try&revision=2e3cc75795988dc2c4b47f8b4df280aa97ca1f60

Comment 29

[Pulsebot]

Pushed by [e-mail]:
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

Comment 30

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/4931f6f5cebd
Backed out 3 changesets for merge conflicts on a CLOSED TREE

Comment 31

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/mozilla-central/rev/6f6280b2e169
Backed out 3 changesets for merge conflicts

Comment 32

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Oh no...  I guess it's my spacing change. :( I just assumed this would merge before mine, since it was many hours ahead...

Comment 33

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

I'm trying to repair this series locally...

Comment 34

[Pulsebot]

Pushed by [e-mail]:
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

Comment 35

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/9788b85f2fb2
Follow up linting fix in Memory tool tests. r=me

Comment 36

[Narcis Beleuzu [:NarcisB]]

https://hg.mozilla.org/mozilla-central/rev/8e5e21375673
https://hg.mozilla.org/mozilla-central/rev/f79b38183eba
https://hg.mozilla.org/mozilla-central/rev/896864ff931d
https://hg.mozilla.org/mozilla-central/rev/9788b85f2fb2

Comment 37

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

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.