Closed Bug 1263016 Opened 4 years ago Closed 4 years ago

Fix test_bug451540.xul to run in e10s

Categories

(Toolkit :: Find Toolbar, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla48
Tracking Status
firefox48 --- fixed

People

(Reporter: jaws, Assigned: jaws)

References

Details

Attachments

(1 file)

No description provided.
Attachment #8739625 - Flags: review?(mdeboer) → review+
Comment on attachment 8739625 [details]
MozReview Request: Bug 1263016 - Fix test_bug451540.xul to run in e10s. r?mikedeboer

https://reviewboard.mozilla.org/r/45323/#review41853

r=me with the missing `yield`s. I'd recommend adding a `SimpleTest.requestLongerTimeout(2)` pre-emptively, because this test will run twice as long and is quite large with many RPCs now; I foresee timeouts on OSX 10.6 on infra without it.

::: toolkit/content/tests/chrome/bug451540_window.xul:8
(Diff revision 1)
>  <!-- 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/. -->
>  
>  <?xml-stylesheet href="chrome://global/skin" type="text/css"?>
> +<?xml-stylesheet href="chrome://mochikit/content/tests/SimpleTest/test.css"?>

Oh, you don't need the type declaration? I indeed like the one-liner better ;)

::: toolkit/content/tests/chrome/bug451540_window.xul:82
(Diff revision 1)
> +        let element = doc.getElementById(aElementId);
> +        element.focus();
> +      });
> +      info(`focused element`);
> +      if (navigator.platform.indexOf("Mac") >= 0) {
> +        BrowserTestUtils.synthesizeKey("VK_LEFT", {metaKey: true}, gBrowser);

I think you forgot to add a `yield` here.

::: toolkit/content/tests/chrome/bug451540_window.xul:88
(Diff revision 1)
> +      } else {
> +        BrowserTestUtils.synthesizeKey("VK_HOME", {}, gBrowser);
> +      }
>      }
>  
> -    function synthesizeKey(target, isKeyCode, key, ctlKey, shiftKey, metaKey) {
> +    function* testSelection(aElementId, aTestTypeText, aExpectedRangeCount, aMessage) {

nit: I think we deprecated the `aFoo` style of function argument naming in favor of `foo`.

::: toolkit/content/tests/chrome/bug451540_window.xul:95
(Diff revision 1)
> -        var event = document.createEvent("KeyEvents");
> -        if (isKeyCode) {
> -          event.initKeyEvent("keypress", true, true, null, ctlKey, false,
> -                             shiftKey, metaKey, key, 0);
> -        } else {
> -          event.initKeyEvent("keypress", true, true, null, ctlKey, false,
> +        let {aElementId, aTestTypeText, aExpectedRangeCount, aMessage} = args;
> +        let doc = content.document;
> +        let aElement = doc.getElementById(aElementId);
> +        let controller = aElement.editor.selectionController;
> +        let selection = controller.getSelection(controller.SELECTION_FIND);
> +        is(selection.rangeCount, aExpectedRangeCount, aMessage);

nit: s/is/Assert.equal/

::: toolkit/content/tests/chrome/bug451540_window.xul:200
(Diff revision 1)
> -        // Test 11: Multiple matches within one editor (part 3)
> +      // Test 11: Multiple matches within one editor (part 3)
> -        // Check second match remains highlighted after deleting selection
> +      // Check second match remains highlighted after deleting selection
> -        // in first match, and that second match highlighting gets correctly
> +      // in first match, and that second match highlighting gets correctly
> -        // removed when it has a selection deleted from it
> +      // removed when it has a selection deleted from it
> -        resetForNextTest(aElement, SEARCH_TEXT + " " + SEARCH_TEXT);
> -        synthesizeKey(aElement, true, KeyEvent.DOM_VK_RIGHT,
> +      yield resetForNextTest(aElementId, SEARCH_TEXT + " " + SEARCH_TEXT);
> +      yield BrowserTestUtils.synthesizeKey("VK_RIGHT", {shiftKey: true}, gBrowser);

I generally like the style of adding a space at the start and end of an object literal, like `{ shiftKey: true }`, but that's up to you.
https://hg.mozilla.org/mozilla-central/rev/1af4c6693497
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
(In reply to Mike de Boer [:mikedeboer] from comment #3)
> ::: toolkit/content/tests/chrome/bug451540_window.xul:88
> > -    function synthesizeKey(target, isKeyCode, key, ctlKey, shiftKey, metaKey) {
> > +    function* testSelection(aElementId, aTestTypeText, aExpectedRangeCount, aMessage) {
> 
> nit: I think we deprecated the `aFoo` style of function argument naming in
> favor of `foo`.

No, a decision wasn't made. I and and others find it saves time reviewing code as I immediately know that it comes from an argument. See the related discussion about a C++ enum value "e" prefix on dev.platform.
(In reply to Matthew N. [:MattN] (behind on reviews) from comment #6)
> No, a decision wasn't made. I and and others find it saves time reviewing
> code as I immediately know that it comes from an argument. See the related
> discussion about a C++ enum value "e" prefix on dev.platform.

Wow, I guess you care about it a lot to drive-by comment about this thing. It's not a big deal, I'd say. I read the first two posts of the enum "e" prefix discussion and then all my internal alarms started ringing, telling me to back away when the thread exploded. As usual.
I'm willing to have a discussion about this subject, though, in a different forum. Even though it's very likely to explode in various directions without any meaningful consensus being reached. Perhaps the fx-team list is small enough to be useful?
You need to log in before you can comment on or make changes to this bug.