Tim Guan-tin Chien [:timdream] (please needinfo) Assignee
	Description • 10 years ago

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?

	John Lu [:mnjul][Please NI or I'll miss]
	Comment 1 • 10 years ago

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.

	Tim Guan-tin Chien [:timdream] (please needinfo) Assignee
	Comment 2 • 10 years ago

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!

	John Lu [:mnjul][Please NI or I'll miss]
	Comment 3 • 10 years ago

Tim, quick note: unit tests are failing

	John Lu [:mnjul][Please NI or I'll miss]
	Comment 4 • 10 years ago

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

Looks good. Please however fix the tests ;)

	John Lu [:mnjul][Please NI or I'll miss]
	Comment 5 • 10 years ago

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)

	John Lu [:mnjul][Please NI or I'll miss]
	Comment 6 • 10 years ago

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

	Tim Guan-tin Chien [:timdream] (please needinfo) Assignee
	Comment 7 • 10 years ago

Comment on attachment 8535450 [details]
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.

	Tim Guan-tin Chien [:timdream] (please needinfo) Assignee
	Comment 8 • 10 years ago

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

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

	John Lu [:mnjul][Please NI or I'll miss]
	Comment 9 • 10 years ago

Comment on attachment 8535450 [details]
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?

	Tim Guan-tin Chien [:timdream] (please needinfo) Assignee
	Comment 10 • 10 years ago

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

	Tim Guan-tin Chien [:timdream] (please needinfo) Assignee
	Comment 11 • 10 years ago

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

	David Flanagan [:djf]
	Comment 12 • 10 years ago

Comment on attachment 8535450 [details]
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.

	Tim Guan-tin Chien [:timdream] (please needinfo) Assignee
	Comment 13 • 10 years ago

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

	Jan Jongboom [:janjongboom] (inactive)
	Comment 14 • 10 years ago

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

Works nice, but a bit late feedback :-)