Closed Bug 1164906 Opened 4 years ago Closed 4 years ago

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

Categories

(DevTools :: General, defect)

defect
Not set

Tracking

(firefox41 fixed)

RESOLVED FIXED
Firefox 41
Tracking Status
firefox41 --- fixed

People

(Reporter: sjakthol, Assigned: sr71pav, Mentored)

Details

(Whiteboard: [good first bug][lang=js])

Attachments

(1 file, 2 obsolete files)

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
Mentor: mratcliffe
Whiteboard: [good first bug][lang=js]
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.
Attached patch b1164906.patch (obsolete) — Splinter Review
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.
Attachment #8606803 - Flags: review?(mratcliffe)
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.
Attachment #8606803 - Flags: review?(mratcliffe)
Attached patch Patch Rev 1 (obsolete) — Splinter Review
I believe this addresses all of the issues.  Hopefully, I did all the Mercurial commands correctly.
Attachment #8606803 - Attachment is obsolete: true
Attachment #8607243 - Flags: review?(mratcliffe)
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.
Attachment #8607243 - Flags: review?(mratcliffe) → review+
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.
Attached patch Patch Rev 2Splinter Review
Updated to add the title to the patch.  Thanks for the help!

Is there anything else I should know or need to do?
Attachment #8607243 - Attachment is obsolete: true
Attachment #8607773 - Flags: review?(mratcliffe)
Comment on attachment 8607773 [details] [diff] [review]
Patch Rev 2

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

Perfect, r+.
Attachment #8607773 - Flags: review?(mratcliffe) → review+
https://hg.mozilla.org/mozilla-central/rev/8348f771bfdb
Assignee: nobody → sr71pav
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 41
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.