583976 - Platform support for focus and key events delivered to cross-platform fake widgets in content processes

Status: RESOLVED FIXED

(Core :: Widget, defect)

Description

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

With bug 582057, we replace platform widgets with fakes that work for painting with both fennec tilemanager and <browser remove> + cross-process layers.  However, key events and focus are broken.  I don't if/to what extent IME works on all fennec platforms, but it would be nice to get that working here too.

Re: IME, Masayuki says
-----
Each native IME event handler must do its jobs in same process because we
cannot access to IME from another process. So, if a chrome process receives IME
events, the chrome process's widget needs to handle the IME and to dispach
Gecko events. (i.e., after that, the events should be redirected to the content
process.) 
-----

Spun off from bug 582057.

Comment 1

[Johnny Stenback (:jst)]

Olli, can you have a look at this? If not, please say so here.

Comment 2

[Benjamin Smedberg]

This undoubtedly blocks fennec alpha-with-remotelayers.

Comment 3

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

I think so.  No authoritative voice has decreed whether event forwarding should be done by the frontend or the platform, but I think we probably want to be doing it in the platform.

Comment 4

[Olli Pettay [:smaug][[e-mail]]]

I'll look at this, but I'm not too familiar with IME handling.

Comment 5

[Olli Pettay [:smaug][[e-mail]]]

When IME is used, it queries data from gecko.
That needs to be synchronous so that things like 
gtk_im_context_set_cursor_location can be called, 
right? Masayuki?

I wonder how we can do that e10s world.
Chrome process can't make synchronous calls to content process.
Perhaps rpc calls?

Comment 6

[Olli Pettay [:smaug][[e-mail]]]

Ok, perhaps better is that content process updates chrome process when needed, 
so that chrome process has always some data to use.

Comment 7

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Should note that it's probably worth investigating what chromium does with IME, since they've likely run into this problem already.

Comment 8

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(In reply to comment #5)
> When IME is used, it queries data from gecko.
> That needs to be synchronous so that things like 
> gtk_im_context_set_cursor_location can be called, 
> right? Masayuki?

Exactly.

Comment 9

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Throwing IME on this bug is probably too much.  Let's worry about that in another, unless we can get it for free with the other changes.

Comment 10

[Brad Lassey [:blassey] (use needinfo?)]

Chris, is this still needed? If so can you please clarify the summary.

Comment 11

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

STR
 (1) Run firefox -chrome chrome://global/content/test-ipc.xul
 (2) Click the google search box, start typing
 (3) Click the "search" button

Expected results: text appears in input box, search request made.
Actual results: nothing happens.

Comment 12

[Olli Pettay [:smaug][[e-mail]]]

This is still on my todo list, but not very high up, since this isn't
FF blocker.

Comment 13

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

This doesn't need to block b2.

Comment 14

[Stuart Parmenter]

cjones says we don't need this for final

Comment 15

[Mark Finkle (:mfinkle) (use needinfo?)]

Should we try to get this for post-4.0? Or is this needed for IPC desktop Firefox

Comment 16

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

This would be extremely useful for simple programs, like the reftest harness and various test programs.  Without this, <browser remote> is impossible to interact with.  I think we should implement this in time for at least multi-process desktop FF.

That said, desktop and mobile FF might want their own (possibly different) event-handling semantics and might want to override the default platform handling.  We already do this for scrolling: there's a default "sync" mode in which scrolling Just Works, which the reftest harness uses.  Advanced users (fennec) can opt in to async mode.

Comment 17

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Also worth noting that because this bug isn't implemented, all reftests that rely on focus are disabled in IPC mode.  That's not good at all.

Comment 18

[Olli Pettay [:smaug][[e-mail]]]

Felipe should have some code for this, which makes focus, mouse and
key events just work, IIRC.

Comment 19

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Where is it?!?  :)  This would have been useful for quite a while now.

Comment 20

[:Felipe Gomes (needinfo for replies!)]

I've got the cross-process mechanism working reasonably well for mouse and simple keyboard events (no IME support). However I tried to hook up focus support in a similar mechanism but it didn't work at the time.

The main idea was to add a DispatchCrossProcessEvent to nsEventStateManager. It has a whitelist approach: for events that you want sent cross process you need to write its ParamTraits and add it to DispatchCrossProcessEvent.

The main patches are here:
support + mouse:
http://hg.mozilla.org/users/felipc_gmail.com/e10s-mq/file/e0bf82838878/cross-process-event-dispatcher
keyboard:
http://hg.mozilla.org/users/felipc_gmail.com/e10s-mq/file/e0bf82838878/generic-key-events

And there are some other random fixes in the later patches in the queue.

We thought the focus would be simple (activate remote frame when <browser> gets focus, deactivate when it gets blur) but I stumbled into various problems. From what I understood it wasn't working because the focus manager relied too much in some nsIDOMWindow and nsIDOMElement pointers to make decisions.

Here was a first attempt:
http://hg.mozilla.org/users/felipc_gmail.com/e10s-mq/file/e0bf82838878/focus-activate-deactivate

Comment 21

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Felipe, do you want to reassign this bug to yourself then?

Comment 22

[:Felipe Gomes (needinfo for replies!)]

Let's do this!

Comment 23

[:Felipe Gomes (needinfo for replies!)]

Attachment: Cross-process events delivery

This implements the mechanism to forward events from parent to child process through the ESM, and adds the basic support for mouse, key and mouse scroll events, plus some preemptive fixes to the mouse cursor.

It's still not clear in which part of the event handling process this should be hooked up, as we talked in the work week. But I believe we can adjust this later as needed. It's currently hooked to PostHandleEvent.

Originally I would return early from PostHandleEvent if the event is dispatched to the child process, but this would skip the focus adjustment code that happens on PostHandleEvent. Now I don't return early anymore, but this would cause both the parent and the child to generate click events (on posthandle for the mouseup), and then the click from the parent would be dispatched to the child, which would get 2 clicks. So that's why I'm only forwarding mousedown/move/up instead of any mouse event.

Comment 24

[:Felipe Gomes (needinfo for replies!)]

Attachment: Cross-process events delivery

Missed removal of some commented out code from the previous patch

The focus part is coming in a separate patch

Comment 25

[:Felipe Gomes (needinfo for replies!)]

Attachment: Basic focus work

Basic automatic handling of frame activation by the focus manager.

This makes it possible to run the reftests that needs focus, and to click/tab to focus in Firefox. Correct tabbing through the content's elements before returning focus to the UI will be done in a followup.

(reftest changes not included in this patch)

Comment 26

[Benjamin Stover (:stechz)]

(This doesn't compile because of a TabParent.cpp missed return.)

Comment 27

[:Felipe Gomes (needinfo for replies!)]

Attachment: Cross-process events delivery

Added missed return

Comment 28

[:Felipe Gomes (needinfo for replies!)]

Attachment: Basic focus work

(moved one function declaration from part 1 to part 2 where it should be)

I had added the missing return in the tree but forgot to update it here. 
But you don't even need the part 1 here, all the focus-related changes are in part 2 (Basic focus work) and do not depend on part 1.

Comment 29

[Benjamin Stover (:stechz)]

Yes, I fixed it locally just thought you'd like to know :)

Comment 30

[Benjamin Stover (:stechz)]

A few questions:
* I had been told that mouse event forwarding was already done, but the first patch seems to be doing just that (along with keyboard events).
* It doesn't look like keyboard events get forwarded back to the parent process if they aren't preventDefault()'d in the content process. I think this is probably the right idea to keep shortcuts working. Is there any reason you aren't doing this yet?
* IIRC, browser focus will now cause key events to be forwarded automatically to the content process, so we should no longer need ContentCustomKeySender (yay!) or the listeners in content.js (yay!).

This is a great patch.

Comment 31

[Benjamin Stover (:stechz)]

> * I had been told that mouse event forwarding was already done, but the first
> patch seems to be doing just that (along with keyboard events).

The question there, btw, is "was I wrong? If not, then what is this code here for?" :)

Comment 32

[Benjamin Stover (:stechz)]

Comment on attachment 528981 [details] [diff] [review]
Basic focus work

It works fine on my Nexus S! This is great work Felipe! :)

Let me know if there's anything I can do to move this bug forward. I'd love to see this landed so we can start eliminating forwarding code in Fennec sooner.

Comment 33

[Alex Pakhotin (:alexp)]

Bug 624163 seems to be related.

Comment 34

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 528981 [details] [diff] [review]
Basic focus work

>diff --git a/content/base/public/nsIFrameLoader.idl b/content/base/public/nsIFrameLoader.idl
>--- a/content/base/public/nsIFrameLoader.idl
>+++ b/content/base/public/nsIFrameLoader.idl
>@@ -183,16 +183,22 @@ interface nsIFrameLoader : nsISupports
update iid

>+nsFocusManager::GetRemoteForContent(nsIContent* aContent) {
>+  if (!aContent ||
>+      aContent->Tag() != nsGkAtoms::browser ||
This isn't enough. You need to check namespace.

>+      aContent->AttrValueIs(kNameSpaceID_None, nsGkAtoms::Remote,
>+                           nsGkAtoms::_true, eIgnoreCase) == 0)
Nit, align nsGkAtoms::_true right under kNameSpaceID_None.


>+    return nsnull;
>+
>+  nsCOMPtr<nsIFrameLoader> frameLoader;
>+  nsCOMPtr<nsIFrameLoaderOwner> loaderOwner = do_QueryInterface(aContent);
>+  if (loaderOwner) {
>+    loaderOwner->GetFrameLoader(getter_AddRefs(frameLoader));
>+  }
>+
>+  if (frameLoader) {
>+    nsFrameLoader* fml = static_cast<nsFrameLoader*>(frameLoader.get());
nsIFrameLoaderOwner has 
already_AddRefed<nsFrameLoader> GetFrameLoader(). No need to

>diff --git a/mobile/chrome/content/browser.js b/mobile/chrome/content/browser.js
Not something I should review.


This needs some tests before pushing to m-c.

Comment 35

[:Felipe Gomes (needinfo for replies!)]

Attachment: Mobile changes and test

Here's the changes that remove fl.activateRemoteFrame from fennec front-end, plus the blind test that I wrote for it :) let's see if it works

mfinkle suggested that if the newtab-button.focus() doesn't work for the test we can try window.focus()

Comment 36

[:Felipe Gomes (needinfo for replies!)]

Attachment: Enable reftests

Enable reftests and minor clean-ups:
  - remove unused setTimeout (the "callback" isn't wrapped in a function)
  - SetVisibility is hit during some reftests and would throw an assertion
  - the crashtest that is not working (note: it's not crashing, just throwing an exception and stopping the harness) is because it uses popup widgets which are currently unsupported, so instead of trying to somehow make it pass, let's skip it for now until we properly support popups.

Comment 37

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 528980 [details] [diff] [review]
Cross-process events delivery

This patch is the one you're going to modify still, right?

Comment 38

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Comment on attachment 531535 [details] [diff] [review]
Enable reftests

>diff --git a/content/html/content/reftests/autofocus/input-load.html b/content/html/content/reftests/autofocus/input-load.html
>--- a/content/html/content/reftests/autofocus/input-load.html
>+++ b/content/html/content/reftests/autofocus/input-load.html
>@@ -1,13 +1,13 @@
> <!DOCTYPE html>
> <html class="reftest-wait">
>   <link rel='stylesheet' type='text/css' href='style.css'>
>   <script>
>     function focusHandler()
>     {
>-      setTimeout(document.documentElement.removeAttribute('class'), 0);
>+      document.documentElement.removeAttribute('class');

Are you sure this is right?  Maybe ask the test author why this was here?

>-load 336744-1.html
>+skip-if(browserIsRemote) load 336744-1.html # needs e10s popup widget support

Link to bug please.

> NS_IMETHODIMP
> TabChild::SetVisibility(PRBool aVisibility)
> {
>-  NS_NOTREACHED("TabChild::SetVisibility not supported in TabChild");
>-
>-  return NS_ERROR_NOT_IMPLEMENTED;
>+  // should the platform support this?
>+  return NS_OK;

Please file a bug for this.

>-    // FIXME/bug 583976: focus doesn't yet work with out-of-process
>-    // content.
>-    if (gBrowserIsRemote) {
>-        return false;
>-    }

\o/

r=me if you check that the test change is OK.

Comment 39

[Olli Pettay [:smaug][[e-mail]]]

Attachment: WIP for the other approach

Felipe told that cjones had suggested dispatching events to
content process when handling event in xul:browser.
This does that. Otherwise uses the event forwarding from the previous patch (so it has a bug with mouse event coordinate handling).

The patch uses explicit NS_ProcessNextEvent loop, but perhaps rpc could be utilized.
Although, this approach allows us to add some time limit there and
not try to process events if it takes too much time to process them in content
process or if we're flooding events too fast to content process.

I tested this only with test.xul

Comment 40

[Olli Pettay [:smaug][[e-mail]]]

http://mozilla.pettay.fi/key_and_sync_xhr.html is one case where
it takes lots of time before the result comes back from content process.

Comment 41

[:Felipe Gomes (needinfo for replies!)]

Attachment: coordinates adjustment

Olli, this is the addition to your patch that I'm using to adjust the coordinates.

using it I noticed that each event is dispatched twice. I'm not entirely sure but it appears to be that both the content group and the system group is triggering the remote dispatch. And the coordinates end up being different in the first and second dispatch, which made it easier to notice this.

Comment 42

[Olli Pettay [:smaug][[e-mail]]]

(In reply to comment #41)
> using it I noticed that each event is dispatched twice. I'm not entirely
> sure but it appears to be that both the content group and the system group
> is triggering the remote dispatch. 
Ah, indeed. It is triggered for default and system event group.
I think we want only default group.

WillHandleEvent could take PRUInt32 aFlags as a parameter and then 
dispatch the event to content only if (!(aFlags & NS_EVENT_FLAG_SYSTEM_EVENT))

Comment 43

[Neil Deakin]

Comment on attachment 528981 [details] [diff] [review]
Basic focus work

+  // check if the element to focus is a remote browser. If yes, activate remote content
+  TabParent* remote = static_cast<TabParent*>(GetRemoteForContent(contentToFocus));
+  if (remote) {
+    remote->Activate();
+#ifdef DEBUG_FOCUS
+    printf("*Remote browser activated\n");
+#endif
+  }

I can't tell for sure since I don't know what Activate/Deactivate are supposed to do, but this looks wrong. contentToFocus may not end up being focused, but you'll have called Activate on it anyway. Also, you could be calling Activate on a window that is in the background.

The call to Deactivate might be in the wrong place also.

Comment 44

[:Felipe Gomes (needinfo for replies!)]

> I can't tell for sure since I don't know what Activate/Deactivate are
> supposed to do, but this looks wrong. contentToFocus may not end up being
> focused, but you'll have called Activate on it anyway. Also, you could be
> calling Activate on a window that is in the background.

Activate/Deactivate just tells the content in the other proccess that it received or lost focus (nothing more to it). 
Any hints on what would be the right place to make this call then? Is it in the same function (::MoveFocus) but further below?

Comment 45

[Benjamin Stover (:stechz)]

Comment on attachment 531534 [details] [diff] [review]
Mobile changes and test

>+++ b/mobile/chrome/tests/browser_focus.js
>@@ -0,0 +1,46 @@
>+
>+let newTab = null;
>+
>+function test() {
>+  waitForExplicitFinish();
>+
>+  messageManager.addMessageListener("pageshow", function(aMessage) {
>+    if (newTab && newTab.browser.currentURI.spec != "about:blank") {
>+      messageManager.removeMessageListener(aMessage.name, arguments.callee);
>+      setTimeout(onTabLoaded, 0);
>+    }
>+  });
>+
>+  newTab = Browser.addTab("about:empty", true);
>+}

Does about:empty make the browser load a remote tab? 

>+
>+function onTabLoaded() {
>+
>+  // ensure that the <browser> is not already focused
>+  document.getElementById("newtab-button").focus();
>+
>+  messageManager.loadFrameScript(chromeRoot + "remote_focus.js", true);
>+
>+  testFocus();
>+}

For some reason, I'm getting a "cannot convert URL" problem and I have no idea why for the loadFrameScript. Also, you should probably pass false to messageManager so this script isn't loaded for other tests that open new tabs. I wouldn't be surprised if other tests aren't doing this, but they need to be.

>+window.addEventListener("focus", function focusReceived() {
>+  window.removeEventListener("focus", focusReceived, false);
>+  sendAsyncMessage("Test:E10SFocusReceived");
>+}, false);
>+
>+window.addEventListener("blur", function blurReceived() {
>+  window.removeEventListener("blur", blurReceived, false);
>+  sendAsyncMessage("Test:E10SBlurReceived");
>+}, false);

I don't have a problem with this living in remote_head.js without the window.removeEventListener stuff. Also, shouldn't this be just "addEventListener" without "window?"

I'm not sure why remote_focus.js isn't loading so I couldn't get very far with this test.

Comment 46

[Mark Finkle (:mfinkle) (use needinfo?)]

(In reply to comment #45)

> >+  messageManager.addMessageListener("pageshow", function(aMessage) {
> >+    if (newTab && newTab.browser.currentURI.spec != "about:blank") {
> >+      messageManager.removeMessageListener(aMessage.name, arguments.callee);
> >+      setTimeout(onTabLoaded, 0);
> >+    }
> >+  });
> >+
> >+  newTab = Browser.addTab("about:empty", true);
> >+}
> 
> Does about:empty make the browser load a remote tab? 

Yes. http://mxr.mozilla.org/mozilla-central/source/mobile/chrome/content/Util.js#111

Comment 47

[:Felipe Gomes (needinfo for replies!)]

> >+  messageManager.loadFrameScript(chromeRoot + "remote_focus.js", true);
> >+
> >+  testFocus();
> >+}
> 
> For some reason, I'm getting a "cannot convert URL" problem and I have no
> idea why for the loadFrameScript. 

That's odd. IIRC I saw some mention of this problem in another test, but I can't find it anymore. There's one test that uses baseURI instead of chromeRoot.. could you try changing that to it? If it doesn't work I'll whip up another patch that puts this in remote_head.js 

> Also, you should probably pass false to
> messageManager so this script isn't loaded for other tests that open new
> tabs. I wouldn't be surprised if other tests aren't doing this, but they
> need to be.
> 

Good catch. The other tests are also using true so I overlooked this. Will fix in an updated patch

Comment 48

[:Felipe Gomes (needinfo for replies!)]

Attachment: Updated mobile patch

Updated the patch, now the test actually works..

FTR the "couldn't convert URL" problem was because I was loading an about:empty tab instead of a chrome URL like the other tests do. I don't know why but the remote scripts (even remote_head.js) fails to load in that situation.

Also I had to use a capturing listener for focus on the content script.

Comment 49

[:Felipe Gomes (needinfo for replies!)]

Attachment: Cross-process events delivery

Updated patch and fixed some rough edges in the ParamTraits. Ready for review now.

(note: to see the blinking caret in textboxes the focus patch is also needed)

Comment 50

[Benjamin Stover (:stechz)]

Comment on attachment 539087 [details] [diff] [review]
Updated mobile patch

Review of attachment 539087 [details] [diff] [review]:
-----------------------------------------------------------------

Looks great overall! I think one more pass should get this done. Also, please run this through Android on try, as race conditions can be problematic there. I don't spot any but we've had problems with this in other tests.

::: mobile/chrome/tests/browser_focus.js
@@ +1,1 @@
> +let testURL = chromeRoot + "browser_focus.html";

Let's make this "use strict" JS while we are here.

@@ +4,5 @@
> +function test() {
> +  waitForExplicitFinish();
> +
> +  messageManager.addMessageListener("pageshow", function(aMessage) {
> +    if (newTab && newTab.browser.currentURI.spec != "about:blank") {

You will have to name this function (something like "listener" is fine with me) because arguments.callee is not supported with use strict.

@@ +39,5 @@
> +}
> +
> +function endTest() {
> +  messageManager.sendAsyncMessage("Test:E10SFocusTestFinished", {});
> +  Browser.closeTab(newTab);

Please use registerCleanupFunction to ensure your tab is always cleaned up and that E10SFocusTestFinished is always called, as per:
https://developer.mozilla.org/en/Browser_chrome_tests#Cleaning_up_after_yourself

Comment 51

[:Felipe Gomes (needinfo for replies!)]

Attachment: Focus patch

Neil, you suggested following mFocusedWindow on IRC the other day, but turns out I can't really do that because as this is meant for out-of-process content, the <browser> element is just a box, and there's no change of window at all.

I followed the messages from DEBUG_FOCUS and tried to place both activate/deactivate in similar places, near the "has been focused"/"has been blurred" messages.

Comment 52

[Neil Deakin]

In that case it should be treated in a similar manner to a plugin and the calls to Activate/Deactivate should be made just before or after the checks for plugins (search for nsIObjectFrame)

Comment 53

[:Felipe Gomes (needinfo for replies!)]

Attachment: Focus patch

(In reply to comment #52)
> In that case it should be treated in a similar manner to a plugin and the
> calls to Activate/Deactivate should be made just before or after the checks
> for plugins (search for nsIObjectFrame)

great, thanks

Comment 54

[:Felipe Gomes (needinfo for replies!)]

Attachment: Updated mobile patch

Addressed comments

Comment 55

[Benjamin Stover (:stechz)]

Comment on attachment 539968 [details] [diff] [review]
Updated mobile patch

>+function onTabLoaded() {
>+
>+  // ensure that the <browser> is not already focused

Remove blank line.

r+

Comment 56

[Neil Deakin]

Comment on attachment 539902 [details] [diff] [review]
Focus patch

>+TabParent*
>+nsFocusManager::GetRemoteForContent(nsIContent* aContent) {

Move this function to somewhere such as just before Blur is declared. As is, this patch adds this method in the middle of the public methods, breaking them up.

Similarly, keep the declaration in the header file in the same order as the definitions.


>+  if (!aContent ||
>+      aContent->Tag() != nsGkAtoms::browser ||
>+      !aContent->IsXUL() ||
>+      aContent->AttrValueIs(kNameSpaceID_None, nsGkAtoms::Remote,
>+                            nsGkAtoms::_true, eIgnoreCase) == 0)
>+    return nsnull;

AttrValueIs returns a PRBool so should be compared with boolean !


>-    // if an object/plug-in is being blurred, move the system focus to the
>-    // parent window, otherwise events will still get fired at the plugin.
>+    // if an object/plug-in/remote browser is being blurred, move the system focus
>+    // to the parent window, otherwise events will still get fired at the plugin.
>     // But don't do this if we are blurring due to the window being lowered,
>     // otherwise, the parent window can get raised again.
>     if (mActiveWindow && aAdjustWidgets) {

While aAdjustWidgets will always be true when called to focus a <browser>, technically it might be clearer to only put the aAdjustWidgets part of the condition around the plugin handling block. A similar case within Focus()

 
>+
>+      TabParent* remote = static_cast<TabParent*>(GetRemoteForContent(content));

GetRemoteForContent already returns a TabParent* so shouldn't need a cast. Similarly with the other case within Focus()


I only reviewed the focus manager changes.

Comment 57

[:Felipe Gomes (needinfo for replies!)]

focus patches landed in e10s:
http://hg.mozilla.org/projects/electrolysis/rev/e24c69aab568
http://hg.mozilla.org/projects/electrolysis/rev/6f366673f0c7

I'll activate the needs-focus reftests after bug 623625 is fixed

Comment 58

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Removing the early return for |if (gBrowserIsRemote)| doesn't depend on bug 623625, unless there's something I'm missing.

Comment 59

[:Felipe Gomes (needinfo for replies!)]

Linux R-IPC reftest-sanity works fine, but when I ran reftests using a fennec build locally (and the same happened on the tegra try push), an extra window with the fennec UI pops up, and the reftest window is not the active one.
the gBrowser.focus() call is not enough, so it needs the fm.activeWindow = window part of bug 623625.

Comment 60

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 539319 [details] [diff] [review]
Cross-process events delivery


>+PRBool
>+nsEventStateManager::IsRemoteTarget(nsIContent* target) {
>+  return target &&
>+         target->Tag() == nsGkAtoms::browser &&
>+         target->AttrValueIs(kNameSpaceID_None, nsGkAtoms::Remote,
>+                             nsGkAtoms::_true, eIgnoreCase);
You should check that target is in XUL namespace.

>+  nsIContent* target = mCurrentTargetContent;
>+  if (!target && aTargetFrame) {
>+    target = aTargetFrame->GetContent();
>+  }
>+
>+  if (*aStatus == nsEventStatus_eConsumeNoDefault ||
>+      !target ||
>+      !IsRemoteTarget(target))
>+    return PR_FALSE;

Be consistent with the coding style.
if (expr) {
  stmt;
}


>+
>+  nsCOMPtr<nsIFrameLoader> frameLoader;
>+  nsCOMPtr<nsIFrameLoaderOwner> loaderOwner = do_QueryInterface(target);
>+  if (!loaderOwner)
>+    return PR_FALSE;
>+
>+  loaderOwner->GetFrameLoader(getter_AddRefs(frameLoader));
nsRefPtr<nsFrameLoader> frameLoader = loaderOwner->GetFrameLoader();

>+TabParent::RecvSetCursor(const PRUint32& aCursor)
>+{
>+  nsCOMPtr<nsIWidget> widget = GetWidget();
>+  if (widget)
>+    widget->SetCursor((nsCursor) aCursor);
>+  return true;

if (expr) {
  stmt;
}

Comment 61

[:Felipe Gomes (needinfo for replies!)]

Comment on attachment 539319 [details] [diff] [review]
Cross-process events delivery

stechz, can you rs this? there's one line in mobile/ here. We talked about this some time ago, the event forwarding is enabled by default and this just disables for fennec for the time being until we use it to get rid of the manual forwarding

Comment 62

[:Felipe Gomes (needinfo for replies!)]

Part 3, event forwarding:
http://hg.mozilla.org/projects/electrolysis/rev/8158317cb497

Comment 63

[Marco Bonardo [:mak]]

I backed out everything from central since Android and Maemo were unhappy about the push these changes were part of.

Comment 64

[:Felipe Gomes (needinfo for replies!)]

http://hg.mozilla.org/mozilla-central/rev/238b9a9479ed
http://hg.mozilla.org/mozilla-central/rev/6759afeead2b
http://hg.mozilla.org/mozilla-central/rev/fb5ad0e555fd
http://hg.mozilla.org/mozilla-central/rev/9d3abcd6161d

I'll close the bug when I enable reftest-ipc

Comment 65

[:Felipe Gomes (needinfo for replies!)]

With bug 623625 fixed, reftest-ipc can run tests marked as needs-focus:

http://hg.mozilla.org/integration/mozilla-inbound/rev/37c5482971b1

Comment 66

[Daniel Holbert [:dholbert]]

https://hg.mozilla.org/mozilla-central/rev/37c5482971b1

It sounds like this can now be closed, but I'll leave that to Felipe to be sure.

Comment 67

[(no longer active)]

I backed out the last changeset because of Android R2 failures:

https://hg.mozilla.org/mozilla-central/rev/149fc4a6efca
https://hg.mozilla.org/integration/mozilla-inbound/rev/86b2eda57700

Reftest log:
https://tbpl.mozilla.org/php/getParsedLog.php?id=6663032&tree=Mozilla-Inbound&full=1

The failure seems to have been caused because of different selection colors, which seems to be because the window was not focused.

Comment 68

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

We have cross-process focus, but no multi-process product has used real key events yet so I don't know about the status there.

Comment 69

[:Felipe Gomes (needinfo for replies!)]

This bug was finished a long time ago (see comment 64) but it was left open pending the activation of some extra tests for reftest-ipc that this bug + bug 623625 allowed to activate. It bounced once and was never relanded. I sent to tryserver and got green runs, so I relanded it:

desk & b2g: https://tbpl.mozilla.org/?tree=Try&rev=16191d87c3fa
android: https://tbpl.mozilla.org/?tree=Try&rev=acc54801d561

https://hg.mozilla.org/integration/mozilla-inbound/rev/9e613392a170

Comment 70

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/9e613392a170

Comment 71

[:Felipe Gomes (needinfo for replies!)]

(per comment 64)