899073 - Move MozInputMethod, MozInputMethodManager, MozInputContext to WebIDL

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Gaia::Keyboard, defect)

Description

[Jan Jongboom [:janjongboom] (inactive)]

As specified in the new API (https://wiki.mozilla.org/WebAPI/KeboardIME) we want to start using EventTarget. However this is only available in WebIDL as far as I know so we can't get to that from mozKeyboard. It'd be nice if someone could hand a solution (move it? did I miss something obvious? etc.)

Comment 1

[[:fabrice] Fabrice Desré]

We just need to move the mozKeyboard implementation to use webIDL

Comment 2

[Jan Jongboom [:janjongboom] (inactive)]

Can we keep using it in desktop FF then? Because now we build idl -> xpt, and sideload that when starting gaia in desktop FF.

Comment 3

[Boris Zbarsky [:bzbarsky]]

Moving everything web-facing to WebIDL is the long-term plan, so ideally we'd just move mozKeyboard to WebIDL.

I'd like to understand the desktop FF issue.  The simplest situation there is that we compile the binding in both FxOS and desktop, but only expose the objects to JS when we want to.  Is that a feasible approach?

Comment 4

[Jan Jongboom [:janjongboom] (inactive)]

Uhm, I don't really know, can we compile it into an extension and then load it in desktop? Because that's how we do it today and that makes development a lot easier. In general everything in mozKeyboard.idl and mozKeyboard.js can be exposed because all the heavy lifting is done in Keyboard.jsm already (js and idl only have API methods).

Comment 5

[Boris Zbarsky [:bzbarsky]]

Unfortunately, the generated WebIDL code ends up living in libxul.  So you can't put the .webidl file and its generated output into an extension at the moment.

And it needs to live in libxul because the generated class needs to inherit from nsDOMEventTargetHelper and whatnot, which is not really possible for code outside libxul.

Comment 6

[Yuan Xulei [:yxl] (Leave from May 18)]

We have a plan to use WebIDL to expose mozKeyboard as a DOM API for both FxOS and desktop. If that is true, we can directly access mozKeyboard in desktop without extension's support.

Comment 7

[Boris Zbarsky [:bzbarsky]]

One other note: even if for some reason we don't want to ship mozKeyboard.js on desktop, we can still ship the .webidl there and just pref it off or whatever.  That does mean that changes to the API will need a Gecko recompile, of course.

Comment 8

[Jan Jongboom [:janjongboom] (inactive)]

Alright, sounds like a plan. :yxl are you taking care of this?

Comment 9

[Yuan Xulei [:yxl] (Leave from May 18)]

@Boris, Shipping with pref is a good compromise.
@Jan, I'm taking care of it, but the priority of rewriting with WebIDL is lower than implementing the new keyboard API. I think late August is the appropriate time for us to start working on the DOM and WebIDL stuff after we finish most part of the new API.

Comment 10

[Boris Zbarsky [:bzbarsky]]

I suggest keeping WebIDL in mind when designing the new API; in particular, you may want to avoid APIs that WebIDL goes out of its way to not allow...

Comment 11

[Jan Jongboom [:janjongboom] (inactive)]

But how do we implement the new API if we can't use EventTarget or promises? Just implement it with the nsIDOMEventListener and DOMRequest?

Comment 12

[Kan-Ru Chen [:kanru] (UTC+9)]

I think we should build the new WebIDL based keyboard API (to implement EventTarget) into b2g and browser desktop but make it pref enabled. That way we don't have to worry about how to ship that in an extension.

Move the current keyboard API to js implemented WebIDL should be very straightforward. I could help that when :yxl is busy implementing the new APIs.

Comment 13

[Kan-Ru Chen [:kanru] (UTC+9)]

(In reply to Jan Jongboom [:janjongboom] from comment #11)
> But how do we implement the new API if we can't use EventTarget or promises?
> Just implement it with the nsIDOMEventListener and DOMRequest?

We are still planning a migration path from DOMRequests to Promises. If we couldn't use Promises yet, we could use DOMRequest first.

Comment 14

[Yuan Xulei [:yxl] (Leave from May 18)]

(In reply to Jan Jongboom [:janjongboom] from comment #11)
> But how do we implement the new API if we can't use EventTarget or promises?
> Just implement it with the nsIDOMEventListener and DOMRequest?

For EventTarget, we'll temporarily use nsIDOMEventListener instead. And for promise, I think we're able to use it already.

If Kan-Ru could help, we may be ahead of our schedule.

Comment 15

[Jan Jongboom [:janjongboom] (inactive)]

Great. Are you already working on implementation? I have a 'freeish' afternoon so I could make a start for the input context.

Comment 16

[Yuan Xulei [:yxl] (Leave from May 18)]

That's really good. I'm going to, but haven't started yet. So feel free to create bug and start for the input context.

Comment 17

[Jan Jongboom [:janjongboom] (inactive)]

Implementation work in bug 899999.

Comment 18

[Kan-Ru Chen [:kanru] (UTC+9)]

Attachment: 0001-Bug-899073-Move-MozInputMethod-MozInputMethodManager.patch

The first Promise user, yay!

@Xulei please review the API changes
@khuey could you review the WebIDL part?
@mounir please check the Promise usage, thanks!

Comment 19

[Mounir Lamouri (:mounir)]

Comment on attachment 788863 [details] [diff] [review]
0001-Bug-899073-Move-MozInputMethod-MozInputMethodManager.patch

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

Andrea will have a look at the promise usage.

Comment 20

[Kan-Ru Chen [:kanru] (UTC+9)]

TODO: Use the new DOMRequestIPCHelper from bug 902030

Comment 21

[Jan Jongboom [:janjongboom] (inactive)]

Awesome!

Comment 22

[Yuan Xulei [:yxl] (Leave from May 18)]

Comment on attachment 788863 [details] [diff] [review]
0001-Bug-899073-Move-MozInputMethod-MozInputMethodManager.patch

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

r=+. I think we can drop moz prefix now.

::: dom/webidl/InputMethod.webidl
@@ +3,5 @@
> + * License, v. 2.0. If a copy of the MPL was not distributed with this file,
> + * You can obtain one at http://mozilla.org/MPL/2.0/.
> + */
> +
> +[Constructor, JSImplementation="@mozilla.org/b2g-inputmethod;1", NavigatorProperty="mozInputMethod"]

Please help to drop the moz prefix.

@@ +26,5 @@
> +
> +// Manages the list of IMEs, enables/disables IME and switches to an
> +// IME.
> +[Constructor, JSImplementation="@mozilla.org/b2g-imm;1"]
> +interface MozInputMethodManager {

Idem.

@@ +54,5 @@
> +// It also hosts the methods available to the keyboard app to mutate the input field represented.
> +// An "input context" gets void when the app is no longer allowed to interact with the text field,
> +// e.g., the text field does no longer exist, the app is being switched to background, and etc.
> +[Constructor, JSImplementation="@mozilla.org/b2g-inputcontext;1"]
> +interface MozInputContext: EventTarget {

Idem.

@@ +153,5 @@
> +     * Set current composition. It will start or update composition.
> +     * @param cursor Position in the text of the cursor.
> +     *
> +     * The API implementation should automatically ends the composition
> +     * session (with event and confirm the current composition) if 

nit: remove trailing spaces.

Comment 23

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Comment on attachment 788863 [details] [diff] [review]
0001-Bug-899073-Move-MozInputMethod-MozInputMethodManager.patch

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

::: dom/webidl/WebIDL.mk
@@ +468,4 @@
>    StyleSheetApplicableStateChangeEvent.webidl \
>    StyleSheetChangeEvent.webidl \
>    UserProximityEvent.webidl \
> +  InputMethod.webidl \

This list is sorted

Comment 24

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Isn't Andrea on vacation for most of August?

Comment 25

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Comment on attachment 788863 [details] [diff] [review]
0001-Bug-899073-Move-MozInputMethod-MozInputMethodManager.patch

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

This looks pretty good, but I want to resolve the Constructor issue before giving r+.

::: dom/webidl/InputMethod.webidl
@@ +3,5 @@
> + * License, v. 2.0. If a copy of the MPL was not distributed with this file,
> + * You can obtain one at http://mozilla.org/MPL/2.0/.
> + */
> +
> +[Constructor, JSImplementation="@mozilla.org/b2g-inputmethod;1", NavigatorProperty="mozInputMethod"]

Why is this marked 'Constructor'?  The JS implementation does not have an '__init' method as far as I can tell, so new MozInputMethod will just throw ...

If we're going to unprefix this lets do that in a separate bug.

@@ +26,5 @@
> +
> +// Manages the list of IMEs, enables/disables IME and switches to an
> +// IME.
> +[Constructor, JSImplementation="@mozilla.org/b2g-imm;1"]
> +interface MozInputMethodManager {

Same here.

@@ +54,5 @@
> +// It also hosts the methods available to the keyboard app to mutate the input field represented.
> +// An "input context" gets void when the app is no longer allowed to interact with the text field,
> +// e.g., the text field does no longer exist, the app is being switched to background, and etc.
> +[Constructor, JSImplementation="@mozilla.org/b2g-inputcontext;1"]
> +interface MozInputContext: EventTarget {

This too.

::: dom/webidl/WebIDL.mk
@@ +468,4 @@
>    StyleSheetApplicableStateChangeEvent.webidl \
>    StyleSheetChangeEvent.webidl \
>    UserProximityEvent.webidl \
> +  InputMethod.webidl \

Actually this list is just for events, so it doesn't belong here at all.  This needs to be in a list that's ifdef MOZ_B2G so that we don't expose these properties on desktop (where there will be no implementation since MozKeyboard.js is still in b2g/)

Comment 26

[Kan-Ru Chen [:kanru] (UTC+9)]

(In reply to Kyle Huey [:khuey] ([e-mail]) from comment #25)
> Comment on attachment 788863 [details] [diff] [review]
> ::: dom/webidl/InputMethod.webidl
> @@ +3,5 @@
> > + * License, v. 2.0. If a copy of the MPL was not distributed with this file,
> > + * You can obtain one at http://mozilla.org/MPL/2.0/.
> > + */
> > +
> > +[Constructor, JSImplementation="@mozilla.org/b2g-inputmethod;1", NavigatorProperty="mozInputMethod"]
> 
> Why is this marked 'Constructor'?  The JS implementation does not have an
> '__init' method as far as I can tell, so new MozInputMethod will just throw
> ...

Ah. The wiki says we only support JS implemented WebIDL that has a Constructor. If that is not true anymore I'll update the wiki.

Comment 27

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

I think it's no longer true.  SettingsManager has no constructor.  http://mxr.mozilla.org/mozilla-central/source/dom/webidl/SettingsManager.webidl#29

Comment 28

[Kan-Ru Chen [:kanru] (UTC+9)]

Attachment: 0001-Bug-899073-Move-MozInputMethod-MozInputMethodManager.patch

Removed all Constructor and fixed the WebIDL.mk.

Also changed to use DOMRequestHelper.jsm

Comment 29

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Comment on attachment 789334 [details] [diff] [review]
0001-Bug-899073-Move-MozInputMethod-MozInputMethodManager.patch

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

r=me

::: dom/webidl/WebIDL.mk
@@ +526,5 @@
> +ifdef MOZ_B2G
> +webidl_files += \
> +  InputMethod.webidl \
> +  $(NULL)
> +endif

Nit: Please put this above the test files.

Comment 30

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

Comment on attachment 789334 [details] [diff] [review]
0001-Bug-899073-Move-MozInputMethod-MozInputMethodManager.patch

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

Stealing from Andrea on Kanru's request.

Code works and looks good, but please fix 904333 since its a small change, and then update this patch to use that.

Also some nits.

::: b2g/components/MozKeyboard.js
@@ +425,5 @@
>      // because the context is no longer valid.
>      Object.keys(self._requests).forEach(function(k) {
>        // takeRequest also does a delete from context
> +      let resolver = self.takePromiseResolver(k);
> +      resolver.reject("InputContext got destroyed");

refactor with bug 904333. You'll want to check that each request is a PromiseResolver, or lift that into DOMRequestHelper with two different forEach[Request|PromiseResolver]

@@ +447,5 @@
>        return;
>      }
>  
>      let json = msg.json;
> +    let promise = json.requestId ? this.takePromiseResolver(json.requestId) : null;

nit: s/promise/resolver

There is also no need for the conditional. If takePromiseResolver() is passed a non-existent id, it will return null.

@@ +476,3 @@
>          break;
>        default:
> +        promise.reject("Could not find a handler for " + msg.name);

Do you want to expose an implementation failure to content code?
Better to log an error in debug mode and reject the promise with no arguments. It is going to be truly rare (impossible?) and bad if the code was to hit this path.

@@ +539,5 @@
>    },
>  
>    getText: function ic_getText(offset, length) {
> +    let self = this;
> +    let promise = self.createPromise(function(resolver) {

nit: Please use 'this' on this line. The JS developer tendency on seeing a 'self' is to look at the outer block, which is confusing here, though not a big deal.

You can also skip the 'promise' temp variable, directly 'return this.createPromise(...'

@@ +540,5 @@
>  
>    getText: function ic_getText(offset, length) {
> +    let self = this;
> +    let promise = self.createPromise(function(resolver) {
> +      let promiseId = self.getPromiseResolverId(resolver);

nit: s/promiseId/resolverId

@@ +546,5 @@
> +        contextId: self._contextId,
> +        requestId: promiseId,
> +        offset: offset,
> +        length: length
> +      });

Looks good.

This pattern of getting the id in a lot of APIs makes me wonder if DOMRequestIpcHelper should have a createPromise() variant that just returns the id to the callback after storing the resolver in the table. Do you think that is a good idea? If you think so, feel free to file it and CC me. It would be a good first bug, I can mentor.

@@ +570,5 @@
>    },
>  
>    setSelectionRange: function ic_setSelectionRange(start, length) {
> +    let self = this;
> +    let promise = self.createPromise(function(resolver) {

as above s/self/this

@@ +571,5 @@
>  
>    setSelectionRange: function ic_setSelectionRange(start, length) {
> +    let self = this;
> +    let promise = self.createPromise(function(resolver) {
> +      let promiseId = self.getPromiseResolverId(resolver);

as above.

@@ +601,5 @@
>    },
>  
>    replaceSurroundingText: function ic_replaceSurrText(text, offset, length) {
> +    let self = this;
> +    let promise = self.createPromise(function(resolver) {

as above

@@ +602,5 @@
>  
>    replaceSurroundingText: function ic_replaceSurrText(text, offset, length) {
> +    let self = this;
> +    let promise = self.createPromise(function(resolver) {
> +      let promiseId = self.getPromiseResolverId(resolver);

same here.

@@ +621,5 @@
>    },
>  
>    sendKey: function ic_sendKey(keyCode, charCode, modifiers) {
> +    let self = this;
> +    let promise = self.createPromise(function(resolver) {

here too

@@ +622,5 @@
>  
>    sendKey: function ic_sendKey(keyCode, charCode, modifiers) {
> +    let self = this;
> +    let promise = self.createPromise(function(resolver) {
> +      let promiseId = self.getPromiseResolverId(resolver);

and here

@@ +637,4 @@
>    },
>  
>    setComposition: function ic_setComposition(text, cursor) {
>      throw "Not implemented";

Orthogonal, but please use a DOM NotSupportedError if it won't break the API across versions.

http://dom.spec.whatwg.org/#error-names-0

@@ +640,5 @@
>      throw "Not implemented";
>    },
>  
>    endComposition: function ic_endComposition(text) {
>      throw "Not implemented";

here too

Comment 31

[Kan-Ru Chen [:kanru] (UTC+9)]

Attachment: Final patch

Comment 32

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

Comment on attachment 789394 [details] [diff] [review]
Final patch

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

r=me for the promises part.

Comment 33

[Mounir Lamouri (:mounir)]

(In reply to Kyle Huey [:khuey] ([e-mail]) from comment #24)
> Isn't Andrea on vacation for most of August?

As far as I know, he is coming back next week.

Comment 34

[Kan-Ru Chen [:kanru] (UTC+9)]

https://hg.mozilla.org/integration/b2g-inbound/rev/76a010d2a973

Comment 35

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/76a010d2a973

Comment 36

[Yuan Xulei [:yxl] (Leave from May 18)]

\o/

Comment 37

[Boris Zbarsky [:bzbarsky]]

Kan-Ru, do you have time to fix the documentation, per comment 26?

Comment 38

[Kan-Ru Chen [:kanru] (UTC+9)]

(In reply to Boris Zbarsky [:bz] from comment #37)
> Kan-Ru, do you have time to fix the documentation, per comment 26?

ah, yes, I'll do it.