While working on bug 1387128 I realized that we were not waiting correctly for TextEditor.update() completion. Which leads to test failures. http://searchfox.org/mozilla-central/rev/f6dc0e40b51a37c34e1683865395e72e7fca592c/devtools/client/inspector/markup/views/text-editor.js#102-107 this.value.textContent = str; This is done asynchronously, and nothing in these tests was waitng correctly for that to happen: http://searchfox.org/mozilla-central/rev/f6dc0e40b51a37c34e1683865395e72e7fca592c/devtools/client/inspector/test/head.js#791-801 http://searchfox.org/mozilla-central/rev/f6dc0e40b51a37c34e1683865395e72e7fca592c/devtools/client/inspector/markup/test/browser_markup_textcontent_display.js#76 Bug 1387128 slightly changes promise execution order and make these tests fail.

Last try was almost green: https://treeherder.mozilla.org/#/jobs?repo=try&revision=f4a18618bf53ee44028134fb50bb8d576d42abfd&selectedJob=133150363 But there may be some fixes left to do in tests.

Got a green try with some tweaks to devtools/client/inspector/markup/test/browser_markup_tag_edit_09.js. But still suspect devtools/client/inspector/test/browser_inspector_addNode_03.js from being intermittent. https://treeherder.mozilla.org/#/jobs?repo=try&revision=e39168c60aa540d75aff388c81317da386d50bbb&selectedJob=133606097

Getting tests to stay green is challenging, still issues with addNode test: https://treeherder.mozilla.org/#/jobs?repo=try&revision=9d49449179e7854691838c412057eda01d304729&selectedJob=133673519

Oh my... I had not seen this review request, sorry I missed it. And now I'm going to have a hard time reviewing unfortunately. Sorry, my bad. But today isn't good, and I'll be less available next week (traveling), so it's better if someone else picks this up at this stage. I will suggest Gabriel because he knows the inspector code. I'll just ask one question after looking very quickly at the code: is there a risk that this will make node selection more sequential and take more time? await will interrupt the function execution for as long as the promise has not resolved, while before we were doing more things in parallel, right? But maybe our parallel steps were actually not.

(In reply to Patrick Brosset <:pbro> from comment #5) > I'll just ask one question after looking very quickly at the code: is there > a risk that this will make node selection more sequential and take more > time? Yes, that's possible in theory. > await will interrupt the function execution for as long as the promise > has not resolved, while before we were doing more things in parallel, right? > But maybe our parallel steps were actually not. Also likely true. We may parallelize things between parent and content processes if this code involves doing multiple RDP requests. But at the end both client and actors run on the main thread, so that they will end up doing only one thing at a time. My experience on bug 1393086 proved that it is actually extremelly challenging to ensure that code really run in parallel between client and actors and we are missing low level API to really be able to do that in a deterministic way. It will be hard to notice any user visible difference, but I'll try to see if we can have this covered by DAMP. On my side, I'm more concerned about the possible races that such patch could introduce rather than feature completion speed. I'm still having a hard time getting tests to pass. I'm surprised as waiting for more things to happen should make test writing easier as you better know when an action is fully completed.

Comment on attachment 8912234 [details] Bug 1402846 - Wait for TextEditor updates in inspector codebase. https://reviewboard.mozilla.org/r/183608/#review190632 The changes look good to me, but I am gonna pass the final review to bgrins since he also has deeper knowledge regarding the markup compare to me.

Comment on attachment 8912234 [details] Bug 1402846 - Wait for TextEditor updates in inspector codebase. I'm going to clear this review to reflect reality, because I'm not going to be able to help figure out why the addNode tests are failing in the short term. I'm generally in favor of the change though. If nothing else, getting some of the async/await conversion split out into a separate patch and landed could help prevent bitrot and make the existing code easier to follow while the tests get figured out here.