874895 - Upgrade EventUtils.js

Status: RESOLVED FIXED

(Testing Graveyard :: Mozmill, defect)

Description

[Andrei Eftimie]

EventUtils.js needs to be updated to the latest version:
http://hg.mozilla.org/mozilla-central/raw-file/feccfce43b59/testing/mochitest/tests/SimpleTest/EventUtils.js

The latest version removes some methods and adds a bunch of others, might need to update code in lots of places.

Comment 1

[Henrik Skupin [:whimboo][⌚️UTC+1]]

JFI we had to use a patch on top of the original module before which can be found here:
https://github.com/mozilla/mozmill/blob/master/mozmill/patches/eventUtils.patch

Not sure if this is still needed or need an update.

Comment 2

[Andrei Eftimie]

Attachment: patch 1

This went easier than expected.

Updated EventUtils.js with the one referenced in comment 1
Also updated the eventUtils.patch with our differences in regard to the new EventUtils.js file

Mutt tests passing, testruns passing.

No patch:
OSX: http://mozmill-crowd.blargon7.com/#/functional/report/90a0b225333a4e0c868fcb2b446930d9
Windows: http://mozmill-crowd.blargon7.com/#/functional/report/90a0b225333a4e0c868fcb2b446d7d80
Linux: http://mozmill-crowd.blargon7.com/#/functional/report/90a0b225333a4e0c868fcb2b44723d2a

Patch applied:
OSX: http://mozmill-crowd.blargon7.com/#/functional/report/90a0b225333a4e0c868fcb2b4467a76f
Windows: http://mozmill-crowd.blargon7.com/#/functional/report/90a0b225333a4e0c868fcb2b446c34f2
Linux: http://mozmill-crowd.blargon7.com/#/functional/report/90a0b225333a4e0c868fcb2b447119d7

Comment 3

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Comment on attachment 753302 [details] [diff] [review]
patch 1

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

The changes to eventUtils.patch seem to be incomplete. So I will not check any changes for EventUtils.js for now. Please check that.

::: mozmill/patches/eventUtils.patch
@@ -1,2 @@
> -diff --git a/mozmill/mozmill/extension/resource/stdlib/EventUtils.js b/mozmill/mozmill/extension/resource/stdlib/EventUtils.js
> -index 27fbbc7..0329d47 100644

You mentioned that you have changed something in the local patch. It's not visible here. Can you please verify.

::: mutt/mutt/tests/js/controller/expected_events.js
@@ +21,4 @@
>    }, "Error", "click() on a text field raises a focus event.");
>  
>    expect.throws(function () {
> +    controller.click(fname, 2, 2, {type: "!keypress"});

Why this change? We are issuing a click here and expecting that an exception gets thrown. When we use !keypress no exception will be thrown and the test itself would fail.

Comment 4

[Andrei Eftimie]

Attachment: patch v2

Seems bugzilla chokes on patch that contains a patch.
Trying a different export.

Comment 5

[Andrei Eftimie]

Attachment: patch v3

3rd time's a charm

Comment 6

[Andrei Eftimie]

Fixed eventUtils.patch so that bugzilla accepts it.

> ::: mutt/mutt/tests/js/controller/expected_events.js
> @@ +21,4 @@
> >    }, "Error", "click() on a text field raises a focus event.");
> >  
> >    expect.throws(function () {
> > +    controller.click(fname, 2, 2, {type: "!keypress"});
> 
> Why this change? We are issuing a click here and expecting that an exception gets thrown. When we use !keypress no exception will be thrown and the test itself would fail.

That's how the _checkExpectedEvent is made:
> var expectEvent = (aExpectedEvent.charAt(0) != "!");

Comment 7

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Comment on attachment 757720 [details] [diff] [review]
patch v3

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

::: mozmill/mozmill/extension/resource/stdlib/EventUtils.js
@@ +611,4 @@
>    var eventHandler = function(event) {
>      var epassed = (!_gSeenEvent && event.originalTarget == aExpectedTarget &&
>                     event.type == type);
> +    is(epassed, true, aTestName + " " + type + " event target " + (_gSeenEvent ? "twice" : ""));

You also want to remove this is() call.

@@ +634,3 @@
>        desc += " not";
> +
> +    if (_gSeenEvent == expectEvent) {

This has to be !==.

::: mozmill/patches/eventUtils.patch
@@ +5,5 @@
>  @@ -1,3 +1,24 @@
> ++/* This Source Code Form is subject to the terms of the Mozilla Public
> ++ * License, v. 2.0. If a copy of the MPL was not distributed with this
> ++ * file, you can obtain one at http://mozilla.org/MPL/2.0/. */
> ++

Please do not modify licenses. Those have to be kept the same. If there is none we have to respect this.

@@ +13,5 @@
> ++                        "synthesizeMouseAtPoint", "synthesizeTouchAtPoint",
> ++                        "synthesizeMouseAtCenter", "synthesizeTouchAtCenter",
> ++                        "synthesizeWheel", "synthesizeKey",
> ++                        "synthesizeMouseExpectEvent", "synthesizeKeyExpectEvent",
> ++                        "disableNonTestMouseEvents", "synthesizeText",

Can you please move disableNonTestMouseEvents up so we have an alphabetical order?

@@ +43,5 @@
> +  *  synthesizeWheel
> +  *  synthesizeKey
> +  *  synthesizeMouseExpectEvent
> +  *  synthesizeKeyExpectEvent
> ++ *  disableNonTestMouseEvents

Same here for the ordering.

@@ +61,5 @@
> +-  // placebo for compat. An easy way to differentiate this from the real thing
> +-  // is whether the property is read-only or not.
> +-  var c = Object.getOwnPropertyDescriptor(window, 'Components');
> +-  return c.value && !c.writable ? Components.interfaces : SpecialPowers.Ci;
> +-});

I would really like to keep this getter so that we do not have to modify that many other lines. Can't we simply add this to the hwindow?

@@ +116,5 @@
>   {
> +   if (typeof(aDOMKeyCode) == "string") {
> +     if (aDOMKeyCode.indexOf("VK_") == 0) {
> +-      aDOMKeyCode = KeyEvent["DOM_" + aDOMKeyCode];
> ++      aDOMKeyCode = getKeyEvent(aWindow)["DOM_" + aDOMKeyCode];

I don't think we would need getKeyEvent() anymore if we make the hwindow the global 'window' object.

@@ +166,5 @@
> +       desc += " not";
> +-    is(_gSeenEvent, expectEvent, aTestName + " " + desc + " fired");
> ++
> ++    if (_gSeenEvent == expectEvent) {
> ++      throw new Error(aTestName + " " + desc + " fired")

=== please and add the missing semicolon.

::: mutt/mutt/tests/js/controller/expected_events.js
@@ +21,4 @@
>    }, "Error", "click() on a text field raises a focus event.");
>  
>    expect.throws(function () {
> +    controller.click(fname, 2, 2, {type: "!keypress"});

For this test we really expect an exception to be thrown by click(). And this will only happen if the condition given will not happen at all. In case of '!keypress' we already say that we do not expect a keypress event. It will register an keypress event handler, which will never be called and _gSeenEvent stays at false. With the negation (!) it will become true.

You will see that you don't have to change it once you have fixed the most likely accidental change in EventUtils.js.

Comment 8

[Andrei Eftimie]

Attachment: patch v4

(In reply to Henrik Skupin (:whimboo) from comment #7)
> Comment on attachment 757720 [details] [diff] [review]
> patch v3
>
> Review of attachment 757720 [details] [diff] [review]:
> -----------------------------------------------------------------
>
> ::: mozmill/mozmill/extension/resource/stdlib/EventUtils.js
> @@ +611,4 @@
> >    var eventHandler = function(event) {
> >      var epassed = (!_gSeenEvent && event.originalTarget == aExpectedTarget &&
> >                     event.type == type);
> > +    is(epassed, true, aTestName + " " + type + " event target " + (_gSeenEvent ? "twice" : ""));
>
> You also want to remove this is() call.

As we've discussed, I've added a is function and kept this calls.

> @@ +634,3 @@
> >        desc += " not";
> > +
> > +    if (_gSeenEvent == expectEvent) {
>
> This has to be !==.

Done

> ::: mozmill/patches/eventUtils.patch
> @@ +5,5 @@
> >  @@ -1,3 +1,24 @@
> > ++/* This Source Code Form is subject to the terms of the Mozilla Public
> > ++ * License, v. 2.0. If a copy of the MPL was not distributed with this
> > ++ * file, you can obtain one at http://mozilla.org/MPL/2.0/. */
> > ++
>
> Please do not modify licenses. Those have to be kept the same. If there is
> none we have to respect this.

There is no license in the original file:
http://hg.mozilla.org/mozilla-central/raw-file/feccfce43b59/testing/mochitest/tests/SimpleTest/EventUtils.js

There is a license in our previous adaptation:
https://github.com/mozilla/mozmill/blob/master/mozmill/mozmill/extension/resource/stdlib/EventUtils.js#L1

In this case, based on your comment I assume we'll have no licence in this file.
I'm taking it out

> @@ +13,5 @@
> > ++                        "synthesizeMouseAtPoint", "synthesizeTouchAtPoint",
> > ++                        "synthesizeMouseAtCenter", "synthesizeTouchAtCenter",
> > ++                        "synthesizeWheel", "synthesizeKey",
> > ++                        "synthesizeMouseExpectEvent", "synthesizeKeyExpectEvent",
> > ++                        "disableNonTestMouseEvents", "synthesizeText",
>
> Can you please move disableNonTestMouseEvents up so we have an alphabetical
> order?

Sure.

>
> @@ +61,5 @@
> > +-  // placebo for compat. An easy way to differentiate this from the real thing
> > +-  // is whether the property is read-only or not.
> > +-  var c = Object.getOwnPropertyDescriptor(window, 'Components');
> > +-  return c.value && !c.writable ? Components.interfaces : SpecialPowers.Ci;
> > +-});
>
> I would really like to keep this getter so that we do not have to modify
> that many other lines. Can't we simply add this to the hwindow?

Yep, seems to be working fine.
Our patching of the original EventUtils.js file is much smaller now.

> @@ +116,5 @@
> >   {
> > +   if (typeof(aDOMKeyCode) == "string") {
> > +     if (aDOMKeyCode.indexOf("VK_") == 0) {
> > +-      aDOMKeyCode = KeyEvent["DOM_" + aDOMKeyCode];
> > ++      aDOMKeyCode = getKeyEvent(aWindow)["DOM_" + aDOMKeyCode];
>
> I don't think we would need getKeyEvent() anymore if we make the hwindow the
> global 'window' object.

Correct, done.

> @@ +166,5 @@
> > +       desc += " not";
> > +-    is(_gSeenEvent, expectEvent, aTestName + " " + desc + " fired");
> > ++
> > ++    if (_gSeenEvent == expectEvent) {
> > ++      throw new Error(aTestName + " " + desc + " fired")
>
> === please and add the missing semicolon.

Done

> ::: mutt/mutt/tests/js/controller/expected_events.js
> @@ +21,4 @@
> >    }, "Error", "click() on a text field raises a focus event.");
> >
> >    expect.throws(function () {
> > +    controller.click(fname, 2, 2, {type: "!keypress"});
>
> For this test we really expect an exception to be thrown by click(). And
> this will only happen if the condition given will not happen at all. In case
> of '!keypress' we already say that we do not expect a keypress event. It
> will register an keypress event handler, which will never be called and
> _gSeenEvent stays at false. With the negation (!) it will become true.
>
> You will see that you don't have to change it once you have fixed the most
> likely accidental change in EventUtils.js.

Correct. Working fine now.

Also a report:
http://mozmill-crowd.blargon7.com/#/functional/report/8aa237803fd4b0e7348adc8bbe05f248

Comment 9

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Comment on attachment 758731 [details] [diff] [review]
patch v4

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

::: mozmill/patches/eventUtils.patch
@@ +16,5 @@
> ++const Ci = Components.interfaces;
> ++const Cc = Components.classes;
> ++
> ++window = Cc["@mozilla.org/appshell/appShellService;1"]
> ++            .getService(Ci["nsIAppShellService"]).hiddenDOMWindow;

You missed the var keyword. And it should be Ci.nsIAppShellService.

Also please fix the indentation.

@@ +43,5 @@
>    */
>   function _parseModifiers(aEvent)
>   {
> +-  const nsIDOMWindowUtils = _EU_Ci.nsIDOMWindowUtils;
> ++  const nsIDOMWindowUtils = window._EU_Ci.nsIDOMWindowUtils;

Lets get _EU_Ci created as a getter attached to the global scope. So we wouldn't have to change that.

@@ +52,5 @@
> +     mval |= nsIDOMWindowUtils.MODIFIER_META;
> +   }
> +   if (aEvent.accelKey) {
> +-    mval |= (navigator.platform.indexOf("Mac") >= 0) ?
> ++    mval |= (window.navigator.platform.indexOf("Mac") >= 0) ?

Same here. Create a new navigator object at the top of the file.

@@ +61,5 @@
> +   if (aChar.length != 1) {
> +     return 0;
> +   }
> +-  const nsIDOMKeyEvent = _EU_Ci.nsIDOMKeyEvent;
> ++  const nsIDOMKeyEvent = window._EU_Ci.nsIDOMKeyEvent;

As above.

@@ +70,5 @@
>   {
> +   if (typeof(aDOMKeyCode) == "string") {
> +     if (aDOMKeyCode.indexOf("VK_") == 0) {
> +-      aDOMKeyCode = KeyEvent["DOM_" + aDOMKeyCode];
> ++      aDOMKeyCode = window.KeyEvent["DOM_" + aDOMKeyCode];

It should also work here.

@@ +110,5 @@
> +     return SpecialPowers.getDOMWindowUtils(aWindow);
> +   }
> +-  if ("SpecialPowers" in parent && parent.SpecialPowers != undefined) {
> +-    return parent.SpecialPowers.getDOMWindowUtils(aWindow);
> +-  }

Please leave this in here. It does not harm us.

@@ +116,5 @@
> +   //TODO: this is assuming we are in chrome space
> +-  return aWindow.QueryInterface(_EU_Ci.nsIInterfaceRequestor).
> +-                               getInterface(_EU_Ci.nsIDOMWindowUtils);
> ++  return aWindow.QueryInterface(window._EU_Ci.nsIInterfaceRequestor).
> ++                               getInterface(window._EU_Ci.nsIDOMWindowUtils);

As mentioned above we most likely don't have to update this code.

Comment 10

[Andrei Eftimie]

Attachment: patch v5

Fixed nits.
Made global shortcuts to used variables, much cleaner eventUtils.patch

> @@ +110,5 @@
> > +     return SpecialPowers.getDOMWindowUtils(aWindow);
> > +   }
> > +-  if ("SpecialPowers" in parent && parent.SpecialPowers != undefined) {
> > +-    return parent.SpecialPowers.getDOMWindowUtils(aWindow);
> > +-  }
> 
> Please leave this in here. It does not harm us.

This actually fails.
We have no "parent".

Comment 11

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Comment on attachment 758786 [details] [diff] [review]
patch v5

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

::: mozmill/patches/eventUtils.patch
@@ +45,5 @@
> +   return ((typeof(id) == "string") ?
> +-    document.getElementById(id) : id);
> +-};
> ++    document.getElementById(id) : id);
> ++};

So what's different here? I don't see it.

@@ +56,4 @@
>     }
> +-  if ("SpecialPowers" in parent && parent.SpecialPowers != undefined) {
> +-    return parent.SpecialPowers.getDOMWindowUtils(aWindow);
> +-  }

Lets define parent globally as window.parent. If this doesn't work with the hidden window, just define it as null.

Comment 12

[Andrei Eftimie]

Attachment: patch v6

Declared "parent".

> ::: mozmill/patches/eventUtils.patch
> @@ +45,5 @@
> > +   return ((typeof(id) == "string") ?
> > +-    document.getElementById(id) : id);
> > +-};
> > ++    document.getElementById(id) : id);
> > ++};
> 
> So what's different here? I don't see it.

There were some trailing whitespaces which my editor automatically removed.
Disabled that to get a cleaner patch ;)

Comment 13

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Comment on attachment 758827 [details] [diff] [review]
patch v6

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

::: mozmill/mozmill/extension/resource/stdlib/EventUtils.js
@@ +40,5 @@
> +// This file is used both in privileged and unprivileged contexts, so we have to
> +// be careful about our access to Components.interfaces. We also want to avoid
> +// naming collisions with anything that might be defined in the scope that imports
> +// this script.
> +window.__defineGetter__('_EU_Ci', function() {

I get framework failures for this line when running the mutt tests:

CRITICAL | Framework Failure | {
  "message": "[JavaScript Error: \"window.__defineGetter__ is not a function\" {file: \"resource://mozmill/stdlib/EventUtils.js\" line: 44}]"
}

As talked this could be that the __defineGetter__ method is not available for hidden windows on platforms others than OS X.

::: mozmill/patches/eventUtils.patch
@@ +30,5 @@
>    * Current methods:
> +@@ -27,6 +49,11 @@ window.__defineGetter__('_EU_Ci', function() {
> +   return c.value && !c.writable ? Components.interfaces : SpecialPowers.Ci;
> + });
> + 

nit: trailing white-space

Comment 14

[Andrei Eftimie]

Attachment: patch v7

Fixed the problem under Linux.

__defineGetter__ seems to be deprecated: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Object/defineGetter

Updated code to use Object.defineProperty

Comment 15

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Given that we need the new version of EventUtils for touch events we have to block mozmill 2.0 on it.

Comment 16

[Andrei Eftimie]

Attachment: patch v8

Updated patch to make our changes to the original file even smaller.

Comment 17

[Andrei Eftimie]

Attachment: patch v9

Hopefully final version.
eventUtils.patch now correctly applies over EventUtils.js via sync_dependency.py

Comment 18

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Comment on attachment 759355 [details] [diff] [review]
patch v9

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

That looks great now! Thanks.

Comment 19

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Landed on master:
https://github.com/mozilla/mozmill/commit/b6f02678eafe72d96ceb2b5c6baa96af61482402