Closed
Bug 1263016
Opened 5 years ago
Closed 5 years ago
Fix test_bug451540.xul to run in e10s
Categories
(Toolkit :: Find Toolbar, defect)
Toolkit
Find Toolbar
Tracking
()
RESOLVED
FIXED
mozilla48
Tracking | Status | |
---|---|---|
firefox48 | --- | fixed |
People
(Reporter: jaws, Assigned: jaws)
References
Details
Attachments
(1 file)
No description provided.
Assignee | ||
Comment 1•5 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=4e397f9bbec6
Assignee | ||
Comment 2•5 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/45323/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/45323/
Attachment #8739625 -
Flags: review?(mdeboer)
Updated•5 years ago
|
Attachment #8739625 -
Flags: review?(mdeboer) → review+
Comment 3•5 years ago
|
||
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.
Assignee | ||
Comment 4•5 years ago
|
||
https://hg.mozilla.org/integration/fx-team/rev/1af4c669349718874f6a366f3f1f52ff2f86adfc Bug 1263016 - Fix test_bug451540.xul to run in e10s. r=mikedeboer
Comment 5•5 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/1af4c6693497
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
status-firefox48:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
Comment 6•5 years ago
|
||
(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•5 years ago
|
||
(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.
Description
•