Yuan Xulei [:yxl] (Leave from May 18) Assignee
	Description • 11 years ago

As mentioned in bug 842436, the firefox os IME manager will manage a list of IME windows, each of which is a mozbrowser iframe. For security and privacy reason, there should be only one active IME window at a time. 

After discussed with gaia keyboard team, we want to add an API(named setInputMethodActive) on iframe to allow gaia IME manger explicitly setting the working IME window.

Calling `setInputMethodActive` on an iframe will set it as active IME window and other iframes as non-active IME windows.

The API is defined as 
  
  DOMRequest setInputMethodActive();

and can be used with the following code.

  document.createElement('iframe');
  iframe.mozbrowser = true;
  var req = iframe.setInputMethodActive();
  req.onsuccess = function() { .... }

	Justin Lebar (not reading bugmail)
	Comment 1 • 11 years ago

Why is the concept of "focus" not sufficient here?

	Yuan Xulei [:yxl] (Leave from May 18) Assignee
	Comment 2 • 11 years ago

It is much like the "focus". But when keyboard shows, user focus is not on the active IME window, but the app receiving input.

	Yuan Xulei [:yxl] (Leave from May 18) Assignee
	Comment 3 • 11 years ago

The patch adds the method setInputMethodActive. For security reason, I restrict this method to gaia system app only. 

When system app calls setInputMethodActive on an IME app iframe, a child process message named 'Keyboard:SetInputMethodActive' along the window ID will be sent to Keyboard.jsm(b2g/components/Keyboard.jsm), where the active IME window info is stored. After Keyboard.jsm recevied the message, an ack named 'Keyboard:SetInputMethodActive:Result' will be returned.

setInputMethodActive will fail if the browser iframe doesn't contain an IME app(a privileged app with keyboard permission).

Note: We want to manage the active IME info in Keyboard.jsm, but bug 842436 is more suitable for keyboard stuff. Therefor this patch only builds the communication channel between browser element and Keyboard.jsm.

	Yuan Xulei [:yxl] (Leave from May 18) Assignee
	Comment 4 • 11 years ago

Hi Justin, could you help to review the patch?

	Justin Lebar (not reading bugmail)
	Comment 5 • 11 years ago

We don't do security checks by checking (app's origin) + (whether the app is certified).

Instead, please add a proper permission for this, and make the permission available only to certified apps.

Then you don't need this testing permission; instead, you can add the permission to the test page.

More to come...

	Justin Lebar (not reading bugmail)
	Comment 6 • 11 years ago

> Then you don't need this testing permission

s/permission/pref

	Justin Lebar (not reading bugmail)
	Comment 7 • 11 years ago

Comment on attachment 791355 [details] [diff] [review]
add setInputMethodActive.

Sorry, this is kind of a stream-of-consciousness review.  Fast, thorough, or
coherent -- choose two of the three.  :)

>diff --git a/dom/browser-element/BrowserElementChildPreload.js b/dom/browser-element/BrowserElementChildPreload.js
>--- a/dom/browser-element/BrowserElementChildPreload.js
>+++ b/dom/browser-element/BrowserElementChildPreload.js

>@@ -158,16 +158,27 @@ BrowserElementChild.prototype = {
>     securityUI.init(content);
> 
>     // A cache of the menuitem dom objects keyed by the id we generate
>     // and pass to the embedder
>     this._ctxHandlers = {};
>     // Counter of contextmenu events fired
>     this._ctxCounter = 0;
> 
>+    this._innerWindowID = content.document.defaultView
>+                          .QueryInterface(Ci.nsIInterfaceRequestor)
>+                          .getInterface(Ci.nsIDOMWindowUtils)
>+                          .currentInnerWindowID;

The BEC's inner window ID changes every time the page inside the <iframe>
navigates.  (This is in contrast to the outer window ID which does not
change.)

So caching the inner window ID like this doesn't make sense to me.

>+    // Request ID used to notify the parent when setInputMethodActive is done.
>+    this._setInputMethodActiveRequestId = null;
>+
>+    // The cached child process message manager.
>+    this._cpmm = null;

Is there a reason we need to cache the cpmm?  I'd prefer just to get it each
time, unless you've measured that to be unacceptably slow.

>@@ -284,16 +296,28 @@ BrowserElementChild.prototype = {
>   /**
>    * Called when our TabChildGlobal starts to die.  This is not called when the
>    * page inside |content| unloads.
>    */
>   _unloadHandler: function() {
>+    if (this._cpmm) {
>+      this._cpmm.removeMessageListener('Keyboard:SetInputMethodActive:Result',
>+                                       this);
>+    }
>+    // Handle the pending setInputMethodActive request.
>+    if (this._setInputMethodActiveRequestId) {
>+      sendAsyncMsg('got-set-input-method-active', {
>+        id: this._setInputMethodActiveRequestId,
>+        errorMsg: 'Window is unloaded.'
>+      });
>+      this._setInputMethodActiveRequestId = null;
>+    }
>     this._shuttingDown = true;
>   },

>@@ -858,16 +882,72 @@ BrowserElementChild.prototype = {
>     }
>   },
> 
>   _recvStop: function(data) {
>     let webNav = docShell.QueryInterface(Ci.nsIWebNavigation);
>     webNav.stop(webNav.STOP_NETWORK);
>   },
> 
>+  _recvSetInputMethodActive: function(data) {
>+    try {
>+      if (this._setInputMethodActiveRequestId) {
>+        throw 'The previous operation has not finished.';
>+      }
>+
>+      // Check preference to see if testing is enabled.
>+      let testing = false;
>+      try {
>+        testing = Services.prefs.getBoolPref(
>+          'dom.mozBrowserFrames.setInputMethodActive.testing');
>+      } catch(e) {}
>+
>+      if (!testing) {
>+        let principal = content.document.nodePrincipal;
>+        let perm = Services.perms
>+                   .testExactPermissionFromPrincipal(principal, "keyboard");
>+        if (perm != Ci.nsIPermissionManager.ALLOW_ACTION) {
>+          throw 'Don\'t have the permission to set as input method.\n';
>+        }
>+      }
>+
>+      this._setInputMethodActiveRequestId = data.json.id;
>+
>+      if (!this._cpmm) {
>+        this._cpmm = Cc['@mozilla.org/childprocessmessagemanager;1']
>+                    .getService(Ci.nsIMessageSender);
>+        // Listen to keyboard.jsm to get setInputMethodActive result.
>+        this._cpmm.addMessageListener('Keyboard:SetInputMethodActive:Result',
>+                                      this);

We should use addWeakMessageListener (and removeWeakMessageListener) if this
lands after bug 902714 lands, so please keep an eye on that one.

>+      }
>+
>+      // Send a message to ask Keyboard.jsm to activate this IME window.
>+      this._cpmm.sendAsyncMessage('Keyboard:SetInputMethodActive', {
>+        innerWindowID: this._innerWindowID
>+      });

I really don't think innerWindowID is what you want here.  What are you trying
to do?

>+    } catch(errorMsg) {
>+      sendAsyncMsg('got-set-input-method-active', {
>+        id: data.json.id,
>+        errorMsg: errorMsg
>+      });
>+    }

If the only exception you expect to catch is the one you explicitly throw
above, please don't use try/catch.

>+  },
>+
>+  receiveMessage: function(msg) {

I'd prefer if the cpmm called into a named function here, just like we do for
the frame message manager.  Then you wouldn't have to do the msg.name check.

>+    if (msg.name === 'Keyboard:SetInputMethodActive:Result' &&
>+        msg.json.innerWindowID == this._innerWindowID) {
>+      // Get SetInputMethodActive result and forward it to parent.
>+      sendAsyncMsg('got-set-input-method-active', {
>+        id: this._setInputMethodActiveRequestId,
>+        successRv: null
>+      });
>+      this._setInputMethodActiveRequestId = null;
>+    }
>+  },

It's not clear to me why this has to go through the child process at all.  Does
Keyboard.jsm live in the parent?

>diff --git a/dom/browser-element/BrowserElementParent.jsm b/dom/browser-element/BrowserElementParent.jsm
>--- a/dom/browser-element/BrowserElementParent.jsm
>+++ b/dom/browser-element/BrowserElementParent.jsm

>@@ -286,16 +288,41 @@ BrowserElementParent.prototype = {
> 
>     if (!evt.defaultPrevented) {
>       cancelCallback();
>     }
>   },
> 
>   _sendAsyncMsg: function(msg, data) {
>     try {
>+      if (msg === 'set-input-method-active') {
>+        // Check preferences to see if testing is enabled.
>+        let testing = false;
>+        try {
>+          testing = Services.prefs.getBoolPref(
>+            'dom.mozBrowserFrames.setInputMethodActive.testing');
>+        } catch(e) {}
>+
>+        // If tesing is not enabled, only allow the system app to call
>+        // setInputMethodActive.
>+        if (!testing) {
>+          let principal = this._window.document.nodePrincipal;
>+          if (principal.appStatus != Ci.nsIPrincipal.APP_STATUS_CERTIFIED) {
>+            dump('setInputMethodActive should be called by certified app.\n');

Please use debug() instead of dump().

>+            return false;
>+          }
>+
>+          let app = DOMApplicationRegistry.getAppByLocalId(principal.appId);
>+          if (!app || app.id !== 'system.gaiamobile.org') {
>+            dump('setInputMethodActive can only be called by system app.\n');
>+            return false;
>+          }
>+        }
>+      }

_sendAsyncMsg is just for sending messages; if we had logic like this in
_sendAsyncMsg for every message, this method would quickly become unmanageable.

Instead of

>+  defineDOMRequestMethod('setInputMethodActive', 'set-input-method-active');

You'll need to do

    defineMethod('setInputMethodActive', this._setInputMethodActive)

and do the security check in there, and then somehow set up the DOM request.

But in fact, I think we can be a bit more clever.  We can simply define this
method only if you're the system app.  That is, I think you could do the
permission check in the BEC constructor, and then do

  if (hasPermission) {
    defineDOMRequestMethod('setInputMethodActive', 'set-input-method-active');
  }

without modifying _sendAsyncMsg at all.

After reading this patch, I have no idea what it's trying to accomplish.  It
looks like the flow is:

 * (Parent) System app calls call a method on the iframe.
 * (Parent) BrowserElementParent sends a message to the child.
 * (Child) Send a message to the parent (keyboard.jsm) including the inner window ID
 * (Parent) Do something in keyboard.jsm, then send a message to the child indicating success.
 * (Child) Send a message to parent indicating that we got the success message.
 * (Parent) Get success message, fire success on the DOMRequest.

As far as I can tell, the only reason we interact with the child at all is to
get the inner window ID.  I don't understand why we need that.  So overall, I
am pretty confused.  :)

	Yuan Xulei [:yxl] (Leave from May 18) Assignee
	Comment 8 • 11 years ago

What I want to do is:

1. Get an unchanged unique ID for each iframe.

2. When system app calls the method on the iframe to activate, let Keyboard.jsm(in the parent) know the iframe ID. Keyboard.jsm will block other non-active iframes by checking their IDs.

	Justin Lebar (not reading bugmail)
	Comment 9 • 11 years ago

> 1. Get an unchanged unique ID for each iframe.

I guess you want an ID that's globally unique across all Gecko processes?

What about if you took a weakref to the iframe itself?

	Yuan Xulei [:yxl] (Leave from May 18) Assignee
	Comment 10 • 11 years ago

Yes, I want an global ID across processes. And the ID can be accessed from both the parent and the child. 

One case we have is:

The navigator.mozKeyboard of the child sends messages to Keyboard.jsm with the iframe ID. Keyboard.jsm will check the ID and forward those with correct ID to the normal app receiving user input.

Can we use the outer window ID? MDN(https://developer.mozilla.org/en-US/docs/Code_snippets/Windows) says it is a unique number generated when window is created.

	Justin Lebar (not reading bugmail)
	Comment 11 • 11 years ago

> Can we use the outer window ID? MDN(https://developer.mozilla.org/en-US/docs/Code_snippets/Windows) 
> says it is a unique number generated when window is created.

The outer window ID is unique only per-process.

If I understand you correctly, you can probably use a WeakSet keyed on the <iframe> to do what you're describing.

	Yuan Xulei [:yxl] (Leave from May 18) Assignee
	Comment 12 • 11 years ago

This patch changes the method signiture to:
  void setInputMethodActive()

It chooses outer window ID as iframe ID. The ID of the active input method is stored in BrowserElementParent.jsm. 

As the Keyboard.jsm lives in the parent process, it can import BrowserElementParent.jsm and get the active input method ID directly without sending an asynchronous message.

To access the method, "inputmethod-manage" permission is required.

	Justin Lebar (not reading bugmail)
	Comment 13 • 11 years ago

Comment on attachment 791621 [details] [diff] [review]
setInputMethodActive

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

Sorry, this still can't work.

This patch uses as an identifier the outer window ID of the page which contains the <iframe mozbrowser>.  We call this page the "embedder".

This won't work because a given page may contain multiple <iframe mozbrowser> frames.  In this case, the outer window ID will not uniquely identify an <iframe mozbrowser>.

I still think that what you probably want is to use a weak reference to the <iframe> as its ID.  You can pass the iframe object to the keyboard code, and the keyboard code can add the iframe to a WeakSet, using the iframe as the key.

	Yuan Xulei [:yxl] (Leave from May 18) Assignee
	Comment 14 • 11 years ago

I'll try to use <iframe> as ID, but the problem is how the document of the child side knows which iframe it belongs to.

Ideally the document within iframe should know whether its iframe is active and block any keyboard operations from the document if not active.

	Justin Lebar (not reading bugmail)
	Comment 15 • 11 years ago

> Ideally the document within iframe should know whether its iframe is active and block 
> any keyboard operations from the document if not active.

Does the latest patch accomplish this?  I don't see it, but I might be missing it.

You can use a weak reference to the <iframe> as the ID and also send a message down to the iframe to notify it that its IME is active or not; that may solve your problem.

	Yuan Xulei [:yxl] (Leave from May 18) Assignee
	Comment 16 • 11 years ago

The patch maps each input method iframe to an ID, which is generated by the Keyboard.jsm. The IME within the iframe can only interact with the input field by exchanging message with Keyboard.jsm. Each message contains the ID of the input method iframe. Keyboard.jsm will check message ID and discard those from inactive input methods.

When setInputMethodActive is called, the corresponding input method is activated and the ID is sent to the child side of the iframe by `navigator.mozInputMethod.setActive`.

	Justin Lebar (not reading bugmail)
	Comment 17 • 11 years ago

Comment on attachment 794615 [details] [diff] [review]
setInputMethodActive.patch

I'll review this soon, but in the meantime, is there anything we can do to improve this test?  As-is, it's not really checking anything.

	Justin Lebar (not reading bugmail)
	Comment 18 • 11 years ago

Comment on attachment 794615 [details] [diff] [review]
setInputMethodActive.patch

Also, in the future, please attach -U8 -M -C diffs.  There's no way to change git's default, but if you use git bz attach, it will automatically do the right thing.

https://github.com/jlebar/moz-git-tools

Alternatively, you could make a git moz-format-patch alias which does the right thing.

	Yuan Xulei [:yxl] (Leave from May 18) Assignee
	Comment 19 • 11 years ago

Generate hg patch and improve the test.

	Ivan Tsay (:ITsay)
	Comment 20 • 11 years ago

Remove ucid from white board since this is engineering bug

	Yuan Xulei [:yxl] (Leave from May 18) Assignee
	Comment 21 • 11 years ago

Hi Justin, 

Do you still have time to review the patch, or will you pass it on?

	Justin Lebar (not reading bugmail)
	Comment 22 • 11 years ago

I'm not sure; my last day is on Friday, so if I can get to this today or tomorrow, I will.  Otherwise, I won't.  :)

	Justin Lebar (not reading bugmail)
	Comment 23 • 11 years ago

Comment on attachment 795722 [details] [diff] [review]
setInputMethodActive.patch


>diff --git a/dom/browser-element/BrowserElementParent.jsm b/dom/browser-element/BrowserElementParent.jsm
>--- a/dom/browser-element/BrowserElementParent.jsm
>+++ b/dom/browser-element/BrowserElementParent.jsm

>@@ -20,16 +20,24 @@ Cu.import("resource://gre/modules/Servic
>+XPCOMUtils.defineLazyGetter(this, "Keyboard", function() {
>+  // This is a produc-specific file that's sometimes unavailable.

product-specific

>@@ -182,16 +190,25 @@ function BrowserElementParent(frameLoade
>+  let principal = this._frameElement.ownerDocument.nodePrincipal;
>+  let perm = Services.perms
>+             .testExactPermissionFromPrincipal(principal, "inputmethod-manage");
>+  if (perm === Ci.nsIPermissionManager.ALLOW_ACTION) {
>+    if (Keyboard) {
>+      defineMethod('setInputMethodActive', this._setInputMethodActive);
>+    }
>+  }

  if (perm === Ci.nsIPermissionManager.ALLOW_ACTION && Keyboard)

>@@ -575,16 +592,23 @@ BrowserElementParent.prototype = {
>+  _setInputMethodActive: function() {
>+    let id = Keyboard.setActiveInputMethod(this._frameElement);
>+    if (id !== -1) {
>+      this._sendAsyncMsg('set-input-method-active', {inputMethodId: id});
>+    }
>+  },

I still don't quite get this.  As designed, don't we also need to inform the
frame which previously had input-method-active set that its input method has
been deactivated?  As written, I don't see how we avoid having two processes
which both think their input methods are active.  Unless that's the purpose of
the ActiveInputMethodChange message dispatched in MozKeyboard.js?  But why do
we have to go down to the child to send that message?

I also don't quite get why we need to inform the child process at all.  When
the child process wants to type something, it calls into the keyboard, which
eventually calls into the main process.  When the main process receives the
message from the child process, can't it then check whether the child process's
input method is active?

This is better from a security pov because it means that a malicious child
process can't send bogus key events.  It also prevents the main process from
colliding with a child process's input method (and vice versa), which I'd guess
this code doesn't prevent at the moment.  It's also better from a performance
and battery-life pov, because broadcasting messages to all processes is very
expensive.

>diff --git a/dom/browser-element/mochitest/browserElement_SetInputMethodActive.js b/dom/browser-element/mochitest/browserElement_SetInputMethodActive.js
>new file mode 100644
>--- /dev/null
>+++ b/dom/browser-element/mochitest/browserElement_SetInputMethodActive.js

If this code in fact has the bug described, please modify this test so that it
checks for this case.

>diff --git a/dom/browser-element/mochitest/file_inputmethod.html b/dom/browser-element/mochitest/file_inputmethod.html
>new file mode 100644
>--- /dev/null
>+++ b/dom/browser-element/mochitest/file_inputmethod.html

>+  SpecialPowers.addPermission('keyboard', true, document);

Huh, I didn't think specialpowers worked in child processes.  That's cool...

This patch now has a lot of keyboard-related code, and I'm not the best person
to review this.  Sorry.  :(

	Yuan Xulei [:yxl] (Leave from May 18) Assignee
	Comment 24 • 11 years ago

Justin, thanks! I'll address your comments and ask Fabrice to review next patch.

(In reply to Justin Lebar [:jlebar] from comment #23)
> Comment on attachment 795722 [details] [diff] [review]
> 
> >@@ -182,16 +190,25 @@ function BrowserElementParent(frameLoade
> >+  let principal = this._frameElement.ownerDocument.nodePrincipal;
> >+  let perm = Services.perms
> >+             .testExactPermissionFromPrincipal(principal, "inputmethod-manage");
> >+  if (perm === Ci.nsIPermissionManager.ALLOW_ACTION) {
> >+    if (Keyboard) {
> >+      defineMethod('setInputMethodActive', this._setInputMethodActive);
> >+    }
> >+  }
> 
>   if (perm === Ci.nsIPermissionManager.ALLOW_ACTION && Keyboard)
> 
> >@@ -575,16 +592,23 @@ BrowserElementParent.prototype = {
> >+  _setInputMethodActive: function() {
> >+    let id = Keyboard.setActiveInputMethod(this._frameElement);
> >+    if (id !== -1) {
> >+      this._sendAsyncMsg('set-input-method-active', {inputMethodId: id});
> >+    }
> >+  },
> 
> I still don't quite get this.  As designed, don't we also need to inform the
> frame which previously had input-method-active set that its input method has
> been deactivated?  As written, I don't see how we avoid having two processes
> which both think their input methods are active.  Unless that's the purpose
> of
> the ActiveInputMethodChange message dispatched in MozKeyboard.js?  But why do
> we have to go down to the child to send that message?
ActiveInputMethodChange message is used to inform the previous input method to
deactivate.  

The navigator.mozInputMethod.inputcontext of the child will be set to null if the
parent has been deactivated; otherwise it will be filled with input context info.
That's the reason why we need to inform the child process.

> 
> I also don't quite get why we need to inform the child process at all.  When
> the child process wants to type something, it calls into the keyboard, which
> eventually calls into the main process.  When the main process receives the
> message from the child process, can't it then check whether the child
> process's
> input method is active?
> 

Yes, the main process(Keyboard.jsm) will also check the message from the child
process and only allow the message from active input method to pass.

178 	
179 	// Discard messages from inactive input method.
180 	if (msg.data && ('inputMethodId' in msg.data) &&
181 	    msg.data.inputMethodId !== this._activeInputMethodId) {
182       return;
183     }

> This is better from a security pov because it means that a malicious child
> process can't send bogus key events.  It also prevents the main process from
> colliding with a child process's input method (and vice versa), which I'd
> guess
> this code doesn't prevent at the moment.  It's also better from a performance
> and battery-life pov, because broadcasting messages to all processes is very
> expensive.
> 
I think I can improve the patch without broadcasting messages to all processes.
The simply way is to keep a reference to the active IME iframe. If another IME 
iframe is activated, we will deactive the previous one first and notify the child
to nullify navigator.mozInputMethod.inputcontext. Then the new iframe will be
activated.

	Ivan Tsay (:ITsay)
	Comment 25 • 11 years ago

Hi Xulei,

May I know if this one is possible to be completed before 9/13? We are in sprint 4 and approaching the code complete by 9/16 pretty soon. Thank you!

	Ivan Tsay (:ITsay)
	Comment 26 • 11 years ago

Update keyword in white board for the status query more precise

	Yuan Xulei [:yxl] (Leave from May 18) Assignee
	Comment 27 • 11 years ago

(In reply to Ivan Tsay (:ITsay) from comment #25)
> Hi Xulei,
> 
> May I know if this one is possible to be completed before 9/13? We are in
> sprint 4 and approaching the code complete by 9/16 pretty soon. Thank you!

Yes, absolutely. It should be completed before 9/13.

	Yuan Xulei [:yxl] (Leave from May 18) Assignee
	Comment 28 • 11 years ago

Fabrice, could you help to review the patch as Justin has left?

The patch adds the following method to mozbrowser iframe.

DOMRequest setInputMethodActive(boolean);

To activate an input method iframe:

  document.createElement('iframe');
  iframe.mozbrowser = true;
  var req = iframe.setInputMethodActive(true);
  req.onsuccess = function() { .... }

To deactivate:

  document.createElement('iframe');
  iframe.mozbrowser = true;
  var req = iframe.setInputMethodActive(false);
  req.onsuccess = function() { .... }

	Yuan Xulei [:yxl] (Leave from May 18) Assignee
	Comment 29 • 11 years ago

Comment on attachment 800728 [details] [diff] [review]
setInputMethodActive v5

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

::: b2g/components/MozKeyboard.js
@@ +318,5 @@
> +    // Check if we're in testing mode.
> +    let isTesting = false;
> +    try {
> +      isTesting = Services.prefs.getBoolPref(TESTING_ENABLED_PREF);
> +    } catch (e) {}

For mochitest, I can't add keyboard permission for a child process. So I have to use a pref to bypass permssion check.

	Yuan Xulei [:yxl] (Leave from May 18) Assignee
	Comment 30 • 11 years ago

Hi Fabrice, have you started reviewing this patch?

	[:fabrice] Fabrice Desré
	Comment 31 • 11 years ago

(In reply to Yuan Xulei [:yxl] from comment #30)
> Hi Fabrice, have you started reviewing this patch?

Not yet :(

	[:fabrice] Fabrice Desré
	Comment 32 • 11 years ago

Comment on attachment 800728 [details] [diff] [review]
setInputMethodActive v5

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

That looks mostly good. Please add more tests, and explain how we ensure that only one keyboard is active at any time. If this relies on the system app calling newKbd.setInputMethodActive(false), newKbd.setInputMethodActive(true) that doesn't seem good enough to me. I would rather have BEP.jsm enforce that only one input method is active.

::: b2g/components/Keyboard.jsm
@@ +16,5 @@
>  
>  XPCOMUtils.defineLazyServiceGetter(this, "ppmm",
>    "@mozilla.org/parentprocessmessagemanager;1", "nsIMessageBroadcaster");
>  
> +this.Keyboard = {

Why this change? We actually don't need to export anything from this jsm...

@@ +87,5 @@
>      // keyboard permission.
>      if (msg.name.indexOf("Keyboard:") != -1) {
> +      if (!this.messageManager) {
> +        return;
> +      }

Why are you doing that? You don't use messageManager afterward in this function.

::: b2g/components/MozKeyboard.js
@@ +210,5 @@
> +
> +  /*
> +   * Check if the given window is active.
> +   */
> +  isActive: function map_isActive(win) {

nit: no need to name functions anymore.

@@ +771,5 @@
> +      if (!WindowMap.isActive(self._window)) {
> +        self.removePromiseResolver(resolverId);
> +        resolver.reject('Input method is not active.');
> +        return;
> +      }

Can you create a helper function instead of replicating this block in all functions?

::: dom/browser-element/BrowserElementChildPreload.js
@@ +425,5 @@
>      win.modalDepth--;
>    },
>  
> +  _recvSetInputMethodActive: function(data) {
> +    let msgData = {id: data.json.id};

nit: { id... id }

::: dom/browser-element/BrowserElementParent.jsm
@@ +594,5 @@
> +      throw Components.Exception("Invalid argument",
> +                                 Cr.NS_ERROR_INVALID_ARG);
> +    }
> +    return this._sendDOMRequest('set-input-method-active',
> +                                {isActive: isActive});

nit: { isActive: isActive }

::: dom/browser-element/mochitest/browserElement_SetInputMethodActive.js
@@ +11,5 @@
> +
> +function setup() {
> +  SpecialPowers.setBoolPref("dom.mozInputMethod.enabled", true);
> +  SpecialPowers.setBoolPref("dom.mozInputMethod.testing", true);
> +  SpecialPowers.addPermission('inputmethod-manage', true, document);

nit: use either simple or double quote, but be consistent in the file.

@@ +40,5 @@
> +    frames[i].setAttribute('mozapp', location.href.replace(/[^/]+$/, 'file_inputmethod.webapp'));
> +    document.body.appendChild(frames[i]);
> +  }
> +
> +  let cnt = 0;

s/cnt/count

@@ +70,5 @@
> +
> +  function testNextInputMethod() {
> +    if (cnt > 0) {
> +      // Deactivate the previous iframe.
> +      let req = frames[cnt-1].setInputMethodActive(false);

nit: cnt - 1

@@ +84,5 @@
> +      // Activate the first iframe
> +      frames[cnt++].setInputMethodActive(true);
> +    }
> +  }
> +

I'd like to see the test be augmented to test situations where the context in not available anymore.

	Yuan Xulei [:yxl] (Leave from May 18) Assignee
	Comment 33 • 11 years ago

(In reply to Fabrice Desré [:fabrice] from comment #32)
> Comment on attachment 800728 [details] [diff] [review]
> setInputMethodActive v5
> 
> Review of attachment 800728 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> That looks mostly good. Please add more tests, and explain how we ensure
> that only one keyboard is active at any time. If this relies on the system
> app calling newKbd.setInputMethodActive(false),
> newKbd.setInputMethodActive(true) that doesn't seem good enough to me. I
> would rather have BEP.jsm enforce that only one input method is active.
> 
The patch relies on the system app calling setInputMethodActive(false) to
ensure that only one keyboard is active. I'll make a change have BEP.jsm
ensure that.

> ::: b2g/components/Keyboard.jsm
> @@ +16,5 @@
> >  
> >  XPCOMUtils.defineLazyServiceGetter(this, "ppmm",
> >    "@mozilla.org/parentprocessmessagemanager;1", "nsIMessageBroadcaster");
> >  
> > +this.Keyboard = {
> 
> Why this change? We actually don't need to export anything from this jsm...

It is a mistake and should be changed.
> 
> @@ +87,5 @@
> >      // keyboard permission.
> >      if (msg.name.indexOf("Keyboard:") != -1) {
> > +      if (!this.messageManager) {
> > +        return;
> > +      }
> 
> Why are you doing that? You don't use messageManager afterward in this
> function.
When mozInputMethod is activated by calling mozInputMethod.setActive, 'Keyboard:GetContext' message will be sent to Keyboard.jsm.  Then Keyboard.jsm calls the following function to forward the message to form.js.
  getContext: function keyboardGetContext(msg) {
    this.sendAsyncMessage('Forms:GetContext', msg.data);
  },
this.sendAsyncMessage uses messageManager to send message to forms.js.
If there is no focused input element in forms.js, this.messageManager will be null and calling this.sendAsyncMessage will throw error.

'Keyboard:Remove' message will cause the same issue with 'Keyboard:GetContext'.

So I have to check messageManger here.

	Yuan Xulei [:yxl] (Leave from May 18) Assignee
	Comment 34 • 11 years ago

1. Fix nits.
2. Make BEP.jsm enforce that only one input method is active.
3. Augment tests.

	[:fabrice] Fabrice Desré
	Comment 35 • 11 years ago

Comment on attachment 803879 [details] [diff] [review]
setInputMethodActive v6

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

We're almost there!

::: dom/browser-element/BrowserElementParent.jsm
@@ +93,5 @@
>  }
>  
> +
> +// The active input method iframe.
> +var activeInputMethod = null;

nit: s/var/let and rename to activeInputFrame

@@ +603,5 @@
> +
> +    // Deactivate the old input method if needed.
> +    if (activeInputMethod && isActive) {
> +      let self = this;
> +      let reqOld = XPCNativeWrapper.unwrap(activeInputMethod).setInputMethodActive(false);

nit: indentation

@@ +607,5 @@
> +      let reqOld = XPCNativeWrapper.unwrap(activeInputMethod).setInputMethodActive(false);
> +      reqOld.onsuccess = function() {
> +        activeInputMethod = null;
> +        self._sendSetInputMethodActiveDOMRequest(req, isActive);
> +      };

please bind(this) the function and remove 'self'

@@ +614,5 @@
> +          'Failed to deactivate the old input method: ' +
> +          reqOld.error + '.');
> +      };
> +    }
> +    return this._sendSetInputMethodActiveDOMRequest(req, isActive);

if (activeInputMethod && isActive), you're sending that twice.

@@ +627,5 @@
> +    if (this._sendAsyncMsg('set-input-method-active', data)) {
> +      activeInputMethod = this._frameElement;
> +      this._pendingDOMRequests[id] = req;
> +    } else {
> +      Services.DOMRequest.fireErrorAsync(req, "fail");

nit: simple quotes

	Yuan Xulei [:yxl] (Leave from May 18) Assignee
	Comment 36 • 11 years ago

Fabrice, thank you.

This patch addresses all of comment 35.

	[:fabrice] Fabrice Desré
	Comment 37 • 11 years ago

Comment on attachment 804199 [details] [diff] [review]
setInputMethodActive v7

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

r=me. Can you send that to try before landing? thanks!

	Yuan Xulei [:yxl] (Leave from May 18) Assignee
	Comment 38 • 11 years ago

Rebase the patch and temporarily disable tests to avoid breaking mochitest on nightly.
The tests will be enabled after bug 906096 fixes.

	Yuan Xulei [:yxl] (Leave from May 18) Assignee
	Comment 39 • 11 years ago

Hi Kanru, could help to test on try as I don't have the account to do so :-(

	Kan-Ru Chen [:kanru] (UTC+9)
	Comment 40 • 11 years ago

https://tbpl.mozilla.org/?tree=Try&rev=fa34e0ca0cbf

	Yuan Xulei [:yxl] (Leave from May 18) Assignee
	Comment 41 • 11 years ago

Kan-Ru, thanks for your help with this bug and my commit access application.

	Yuan Xulei [:yxl] (Leave from May 18) Assignee
	Comment 42 • 11 years ago

Rebase the patch to avoid merge conflict.

	Ryan VanderMeulen [:RyanVM]
	Comment 43 • 11 years ago

https://hg.mozilla.org/integration/b2g-inbound/rev/20b30db87bf4

	Ryan VanderMeulen [:RyanVM]
	Comment 44 • 11 years ago

https://hg.mozilla.org/mozilla-central/rev/20b30db87bf4

	Ryan VanderMeulen [:RyanVM]
	Comment 45 • 11 years ago

https://hg.mozilla.org/releases/mozilla-aurora/rev/227ace83cdbd