Currently disabled, trying to enable it gets me: 108 INFO TEST-START | chrome://mochitests/content/browser/browser/base/content/test/general/browser_aboutHome.js 109 INFO Check that clearing cookies does not clear storage 110 INFO Waiting for snippets map 111 INFO Wait tab event: AboutHomeLoadSnippetsCompleted 112 INFO Console message: [JavaScript Error: "Error: Permission denied to access object" {file: "resource://gre/modules/RemoteAddonsParent.jsm" line: 484}] 113 INFO Tab event received: AboutHomeLoadSnippetsCompleted after which the test hangs indefinitely.

Paolo, would you like to review this part?

Marco, do you feel comfortable reviewing this?

s/meta/accel/

Comment on attachment 8725245 [details] [diff] [review] Patch 1.1: introduce 'BrowserTestUtils#sendCompositionChange' and frame script fixes Why not just call EventUtils.synthesizeComposition and friend inside a ContentTask? Seems like we could save quite a few lines of code by doing this...

(In reply to :Paolo Amadini from comment #7) > Why not just call EventUtils.synthesizeComposition and friend inside a > ContentTask? > > Seems like we could save quite a few lines of code by doing this... I was not aware we could...

(In reply to Mike de Boer [:mikedeboer] from comment #8) > I was not aware we could... That was a bit sparse in info from me ;-) Please read bug 1241532, which explains why it's not supported and why it will not be. We had the same train of thought, surely.

I'll have to take some time to digest the reasoning in bug 1241532, because what I read in this patch looks exactly like calling EventUtils.synthesizeComposition in a frame script... whether it's done by a content task or specialized message passing should not make any difference on the order of the events.

Comment on attachment 8725219 [details] [diff] [review] Patch 2: no-ws. Review of attachment 8725219 [details] [diff] [review]: ----------------------------------------------------------------- it's a lot of changes, and it's still a quite complex test (we should really split it) but overall the changes look sane. Just please retrigger the appropriate b-c test run on Try at least 20 times, to ensure we're not introducing new intermittent failures, due to the size of changes it's hard to guarantee that just by looking at the patch. ::: browser/base/content/test/general/browser_aboutHome.js @@ +6,3 @@ > > XPCOMUtils.defineLazyModuleGetter(this, "Promise", > "resource://gre/modules/Promise.jsm"); nit: could probably remove this and just rely on DOM promises @@ +36,3 @@ > Cc["@mozilla.org/observer-service;1"] > .getService(Ci.nsIObserverService) > .notifyObservers(null, "cookie-changed", "cleared"); does this work? looks like you are always invoking the setupFn in the content scope, and I'm not sure you can notify like this from content (so maybe this ends up being a no-op)? I actually wonder if the original test worked at all, since it's the first test, thus the map is likely non existent yet and the clearing is asynchronous so it may run after the test. Maybe we should move the test to /dom/tests/mochitest/localstorage... I cannot even find anymore the code in DOM Storage that was responsible to not clearing about pages storage. So maybe it just broke, no idea. @@ +529,5 @@ > + }); > +}); > + > +add_task(function* () { > + info("Pressing Space while the Addons button is focussed should activate it"); while here, typo "focussed" @@ +535,2 @@ > // Skip this test on Mac, because Space doesn't activate the button there. > + if (appInfo.OS == "Darwin") { Why not using AppConstants.platform == 'macosx' and remove the appInfo getter? @@ +561,5 @@ > * @param aSetupFn > * The setup function to be run. > * @return {Promise} resolved when the snippets are ready. Gets the snippets map. > */ > +function* ensureSnippetsMapThen(setupFn, testFn, testArgs = null) { The fact this is names exactly like the method in about:home is confusing and complicates searches through code... I'd rather name this differently, maybe withSnippetsMap?

Comment on attachment 8725245 [details] [diff] [review] Patch 1.1: introduce 'BrowserTestUtils#sendCompositionChange' and frame script fixes Review of attachment 8725245 [details] [diff] [review]: ----------------------------------------------------------------- I see that EventUtils needs some special initialization, done in AsyncUtilsContent. Given that the scope of AsyncUtilsContent is currently separate from ContentTask, I don't see why for now we shouldn't just add another type of event using the same pattern, like this patch does. Since Neil wrote the module initially, I'm redirecting the review to him. ::: testing/mochitest/BrowserTestUtils/BrowserTestUtils.jsm @@ +821,5 @@ > * @returns {Promise} > * @resolves True if the keypress event was synthesized. > */ > + sendChar(char, accelKey = false, browser) { > + // Support the two-argument style invocation. I think it's just easier to overload the first argument to be either the character or an "options" object structured like { char, accelKey }. The second argument would always be the browser.

Re-added ignoreAllUncaughtExceptions() and implemented review comments. Thanks Marco! Carrying over r=mak.

(In reply to Mike de Boer [:mikedeboer] from comment #13) > https://treeherder.mozilla.org/#/jobs?repo=try&revision=047042a32220 Ugh, looks like this one's dirty due to other fx-team landings.

Marco, I see tests timing out with the following error: JavaScript error: chrome://browser/content/abouthome/aboutHome.js, line 155: NotFoundError: The operation failed because the requested database object could not be found. For example, an object store did not exist but was being opened. Have you seen this before? https://hg.mozilla.org/integration/fx-team/file/tip/browser/base/content/abouthome/aboutHome.js#l155

(Win7 OPT M-E10S)

not off-hand, it may happen if a schema upgrade is ongoing and something else tries to access the db...

OK, that looks nice and green.

Comment on attachment 8725245 [details] [diff] [review] Patch 1.1: introduce 'BrowserTestUtils#sendCompositionChange' and frame script fixes Paolo, Neil is swamped atm. Are you sure you can't review this? Please?

For some reason I didn't receive the bugmail for comment 23. Sure, I can review this!

Comment on attachment 8725245 [details] [diff] [review] Patch 1.1: introduce 'BrowserTestUtils#sendCompositionChange' and frame script fixes This seems to be several unrelated changes in one patch so it is a bit hard to follow. For the 'accelKey' change: sendChar is used only for sending printable characters. I would prefer that synthesizeKey was added to BrowserTestUtils (which is how it should have been done originally.) I'd also prefer if the BrowserTestUtils version of synthesizeComposition just called the EventUtils one rather than having a different name and behaviour.

Comment on attachment 8725245 [details] [diff] [review] Patch 1.1: introduce 'BrowserTestUtils#sendCompositionChange' and frame script fixes Clearing the flag since Neil looked at this.

EventUtils usage fixes, only API surface. Carrying over r=mak.

Comment on attachment 8729039 [details] [diff] [review] Patch 1.2: introduce 'BrowserTestUtils#synthesizeKey', 'BrowserTestUtils#synthesizeComposition', 'BrowserTestUtils#synthesizeCompositionChange' and frame script fixes You might want to add a followup to merge the mostly duplicated synthesizeXXX functions.

(In reply to Neil Deakin from comment #29) > You might want to add a followup to merge the mostly duplicated > synthesizeXXX functions. What do you mean with 'merge', exactly?

The synthesizeXXX functions are mostly copies of each other. They could be combined into a helper function.

https://hg.mozilla.org/integration/fx-team/rev/afe0a444c1824bc921242c86de7af4d47405df8a Bug 1093153: introduce 'BrowserTestUtils#synthesizeKey', 'BrowserTestUtils#synthesizeComposition' and 'BrowserTestUtils#synthesizeCompositionChange' to allow mochitests to remotely invoke EventUtils' text composition utilities. Changes to EventUtils include 1) removed dependency on the 'navigator' object when 'nsIXULRuntime' is available and 2) make '_getKeyboardEvent' more robust when used in frame scripts. r=Enn https://hg.mozilla.org/integration/fx-team/rev/5fa79c7ea7ae43359998dfed5fc1109b7ccc758b Bug 1093153: enabled and re-factored browser_aboutHome.js to work correctly in e10s mode. r=mak

Backed out for Mochitest 2 failures. Backouts: https://hg.mozilla.org/integration/fx-team/rev/2c45f42f52c2 https://hg.mozilla.org/integration/fx-team/rev/59d67fe6d470 Push with failures: https://treeherder.mozilla.org/#/jobs?repo=fx-team&revision=5fa79c7ea7ae 07:54:24 INFO - 17 INFO TEST-START | dom/events/test/test_bug1096146.html 07:54:25 INFO - ###################################### BrowserElementCopyPaste.js loaded 07:54:25 INFO - ############################### browserElementPanningAPZDisabled.js loaded 07:54:25 INFO - ############################### browserElementPanning.js loaded 07:54:25 INFO - ######################## BrowserElementChildPreload.js loaded 07:54:25 INFO - ######################## extensions.js loaded 07:54:25 INFO - TEST-INFO | started process screenshot 07:54:25 INFO - TEST-INFO | screenshot: exit 0 07:54:25 INFO - 18 INFO TEST-UNEXPECTED-FAIL | dom/events/test/test_bug1096146.html | uncaught exception - Error: Permission denied to access property "navigator" at http://mochi.test:8888/tests/SimpleTest/EventUtils.js:57 07:54:25 INFO - simpletestOnerror@SimpleTest/SimpleTest.js:1543:11 07:54:25 INFO - JavaScript error: http://mochi.test:8888/tests/SimpleTest/EventUtils.js, line 57: Error: Permission denied to access property "navigator" 07:59:50 INFO - Not taking screenshot here: see the one that was previously logged 07:59:50 INFO - 19 INFO TEST-UNEXPECTED-FAIL | dom/events/test/test_bug1096146.html | Test timed out. 07:59:50 INFO - reportError@SimpleTest/TestRunner.js:114:7 07:59:50 INFO - TestRunner._checkForHangs@SimpleTest/TestRunner.js:134:7 07:59:50 INFO - setTimeout handler*TestRunner._checkForHangs@SimpleTest/TestRunner.js:155:5 07:59:50 INFO - setTimeout handler*TestRunner._checkForHangs@SimpleTest/TestRunner.js:155:5 07:59:50 INFO - setTimeout handler*TestRunner._checkForHangs@SimpleTest/TestRunner.js:155:5 07:59:50 INFO - setTimeout handler*TestRunner._checkForHangs@SimpleTest/TestRunner.js:155:5 07:59:50 INFO - setTimeout handler*TestRunner._checkForHangs@SimpleTest/TestRunner.js:155:5 07:59:50 INFO - setTimeout handler*TestRunner._checkForHangs@SimpleTest/TestRunner.js:155:5 07:59:50 INFO - setTimeout handler*TestRunner._checkForHangs@SimpleTest/TestRunner.js:155:5 07:59:50 INFO - setTimeout handler*TestRunner._checkForHangs@SimpleTest/TestRunner.js:155:5 07:59:50 INFO - setTimeout handler*TestRunner._checkForHangs@SimpleTest/TestRunner.js:155:5 07:59:50 INFO - setTimeout handler*TestRunner._checkForHangs@SimpleTest/TestRunner.js:155:5 07:59:50 INFO - setTimeout handler*TestRunner._checkForHangs@SimpleTest/TestRunner.js:155:5 07:59:50 INFO - TestRunner.runTests@SimpleTest/TestRunner.js:366:5 07:59:50 INFO - RunSet.runtests@SimpleTest/setup.js:186:3 07:59:50 INFO - RunSet.runall@SimpleTest/setup.js:165:5 07:59:50 INFO - hookupTests@SimpleTest/setup.js:258:5 07:59:50 INFO - parseTestManifest@http://mochi.test:8888/manifestLibrary.js:36:5 07:59:50 INFO - getTestManifest/req.onload@http://mochi.test:8888/manifestLibrary.js:49:11 07:59:50 INFO - EventHandlerNonNull*getTestManifest@http://mochi.test:8888/manifestLibrary.js:45:3 07:59:50 INFO - hookup@SimpleTest/setup.js:238:5 07:59:50 INFO - EventHandlerNonNull*@http://mochi.test:8888/tests?autorun=1&closeWhenDone=1&consoleLevel=INFO&hideResultsTable=1&manifestFile=tests.json&dumpOutputDirectory=c%3A%5Cdocume%7E1%5Ccltbld%7E1.t-x%5Clocals%7E1%5Ctemp:11:1 Would importing AppConstants and checking AppConstants.platform work?

The M(oth)s are also failing: https://treeherder.mozilla.org/logviewer.html#?job_id=7991156&repo=fx-team 09:16:01 INFO - 84 INFO TEST-UNEXPECTED-FAIL | chrome://mochitests/content/a11y/accessible/tests/mochitest/events/test_attrs.html | uncaught exception - TypeError: aWindow is null at chrome://mochikit/content/tests/SimpleTest/EventUtils.js:57

Thanks for the backout Sebastian - I just pushed an updated EventUtils.js to try, which should fix the issues reported here. The change I wanted to introduce is in fact trying to rely on AppConstants.jsm when we can and fall back back to user agent sniffing when not. Just like you mentioned in comment 33 ;-)

https://hg.mozilla.org/integration/fx-team/rev/5e4d9f0ec8cffc596378d76de48d3523f4dbe9b6 Bug 1093153: introduce 'BrowserTestUtils#synthesizeKey', 'BrowserTestUtils#synthesizeComposition' and 'BrowserTestUtils#synthesizeCompositionChange' to allow mochitests to remotely invoke EventUtils' text composition utilities. Changes to EventUtils include 1) removed dependency on the 'navigator' object when 'nsIXULRuntime' is available and 2) make '_getKeyboardEvent' more robust when used in frame scripts. r=Enn https://hg.mozilla.org/integration/fx-team/rev/1664c4c4fd7d43d7616208282405d8eeffddb573 Bug 1093153: enabled and re-factored browser_aboutHome.js to work correctly in e10s mode. r=mak

https://hg.mozilla.org/mozilla-central/rev/5e4d9f0ec8cf https://hg.mozilla.org/mozilla-central/rev/1664c4c4fd7d

Comment on attachment 8729040 [details] [diff] [review] Patch 2.2: enabled and re-factored browser_aboutHome.js Approval Request Comment [Feature/regressing bug #]: e10s [User impact if declined]: None, since this is test-only. However, since there's a very small change in non-test code, I believe it should be on RelMan's radar. [Describe test coverage new/current, TreeHerder]: landed on m-c, tests pass. [Risks and why]: minor. [String/UUID change made/needed]: n/a.

Comment on attachment 8729040 [details] [diff] [review] Patch 2.2: enabled and re-factored browser_aboutHome.js Mostly test change, plus handling an exception in ship code, seems safe. Aurora47+

https://hg.mozilla.org/releases/mozilla-aurora/rev/7a9e58c8c8b8 https://hg.mozilla.org/releases/mozilla-aurora/rev/5781664ff8a9