1146804 - Create file front-end for word_list_converter.js

Status: RESOLVED FIXED

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

Description

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

As described in bug 1143633, we'll create a front-end javascript that reads from the wordlist.xml file and outputs to the lang.dict file, using WordListConverter as back-end.

Comment 1

[GitHub Autolander Bot]

Attachment: [gaia] mnjul:bug_1146804_xml2dict_js > mozilla-b2g:master

Comment 2

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

Tim and Ricky,

After manual testing [*] I believe this script should be good to replace xml2dict.py .

As this requires xml2js npm package, to formally drop this in the repository, we'll want to change gaia-node-modules too (along with the usual code review process, of course).

However, I don't know how much l10n resource update this is going to involve; e.g.. wherever we teach localizers how to run xml2dict.py, we want to update the instructions.

I also don't know if it's "safe" to run node on somewhere like ./apps/keyboard/js/imes/latin/dictionaries/ when we're not building.

I'm pretty sure we probably won't have the formal l10n process change probably before the end of the month, so this is more a resource-schedule issue, so I'm leaving an NI on Tim to notify him that this is happening but it's his call on how the whole matter is going to progress.

However I'm more than happy to work toward the r+ of the patch the best as I can, including resolving the issues of:
- where would be more sensible that we should place word_list_converter.js now, if its current place isn't sensible.
- if the "namespace" trick in word_list_converter.js is allowable.

Thanks you all.

[*] Will detail in the next comment.

Comment 3

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

The need for manual testing is because again, we *can't* do byte-by-byte comparisons of the blob written by the JS version with that by the Python version (for the same wordlist xml). This is again because V8 doesn't do stable sorting, and no order can be guaranteed when we're sorting character table [1] and next-pointer-linked nodes [2]

Note that this is different from [3] where I talked about character table random ordering from Python script that I didn't find root cause.

[1] https://wiki.mozilla.org/Gaia/System/Keyboard/IME/Latin/Dictionary_Blob#Character_Table 
[2] https://wiki.mozilla.org/Gaia/System/Keyboard/IME/Latin/Prediction_%26_Auto_Correction#Example_TST
[3] https://wiki.mozilla.org/Gaia/System/Keyboard/IME/Latin/Dictionary_Blob#Same_XML.2C_Different_Dict_Blob

So, if you do a byte-by-byte comparison of the blobs, you'll almost always see a huge amount of byte differences. Aside from the character table, you might sometimes see only three bytes difference within consecutive several same-bytes, such as |aa bb cc dd ee ff gg hh ii| vs |aa bb cc jj kk ll gg hh ii|. This is because the next pointer of a node is storing a different address, since the sorting of next-pointer-linked nodes is unstable.

Other times you might see entire different byte chunks -- This is because the next-pointed node is simply placed somewhere else.

These all don't affect actual tree structure, since the ordering of next-pointed nodes (which have the same frequency) doesn't matter.

It is actually possible that we reconstruct the tree when comparing the blobs to validate the JS version of xml2dict.js, but I doubt if it's worth it.

Comment 4

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

Let's put the Gaia build script integration out of the scope of this bug. You can drop in a temporary package.json on ./apps/keyboard/js/imes/latin/dictionaries/ and modify the Makefile there to ensure the file front-end works, independent of Gaia build script.

I have no opinion on the testing part -- I trust your judgement here.

(You actually didn't needinfo Ricky. Please do if you still feel you need to.)

Comment 5

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

Comment on attachment 8582252 [details] [review]
[gaia] mnjul:bug_1146804_xml2dict_js > mozilla-b2g:master

Tim, please check out the patch for:

(In reply to John Lu [:mnjul] [MoCoTPE] from comment #2)
> However I'm more than happy to work toward the r+ of the patch the best as I
> can, including resolving the issues of:
> - where would be more sensible that we should place word_list_converter.js
> now, if its current place isn't sensible.
> - if the "namespace" trick in word_list_converter.js is allowable.

And the Makefile/package.json plus .gitignore.

Thanks!

Comment 6

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

Comment on attachment 8582252 [details] [review]
[gaia] mnjul:bug_1146804_xml2dict_js > mozilla-b2g:master

I need to cancel the review because the added node_modules will cause jshint failure. We need to think how to blacklist the folder from both jshint and gjslint.

Comment 7

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

Comment on attachment 8582252 [details] [review]
[gaia] mnjul:bug_1146804_xml2dict_js > mozilla-b2g:master

(In reply to John Lu [:mnjul] [MoCoTPE] from comment #6)
> Comment on attachment 8582252 [details] [review]
> [gaia] mnjul:bug_1146804_xml2dict_js > mozilla-b2g:master
> 
> I need to cancel the review because the added node_modules will cause jshint
> failure. We need to think how to blacklist the folder from both jshint and
> gjslint.

Which is now resolved by adding an entry in .jshintignore. Ricky, please check it (along with .gitignore)

For Tim, please still check comment 5 for things to review.

Thanks :)

Comment 8

[Ricky Chien [:rickychien] (inactive)]

Comment on attachment 8582252 [details] [review]
[gaia] mnjul:bug_1146804_xml2dict_js > mozilla-b2g:master

r+ for build part. I'm curious about where is the document for explaining how to execute this converter.

Comment 9

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

(In reply to Ricky Chien [:rickychien] from comment #8)
> Comment on attachment 8582252 [details] [review]
> [gaia] mnjul:bug_1146804_xml2dict_js > mozilla-b2g:master
> 
> r+ for build part. I'm curious about where is the document for explaining
> how to execute this converter.

Buried (no pun intended) at MDN: https://developer.mozilla.org/en-US/Firefox_OS/Developing_Gaia/Customizing_the_keyboard#Building_the_files

Comment 10

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

I forgot to rebase..the previous try will probably fail. Trying again.

Comment 11

[GitHub Autolander Bot]

http://docs.taskcluster.net/tools/task-graph-inspector/#VVUhKxRBTMyt5XRMWAyAwg

The pull request failed to pass integration tests. It could not be landed, please try again.

Comment 12

[GitHub Autolander Bot]

Pull request has landed in master: https://github.com/mozilla-b2g/gaia/commit/d08600b883589cfb556a7a3f39310b6b4e8106a0