Closed Bug 1167174 Opened 4 years ago Closed 4 years ago

Custom highlighter test should wait for show request to finish

Categories

(DevTools :: Inspector, defect)

defect
Not set

Tracking

(firefox41 fixed)

RESOLVED FIXED
Firefox 41
Tracking Status
firefox41 --- fixed

People

(Reporter: ochameau, Assigned: ochameau)

References

Details

Attachments

(1 file, 3 obsolete files)

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!)
Assignee: nobody → poirot.alex
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!
Attached patch patch v2 (obsolete) — Splinter Review
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.
Attachment #8608781 - Attachment is obsolete: true
Attachment #8610402 - Flags: review?(pbrosset)
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.
Attachment #8610402 - Flags: review?(pbrosset)
Attached patch patch v3Splinter Review
Right, I missed that!
protocol.js was complaining and stopped doing that by using nullable:boolean...
Attachment #8610402 - Attachment is obsolete: true
Attachment #8610566 - Flags: review?(pbrosset)
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.
Attachment #8610566 - Flags: review?(pbrosset) → review+
https://hg.mozilla.org/mozilla-central/rev/1c0ecccc1915
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 41
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.