1184316 - DevTools Test:SynthesizeMouse conflicts with general browser test version

Status: RESOLVED FIXED

(DevTools :: General, defect)

Description

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

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

Comment 1

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

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?

Comment 2

[Brian Grinstead [:bgrins]]

(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

Comment 3

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Attachment: bug1184316-1.patch

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

Comment 4

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Try push:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=b6f79486e003

Honza

Comment 5

[Brian Grinstead [:bgrins]]

Comment on attachment 8650391 [details] [diff] [review]
bug1184316-1.patch

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

nice

Comment 6

[Patrick Brosset <:pbro>]

(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.

Comment 7

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/20ac929f95e1

Comment 8

[Jan Honza Odvarko [:Honza] (always need-info? me)]

(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

Comment 9

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Not quite fixed until it merges to m-c...

Comment 10

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Ah, thanks!
Honza

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/20ac929f95e1