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.)

We just need to move the mozKeyboard implementation to use webIDL

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

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?

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).

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.

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.

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.

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

@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.

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...

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

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.

(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.

(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.

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

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.

Implementation work in bug 899999.

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 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.

TODO: Use the new DOMRequestIPCHelper from bug 902030

Awesome!

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 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

Isn't Andrea on vacation for most of August?

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/)

(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) 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.

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

Removed all Constructor and fixed the WebIDL.mk. Also changed to use DOMRequestHelper.jsm

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 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 on attachment 789394 [details] [diff] [review] Final patch Review of attachment 789394 [details] [diff] [review]: ----------------------------------------------------------------- r=me for the promises part.

(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) from comment #24) > Isn't Andrea on vacation for most of August? As far as I know, he is coming back next week.

\o/

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

(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.