1164906 - DevTools tests should use EventUtils.sendString instead of looping and calling EventUtils.sendChar for each char

Status: RESOLVED FIXED

(DevTools :: General, defect)

Description

[Sami Jaktholm]

While working on bug 1158634 I discovered EventUtils.sendString [1] which calls EventUtils.sendChar for each char in the string.

Many DevTools tests do something like:
> for (let char of string) {
>   EventUtils.sendChar(char);
> }

This is exactly what EventUtils.sendString does and the tests should use it instead.

A quick grep reveals that following tests could use EventUtils.sendString:
- browser_dbg_watch-expressions-01.js
- browser_inplace-editor-01.js
- browser_ruleview_add-property-cancel_03.js
- browser_ruleview_edit-property_01.js
- browser_ruleview_edit-property_02.js
- browser_ruleview_edit-property_03.js
- browser_ruleview_edit-property-commit.js
- browser_ruleview_livepreview.js
- browser_ruleview_multiple-properties-unfinished_01.js
- browser_styleinspector_context-menu-copy-color_02.js

[1] https://dxr.mozilla.org/mozilla-central/source/testing/mochitest/tests/SimpleTest/EventUtils.js#187

Comment 1

[John Pavlicek]

I'd like to take a shot at this, as it seems simple enough.  Just replace the existing for-loops with EventUtils.sendString  It's my first time, though, so I may need some help with the overall process.

Comment 2

[John Pavlicek]

Attachment: b1164906.patch

I tried to work through the file list replacing with EventUtils.sendString.  There were a couple, though, that I left as I was unsure about them.

The first was in browser_ruleview_edit-property_02.js.  I placed a comment as I wasn't sure if the check on the warning triangle was needed after each character.

The second was in browser_ruleview_edit-property_03.js.  I tried replacing it before I figured out that it was sending specific special key presses.  As I worked through the rest of the files, I thought maybe it could be switched to EventUtils.synthesizeKey, but I wasn't sure if there was an advantage to this or not, so I left it alone before discussion.

Comment 3

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Comment on attachment 8606803 [details] [diff] [review]
b1164906.patch

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

The description in your patch (line 6) should use me as the reviewer and should not have square brackets. Let's simplify it at the same time:
Bug 1164906 - Use EventUtils.sendString instead of looping and calling EventUtils.sendChar in DevTools tests r=mratcliffe

(In reply to John Pavlicek from comment #2)
> Created attachment 8606803 [details] [diff] [review]
> b1164906.patch
> 
> I tried to work through the file list replacing with EventUtils.sendString. 
> There were a couple, though, that I left as I was unsure about them.
> 
> The first was in browser_ruleview_edit-property_02.js.  I placed a comment
> as I wasn't sure if the check on the warning triangle was needed after each
> character.
> 

You are right, we need to use sendChar here. You should update the comment according to my suggestions below to explain why we are still using sendChar.

> The second was in browser_ruleview_edit-property_03.js.  I tried replacing
> it before I figured out that it was sending specific special key presses. 
> As I worked through the rest of the files, I thought maybe it could be
> switched to EventUtils.synthesizeKey, but I wasn't sure if there was an
> advantage to this or not, so I left it alone before discussion.

We are best leaving that as it is.

Just a couple of small things that need addressing. When you have done that please ask me for review again.

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

::: browser/devtools/styleinspector/test/browser_ruleview_edit-property_02.js
@@ +45,5 @@
>    input.select();
>  
>    info("Entering property name \"border-color\" followed by a colon to focus the value");
>    let onFocus = once(idRuleEditor.element, "focus", true);
> +  EventUtils.sendString("border-color:",ruleView.doc.defaultView);

Nit: Add a space after the comma

@@ +57,5 @@
>    ok(input.selectionStart === 0 && input.selectionEnd === input.value.length, "Editor contents are selected.");
>  
>    info("Entering a value following by a semi-colon to commit it");
>    let onBlur = once(editor.input, "blur");
> +  // Think this needs to stay as sendChar as for-loop is testing propedit.warning.hidden

We should change this comment to:
// Use sendChar() to pass each character as a string so that we can test propEditor.warning.hidden after each character.

Comment 4

[John Pavlicek]

Attachment: Patch Rev 1

I believe this addresses all of the issues.  Hopefully, I did all the Mercurial commands correctly.

Comment 5

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Comment on attachment 8607243 [details] [diff] [review]
Patch Rev 1

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

Awesome work, r+.

We are still waiting for some results from try, but this will land soon.

Comment 6

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Your new patch has no subject line, it should have a line like this where it currently shows reviewer:
Bug 1164906 - Use EventUtils.sendString instead of looping and calling EventUtils.sendChar in DevTools tests r=mratcliffe

You can add it using hg qref -e. If you need more help with this then please let us know.

Comment 7

[John Pavlicek]

Attachment: Patch Rev 2

Updated to add the title to the patch.  Thanks for the help!

Is there anything else I should know or need to do?

Comment 8

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Comment on attachment 8607773 [details] [diff] [review]
Patch Rev 2

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

Perfect, r+.

Comment 9

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/8348f771bfdb

Comment 10

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/8348f771bfdb