Closed
Bug 1440321
Opened 5 years ago
Closed 5 years ago
Convert devtools/client from Task.jsm/yield to async/await
Categories
(DevTools :: General, enhancement)
DevTools
General
Tracking
(firefox61 fixed)
RESOLVED
FIXED
Firefox 61
Tracking | Status | |
---|---|---|
firefox61 | --- | fixed |
People
(Reporter: ochameau, Assigned: ochameau)
References
Details
Attachments
(4 files, 11 obsolete files)
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 | ||
Updated•5 years ago
|
Assignee: nobody → poirot.alex
Assignee | ||
Comment 1•5 years ago
|
||
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
Assignee | ||
Comment 2•5 years ago
|
||
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.
Assignee | ||
Comment 3•5 years ago
|
||
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...
Assignee | ||
Comment 4•5 years ago
|
||
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.
Assignee | ||
Comment 5•5 years ago
|
||
Assignee | ||
Comment 6•5 years ago
|
||
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)
Assignee | ||
Comment 7•5 years ago
|
||
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)
Assignee | ||
Comment 8•5 years ago
|
||
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)
Assignee | ||
Comment 9•5 years ago
|
||
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)
Assignee | ||
Comment 10•5 years ago
|
||
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)
Assignee | ||
Comment 11•5 years ago
|
||
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.
Assignee | ||
Comment 12•5 years ago
|
||
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)
Assignee | ||
Comment 13•5 years ago
|
||
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)
Assignee | ||
Comment 14•5 years ago
|
||
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)
Assignee | ||
Comment 15•5 years ago
|
||
A final automated change, with ./mach eslint --fix. Mostly fixes whitespace missing before '(' in "function()".
Attachment #8957373 -
Flags: review?(jryans)
Assignee | ||
Comment 16•5 years ago
|
||
Finally, a manual patch to address all eslint issues that aren't fixed automatically.
Attachment #8957374 -
Flags: review?(jryans)
Assignee | ||
Updated•5 years ago
|
Attachment #8957367 -
Flags: review?(jryans)
Assignee | ||
Comment 17•5 years ago
|
||
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.
Assignee | ||
Comment 23•5 years ago
|
||
One last try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=6f122715c428e329431ec789fc50449c3fb575d0
Assignee | ||
Comment 24•5 years ago
|
||
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.
Assignee | ||
Comment 25•5 years ago
|
||
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+
Assignee | ||
Updated•5 years ago
|
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
Assignee | ||
Comment 26•5 years ago
|
||
Attachment #8957955 -
Flags: review+
Assignee | ||
Updated•5 years ago
|
Attachment #8957369 -
Attachment is obsolete: true
Assignee | ||
Comment 27•5 years ago
|
||
Attachment #8957956 -
Flags: review+
Assignee | ||
Updated•5 years ago
|
Attachment #8957371 -
Attachment is obsolete: true
Assignee | ||
Comment 28•5 years ago
|
||
Finally, a multi platform *green* try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=2e3cc75795988dc2c4b47f8b4df280aa97ca1f60
Keywords: checkin-needed
Comment 29•5 years ago
|
||
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
Comment 30•5 years ago
|
||
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
Comment 31•5 years ago
|
||
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...
Comment 34•5 years ago
|
||
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
Comment 35•5 years ago
|
||
Pushed by jryans@gmail.com: https://hg.mozilla.org/integration/autoland/rev/9788b85f2fb2 Follow up linting fix in Memory tool tests. r=me
Comment 36•5 years ago
|
||
bugherder |
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
Status: NEW → RESOLVED
Closed: 5 years ago
status-firefox61:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 61
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.
Updated•5 years ago
|
Product: Firefox → DevTools
You need to log in
before you can comment on or make changes to this bug.
Description
•