Since the new IME WebAPI is ready, we should remove the deprecated mozKeyboard API before v1.2 shipping.

If bug 906096 lands it will be removed from FF nightly, so if nothing breaks we can remove the codez from b2g/ as well.

We shouldn't expose the (deprecated) mozKeyboard API to 3rd-party apps.

Before completely removing it, we need to remove the reference of mozKeyboard from keyboard.js first. And as the system value_selector relies on mozKeyboard API, we may keep it for system app temporarily. For a temporary solution, I'd like to change the permission of mozKeyboard to 'inputmethod-manage', which can only be granted to certified app. FYI, the following are references of mozKeyboard of gaia. grep -r mozKeyboard keyboard/js/keyboard.js: * navigator.mozKeyboard keyboard/js/keyboard.js: * them into synthetic key events using the navigator.mozKeyboard API. keyboard/js/keyboard.js: * keycode to mozKeyboard.sendKey(). The IM could call keyboard/js/keyboard.js: * mozKeyboard.sendKey() directly, but doing it this way allows keyboard/js/keyboard.js:// If we get a focuschange event from mozKeyboard for an element with keyboard/js/keyboard.js: window.navigator.mozKeyboard.removeFocus(); keyboard/js/keyboard.js:// This is called when we get an event from mozKeyboard. system/test/unit/value_selector_test.js: realKeyboard = window.navigator.mozKeyboard; system/test/unit/value_selector_test.js: window.navigator.mozKeyboard = sinon.stub(); system/test/unit/value_selector_test.js: window.navigator.mozKeyboard = realKeyboard; system/js/value_selector/value_selector.js: window.navigator.mozKeyboard.setSelectedOption(singleOptionIndex); system/js/value_selector/value_selector.js: window.navigator.mozKeyboard.setSelectedOptions(optionIndices); system/js/value_selector/value_selector.js: window.navigator.mozKeyboard.removeFocus(); system/js/value_selector/value_selector.js: window.navigator.mozKeyboard.setValue(timeValue); system/js/value_selector/value_selector.js: window.navigator.mozKeyboard.setValue(dateValue); system/js/value_selector/value_selector.js: window.navigator.mozKeyboard.setValue(datetimeValue); system/js/value_selector/value_selector.js: window.navigator.mozKeyboard.removeFocus(); system/js/keyboard_manager.js:// If we get a focuschange event from mozKeyboard for an element with system/js/keyboard_manager.js: if (navigator.mozKeyboard) { system/js/keyboard_manager.js: navigator.mozKeyboard.onfocuschange = function onfocuschange(evt) {

I removed the code from keyboard_manager and keyboard.js already in bug 906096.

I removed the code from keyboard_manager and keyboard.js already in bug 906096.

Create! bug 906096 is indeed a fat bug.

Rudy, could you track this bug and find a person to work on it once bug 906096 is resolved? Thanks!

Yes, sure, will help to take care of this until we remove the mozKeyboard API.

Stealing this.

So the problem here is that we have a couple of functions |setSelectedOption|, |setSelectedOptions|, |setValue| that are used to control stuff like drop down lists etc. And we don't have equivalents in the new API. I'm also struggling to think of a logical API for this. Guys, any idea?

My suggestion is that we keep mozKeyboard for system app for internal usage, removing unused functions from mozKeyboard and only leaving those required.

(In reply to Yuan Xulei [:yxl] from comment #11) > My suggestion is that we keep mozKeyboard for system app for internal usage, > removing unused functions from mozKeyboard and only leaving those required. Yeah that probably make the most sense if we want to solve this bug in koi+ time frame. Who will be reviewing the patch? I am trying to find a proper person to ni? before people actually writing that patch.

I'd like to review the patch.

Stealing.

Let's see if this works ...

I can confirm the patch works locally (by testing with console.log() and manual test on value selector and keyboard). https://tbpl.mozilla.org/?tree=Try&rev=aa050c41b00e

Comment on attachment 823836 [details] [diff] [review] WIP I am not sure my try config is sufficient enough, could you verify? Also, I am also not sure whether or not I should modify this test http://dxr.mozilla.org/mozilla-central/source/dom/permission/tests/test_keyboard.html (the only test that mentioned mozKeyboard.

Comment on attachment 823836 [details] [diff] [review] WIP Review of attachment 823836 [details] [diff] [review]: ----------------------------------------------------------------- Hi Tim, we already have limited mozKeyboard to certified app. So privileged app, such as the third party app cannot use mozKeyboard. Will that be enough? Do we still need these changes? (In reply to Tim Guan-tin Chien [:timdream] (MoCo-TPE) (please ni?) from comment #17) > Comment on attachment 823836 [details] [diff] [review] > WIP > > I am not sure my try config is sufficient enough, could you verify? We need also run mochitests on emulator. The try command will be like this: try: -b o -p emulator, unagi,linux_gecko,linux64_gecko,macosx64_gecko -u mochitests, marionette-webapi,gaia-unit,gaia-ui-test -t none > > Also, I am also not sure whether or not I should modify this test > http://dxr.mozilla.org/mozilla-central/source/dom/permission/tests/ > test_keyboard.html > (the only test that mentioned mozKeyboard. The patch will break this test. If we want to limit the usage of mozKeyboard to system app only, we should remove mozKeyboard from this test. ::: dom/inputmethod/MozKeyboard.js @@ +51,5 @@ > } > > + // Make sure the deprecated mozKeyboard API does not > + // expose to any page in apps other than System app. > + Cu.import("resource://gre/modules/PermissionSettings.jsm"); You can import the jsm lazily by using |XPCOMUtils.defineLazyServiceGetter|. And it seems that this module can only be used in main process, but not the content process.

Thanks for the swift feedback! (In reply to Yuan Xulei [:yxl] from comment #18) > Comment on attachment 823836 [details] [diff] [review] > WIP > > Review of attachment 823836 [details] [diff] [review]: > ----------------------------------------------------------------- > > Hi Tim, we already have limited mozKeyboard to certified app. So privileged > app, such as > the third party app cannot use mozKeyboard. Will that be enough? Do we still > need these changes? I don't understand. Are you saying we have already allow mozInputMethod in privileged app but mozKeyboard certified only? How do we do that? I couldn't find anything on that in the codebase ... If so we could probably mark this bug fixed. > > (In reply to Tim Guan-tin Chien [:timdream] (MoCo-TPE) (please ni?) from > comment #17) > > Comment on attachment 823836 [details] [diff] [review] > > WIP > > > > I am not sure my try config is sufficient enough, could you verify? > We need also run mochitests on emulator. The try command will be like this: > > try: -b o -p emulator, unagi,linux_gecko,linux64_gecko,macosx64_gecko -u > mochitests, marionette-webapi,gaia-unit,gaia-ui-test -t none > > > > > Also, I am also not sure whether or not I should modify this test > > http://dxr.mozilla.org/mozilla-central/source/dom/permission/tests/ > > test_keyboard.html > > (the only test that mentioned mozKeyboard. > > The patch will break this test. If we want to limit the usage of mozKeyboard > to system app only, we should remove mozKeyboard from this test. > Sure. > ::: dom/inputmethod/MozKeyboard.js > @@ +51,5 @@ > > } > > > > + // Make sure the deprecated mozKeyboard API does not > > + // expose to any page in apps other than System app. > > + Cu.import("resource://gre/modules/PermissionSettings.jsm"); > > You can import the jsm lazily by using |XPCOMUtils.defineLazyServiceGetter|. > And it seems that this module can only be used in main process, but not the > content process. OK, I am running out of Gecko tricks then. :-/ Let's figure out whether or not we really should do this or not first.

(In reply to Tim Guan-tin Chien [:timdream] (MoCo-TPE) (please ni?) from comment #19) > > Hi Tim, we already have limited mozKeyboard to certified app. So privileged > > app, such as > > the third party app cannot use mozKeyboard. Will that be enough? Do we still > > need these changes? > > I don't understand. Are you saying we have already allow mozInputMethod in > privileged app but mozKeyboard certified only? How do we do that? I couldn't > find anything on that in the codebase ... Sorry, I made a mistake. I forgot we had exposed mozKeyboard to privileged app. We have a certified-app-only permission 'inputmethod-manage', which is used to let sysetem app to set the active keyboard frame(see bug 905573 for details). Currently only system app has this permission. We may use this permission to limit mozKeyboard to system app.

Manually verified. Try submitted: https://tbpl.mozilla.org/?tree=Try&rev=4fd9681d14dd

Comment on attachment 824486 [details] [diff] [review] Tentative patch v2 >+ // Limited the deprecated to certified apps only should be // Limited the deprecated mozKeyboard API to certified apps only

Comment on attachment 824486 [details] [diff] [review] Tentative patch v2 Review of attachment 824486 [details] [diff] [review]: ----------------------------------------------------------------- Thanks for enhancing the permission test. It's almost good for checking in. ::: dom/inputmethod/MozKeyboard.js @@ +43,5 @@ > init: function mozKeyboardInit(win) { > let principal = win.document.nodePrincipal; > + // Limited the deprecated to certified apps only > + let perm = Services.perms.testExactPermissionFromPrincipal(principal, > + "inputmethod-manage"); It will be better if you break this long line ;) ::: dom/permission/tests/test_keyboard.html @@ +19,5 @@ > var gData = [ > { > perm: ["keyboard"], > + obj: "mozInputMethod", > + idl: "nsIB2GKeyboard", Since mozInputMethod is implemented by webidl other than idl, we need change this line to: webidl: "InputMethod",

(In reply to Yuan Xulei [:yxl] from comment #23) > ::: dom/inputmethod/MozKeyboard.js > @@ +43,5 @@ > > init: function mozKeyboardInit(win) { > > let principal = win.document.nodePrincipal; > > + // Limited the deprecated to certified apps only > > + let perm = Services.perms.testExactPermissionFromPrincipal(principal, > > + "inputmethod-manage"); > > It will be better if you break this long line ;) I just noticed that you had broken the line into two, but the second line is longer than 80 characters and is wrapped to the third line strangely by bugzilla diff view.

(In reply to Yuan Xulei [:yxl] from comment #24) > (In reply to Yuan Xulei [:yxl] from comment #23) > > ::: dom/inputmethod/MozKeyboard.js > > @@ +43,5 @@ > > > init: function mozKeyboardInit(win) { > > > let principal = win.document.nodePrincipal; > > > + // Limited the deprecated to certified apps only > > > + let perm = Services.perms.testExactPermissionFromPrincipal(principal, > > > + "inputmethod-manage"); > > > > It will be better if you break this long line ;) > I just noticed that you had broken the line into two, but the second line is > longer than 80 characters and is > wrapped to the third line strangely by bugzilla diff view. Yeah, judging by the code present in the file I don't think we have a hard requirement on 80 chars there. So I am keeping the alignment. Going to push to try again now.

This is probably ready for review once Try passes. https://tbpl.mozilla.org/?tree=Try&rev=675b93bec0c8

Comment on attachment 824546 [details] [diff] [review] Tentative patch v2.1 Try failed

According to file_framework.js and file_shim.html: * The IDL describing the navigator object. The returned object * during tests /must/ be an instanceof this According to the error TypeError: invalid 'instanceof' operand window[this.webidl] It looks like we did not expose the constructor function to the frame? Alternatively, I did not set the pref dom.mozInputMethod.enabled. Going to try that.

(In reply to Tim Guan-tin Chien [:timdream] (MoCo-TPE) (please ni?) from comment #28) > Alternatively, I did not set the pref dom.mozInputMethod.enabled. Going to > try that. https://tbpl.mozilla.org/?tree=Try&rev=798a9d48492d not working.

(In reply to Tim Guan-tin Chien [:timdream] (MoCo-TPE) (please ni?) from comment #28) > According to file_framework.js and file_shim.html: > > * The IDL describing the navigator object. The returned object > * during tests /must/ be an instanceof this > > > According to the error > > TypeError: invalid 'instanceof' operand window[this.webidl] > > It looks like we did not expose the constructor function to the frame? > > Alternatively, I did not set the pref dom.mozInputMethod.enabled. Going to > try that. The WebIDL interface name should be MozInputMethod :-)

https://tbpl.mozilla.org/?tree=Try&rev=4fbc098b66cf Yeah, I figured out that too.

(In reply to Tim Guan-tin Chien [:timdream] (MoCo-TPE) (please ni?) from comment #31) > https://tbpl.mozilla.org/?tree=Try&rev=4fbc098b66cf The failure on test 9 seems unrelated but I just hit rerun anyway.