Attempting to use waitForFocus on a CPOW window throws an exception calling getFocusedElementForWindow(). We probably need to either fix this to work or make a waitForContentFocus method that can do something sane.

Do you have an example of this? It seems to work ok when running in several cases I tried.

toolkit/mozapps/extensions/test/xpinstall/browser_bug638292.js was the one I was hitting. You'll need to enable it in e10s in browser.ini and make sure to run the test with --e10s to see it fail.

The issue here is that the following is being done: waitForFocus(callback, gBrowser.contentWindow), where contentWindow is a wrapper to child process window. We need to: 1. Focus on gBrowser instead. 2. Wait for the child process to receive the focus request that will occur due to 1, and then inform the parent when this happens. The parent will then call the callback passed to waitForFocus. With regards to 1, we could change the test to supply gBrowser instead, changing waitForFocus to accept frame elements as well. However, I suspect many other tests do something similar and I ideally don't want to have to change lots of tests. One possibility is to be able to retrieve the <browser> from the wrapper from inside waitForFocus. Bill, I'm told you have some insight into this? For 2, it seems like we would need to have waitForFocus in the parent call a similar waitForFocus function in the child, then send a message up to the parent. Is there some kind of place to add testing only code in the child, that could listen and send these notifications?

Part 1 seems like it should be fixed by bug 1085537.

This patch implements a basic form of waitForFocus. It doesn't handle waiting for the load event nor does it handle child frames. It does make the call to waitForFocus in browser_bug638292.js succeed, although that test then fails calling synthesizeMouseAtCenter with the content window for similar reasons.

Comment on attachment 8510466 [details] [diff] [review] remotewaitforfocus Review of attachment 8510466 [details] [diff] [review]: ----------------------------------------------------------------- Looks good to me ::: testing/mochitest/tests/SimpleTest/SimpleTest.js @@ +721,5 @@ > SpecialPowers.focus(childTargetWindow); > } > }; > > +// This method is called by waitForFocus to focus a remotre child window. It only s/remotre/remote/

Hi Neil, can you assign a point value.

This is a more complete implementation that: 1. waits for focus and load events in either process 2. handles child frames in the content process 3. Merges the two code paths to avoid duplication 4. Adds a promise version of waitForFocus 5. Adds a test for waitForFocus There's one issue with the test in that I can't consistently access Components.utils.

what about promiseFocus instead of promiseWaitForFocus?

I think there's a separate bug on changing the names of these methods. Successful builds at: https://treeherder.mozilla.org/#/jobs?repo=try&revision=8a0a5ab9bf17

Comment on attachment 8540186 [details] [diff] [review] Wait for focus, minor polish Review of attachment 8540186 [details] [diff] [review]: ----------------------------------------------------------------- Overall this makes sense, a few questions and maybe the answers and a second look will get me to understanding this fully. ::: testing/mochitest/tests/SimpleTest/SimpleTest.js @@ +729,5 @@ > + try { > + if (event) { > + if (event.type == "load") { > + if (expectBlankPage != (event.target.location == "about:blank")) { > + return; I assume this handles when we have about:blank and the page hasn't loaded? why would we have a 'load' event for about:blank? Is it realistic to expect about:blank for all blank pages? @@ +737,5 @@ > + } else if (event.type == "focus") { > + focused = true; > + } > + > + event.currentTarget.removeEventListener(event.type, focusedOrLoaded, true); should this be on childprocess AND chromeprocess? @@ +740,5 @@ > + > + event.currentTarget.removeEventListener(event.type, focusedOrLoaded, true); > + } > + > + if (loaded && focused && !finished) { where is finished defined? @@ +750,5 @@ > + SimpleTest.executeSoon(function() { callback(targetWindow) }); > + } > + } > + } catch (e) { > + if (!isChildProcess) { should we do anything if there is an exception caught in the target process? @@ +769,5 @@ > + getHref(desiredWindow) != "about:blank" && > + desiredWindow.document.readyState == "complete"; > + if (!loaded) { > + info("must wait for load"); > + desiredWindow.addEventListener("load", focusedOrLoaded, true); should this be on childprocess AND chromeprocess? @@ +786,5 @@ > + /* If this is a child frame, ensure that the frame is focused. */ > + focused = (focusedWindow() == childDesiredWindow); > + if (!focused) { > + info("must wait for focus"); > + desiredWindow.addEventListener("focus", focusedOrLoaded, true); could this be adding it twice? and does this work for child and chrome process?

> I assume this handles when we have about:blank and the page hasn't loaded? > why would we have a 'load' event for about:blank? Is it realistic to expect > about:blank for all blank pages? > When a new tab or window opens, it will load about:blank and send a load event at it, then load the desired page. Usually, the test wants to wait until the desired page is loaded before continuing, so the extra load event is ignored. However, if a blank page is actually expected, the expectBlankPage argument may be set to true to not ignore the load event. This patch shouldn't be changing this behaviour from the current behaviour. > where is finished defined? > Earlier, in the same place as 'loaded" and 'focused' > should we do anything if there is an exception caught in the target process? > Possibly, although I don't think there's any value of doing so in the parent process either, but didn't want to change the existing code. > @@ +769,5 @@ > > + getHref(desiredWindow) != "about:blank" && > > + desiredWindow.document.readyState == "complete"; > > + if (!loaded) { > > + info("must wait for load"); > > + desiredWindow.addEventListener("load", focusedOrLoaded, true); > > should this be on childprocess AND chromeprocess? Do you mean should I be waiting for load/focus events on both the child and the parent? The child won't fire a focus event if the parent isn't focused, so it should be ok as is. > > @@ +786,5 @@ > > + /* If this is a child frame, ensure that the frame is focused. */ > > + focused = (focusedWindow() == childDesiredWindow); > > + if (!focused) { > > + info("must wait for focus"); > > + desiredWindow.addEventListener("focus", focusedOrLoaded, true); > > could this be adding it twice? and does this work for child and chrome > process? Which code path do you see this being added twice? The try run (comment 10) shows that this works on all existing browser tests, and passes the test added by this patch.

My only confusion here is: > @@ +769,5 @@ > > + getHref(desiredWindow) != "about:blank" && > > + desiredWindow.document.readyState == "complete"; > > + if (!loaded) { > > + info("must wait for load"); > > + desiredWindow.addEventListener("load", focusedOrLoaded, true); > > should this be on childprocess AND chromeprocess? Do you mean should I be waiting for load/focus events on both the child and the parent? The child won't fire a focus event if the parent isn't focused, so it should be ok as is. ^ I read this code as it would listed for load events on both child and the parent. Is that assumption incorrect?

If the target window is in the child process, waitForFocusInner will be called only on the child. If the target window is in the parent process, waitForFocusInner will be called only on the parent. Thus, the listeners will only be added in one process.

Comment on attachment 8540186 [details] [diff] [review] Wait for focus, minor polish Review of attachment 8540186 [details] [diff] [review]: ----------------------------------------------------------------- as my r- was answered via questions in the bug comments, changing this to a r+, thanks!