Closed
Bug 1055975
Opened 10 years ago
Closed 10 years ago
Bad comparison in libpinyin.js
Categories
(Firefox OS Graveyard :: Gaia::Keyboard, defect)
Tracking
(b2g-v2.2 fixed)
RESOLVED
FIXED
2.1 S4 (12sep)
Tracking | Status | |
---|---|---|
b2g-v2.2 | --- | fixed |
People
(Reporter: max, Assigned: theachalaggarwal)
References
Details
(Whiteboard: [good first bug][lang=js])
Attachments
(1 file)
Line 124 (https://github.com/mozilla-b2g/gaia/blob/master/apps/keyboard/js/imes/jspinyin/libpinyin.js#L124) has the following test: if (!Module['load'] == 'undefined' && Module['read']) { Note that "!Module['load']" is a Boolean, so it can never be equal to the string "undefined".
Comment 1•10 years ago
|
||
Thanks Max! Luke, Looks like this problem was introduced in your changes for Bug 900901 "Replace the pinyin IME engine with the emscripten'ed one". This error will cause Module.load() to be undefined rather than implemented using the Module.read() fallback implementation in non-Node[1] and non-WebWorker[2] environments: if (!Module['load'] == 'undefined' && Module['read']) { Module['load'] = function(f) { globalEval(Module['read'](f)); }; } Can you fix this? Thanks! Mike [1] https://github.com/mozilla-b2g/gaia/blob/master/apps/keyboard/js/imes/jspinyin/libpinyin.js#L67 [2] https://github.com/mozilla-b2g/gaia/blob/master/apps/keyboard/js/imes/jspinyin/libpinyin.js#L115
Comment 2•10 years ago
|
||
Max, Thanks for identifying. I currently have some bugs need to be handled first so I'll take a look in a few more days. Prior to it, if anyone would like to take this bug, please feel free to do that. :)
Flags: needinfo?(lchang)
Whiteboard: [good first bug][lang=js]
Hello, As Max mentioned that it is just a bad comparison which never evaluates to true, I have corrected it, please review the PR. I don't know to test it, hoping it passes tbpl.
Comment 4•10 years ago
|
||
Hi Achal, Thanks for your help and assign to you since you've had a patch. The patch is good to me. Let's call for keyboard owner's review.
Assignee: nobody → theachalaggarwal
Status: NEW → ASSIGNED
Comment 5•10 years ago
|
||
Comment on attachment 8484442 [details] [review] PR to correct the first condition. Hi Rudy, Could you help to review Achal's patch? Thanks.
Attachment #8484442 -
Flags: review?(rlu)
Comment 6•10 years ago
|
||
Comment on attachment 8484442 [details] [review] PR to correct the first condition. r=me. Thanks for patching this up. Luke, May I know if this part of code is generated by emscripten or manually written? If this is generated by emscripten, then maybe the upstream code might need to be fixed as well.
Attachment #8484442 -
Flags: review?(rlu) → review+
Flags: needinfo?(lchang)
Comment 7•10 years ago
|
||
Hi Rudy, Yes, this part is generated by emscripten. Agree with you that we may need to notify them about this issue.
Flags: needinfo?(lchang)
Comment 8•10 years ago
|
||
Landed to Gaia master, https://github.com/mozilla-b2g/gaia/commit/e84af5486c3b513a39884e6dac42b67f6a2a855b -- Achal, Luke, Thanks for working on this. -- [CI status] The Gij failures happened in different test cases within 3 run, so I don't think they're relevant to this change.
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
status-b2g-v2.2:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → 2.1 S4 (12sep)
Comment 9•10 years ago
|
||
Just out of curiosity, Max, could you share with us how you find this issue? You're studying this part of code or try to make it work on plain web?
Flags: needinfo?(max)
Comment 12•10 years ago
|
||
Hi Alon, Are you the right contact to notify re: possible Emscripten issues? If not can you pass this issue on to whomever that is? See comment 0, comment 6 and comment 7 for details. Thanks, Mike (In reply to Rudy Lu [:rudyl] from comment #6) > > ... > > Luke, > > May I know if this part of code is generated by emscripten or manually > written? > If this is generated by emscripten, then maybe the upstream code might need > to be fixed as well. (In reply to Luke Chang [:lchang] from comment #7) > Hi Rudy, > > Yes, this part is generated by emscripten. Agree with you that we may need > to notify them about this issue.
Flags: needinfo?(azakai)
You need to log in
before you can comment on or make changes to this bug.
Description
•