936724 - [keyboard] allow keyboard apps to add new layouts dynamically

Status: RESOLVED FIXED

(Core :: DOM: Device Interfaces, defect)

Description

[David Flanagan [:djf]]

The 3rd party keyboard framework that landed in v1.2 requires keyboard apps to list all their supported layouts as entry points in the manifest.

The problem with this is that it means that the list of layouts is completely static. Because layouts and dictionaries are large, we would like to allow the user to download new layouts and/or dictionaries at runtime.  And in order to do that, we will have to give keyboard apps some other way of advertising the set of layouts that they support.

In 1.3, I suspect we can use the new DataStore API to achieve this, but I don't have a fully worked-out plan.

Comment 1

[David Flanagan [:djf]]

Needinfo for Rudy and Tim in case either of them has ideas about how to fix this.

Comment 2

[David Flanagan [:djf]]

Setting needinfo for Stephany to bring this to her attention.

Stephany: there is talk about keyboard customization via downloadable layouts. It is a great idea, but cannot happen until this bug is addressed.  Do you know of any plans that require keyboard customizability in 1.3?

Comment 3

[Tim Guan-tin Chien [:timdream] (please needinfo)]

navigator.mozInputMethod.mgmt.addInput(id, inputManafest)
navigator.mozInputMethod.mgmt.removeInput(id)

and in FxOS, Gecko would loop the information to Gaia System keyboard manager. And for the keyboard helper, instead of parsing manifests of keyboard apps we would probably need to save the keyboard registry in system app datastore.

Does that sounds good?

Comment 4

[Jan Jongboom [:janjongboom] (inactive)]

+1 from me. We probably need an iterator over all registered inputs for the current keyboard app.

Comment 5

[David Flanagan [:djf]]

(In reply to Tim Guan-tin Chien [:timdream] (MoCo-TPE) (please ni?) from comment #3)
> navigator.mozInputMethod.mgmt.addInput(id, inputManafest)
> navigator.mozInputMethod.mgmt.removeInput(id)
> 
> and in FxOS, Gecko would loop the information to Gaia System keyboard
> manager. And for the keyboard helper, instead of parsing manifests of
> keyboard apps we would probably need to save the keyboard registry in system
> app datastore.
> 
> Does that sounds good?

Rather than adding an API and using the system app datastore, I was just imagining that each keyboard app would use its own datastore and that keyboard would still enumerate keyboard apps, but then would query their datastore rather than their manifest.

If we're adding addInput/removeInput methods to the mozInputMethod API, couldn't we just add a getInputs() method to query all known layouts and skip the DataStore completely?

Also: how do we deal with the initial set of layouts that a keyboard app defines? When the app is first installed and has not been run yet, it cannot have called the new addInput() method but we still need to know what layouts it defines.  I have been hoping that the DataStore API allows apps to be installed with a pre-populated datastore...  But if that is not possible, maybe we have to retain the current manifest-based solution and allow that to be extended somehow?

Comment 6

[GaryChen [:GaryChen][:PYChen][:gchen][:陳柏宇]]

Is it possible let keyboard app handle 'download dictionary package' by itself while user enable keyboard layout?

FxOS might have 3rd party keyboard in the further, each keyboard app has own download/update dictionary process and strategy.

In system side, we just need to notify keyboard app which layout is enable need to schedule 'download dictionary' in background and let keyboard manager focus on switch, show and hide keyboard frames.

Any idea?

Comment 7

[Tim Guan-tin Chien [:timdream] (please needinfo)]

(In reply to David Flanagan [:djf] from comment #5)
> (In reply to Tim Guan-tin Chien [:timdream] (MoCo-TPE) (please ni?) from
> comment #3)
> > navigator.mozInputMethod.mgmt.addInput(id, inputManafest)
> > navigator.mozInputMethod.mgmt.removeInput(id)
> > 
> > and in FxOS, Gecko would loop the information to Gaia System keyboard
> > manager. And for the keyboard helper, instead of parsing manifests of
> > keyboard apps we would probably need to save the keyboard registry in system
> > app datastore.
> > 
> > Does that sounds good?
> 
> Rather than adding an API and using the system app datastore, I was just
> imagining that each keyboard app would use its own datastore and that
> keyboard would still enumerate keyboard apps, but then would query their
> datastore rather than their manifest.
> 

Asking each keyboard app to expose a DataStore is too complex in this scenario. I would like to stick with manifests even though I don't like it that much either. You are asking for a solution extending the current way of doing things, rather than replacing it. This approach is also an analogy to Activity registry -- manifest entries + API.

We still need one, and one only, keyboard registry. That currently lives in mozApps (installed apps and available inputs) and mozSettings (enabled inputs). IMHO to enable keyboards to dynamically add/remove inputs, replacing our use of mozApps and mozSettings with Datastore in System app seems to be the simplest approach (architecture-wise), since only Settings app would need to access that Datastore.

> If we're adding addInput/removeInput methods to the mozInputMethod API,
> couldn't we just add a getInputs() method to query all known layouts and
> skip the DataStore completely?

I am talking about keyboard app-facing API here.

> 
> Also: how do we deal with the initial set of layouts that a keyboard app
> defines? When the app is first installed and has not been run yet, it cannot
> have called the new addInput() method but we still need to know what layouts
> it defines.  I have been hoping that the DataStore API allows apps to be
> installed with a pre-populated datastore...  

> But if that is not possible,
> maybe we have to retain the current manifest-based solution and allow that
> to be extended somehow?

Yes, that's what I have in mind. Keyboard gets to define their initial, or static layouts in the manifest, and add/remove layouts when the user turn to it's settings page and attempt to create/delete other layout(s).

Comment 8

[Rudy Lu [:rudyl] (inactive after 2015/7/3)]

While I think what Tim propose here is a reasonable way, I would like to second the alternative way as  Gary mentioned in Comment 6.

Originally, the issue we want to address here is about dictionary size - we cannot put all dictionaries into one build due to size limit.

We could make the keyboard app still expose all the layouts in the (static) manifest.
And when the user triggers one of the layouts that lacks the dictionary, we can let the keyboard app download it by itself to devicestorage or something.

Comment 9

[Stephany Wilkes]

Is there still a UX need here? Let me know. Seems like this is more about how keyboard layouts will change, technically speaking, but I'm happy to help as we can.

Comment 10

[Stephany Wilkes]

I do not know about customization issues. Removing the flag for me and adding it for Carrie, who owns keyboard UX.

Comment 11

[Carrie Wang [New account: [e-mail]]]

I just had some discussion with Rudy. I'm with what Gary has proposed on comment 6 (In this case, UX might need to add some settings in Keyboard settings). 
ni? Bruce to see if we have any related plan for 1.4.

Comment 12

[Bruce Huang [:bhuang] (inactive)]

Backlogging this for future versions.

Comment 13

[Rudy Lu [:rudyl] (inactive after 2015/7/3)]

Separate the idea mentioned in comment 8 to bug 1029951.

Comment 14

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Let's revive this bug. The requirement for v2.2 actually only require us to implement this feature usable for our own keyboard, but hacking the API in comment 2 with IAC is actually harder and unproductive than doing the real API. 

Let me bring this up to dev-webapi to see if there is any concerns.

Comment 15

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Attachment: Patch v1.0

This patch implements |addInput| and |removeInput| as mentioned in the previous comment, and dev-webapi. Since the "backend" of these APIs exists in Gaia System app, the patch here only, simply, convert the request into a Promise and sent it to chrome process as mozChromeEvent, and receive mozContentEvent to resolve/reject promises.

I can't test this patch in Firefox desktop because SystemAppProxy does not exist there, nor I can test this in B2G (emulator/desktop) because the current test setup prevent us from faking System app with mochitests (see bug 1094055). So no tests can be attached in this bug.

I plan to create Gij tests to test out the flow altogether when the Gaia System part is completed in bug 1094559.

Comment 16

[Tim Guan-tin Chien [:timdream] (please needinfo)]

https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=3b0635e8430a

Comment 17

[[:fabrice] Fabrice Desré]

Comment on attachment 8521187 [details] [diff] [review]
Patch v1.0

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

::: dom/inputmethod/Keyboard.jsm
@@ +19,5 @@
>  XPCOMUtils.defineLazyModuleGetter(this, "SystemAppProxy",
>                                    "resource://gre/modules/SystemAppProxy.jsm");
>  
> +XPCOMUtils.defineLazyModuleGetter(this, "DOMApplicationRegistry",
> +                                  "resource://gre/modules/Webapps.jsm");

You should not need to import that directly, but see below.

@@ +418,5 @@
> +    requestId: msg.data.requestId
> +  });
> +
> +  let manifestURL =
> +    DOMApplicationRegistry.getManifestURLByLocalId(msg.data.appId);

use the appsService instead. You can even use it in MozKeyboard.js and send directly the manifestURL instead of the appId.

::: dom/inputmethod/MozKeyboard.js
@@ +348,5 @@
>        }
>      }
>    },
>  
> +  addInput: function(inputId, inputManafest) {

nit: here and everywhere else this has been copy/pasted: s/inputManafest/inputManifest

@@ +349,5 @@
>      }
>    },
>  
> +  addInput: function(inputId, inputManafest) {
> +    return this._sendPromise(function(resolverId) {

maybe |this._sendPromise(resolverId => {...| to not have to bind() ?

Comment 18

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

Comment on attachment 8521187 [details] [diff] [review]
Patch v1.0

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

::: dom/inputmethod/Keyboard.jsm
@@ +376,5 @@
> +  ppmm.addMessageListener('InputRegistry:Add', this);
> +  ppmm.addMessageListener('InputRegistry:Remove', this);
> +};
> +
> +InputRegistryGlue.prototype.receiveMessage = function(msg) {

Why not reuse the Keyboard.receiveMessage to handle the received messages?

Creating a new receiveMessage handler does not increase the old receiveMessage, but complicates maintenance.

I'd rather have one big receiveMessage than two smaller ones with duplicated code.

::: dom/webidl/InputMethod.webidl
@@ +30,5 @@
>    void setActive(boolean isActive);
>  
> +  // Add a dynamically declared input.
> +  //
> +  // The id must not the same with any statically declared input in the app

nit: must not be .. missing the `be`

@@ +35,5 @@
> +  // manifest. If an input of the same id is already declared, the info of that
> +  // input will be updated.
> +  //
> +  // Return a promise resolve to the passed id, or rejected with reasons.
> +  Promise<DOMString> addInput(DOMString inputId, any inputManafest);

Might use Promise<void>?

@@ +39,5 @@
> +  Promise<DOMString> addInput(DOMString inputId, any inputManafest);
> +
> +  // Remove a dynamically declared input.
> +  //
> +  // The id must not the same with any statically declared input in the app

nit: missing the `be`

@@ +44,5 @@
> +  // manifest. Sliently resolves if the input is not previously declared;
> +  // rejects if attempt to remove a statically declared input.
> +  //
> +  // Return a promise resolve to the passed id, or reject with reason.
> +  Promise<DOMString> removeInput(DOMString id);

Might use Promise<void>?

Comment 19

[Tim Guan-tin Chien [:timdream] (please needinfo)]

(In reply to Yuan Xulei [:yxl] from comment #18)
> Comment on attachment 8521187 [details] [diff] [review]
> Patch v1.0
> 
> Review of attachment 8521187 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/inputmethod/Keyboard.jsm
> @@ +376,5 @@
> > +  ppmm.addMessageListener('InputRegistry:Add', this);
> > +  ppmm.addMessageListener('InputRegistry:Remove', this);
> > +};
> > +
> > +InputRegistryGlue.prototype.receiveMessage = function(msg) {
> 
> Why not reuse the Keyboard.receiveMessage to handle the received messages?
> 
> Creating a new receiveMessage handler does not increase the old
> receiveMessage, but complicates maintenance.
> 
> I'd rather have one big receiveMessage than two smaller ones with duplicated
> code.
> 

The problem is that it just too big. I would have to add more complex expressions in these |if|s to properly process different type of messages. In the Gaia world we would be split the code already.

http://dxr.mozilla.org/mozilla-central/source/dom/inputmethod/Keyboard.jsm#116-123

> ::: dom/webidl/InputMethod.webidl
> @@ +30,5 @@
> >    void setActive(boolean isActive);
> >  
> > +  // Add a dynamically declared input.
> > +  //
> > +  // The id must not the same with any statically declared input in the app
> 
> nit: must not be .. missing the `be`
> 
> @@ +35,5 @@
> > +  // manifest. If an input of the same id is already declared, the info of that
> > +  // input will be updated.
> > +  //
> > +  // Return a promise resolve to the passed id, or rejected with reasons.
> > +  Promise<DOMString> addInput(DOMString inputId, any inputManafest);
> 
> Might use Promise<void>?
> 

The proposal I send out will return the inputId. Any reason I shouldn't?

> @@ +39,5 @@
> > +  Promise<DOMString> addInput(DOMString inputId, any inputManafest);
> > +
> > +  // Remove a dynamically declared input.
> > +  //
> > +  // The id must not the same with any statically declared input in the app
> 
> nit: missing the `be`
> 
> @@ +44,5 @@
> > +  // manifest. Sliently resolves if the input is not previously declared;
> > +  // rejects if attempt to remove a statically declared input.
> > +  //
> > +  // Return a promise resolve to the passed id, or reject with reason.
> > +  Promise<DOMString> removeInput(DOMString id);
> 
> Might use Promise<void>?

Comment 20

[Tim Guan-tin Chien [:timdream] (please needinfo)]

How about let's factor out this part into Util.getMMFromMessage(msg) and Util.checkPermissionForMM(mm, permName) and callable to both Keyboard and InputRegistryGlue?

http://dxr.mozilla.org/mozilla-central/source/dom/inputmethod/Keyboard.jsm#125-151

This would remove some code dup I think.

Comment 21

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

Do you just want me to review the WebIDL or is there more I should do here?

Comment 22

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

(In reply to Tim Guan-tin Chien [:timdream] (MoCo-TPE) (please ni?) from comment #20)
> How about let's factor out this part into Util.getMMFromMessage(msg) and
> Util.checkPermissionForMM(mm, permName) and callable to both Keyboard and
> InputRegistryGlue?
> 
> http://dxr.mozilla.org/mozilla-central/source/dom/inputmethod/Keyboard.
> jsm#125-151
> 
> This would remove some code dup I think.
OK, go ahead. It's acceptable way to remove duplicate.

Comment 23

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Attachment: Patch v1.1

Updated patch according to review comments.

I didn't update the DOMApplicationRegistry part because I don't know the benefit of it (since Keyboard.jsm is always run in the chrome process).

Comment 24

[Tim Guan-tin Chien [:timdream] (please needinfo)]

(In reply to Kyle Huey [:khuey] ([e-mail]) from comment #21)
> Do you just want me to review the WebIDL or is there more I should do here?

Yes, it would be great if you could review the WebIDL. I am not sure if you want to look into the code or not, or if :yxl need you to.

Comment 25

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

Comment on attachment 8522772 [details] [diff] [review]
Patch v1.1

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

::: dom/inputmethod/Keyboard.jsm
@@ +398,5 @@
> +  let testing = false;
> +  try {
> +    testing = Services.prefs.getBoolPref("dom.mozInputMethod.testing");
> +  } catch (e) {
> +  }

The above pref check is not needed now.

@@ +427,5 @@
> +    requestId: msg.data.requestId
> +  });
> +
> +  let manifestURL =
> +    DOMApplicationRegistry.getManifestURLByLocalId(msg.data.appId);

Fabrice suggested you use xpcom service instead of importing a large jsm:
let appsService = Cc["@mozilla.org/AppsService;1"]
                    .getService(Ci.nsIAppsService);
let manifestURL = appsService.getManifestURLByLocalId(msg.data.appId);

The xpcom service interface is more stable and of lower cost.

Comment 26

[Tim Guan-tin Chien [:timdream] (please needinfo)]

(In reply to Yuan Xulei [:yxl] from comment #25)
> Comment on attachment 8522772 [details] [diff] [review]
> Patch v1.1
> 
> Review of attachment 8522772 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/inputmethod/Keyboard.jsm
> @@ +398,5 @@
> > +  let testing = false;
> > +  try {
> > +    testing = Services.prefs.getBoolPref("dom.mozInputMethod.testing");
> > +  } catch (e) {
> > +  }
> 
> The above pref check is not needed now.

What do you mean this is not needed? Per finding back in bug 974770, we would need this for Emulator tests.

Comment 27

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

(In reply to Tim Guan-tin Chien [:timdream] (MoCo-TPE) (please ni?) from comment #26)
> (In reply to Yuan Xulei [:yxl] from comment #25)
> > Comment on attachment 8522772 [details] [diff] [review]
> > Patch v1.1
> > 
> > Review of attachment 8522772 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > ::: dom/inputmethod/Keyboard.jsm
> > @@ +398,5 @@
> > > +  let testing = false;
> > > +  try {
> > > +    testing = Services.prefs.getBoolPref("dom.mozInputMethod.testing");
> > > +  } catch (e) {
> > > +  }
> > 
> > The above pref check is not needed now.
> 
> What do you mean this is not needed? Per finding back in bug 974770, we
> would need this for Emulator tests.
You did twice pref checks inside `InputRegistryGlue.prototype.receiveMessage` :-) And we only need once.

Comment 28

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Attachment: Patch v1.1 updated per review comment

Comment 29

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Attachment: Patch v1.1 updated per review comment & use lazily getter

Comment 30

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Attachment: Patch for commit

Comment 31

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/a0a2ada42652

Comment 32

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/a0a2ada42652

Comment 33

[Ryan VanderMeulen [:RyanVM]]

Something in the push from comment 31 made Gij(2) permafail (see the log link below). In the interest of getting the trees reopened sooner, I've backed out the entire push for now. I've also kicked off Try runs of each bug individually backed out and will re-land the innocent patches. Sorry for the hassle.
https://hg.mozilla.org/integration/mozilla-inbound/rev/920f29e2e97a

https://treeherder.mozilla.org/ui/logviewer.html#?job_id=4455263&repo=mozilla-inbound

Comment 34

[Ryan VanderMeulen [:RyanVM]]

Exonerated by Try! Relanded :)
https://hg.mozilla.org/integration/mozilla-inbound/rev/01fd330ca282

Comment 35

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/01fd330ca282

Comment 36

[howie [:howie]]

Remove the feature b2g tag to match the new v2.2 scope.