[keyboard] allow keyboard apps to add new layouts dynamically

RESOLVED FIXED in mozilla37

Status

()

defect
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: djf, Assigned: timdream)

Tracking

unspecified
mozilla37
x86
macOS
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(tracking-b2g:backlog)

Details

(Whiteboard: [FT:System-Platform])

Attachments

(1 attachment, 4 obsolete attachments)

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.
Needinfo for Rudy and Tim in case either of them has ideas about how to fix this.
Flags: needinfo?(timdream)
Flags: needinfo?(rlu)
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?
Flags: needinfo?(swilkes)
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?
Flags: needinfo?(timdream)
Flags: needinfo?(gchen)
+1 from me. We probably need an iterator over all registered inputs for the current keyboard app.
(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?
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?
Flags: needinfo?(gchen)
(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).
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.
Flags: needinfo?(rlu)
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.
I do not know about customization issues. Removing the flag for me and adding it for Carrie, who owns keyboard UX.
Flags: needinfo?(swilkes) → needinfo?(cawang)
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.
Flags: needinfo?(cawang) → needinfo?(bhuang)
Backlogging this for future versions.
Blocks: 908549
Flags: needinfo?(bhuang)
Separate the idea mentioned in comment 8 to bug 1029951.
blocking-b2g: --- → backlog
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.
Assignee: nobody → timdream
Status: NEW → ASSIGNED
feature-b2g: --- → 2.2?
Component: Gaia::Keyboard → DOM: Device Interfaces
Product: Firefox OS → Core
Whiteboard: [FT:System-Platform]
Posted patch Patch v1.0 (obsolete) — Splinter Review
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.
Attachment #8521187 - Flags: superreview?(khuey)
Attachment #8521187 - Flags: review?(xyuan)
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 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>?
Attachment #8521187 - Flags: review?(xyuan) → feedback+
(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>?
Flags: needinfo?(xyuan)
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.
Do you just want me to review the WebIDL or is there more I should do here?
(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.
Flags: needinfo?(xyuan)
Posted patch Patch v1.1 (obsolete) — Splinter Review
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).
Attachment #8521187 - Attachment is obsolete: true
Attachment #8521187 - Flags: superreview?(khuey)
Attachment #8522772 - Flags: superreview?(khuey)
Attachment #8522772 - Flags: review?(xyuan)
(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) 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 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.
Attachment #8522772 - Flags: review?(xyuan) → review+
feature-b2g: 2.2? → 2.2+
(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.
Flags: needinfo?(xyuan)
(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.
Flags: needinfo?(xyuan)
Attachment #8522772 - Attachment is obsolete: true
Attachment #8522772 - Flags: superreview?(khuey)
Attachment #8524257 - Flags: superreview?(khuey)
Attachment #8524257 - Attachment is obsolete: true
Attachment #8524257 - Flags: superreview?(khuey)
Attachment #8524284 - Flags: superreview?(khuey)
Attachment #8524284 - Attachment is obsolete: true
Attachment #8533478 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/a0a2ada42652
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla37
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
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Target Milestone: mozilla37 → ---
https://hg.mozilla.org/mozilla-central/rev/01fd330ca282
Status: REOPENED → RESOLVED
Closed: 5 years ago5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla37
Remove the feature b2g tag to match the new v2.2 scope.
feature-b2g: 2.2+ → ---
blocking-b2g: backlog → ---
You need to log in before you can comment on or make changes to this bug.