Closed
Bug 905573
Opened 11 years ago
Closed 11 years ago
Add setInputMethodActive to browser elements to allow gaia system set the active IME app.
Categories
(Core :: DOM: Core & HTML, defect, P1)
Core
DOM: Core & HTML
Tracking
()
People
(Reporter: xyuan, Assigned: xyuan)
References
Details
(Whiteboard: [FT:System-Platform, Sprint:2])
Attachments
(1 file, 8 obsolete files)
30.40 KB,
patch
|
xyuan
:
review+
|
Details | Diff | Splinter Review |
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() { .... }
Comment 1•11 years ago
|
||
Why is the concept of "focus" not sufficient here?
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.
Updated•11 years ago
|
blocking-b2g: --- → koi+
Priority: -- → P1
Whiteboard: [ucid:SystemPlatform1], [FT: System Platform], [Sprint: 2]
Assignee | ||
Updated•11 years ago
|
Assignee: nobody → xyuan
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.
Status: NEW → ASSIGNED
Attachment #791355 -
Flags: review?(justin.lebar+bug)
Assignee | ||
Comment 4•11 years ago
|
||
Hi Justin, could you help to review the patch?
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...
Comment 6•11 years ago
|
||
> Then you don't need this testing permission
s/permission/pref
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. :)
Attachment #791355 -
Flags: review?(justin.lebar+bug)
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.
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?
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.
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.
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.
Attachment #791355 -
Attachment is obsolete: true
Attachment #791621 -
Flags: review?(justin.lebar+bug)
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.
Attachment #791621 -
Flags: review?(justin.lebar+bug) → review-
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.
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.
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`.
Attachment #791621 -
Attachment is obsolete: true
Attachment #794615 -
Flags: review?(justin.lebar+bug)
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.
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.
Assignee | ||
Comment 19•11 years ago
|
||
Generate hg patch and improve the test.
Attachment #794615 -
Attachment is obsolete: true
Attachment #794615 -
Flags: review?(justin.lebar+bug)
Attachment #795722 -
Flags: review?(justin.lebar+bug)
Updated•11 years ago
|
Whiteboard: [ucid:SystemPlatform1], [FT: System Platform], [Sprint: 2] → [ucid:SystemPlatform1, FT: System-Platform, koi:p1, Sprint: 2]
Updated•11 years ago
|
Whiteboard: [ucid:SystemPlatform1, FT: System-Platform, koi:p1, Sprint: 2] → [ucid:SystemPlatform1, FT:System-Platform, koi:p1, Sprint: 2]
Comment 20•11 years ago
|
||
Remove ucid from white board since this is engineering bug
Whiteboard: [ucid:SystemPlatform1, FT:System-Platform, koi:p1, Sprint: 2] → [FT:System-Platform, koi:p1, Sprint: 2]
Assignee | ||
Comment 21•11 years ago
|
||
Hi Justin, Do you still have time to review the patch, or will you pass it on?
Flags: needinfo?(justin.lebar+bug)
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. :)
Flags: needinfo?(justin.lebar+bug)
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. :(
Attachment #795722 -
Flags: review?(justin.lebar+bug)
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.
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!
Flags: needinfo?(xyuan)
Comment 26•11 years ago
|
||
Update keyword in white board for the status query more precise
Whiteboard: [FT:System-Platform, koi:p1, Sprint: 2] → [FT:System-Platform, Sprint:2]
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.
Flags: needinfo?(xyuan)
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() { .... }
Attachment #795722 -
Attachment is obsolete: true
Attachment #800728 -
Flags: review?(fabrice)
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.
Assignee | ||
Comment 30•11 years ago
|
||
Hi Fabrice, have you started reviewing this patch?
Flags: needinfo?(fabrice)
Comment 31•11 years ago
|
||
(In reply to Yuan Xulei [:yxl] from comment #30) > Hi Fabrice, have you started reviewing this patch? Not yet :(
Flags: needinfo?(fabrice)
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.
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.
Assignee | ||
Comment 34•11 years ago
|
||
1. Fix nits. 2. Make BEP.jsm enforce that only one input method is active. 3. Augment tests.
Attachment #800728 -
Attachment is obsolete: true
Attachment #800728 -
Flags: review?(fabrice)
Attachment #803879 -
Flags: review?(fabrice)
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
Attachment #803879 -
Flags: review?(fabrice) → feedback+
Assignee | ||
Comment 36•11 years ago
|
||
Fabrice, thank you. This patch addresses all of comment 35.
Attachment #803879 -
Attachment is obsolete: true
Attachment #804199 -
Flags: review?(fabrice)
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!
Attachment #804199 -
Flags: review?(fabrice) → review+
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.
Attachment #804199 -
Attachment is obsolete: true
Attachment #804567 -
Flags: review+
Assignee | ||
Comment 39•11 years ago
|
||
Hi Kanru, could help to test on try as I don't have the account to do so :-(
Flags: needinfo?(kchen)
Comment 40•11 years ago
|
||
https://tbpl.mozilla.org/?tree=Try&rev=fa34e0ca0cbf
Flags: needinfo?(kchen)
Assignee | ||
Comment 41•11 years ago
|
||
Kan-Ru, thanks for your help with this bug and my commit access application.
Keywords: checkin-needed
Assignee | ||
Comment 42•11 years ago
|
||
Rebase the patch to avoid merge conflict.
Attachment #804567 -
Attachment is obsolete: true
Attachment #808263 -
Flags: review+
Comment 43•11 years ago
|
||
https://hg.mozilla.org/integration/b2g-inbound/rev/20b30db87bf4
Flags: in-testsuite+
Keywords: checkin-needed
Comment 44•11 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/20b30db87bf4
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla27
Comment 45•11 years ago
|
||
https://hg.mozilla.org/releases/mozilla-aurora/rev/227ace83cdbd
status-b2g-v1.2:
--- → fixed
status-firefox25:
--- → wontfix
status-firefox26:
--- → fixed
status-firefox27:
--- → fixed
Updated•6 years ago
|
Component: DOM → DOM: Core & HTML
You need to log in
before you can comment on or make changes to this bug.
Description
•