Closed Bug 1184316 Opened 6 years ago Closed 6 years ago

DevTools Test:SynthesizeMouse conflicts with general browser test version

Categories

(DevTools :: General, defect)

defect
Not set
normal

Tracking

(firefox43 fixed)

RESOLVED FIXED
Firefox 43
Tracking Status
firefox43 --- fixed

People

(Reporter: jryans, Assigned: Honza)

Details

Attachments

(1 file)

Some DevTools tests use a frame script message "Test:SynthesizeMouse", which the shared DevTools frame-script-utils.js[1] will listen for.  This DevTools version was added in bug 1036324, and landed on 2015-04-21.

The same ability was independently added[2] to all browser tests in bug 1131818, and landed on 2015-04-17 (so, actually 4 days *before* the DevTools version!).

The general browser test version is enabled for all browser tests.

For additional enjoyment, each version listens for the same message name, but takes different arguments.

[1]: https://dxr.mozilla.org/mozilla-central/source/browser/devtools/shared/frame-script-utils.js#222
[2]: https://hg.mozilla.org/mozilla-central/rev/4ddadc870ef6
Brian, it looks like you added the common DevTools version of this pattern.  Should we remove the DevTools version and use the general browser one instead?
Flags: needinfo?(bgrinstead)
(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #1)
> Brian, it looks like you added the common DevTools version of this pattern. 
> Should we remove the DevTools version and use the general browser one
> instead?

Yes we should absolutely use the existing one, especially if it's already enabled by default in all tests.  Looks like ours isn't not too widely used [0] so hopefully converting won't be too big of a deal.  As an aside, if there are other things that we are using that could be generally applicable to other browser tests we should probably look into adding them to the BrowserTestUtils directly.

[0]: https://dxr.mozilla.org/mozilla-central/search?q=Test%3ASynthesizeMouse&redirect=true
Flags: needinfo?(bgrinstead)
I ran into the problem when implementing tests for another issue and here is a quick patch that removes "Test:SynthesizeMouse" content handler from DevTools shared-head and uses the one from Mochikit.

There is yet another "Test:SynthesizeMouse" content handler within devtools/inspector, that could be also removed. But its response message is different: "Test:SynthesizeMouseDone" vs. "Test:SynthesizeMouse" (mochikit). So, adoption would require changing also executeInContent() method in devtools/inspector/test/head.js (to wait for the right response).

For example:

if (name == "Test:SynthesizeMouse") {
  return waitForContentMessage(name + "Done"); 
}

Or convince Mochikit owners to use the same response message.

I don't know if this should be done as part of this report, Patrick?

Honza
Flags: needinfo?(pbrosset)
Attachment #8650391 - Flags: review?(bgrinstead)
Comment on attachment 8650391 [details] [diff] [review]
bug1184316-1.patch

Review of attachment 8650391 [details] [diff] [review]:
-----------------------------------------------------------------

nice
Attachment #8650391 - Flags: review?(bgrinstead) → review+
Assignee: nobody → odvarko
Status: NEW → ASSIGNED
(In reply to Jan Honza Odvarko [:Honza] from comment #3)
> There is yet another "Test:SynthesizeMouse" content handler within
> devtools/inspector, that could be also removed. But its response message is
> different: "Test:SynthesizeMouseDone" vs. "Test:SynthesizeMouse" (mochikit).
> So, adoption would require changing also executeInContent() method in
> devtools/inspector/test/head.js (to wait for the right response).
> 
> For example:
> 
> if (name == "Test:SynthesizeMouse") {
>   return waitForContentMessage(name + "Done"); 
> }
> 
> Or convince Mochikit owners to use the same response message.
> 
> I don't know if this should be done as part of this report, Patrick?
We should definitely get rid of the inspector's synthesizeMouse and use BrowserTestUtils instead.
This being said, bug 1137285 is about to change the way inspector tests are run quite substantially. It's blocked on a big review that I have to do, but when I've done it, and if try is green, inspector tests won't be using a frame script anymore.
Flags: needinfo?(pbrosset)
(In reply to Patrick Brosset [:pbrosset] [:pbro] from comment #6)
> We should definitely get rid of the inspector's synthesizeMouse and use
> BrowserTestUtils instead.
> This being said, bug 1137285 is about to change the way inspector tests are
> run quite substantially. It's blocked on a big review that I have to do, but
> when I've done it, and if try is green, inspector tests won't be using a
> frame script anymore.
OK, let's fix that as part of that bug or another report then.

Honza
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Not quite fixed until it merges to m-c...
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Ah, thanks!
Honza
https://hg.mozilla.org/mozilla-central/rev/20ac929f95e1
Status: REOPENED → RESOLVED
Closed: 6 years ago6 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → Firefox 43
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.