Closed Bug 1055975 Opened 10 years ago Closed 10 years ago

Bad comparison in libpinyin.js

Categories

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

x86_64
Linux
defect
Not set
normal

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)

46 bytes, text/x-github-pull-request
rudyl
: review+
Details | Review
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".
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
Status: UNCONFIRMED → NEW
Ever confirmed: true
Flags: needinfo?(lchang)
See Also: → 900901
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.
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 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 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)
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)
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
Resolution: --- → FIXED
Target Milestone: --- → 2.1 S4 (12sep)
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)
I found it using a static analysis tool.
Flags: needinfo?(max)
Hello Rudy, thanks for reviewing and merging the patch. :)
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)
Fixed in upstream, thanks!
Flags: needinfo?(azakai)
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: