1110028 - Offer a blob/dictionary loader for keyboard to IMEngines

Status: RESOLVED FIXED

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

Description

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

After thinking about the arch again, I think it's better to generalize the blob loading part of the engine and move it the the keyboard app, so that user and app can load/unload it altogether.

For latin Engine, it currently load the data here:

https://github.com/mozilla-b2g/gaia/blob/a5b720890122b262d32e9f43bb6a162c2ccae592/apps/keyboard/js/imes/latin/worker.js#L69-L73

:mnjul, is this a good approach? Does that affect your work on bug 879145 etc?

Comment 1

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

I think it's good as long as the async "try load - then process - or error" interface is available as my bug will load from indexedDB.

Comment 2

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

Attachment: Github: https://github.com/mozilla-b2g/gaia/pull/26733

Unfortunately latin.js and worker.js is a bit complex, but the good thing is if I touch it here I won't have to touch it in bug 1029951!

Comment 3

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

Tim, quick note: unit tests are failing

Comment 4

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

Comment on attachment 8535450 [details] [review]
Github: https://github.com/mozilla-b2g/gaia/pull/26733

Looks good. Please however fix the tests ;)

Comment 5

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

Also, when we do the following bug (load from indexedDB), we need to make sure there is flexibility, in InputMethodDatabaseLoader, to differentiate loading from downloaded dictionary and from user dictionary, as from offline discussion we're only planning differentiating loading from app package with xhr and from downloaded dictionary.

I kinda think it should be internal to InputMethodDatabaseLoader though (unless we make strong contract that user dictionary is only for Latin IME, which I believe we really shouldn't)

Comment 6

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

(In reply to John Lu [:mnjul] [MoCoTPE] from comment #5)
> I kinda think it should be internal to InputMethodDatabaseLoader though
> (unless we make strong contract that user dictionary is only for Latin IME,
> which I believe we really shouldn't)

"should". Since the user dictionary format is tied with latin prediction logics.

Comment 7

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

Comment on attachment 8535450 [details] [review]
Github: https://github.com/mozilla-b2g/gaia/pull/26733

John,

I can't stand the way latin.js tests is being run so I ended up fix the way the way the tests are set up in the first commit (Part Zero). I merged the latin_test.js and suggest_test.js and delete the latter. I also formalized the way each test is run by make sure latin.js is init() once and activate()/deactivate() in each of the tests. Please review that part, and the test script modification in Patch II.

Jan, David, since this is a rather big change in latin.js I am asking for your feedback as well should you have time to address them. It's not required (unless you think it is). Please read the bug to understand why we need this change.

Comment 8

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

And no. There is no zero in Roman numerals. "Part Nulla" maybe?

https://en.wikipedia.org/wiki/Roman_numerals#Zero

Comment 9

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

Comment on attachment 8535450 [details] [review]
Github: https://github.com/mozilla-b2g/gaia/pull/26733

Looks good. Just curious: is the use of double underscore prefix in |glue| some naming convention?

Comment 10

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

> Looks good. Just curious: is the use of double underscore prefix in |glue|
> some naming convention?

No. On a second thought, they should have been mOutput and mIsUpperCase.

Comment 11

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

master: https://github.com/mozilla-b2g/gaia/commit/457e94636b74516e10abc1da026ce2b622713d95

Gaia-Try before force push to update the comment and variable naming: 
https://treeherder.mozilla.org/ui/#/jobs?repo=gaia-try&revision=cc29c9dc3d15

Comment 12

[David Flanagan [:djf]]

Comment on attachment 8535450 [details] [review]
Github: https://github.com/mozilla-b2g/gaia/pull/26733

Tim: thanks for flagging me. The dictionary loading changes look good to me, especially if the second argument to postMessage() actually works to transfer the data without copying it. Is the code that frees the dictionary (or destroys the worker, I forget which) after 30s or 1 minute of inactivity is still working? If so, then this seems great.  I didn't look at the test changes.

Comment 13

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

(In reply to David Flanagan [:djf] from comment #12)
> Tim: thanks for flagging me. The dictionary loading changes look good to me,
> especially if the second argument to postMessage() actually works to
> transfer the data without copying it. Is the code that frees the dictionary
> (or destroys the worker, I forget which) after 30s or 1 minute of inactivity
> is still working? If so, then this seems great.  I didn't look at the test
> changes.

Yes, the worker is still destroyed after 250ms.

Comment 14

[Jan Jongboom [:janjongboom] (inactive)]

Comment on attachment 8535450 [details] [review]
Github: https://github.com/mozilla-b2g/gaia/pull/26733

Works nice, but a bit late feedback :-)