721495 - Passwordmgr tests: Use EventUtils sendChar() and sendKey(), instead of calling synthesizeKey() directly

Status: VERIFIED FIXED

(Toolkit :: Password Manager, defect)

Description

[Karun Dambiec]

Referencing bug Bug 720087, I plan on fixing this in the passwordmgr tests first. Ive created an initial bug for this section.

Comment 1

[Karun Dambiec]

Attachment: Patch for this bug to replace synthesizeKey with sendKey

Here is the patch for this bug for password manager. I have ran the mochitests locally, and have not encountered any errors with this patch.

Comment 2

[Karun Dambiec]

Attachment: Patch for this bug to replace synthesizeKey with sendKey - With code formatting corrected

I added the patch again, with the code formatting fixed. For review.

Comment 3

[Serge Gautherie (:sgautherie)]

Comment on attachment 591916 [details] [diff] [review]
Patch for this bug to replace synthesizeKey with sendKey - With code formatting corrected

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

::: toolkit/components/passwordmgr/test/browser/browser_passwordmgr_sort.js
@@ +113,5 @@
>                          Services.ww.unregisterNotification(arguments.callee);
>                      else if (aTopic == "domwindowopened") {
>                          let win = aSubject.QueryInterface(Ci.nsIDOMEventTarget);
>                          SimpleTest.waitForFocus(function() {
> +                            EventUtils.sendKey("VK_RETURN")

No, this should become
EventUtils.sendKey("RETURN", win)

::: toolkit/components/passwordmgr/test/browser/browser_passwordmgrdlg.js
@@ +123,5 @@
>                          Services.ww.unregisterNotification(arguments.callee);
>                      else if (aTopic == "domwindowopened") {
>                          let win = aSubject.QueryInterface(Ci.nsIDOMEventTarget);
>                          SimpleTest.waitForFocus(function() {
> +			    EventUtils.sendKey("VK_RETURN")

There are still tabs on your new line :-/

Comment 4

[Serge Gautherie (:sgautherie)]

(In reply to Karun Dambiec from comment #1)
> I have ran the mochitests locally, and have not encountered any errors with this patch.

Hum, at least with a debug build, you should have gotten something like
"JavaScript error: , line 0: uncaught exception: Unknown key: VK_VK_RETURN"

Comment 5

[Karun Dambiec]

(In reply to Serge Gautherie (:sgautherie) from comment #4)
> (In reply to Karun Dambiec from comment #1)
> > I have ran the mochitests locally, and have not encountered any errors with this patch.
> 
> Hum, at least with a debug build, you should have gotten something like
> "JavaScript error: , line 0: uncaught exception: Unknown key: VK_VK_RETURN"

Ok, thanks. Maybe I need to have a look at how my build is setup. I will make a new patch for this bug, with it fixed to be RETURN, and not VK_Return.

Comment 6

[Karun Dambiec]

Attachment: Patch for this bug to replace synthesizeKey with sendKey - With code formatting corrected and use of Sendkey corrected

I have fixed the code formatting, and the use of sendKey with this version of the patch

Comment 7

[Serge Gautherie (:sgautherie)]

Comment on attachment 591932 [details] [diff] [review]
Patch for this bug to replace synthesizeKey with sendKey - With code formatting corrected and use of Sendkey corrected

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

::: toolkit/components/passwordmgr/test/browser/browser_passwordmgr_sort.js
@@ +113,5 @@
>                          Services.ww.unregisterNotification(arguments.callee);
>                      else if (aTopic == "domwindowopened") {
>                          let win = aSubject.QueryInterface(Ci.nsIDOMEventTarget);
>                          SimpleTest.waitForFocus(function() {
> +                            EventUtils.sendKey("RETURN", win)

Nit: while here, could you try adding a ';'.

Comment 8

[Karun Dambiec]

(In reply to Serge Gautherie (:sgautherie) from comment #7)
> Comment on attachment 591932 [details] [diff] [review]
> Patch for this bug to replace synthesizeKey with sendKey - With code
> formatting corrected and use of Sendkey corrected
> 
> Review of attachment 591932 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: toolkit/components/passwordmgr/test/browser/browser_passwordmgr_sort.js
> @@ +113,5 @@
> >                          Services.ww.unregisterNotification(arguments.callee);
> >                      else if (aTopic == "domwindowopened") {
> >                          let win = aSubject.QueryInterface(Ci.nsIDOMEventTarget);
> >                          SimpleTest.waitForFocus(function() {
> > +                            EventUtils.sendKey("RETURN", win)
> 
> Nit: while here, could you try adding a ';'.

Yes a ; is good. I was not sure if this was a specific coding practice for the password manager/firefox, and did not want to change it  much initially.

Comment 9

[Karun Dambiec]

Attachment: Patch for this bug to replace synthesizeKey with sendKey - Code Formatting fixed

This is a revision of the previous patch with a ; added to the javascript.

Comment 10

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

Comment on attachment 591941 [details] [diff] [review]
Patch for this bug to replace synthesizeKey with sendKey - Code Formatting fixed

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

Looks good. Thanks for the contribution!

Comment 11

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/ab27cd796845

Comment 12

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

https://hg.mozilla.org/mozilla-central/rev/ab27cd796845

Comment 13

[Serge Gautherie (:sgautherie)]

V.Fixed, per mxr search.