1094561 - Isolate installed input app list generation from KeyboardHelper

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Gaia::System::Input Mgmt, defect)

Description

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

Before I can work on bug 1094559, I think KeyboardHelper deserves a small refactor for me to make sense of what it is currently doing. This bug will only cover the part where |currentApps| is generated -- which is the part to be changed in bug 1094559.

Comment 1

[Mozilla Shepherd (This account is used for automation and doesn't receive bugmail)]

Attachment: mozilla-b2g:master PR#25975

Comment 2

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

Comment on attachment 8519645 [details] [review]
mozilla-b2g:master PR#25975

This patch isolates the input app list part from KeyboardHelper to InputAppList, so apps that requires KeyboardHelper would now need to include two scripts. Arthur, Sam, please check the respective changes in the app code -- I have manually tested them and confirm that the script is included.

One thing to note on FTU is that I added webapps-mgmt permission. Without the permission KeyboardHelper still work for the purpose of FTU (set the default layouts for language X), but add that permission will suppress a few errors/warnings. In the future we might want to split the KeyboardHelper itself so that FTU only need to require the part it needs (and remove the permission here).

As of the InputAppList itself, just as planned it takes inputs from mozSettings and mozApps and generates a list of current installed input apps. It also responsible of monitoring the changes of mozApps and mozSettings itself. Mocks for these APIs are properly setup'd to cover these functions. John, please review this part. Thanks.

Comment 3

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

Comment on attachment 8519645 [details] [review]
mozilla-b2g:master PR#25975

Adding John Hu for feedback for changes in tv_apps/smart-system.

Comment 4

[John Lu [:mnjul][Please NI or I'll miss]]

Comment on attachment 8519645 [details] [review]
mozilla-b2g:master PR#25975

I did reset-gaia with LOL keyboard from dev-apps enabled [1], and with the patch, there is no layout switching key in (built-in) keyboard, and LOL keyboard is not available in IME Switcher too. Please take a look ;)

[1] I need to enable it in apps-engineering.list https://gist.github.com/mnjul/03c5343e001c1c5981ae and need to reset-gaia.

Comment 5

[John Lu [:mnjul][Please NI or I'll miss]]

(In reply to John Lu [:mnjul] [MoCoTPE] from comment #4)
> Comment on attachment 8519645 [details] [review]
> mozilla-b2g:master PR#25975
> 
> I did reset-gaia with LOL keyboard from dev-apps enabled

OOps, critical information omitted: LOL keyboard is available in Settings. And after I enable it, there is no layout switching key ...etc etc etc.

Comment 6

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

Comment on attachment 8519645 [details] [review]
mozilla-b2g:master PR#25975

After working with John we realized that's maybe an side effect of existing bug 1016268, thus related to this patch.

Comment 7

[Arthur Chen [:arthurcc][inactive after 6/30]]

Comment on attachment 8519645 [details] [review]
mozilla-b2g:master PR#25975

f=me on the settings part.

Comment 8

[John Lu [:mnjul][Please NI or I'll miss]]

Comment on attachment 8519645 [details] [review]
mozilla-b2g:master PR#25975

Looks good. (Y)

Comment 9

[John Hu [:johnhu][:johu][:醬糊小弟]]

Comment on attachment 8519645 [details] [review]
mozilla-b2g:master PR#25975

We should add the loading of input_app_list.js to tv_apps/smart-system/js/keyboard_manager.js[1]

[1] https://github.com/mozilla-b2g/gaia/blob/837e085dbc5c88ea10e034553cfbd85e9a63417e/tv_apps/smart-system/js/keyboard_manager.js#L122

Comment 10

[Sam Foster [:sfoster] (he/him)]

Comment on attachment 8519645 [details] [review]
mozilla-b2g:master PR#25975

FTU part looks fine to me, I verified FTU runs as normal, but I don't have a 3rd-party keyboard app to test that bit. Is there a (non-developer/non-QA) scenario in which the user would run the FTU with a 3rd-party keyboard? We'll want to cover that if we support it - probably with marionette tests?

I don't have a strong opinion at this point on the new webapps-manage permission, but if we do want to remove the need for it, lets at least file a bug so it doesnt get lost.

Comment 11

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

(In reply to Sam Foster [:sfoster] from comment #10)
> Comment on attachment 8519645 [details] [review]
> mozilla-b2g:master PR#25975
> 
> FTU part looks fine to me, I verified FTU runs as normal, but I don't have a
> 3rd-party keyboard app to test that bit. Is there a (non-developer/non-QA)
> scenario in which the user would run the FTU with a 3rd-party keyboard?
> We'll want to cover that if we support it - probably with marionette tests?

It's possible that the vender might preload a second keyboard app in the phone and assign its layout as the default-to-enable layout for language X. If we really want to test this out we would need to create a customized build for that.

> I don't have a strong opinion at this point on the new webapps-manage
> permission, but if we do want to remove the need for it, lets at least file
> a bug so it doesnt get lost.

Bug 1096760 filed.

Comment 12

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

Comment on attachment 8519645 [details] [review]
mozilla-b2g:master PR#25975

Johu, I add the required modification in smart-system/js/keyboard_manager.js. Please see if it's good. Thanks!

Comment 13

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

Comment on attachment 8519645 [details] [review]
mozilla-b2g:master PR#25975

It turned out John is offsite this week... given the fact I have done what he said in the previous feedback here:

https://github.com/mozilla-b2g/gaia/pull/25975/files#r20080584

I think we could safely land this without waiting for the feedback.

Comment 14

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

master: https://github.com/mozilla-b2g/gaia/commit/848b7b74a698d1472151f122e59f85ad07da407c