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

RESOLVED FIXED in mozilla8

Status

()

defect
RESOLVED FIXED
9 years ago
6 years ago

People

(Reporter: cjones, Assigned: Felipe)

Tracking

Trunk
mozilla8
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(fennec-)

Details

(Whiteboard: [e10s])

Attachments

(6 attachments, 8 obsolete attachments)

5.33 KB, patch
cjones
: review+
Details | Diff | Splinter Review
25.36 KB, patch
Details | Diff | Splinter Review
1.61 KB, patch
Details | Diff | Splinter Review
26.10 KB, patch
smaug
: review+
stechz
: review+
Details | Diff | Splinter Review
12.62 KB, patch
enndeakin
: review+
Details | Diff | Splinter Review
5.45 KB, patch
stechz
: review+
Details | Diff | Splinter Review
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.
Olli, can you have a look at this? If not, please say so here.
Assignee: nobody → Olli.Pettay
This undoubtedly blocks fennec alpha-with-remotelayers.
tracking-fennec: --- → ?
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.
I'll look at this, but I'm not too familiar with IME handling.
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?
Ok, perhaps better is that content process updates chrome process when needed, 
so that chrome process has always some data to use.
Should note that it's probably worth investigating what chromium does with IME, since they've likely run into this problem already.
(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.
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.
Summary: Get focus and key events/IME working in content processes with cross-platform fake widgets → Get focus and key events working in content processes with cross-platform fake widgets
tracking-fennec: ? → 2.0b1+
tracking-fennec: 2.0b1+ → 2.0b2+
Chris, is this still needed? If so can you please clarify the summary.
Whiteboard: resolveme 2010-09-30
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.
Summary: Get focus and key events working in content processes with cross-platform fake widgets → Platform support for focus and key events delivered to cross-platform fake widgets in content processes
This is still on my todo list, but not very high up, since this isn't
FF blocker.
Whiteboard: resolveme 2010-09-30
This doesn't need to block b2.
tracking-fennec: 2.0b2+ → 2.0b3+
cjones says we don't need this for final
tracking-fennec: 2.0b3+ → 2.0-
Should we try to get this for post-4.0? Or is this needed for IPC desktop Firefox
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.
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.
Felipe should have some code for this, which makes focus, mouse and
key events just work, IIRC.
Where is it?!?  :)  This would have been useful for quite a while now.
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
Felipe, do you want to reassign this bug to yourself then?
Let's do this!
Assignee: Olli.Pettay → felipc
Status: NEW → ASSIGNED
OS: Linux → All
Hardware: x86_64 → All
Posted patch Cross-process events delivery (obsolete) — Splinter Review
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.
Attachment #528668 - Flags: review?(Olli.Pettay)
Posted patch Cross-process events delivery (obsolete) — Splinter Review
Missed removal of some commented out code from the previous patch

The focus part is coming in a separate patch
Attachment #528668 - Attachment is obsolete: true
Attachment #528671 - Flags: review?(Olli.Pettay)
Attachment #528668 - Flags: review?(Olli.Pettay)
Posted patch Basic focus work (obsolete) — Splinter Review
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)
Attachment #528713 - Flags: review?(Olli.Pettay)
Attachment #528713 - Flags: feedback?(ben)
(This doesn't compile because of a TabParent.cpp missed return.)
Posted patch Cross-process events delivery (obsolete) — Splinter Review
Added missed return
Attachment #528671 - Attachment is obsolete: true
Attachment #528980 - Flags: review?(Olli.Pettay)
Attachment #528671 - Flags: review?(Olli.Pettay)
Posted patch Basic focus work (obsolete) — Splinter Review
(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.
Attachment #528713 - Attachment is obsolete: true
Attachment #528981 - Flags: review?(Olli.Pettay)
Attachment #528981 - Flags: feedback?(ben)
Attachment #528713 - Flags: review?(Olli.Pettay)
Attachment #528713 - Flags: feedback?(ben)
Yes, I fixed it locally just thought you'd like to know :)
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.
> * 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 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.
Attachment #528981 - Flags: feedback?(ben) → feedback+
Bug 624163 seems to be related.
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.
Attachment #528981 - Flags: review?(Olli.Pettay) → review+
Posted patch Mobile changes and test (obsolete) — Splinter Review
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()
Attachment #531534 - Flags: review?(ben)
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.
Attachment #531535 - Flags: review?(jones.chris.g)
Comment on attachment 528980 [details] [diff] [review]
Cross-process events delivery

This patch is the one you're going to modify still, right?
Attachment #528980 - Flags: review?(Olli.Pettay)
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.
Attachment #531535 - Flags: review?(jones.chris.g) → review+
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
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.
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.
(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 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.
Attachment #528981 - Flags: feedback-
> 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 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.
Attachment #531534 - Flags: review?(ben)
(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
> >+  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
Posted patch Updated mobile patch (obsolete) — Splinter Review
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.
Attachment #531534 - Attachment is obsolete: true
Attachment #539087 - Flags: review?(ben)
Whiteboard: [e10s]
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)
Attachment #528980 - Attachment is obsolete: true
Attachment #539319 - Flags: review?(Olli.Pettay)
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
Attachment #539087 - Flags: review?(ben)
Posted patch Focus patch (obsolete) — Splinter Review
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.
Attachment #528981 - Attachment is obsolete: true
Attachment #539674 - Flags: review?(enndeakin)
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)
Posted patch Focus patchSplinter Review
(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
Attachment #539674 - Attachment is obsolete: true
Attachment #539902 - Flags: review?(enndeakin)
Attachment #539674 - Flags: review?(enndeakin)
Addressed comments
Attachment #539087 - Attachment is obsolete: true
Attachment #539968 - Flags: review?(ben)
Depends on: 623625
Comment on attachment 539968 [details] [diff] [review]
Updated mobile patch

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

Remove blank line.

r+
Attachment #539968 - Flags: review?(ben) → review+
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.
Attachment #539902 - Flags: review?(enndeakin) → review+
Removing the early return for |if (gBrowserIsRemote)| doesn't depend on bug 623625, unless there's something I'm missing.
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 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;
}
Attachment #539319 - Flags: review?(Olli.Pettay) → review+
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
Attachment #539319 - Flags: review?(ben)
Attachment #539319 - Flags: review?(ben) → review+
I backed out everything from central since Android and Maemo were unhappy about the push these changes were part of.
With bug 623625 fixed, reftest-ipc can run tests marked as needs-focus:

http://hg.mozilla.org/integration/mozilla-inbound/rev/37c5482971b1
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.
Version: unspecified → Trunk
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
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.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
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.
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
https://hg.mozilla.org/mozilla-central/rev/9e613392a170
Status: REOPENED → RESOLVED
Closed: 8 years ago6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla23
(per comment 64)
Target Milestone: mozilla23 → mozilla8
You need to log in before you can comment on or make changes to this bug.