These two tests both need direct access to the autocomplete popup to work. They probably need to proxy those tests to the parent or something like that.

I'll start with a patch for test_bug_787624.html since it turns out that most of these tests were unrelated, probably just copied from test_bug_511615.html. They are still checked there. Also, the direction="rtl" changes were unrelated and the test works as well without them. I've backed out the patch in bug 787624 and verified that the test fails. I'll work on the other test next. I'll fix both tests in this bug, both because they're related and because we're using the number of bugs for release calculations and it makes sense to have a single item.

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

Comment on attachment 8729570 [details] MozReview Request: Bug 1162338 - Part 1 - Fix test_bug_787624.html to work in e10s. r=mak https://reviewboard.mozilla.org/r/39507/#review36279 finally some sanity!

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

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

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

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

https://reviewboard.mozilla.org/r/39749/#review36387 ::: toolkit/components/satchel/test/test_bug_511615.html:141 (Diff revision 1) > - } > - else { > - ok(false, "Autocomplete popup not expected" + testNum); > - } > + () => doKeyUnprivileged("down"), > + () => doKeyUnprivileged("page_down"), > + () => doKeyUnprivileged("return"), > + () => doKeyUnprivileged("v"), > + () => doKeyUnprivileged(" "), > + () => doKeyUnprivileged("back_space"), Marco, if you think that some of these key presses (or in similar code blocks below) might be redundant, please tell me which ones I could remove, because each one of them delays the test a little bit. In general the security issue tested by this code is less likely to re-appear since window listeners must now explicitly specify if they would like to get untrusted events. This can be seen in the last changeset where I changed the code to fail the tests.

Comment on attachment 8730192 [details] MozReview Request: Bug 1162338 - Part 1 - Fix test_bug_787624.html to work in e10s. r=mak https://reviewboard.mozilla.org/r/39747/#review36619

Comment on attachment 8730194 [details] MozReview Request: Bug 1162338 - Part 2 - Fix test_bug_511615.html to work in e10s. r=mak https://reviewboard.mozilla.org/r/39749/#review36611 ::: toolkit/components/satchel/test/test_bug_511615.html:40 (Diff revision 1) > - var formID = input.parentNode.id; > - is(input.value, expectedValue, "Checking " + formID + " input"); > -} > + * Indicates the time to wait before checking that the state of the autocomplete > + * popup, including whether it is open, has not changed in response to events. > + * > + * Manual testing on a fast machine revealed that 80ms was still unreliable, > + * while 100ms detected a simulated failure reliably. Unfortunately, this means > + * that to take into account slower machines we should use a value like 300ms. this states 300, but then the actual used value is 200... Maybe here we should just state "a larger value". ::: toolkit/components/satchel/test/test_bug_511615.html:45 (Diff revision 1) > + * that to take into account slower machines we should use a value like 300ms. > + * > + * Note that if a machine takes more than this time to show the popup, this > + * would not cause a failure, conversely the machine would not be able to detect > + * whether the test should have failed. In other words, this use of timeouts is > + * never expected to cause intermittent failures with test automation. This comment is unclear here, I'd move it later, where you added the Promise.race then it would be clear why it won't cause intermittent failures. ::: toolkit/components/satchel/test/test_bug_511615.html:189 (Diff revision 1) > + yield checkSelectedIndexAfterResponseTime(0); > + is(input.value, "", "The selected item should not have been used."); > + } > > -SimpleTest.waitForExplicitFinish(); > + // Close the popup. > + doKey("escape"); nit: a input.blur() should also do ::: toolkit/components/satchel/test/test_popup_direction.html:14 (Diff revision 1) > +</head> > +<body> > +Test for Popup Direction > +<p id="display"></p> > + > +<!-- we presumably can't hide the content for this test. --> nit: I'm not sure what's the scope of this comment in all the tests here... ::: toolkit/components/satchel/test/test_popup_direction.html:34 (Diff revision 1) > +function waitForNextPopup() { > + return new Promise(resolve => { resolvePopupShownListener = resolve; }); > +} > + > +add_task(function* test_popup_direction() { > + var input = $_(1, "field1"); nit: let ::: toolkit/components/satchel/test/test_popup_direction.html:54 (Diff revision 1) > + > + let popupState = yield new Promise(resolve => getPopupState(resolve)); > + is(popupState.direction, direction, "Direction should match."); > + > + // Close the popup. > + doKey("escape"); ditto

(In reply to Marco Bonardo [::mak] from comment #11) > ::: toolkit/components/satchel/test/test_bug_511615.html:45 > (Diff revision 1) > > + * that to take into account slower machines we should use a value like 300ms. > > + * > > + * Note that if a machine takes more than this time to show the popup, this > > + * would not cause a failure, conversely the machine would not be able to detect > > + * whether the test should have failed. In other words, this use of timeouts is > > + * never expected to cause intermittent failures with test automation. > > This comment is unclear here, I'd move it later, where you added the > Promise.race then it would be clear why it won't cause intermittent failures. I put the comment in that place because if an unrelated intermittent failure occurs, it's likely the first thing someone would look into is the "requestFlakyTimeout" call, maybe trying to adjust the timeout above it, so putting the explanation there might save some time by explaining that the choice of that value couldn't be responsible for intermittent failures (as far as I can tell!).

(In reply to Marco Bonardo [::mak] from comment #11) > > +<!-- we presumably can't hide the content for this test. --> > > nit: I'm not sure what's the scope of this comment in all the tests here... That's because the mochitest template specifies "display: none" for the "div" element. http://mxr.mozilla.org/mozilla-central/search?string=%3Cdiv+id%3D%22content%22&find=&findi=&filter=^[^\0]*%24&hitlimit=&tree=mozilla-central

https://hg.mozilla.org/integration/fx-team/rev/58fab9169d7b https://hg.mozilla.org/integration/fx-team/rev/a3c038ea446c

https://hg.mozilla.org/mozilla-central/rev/58fab9169d7b https://hg.mozilla.org/mozilla-central/rev/a3c038ea446c