Bug 1161072 highlighter that tests related to custom highlighter (css transform for ex) are not waiting for highlighter.show request to finish before proceeding to the following test. We should wait for this request to finish, and ideally assert it is successfull! (That we actually tried to show the highlighter and not bailed out in various if (xxx) return; there is in this codepath!)

The two tests that are failing in bug 1161072 are: browser/devtools/styleinspector/test/browser_styleinspector_transform-highlighter-02.js browser/devtools/styleinspector/test/browser_styleinspector_transform-highlighter-04.js I'll just correct these two, but there may be some others silently failing!

https://treeherder.mozilla.org/#/jobs?repo=try&revision=85d59a66a923 I could have used waitForHighlighterEvent and check if the actors were dispatching shown/hidden events, but it is better to ensure that the frontend code, HighlightersOverlay correctly calls highlighter.show and that the actor reports that it actually managed to show the highlighter. I had to tweak browser_styleinspector_transform-highlighter-03.js and used regular *async* promises for mocks which makes the tests sligthly more real. This test is still listening for events on the actor, but it would be great if we could come up with similar changes for highlighters hidding.

Comment on attachment 8610402 [details] [diff] [review] patch v2 Review of attachment 8610402 [details] [diff] [review]: ----------------------------------------------------------------- I like this a lot, and I have no concerns about the code changes except for one general thing: you're adding a return value to a protocol.js actor method, this means the signature of the method changes. Now, I have never tested this particular use case yet, but I'm concerned if you connect a toolbox with this patch to an older firefox/b2g/whatever then protocol.js will complain about it. Can you give it a try if you haven't so far? If it's a problem, then we'll need some backward compatibility handling in HighlightersOverlay.

Right, I missed that! protocol.js was complaining and stopped doing that by using nullable:boolean...

Comment on attachment 8610566 [details] [diff] [review] patch v3 Review of attachment 8610566 [details] [diff] [review]: ----------------------------------------------------------------- Looks good now. I'll try to keep the nullable trick in mind, it might come in handy in the future when modifying existing methods.