908577 - Update JSZhuyin IME to v2013-Oct

Status: RESOLVED FIXED

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

Description

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

I've spend some personal time of last month and rewrite the Zhuyin IME from scratch. Huge performance boost. I would like to land it to Gaia too.

The code can be found at https://github.com/timdream/jszhuyin

Comment 1

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

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

Rudy, do you mind reviewing this, please?

You could safely ignore the file removed and added as they are simply the JSZhuyin library and database.

./jszhuyin/jszhuyin.js have been rewritten to a iframe loader and message passer for the library, and there are some minor layout changes in layout.js.

Comment 2

[Rudy Lu [:rudyl] (inactive after 2015/7/3)]

Yeah, sure, put this in my review queue.
I have to review Luke's pinyin first and other patches, but will try to review this soon.

Thanks for you guys' effort to make us have Chinese input method!

Comment 3

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

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

Pull request updated.

Comment 4

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

Note that I need to update Glue code here because bug 906617 updated the internal API.

Comment 5

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

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

Pull request updated.

Comment 6

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

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

Pull request updated to adopt changes made in bug 884752.
One should enable it in the build in order to test the patch here.

GAIA_KEYBOARD_LAYOUTS=en,zh-Hant-Zhuyin make reset-gaia

Comment 7

[Rudy Lu [:rudyl] (inactive after 2015/7/3)]

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

First of all, really sorry to get to this review so late.
I've done a review on jszhuyin.js while leaving the lib/* as is without going into details of it.

The code looks quite good, except one thing about if we would release the memory by terminating the worker, since I guess the large database is loaded by the worker.

Please flag me for the review again when this is done or if this is not an issue to worry about.

Thanks for this nice work!

Comment 8

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

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

nit addressed on the original commit, with a WIP commit addresses the worker unloader you mentioned. I left it as another commit so you will be able to inspect it's non-trivial logic.

Being able to load/unload the worker introduce yet two more states to the instance, before:

init -a-> init:workerReady <-b-> uninit

after:

init <-c-> init:workerLoaded --d-> init:workerLoaded:workerReady <-e-> uninit

The workerReady state is a sub-state only matters in sendMessage. The WIP patch implements method to go left and right on what denoted "c" above, and clean-up all variables in both init:workerLoaded and init:workerLoaded:workerReady state.

Comment 9

[Rudy Lu [:rudyl] (inactive after 2015/7/3)]

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

Looks good to me. 

I'll set feedback+ only since you said it is a WIP.
Let me know when it is ready to review again.

Comment 10

[Rudy Lu [:rudyl] (inactive after 2015/7/3)]

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

r+, since this is ready as Tim said offline.

Thanks, Tim.

Comment 11

[Luke Chang [:lchang] (inactive)]

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

Sorry for my late feedback.
Actually it looks very well and I really look forward to input Trad. Chinese in FxOS.

Comment 12

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

master: https://github.com/mozilla-b2g/gaia/commit/8f8de161501b016a44f5d787fff70071b060dc3a

I will attempt to uplift this patch to v1.2 to support community effort after a day or two.

Comment 13

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

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

[Approval Request Comment]
[Bug caused by]: feature
[User impact] if declined: Zhuyin IME will be left with it's broken state in v1.2, hamper the community-driven l10n effort.
[Testing completed]: Yes, manually.
[Risk to taking this patch] (and alternatives if risky): no likely risk because since bug 884752 Zhuyin will not be included in the build unless the user have select it in build config.
[String changes made]: None, the localized name of keyboards have been removed

Comment 14

[John Ford [:jhford] CET/CEST Berlin Time]

Uplifted 8f8de161501b016a44f5d787fff70071b060dc3a to:
v1.2: f6bee023170f15d54ca8464c1797237dbca8a48b

Comment 15

[John Ford [:jhford] CET/CEST Berlin Time]

Uplifted 8f8de161501b016a44f5d787fff70071b060dc3a to:
v1.3 already had this commit