Closed
Bug 1271115
Opened 9 years ago
Closed 8 years ago
Merge ChromeUtils.js into EventUtils.js
Categories
(Testing :: Mochitest, defect)
Testing
Mochitest
Tracking
(firefox51 fixed)
RESOLVED
FIXED
mozilla51
| Tracking | Status | |
|---|---|---|
| firefox51 | --- | fixed |
People
(Reporter: ayg, Unassigned)
References
Details
Attachments
(2 files)
|
(deleted),
patch
|
jmaher
:
review+
|
Details | Diff | Splinter Review |
|
(deleted),
patch
|
Details | Diff | Splinter Review |
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.
|
Reporter | |
Comment 1•9 years ago
|
||
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)
|
|
Reporter | |
Comment 2•9 years ago
|
||
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.
|
|
Reporter | |
Updated•9 years ago
|
|
||
Comment 3•9 years ago
|
||
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-
|
|
Reporter | |
Comment 4•8 years ago
|
||
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 5•8 years ago
|
||
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+
|
|
Reporter | |
Comment 6•8 years ago
|
||
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)
|
|
||
Comment 7•8 years ago
|
||
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)
|
|
Reporter | |
Comment 8•8 years ago
|
||
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
|
||
Comment 10•8 years ago
|
||
| bugherder | ||
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox51:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
You need to log in
before you can comment on or make changes to this bug.
Description
•