Comment on attachment 9005923 [details] [diff] [review] 1471764.patch [1.0] Review of attachment 9005923 [details] [diff] [review]: ----------------------------------------------------------------- The tests look good, thanks Gabriel! I am not sure what motivated the refactor from addHighlightersToView to flags.testing? I found the previous pattern a bit easier to maintain, but up to you :) ::: devtools/client/inspector/flexbox/flexbox.js @@ +362,5 @@ > // Fetch the flex items for the given flex container and the flex item NodeFronts. > const flexItems = []; > + let flexItemFronts; > + > + try { This method contains 4 try/catch blocks with the exact same comment. Maybe we could wrap most of the method's body in a try catch rather than repeating the pattern? ::: devtools/client/inspector/markup/test/.eslintrc.js @@ +3,5 @@ > module.exports = { > // Extend from the shared list of defined globals for mochitests. > + "extends": "../../../../.eslintrc.mochitests.js", > + "globals": { > + "waitUntilState": true We might want to add this to our .eslintrc.mochitests.js ? ::: devtools/client/inspector/test/shared-head.js @@ +171,5 @@ > * @return {CssComputedView} the computed view > */ > function selectComputedView(inspector) { > inspector.sidebar.select("computedview"); > + return inspector.getPanel("computedview").computedView; Was there a strong reason to stop using this pattern? Was "addHighlightersToView()" called too late here? flags.testing is almost never used in the codebase.

Comment on attachment 9005923 [details] [diff] [review] 1471764.patch [1.0] Review of attachment 9005923 [details] [diff] [review]: ----------------------------------------------------------------- ::: devtools/client/inspector/flexbox/flexbox.js @@ +362,5 @@ > // Fetch the flex items for the given flex container and the flex item NodeFronts. > const flexItems = []; > + let flexItemFronts; > + > + try { Fixed. ::: devtools/client/inspector/markup/test/.eslintrc.js @@ +3,5 @@ > module.exports = { > // Extend from the shared list of defined globals for mochitests. > + "extends": "../../../../.eslintrc.mochitests.js", > + "globals": { > + "waitUntilState": true Fixed. ::: devtools/client/inspector/test/shared-head.js @@ +171,5 @@ > * @return {CssComputedView} the computed view > */ > function selectComputedView(inspector) { > inspector.sidebar.select("computedview"); > + return inspector.getPanel("computedview").computedView; This was recommended by pbro in https://bugzilla.mozilla.org/show_bug.cgi?id=1317102#c16. It was easier to reason the code with the flags.testing compare to figuring out all the entry points in the unit tests when to addHighlightersToView.

(In reply to Gabriel [:gl] (ΦωΦ) from comment #5) > Comment on attachment 9005923 [details] [diff] [review] > 1471764.patch [1.0] > > > ::: devtools/client/inspector/test/shared-head.js > @@ +171,5 @@ > > * @return {CssComputedView} the computed view > > */ > > function selectComputedView(inspector) { > > inspector.sidebar.select("computedview"); > > + return inspector.getPanel("computedview").computedView; > > This was recommended by pbro in > https://bugzilla.mozilla.org/show_bug.cgi?id=1317102#c16. It was easier to > reason the code with the flags.testing compare to figuring out all the entry > points in the unit tests when to addHighlightersToView. Thanks for replying. Reading the review from pbro, it looks like the goal is to avoid adding the listeners twice. Makes sense and I guess handling that in addHighlighters... would be non trivial and hard to understand as well.

Pushed by gabriel.luong@gmail.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/d8ff6c1c6f3a Add unit tests for the toggling the flexbox and grid highlighter from the markup display badges. r=jdescottes

This got backed out for a beta simulation failure, see bug 1488450. It had to be backed out to unblock the last central to beta merge. Feel encouraged to request beta uplift once it has landed on central.

Backout by archaeopteryx@coole-files.de: https://hg.mozilla.org/mozilla-central/rev/bdf475b97f93 Backed out changeset d8ff6c1c6f3a for failing devtools' browser_markup_flex_display_badge.js in beta simulations. a=backout

Pushed by gabriel.luong@gmail.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/3509e0b96bc1 Add unit tests for the toggling the flexbox and grid highlighter from the markup display badges. r=jdescottes