Closed Bug 1110028 Opened 5 years ago Closed 5 years ago

Offer a blob/dictionary loader for keyboard to IMEngines

Categories

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

x86
macOS
defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
2.2 S2 (19dec)

People

(Reporter: timdream, Assigned: timdream)

References

Details

(Whiteboard: [p=3])

Attachments

(1 file)

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?
Flags: needinfo?(jlu)
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.
Flags: needinfo?(jlu)
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!
Attachment #8535450 - Flags: review?(jlu)
Tim, quick note: unit tests are failing
Comment on attachment 8535450 [details] [review]
Github: https://github.com/mozilla-b2g/gaia/pull/26733

Looks good. Please however fix the tests ;)
Attachment #8535450 - Flags: review?(jlu) → review+
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)
(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 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.
Attachment #8535450 - Flags: review?(jlu)
Attachment #8535450 - Flags: review+
Attachment #8535450 - Flags: feedback?(janjongboom)
Attachment #8535450 - Flags: feedback?(dflanagan)
And no. There is no zero in Roman numerals. "Part Nulla" maybe?

https://en.wikipedia.org/wiki/Roman_numerals#Zero
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?
Attachment #8535450 - Flags: review?(jlu) → review+
> 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.
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
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Whiteboard: [p=3]
Target Milestone: --- → 2.2 S2 (19dec)
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.
Attachment #8535450 - Flags: feedback?(dflanagan) → feedback+
(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 on attachment 8535450 [details] [review]
Github: https://github.com/mozilla-b2g/gaia/pull/26733

Works nice, but a bit late feedback :-)
Attachment #8535450 - Flags: feedback?(janjongboom)
You need to log in before you can comment on or make changes to this bug.