1263016 - Fix test_bug451540.xul to run in e10s

Status: RESOLVED FIXED

(Toolkit :: Find Toolbar, defect)

Comment 1

[Jared Wein [:jaws] (please needinfo? me)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=4e397f9bbec6

Comment 2

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: MozReview Request: Bug 1263016 - Fix test_bug451540.xul to run in e10s. r?mikedeboer

Review commit: https://reviewboard.mozilla.org/r/45323/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/45323/

Comment 3

[Mike de Boer [:mikedeboer]]

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.

Comment 4

[Jared Wein [:jaws] (please needinfo? me)]

https://hg.mozilla.org/integration/fx-team/rev/1af4c669349718874f6a366f3f1f52ff2f86adfc
Bug 1263016 - Fix test_bug451540.xul to run in e10s. r=mikedeboer

Comment 5

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/1af4c6693497

Comment 6

[Matthew N. [:MattN] (PM me if requests are blocking you)]

(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.

Comment 7

[Mike de Boer [:mikedeboer]]

(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?