Closed Bug 1271115 Opened 4 years ago Closed 3 years ago

Merge ChromeUtils.js into EventUtils.js

Categories

(Testing :: Mochitest, defect)

defect
Not set

Tracking

(firefox51 fixed)

RESOLVED FIXED
mozilla51
Tracking Status
firefox51 --- fixed

People

(Reporter: ayg, Unassigned)

References

Details

Attachments

(2 files)

In bug 1269209, I was trying to port mochitest-chrome tests to -plain so that they work on e10s.  One of them depends on ChromeUtils.js.  Bug 1269209 comment 8 suggests importing the synthesizeDrop stuff into EventUtils.js wholesale.  The drop stuff is almost the whole file, so I figured I'd just move all of it.  I have a patch that almost works, with one try failure that I haven't figured out yet.
Note: I will not be working from the end of today until August 15, which is why I didn't assign this bug to myself.  If anyone wants this landed before then, they should take it over.

Almost-green try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=c4305eeb1be9&selectedJob=20423924

I will attach a separate diff between the deleted ChromeUtils.js and the new EventUtils.js so you can see the changes I made.  They all appear to be quite boring.

One thing I don't understand in this patch is the specialpowersAPI.js changes.  I suspect they're somehow wrong, but don't know how.  Without them, at some iteration of the patch I got errors that this.Components was undefined.  Possibly a more correct fix would be to alter the callers somehow?

https://treeherder.mozilla.org/#/jobs?repo=try&revision=15e5a9247d6d
Attachment #8750073 - Flags: review?(jmaher)
This is not actually a patch for the code, it's a diff between two logically different files, but I marked it as a patch on the theory that someone might want to use the diff analysis tools somehow.
Blocks: 1271120
No longer blocks: 1269209
No longer blocks: 1271120
Comment on attachment 8750073 [details] [diff] [review]
0001-Bug-1271115-Merge-ChromeUtils.js-into-EventUtils.js.patch

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

the biggest hurdle here is that now mochitest-plain tests have access to all these functions in eventutils.js and I am not sure if they work smoothly in the land of specialpowers. This could lead to confusion in the future. 

Overall this looks good, but there are a lot of small questions I have about if we need these scripts imported.

::: dom/plugins/test/mochitest/test_clear_site_data.html
@@ +1,5 @@
>  <html>
>  <head>
>    <title>NPAPI ClearSiteData/GetSitesWithData Functionality</title>
>    <script type="application/javascript" src="chrome://mochikit/content/tests/SimpleTest/SimpleTest.js"></script>
> +  <script type="application/javascript" src="chrome://mochikit/content/tests/SimpleTest/EventUtils.js"></script>

can we just delete this?

::: dom/plugins/test/mochitest/test_plugin_tag_clicktoplay.html
@@ +3,5 @@
>    <head>
>      <meta><charset="utf-8"/>
>      <title>Test Modifying Plugin click-to-play Flag</title>
>      <script type="application/javascript" src="chrome://mochikit/content/tests/SimpleTest/SimpleTest.js"></script>
> +    <script type="application/javascript" src="chrome://mochikit/content/tests/SimpleTest/EventUtils.js"></script>

can we just delete this?

::: dom/plugins/test/mochitest/test_refresh_navigator_plugins.html
@@ +4,5 @@
>    <head>
>      <meta><charset="utf-8"/>
>      <title>Test Refreshing navigator.plugins (bug 820708)</title>
>      <script type="application/javascript" src="chrome://mochikit/content/tests/SimpleTest/SimpleTest.js"></script>
> +    <script type="application/javascript" src="chrome://mochikit/content/tests/SimpleTest/EventUtils.js"></script>

can we just delete this?

::: testing/mochitest/chrome/test_sanityPluginUtils.html
@@ +3,5 @@
>  <head>
>    <script type="text/javascript">
>    var start = new Date();
>    </script>
> +  <script type="text/javascript" src="chrome://mochikit/content/tests/SimpleTest/EventUtils.js"></script>

can we just delete this line?

::: toolkit/components/formautofill/test/browser/loader.js
@@ +19,2 @@
>  Services.scriptloader.loadSubScript(
> +  "chrome://mochikit/content/tests/SimpleTest/EventUtils.js", EventUtils);

do we even use eventutils?

::: toolkit/components/formautofill/test/chrome/loader.js
@@ +27,2 @@
>  Services.scriptloader.loadSubScript(
> +  "chrome://mochikit/content/tests/SimpleTest/EventUtils.js", EventUtils);

do we even use event utils?
Attachment #8750073 - Flags: review?(jmaher) → review-
Comment on attachment 8750073 [details] [diff] [review]
0001-Bug-1271115-Merge-ChromeUtils.js-into-EventUtils.js.patch

(In reply to Joel Maher ( :jmaher) from comment #3)
> ::: dom/plugins/test/mochitest/test_clear_site_data.html
> can we just delete this?

No, it breaks the test, because it uses PluginUtils, which is defined in EventUtils (now).

> ::: dom/plugins/test/mochitest/test_plugin_tag_clicktoplay.html
> ::: dom/plugins/test/mochitest/test_refresh_navigator_plugins.html
> can we just delete this?

Seems so.

> ::: testing/mochitest/chrome/test_sanityPluginUtils.html
> can we just delete this line?

Uses PluginUtils.

> ::: toolkit/components/formautofill/test/browser/loader.js
> ::: toolkit/components/formautofill/test/chrome/loader.js
> do we even use event utils?

Nope, you're right, deleting them seems to work fine.

Are you fine with the patch other than that?
Attachment #8750073 - Flags: review- → review?(jmaher)
Comment on attachment 8750073 [details] [diff] [review]
0001-Bug-1271115-Merge-ChromeUtils.js-into-EventUtils.js.patch

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

thanks for this, please update the patch and delete what you can!

::: dom/plugins/test/mochitest/test_clear_site_data.html
@@ +1,5 @@
>  <html>
>  <head>
>    <title>NPAPI ClearSiteData/GetSitesWithData Functionality</title>
>    <script type="application/javascript" src="chrome://mochikit/content/tests/SimpleTest/SimpleTest.js"></script>
> +  <script type="application/javascript" src="chrome://mochikit/content/tests/SimpleTest/EventUtils.js"></script>

can we just delete this?

::: dom/plugins/test/mochitest/test_plugin_tag_clicktoplay.html
@@ +3,5 @@
>    <head>
>      <meta><charset="utf-8"/>
>      <title>Test Modifying Plugin click-to-play Flag</title>
>      <script type="application/javascript" src="chrome://mochikit/content/tests/SimpleTest/SimpleTest.js"></script>
> +    <script type="application/javascript" src="chrome://mochikit/content/tests/SimpleTest/EventUtils.js"></script>

can we just delete this?

::: dom/plugins/test/mochitest/test_refresh_navigator_plugins.html
@@ +4,5 @@
>    <head>
>      <meta><charset="utf-8"/>
>      <title>Test Refreshing navigator.plugins (bug 820708)</title>
>      <script type="application/javascript" src="chrome://mochikit/content/tests/SimpleTest/SimpleTest.js"></script>
> +    <script type="application/javascript" src="chrome://mochikit/content/tests/SimpleTest/EventUtils.js"></script>

can we just delete this?

::: testing/mochitest/chrome/test_sanityPluginUtils.html
@@ +3,5 @@
>  <head>
>    <script type="text/javascript">
>    var start = new Date();
>    </script>
> +  <script type="text/javascript" src="chrome://mochikit/content/tests/SimpleTest/EventUtils.js"></script>

can we just delete this line?

::: toolkit/components/formautofill/test/browser/loader.js
@@ +19,2 @@
>  Services.scriptloader.loadSubScript(
> +  "chrome://mochikit/content/tests/SimpleTest/EventUtils.js", EventUtils);

do we even use eventutils?

::: toolkit/components/formautofill/test/chrome/loader.js
@@ +27,2 @@
>  Services.scriptloader.loadSubScript(
> +  "chrome://mochikit/content/tests/SimpleTest/EventUtils.js", EventUtils);

do we even use event utils?
Attachment #8750073 - Flags: review?(jmaher) → review+
This caused two failures that I wasn't able to debug:

https://treeherder.mozilla.org/logviewer.html#?job_id=26200045&repo=try

Do you know how to fix them?
Flags: needinfo?(jmaher)
just looking at the failures, it looks as though we have an incorrect value for the 'window' variable.  Is it possible that the changes here are overwriting a definition of window (or win in many cases)?
Flags: needinfo?(jmaher)
Conclusion from IRC: there are now two redundant definitions of synthesizeQueryTextContent, one with three arguments and one with four.  Dropping the three-argument version should fix the problem.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=aa6d38d1df3a
Pushed by ayg@aryeh.name:
https://hg.mozilla.org/integration/mozilla-inbound/rev/291ac62acc1d
Merge ChromeUtils.js into EventUtils.js; r=jmaher
https://hg.mozilla.org/mozilla-central/rev/291ac62acc1d
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
You need to log in before you can comment on or make changes to this bug.