Currently when we are decoding Comprehension TLV for Event List, the eventList property will become an object instead of an array since it is read as Uint8Array, we should make it as Array.

Comment on attachment 668612 [details] [diff] [review] Part 1: Patch Review of attachment 668612 [details] [diff] [review]: ----------------------------------------------------------------- That's bug 737202. Since there is several users of readHexOctetArray, and I don't think we're going to fix them all, and I don't think fixing it in ril_worker is a good idea. I would rather: 1. Let readHexOctetArray returns javascript Array instead. One will have to take care of all the readHexOctetArray users. 2. Do as done in RadioInterfaceLayer.js, line 1081, function handleSmsReceived.

Comment on attachment 668613 [details] [diff] [review] Part 2: xpcshell test Review of attachment 668613 [details] [diff] [review]: ----------------------------------------------------------------- Since part 1 is a r-, so is this. Besides, this test script doesn't test whether or not is the returned value of readHexOctetArray a javascript Array. Please verify it in the content side.

(In reply to Vicamo Yang [:vicamo][:vyang] from comment #4) > Comment on attachment 668613 [details] [diff] [review] > Part 2: xpcshell test > > Review of attachment 668613 [details] [diff] [review]: > ----------------------------------------------------------------- > > Since part 1 is a r-, so is this. Besides, this test script doesn't test > whether or not is the returned value of readHexOctetArray a javascript > Array. Please verify it in the content side. No, from chrome process it's already an object I/Gecko ( 1823): -*- RadioInterfaceLayer: Received message from worker: {"commandNumber":1,"typeOfCommand":5,"commandQualifier":0,"rilMessageType":"stkcommand","options":{"eventList":{"0":0,"1":1,"2":2,"3":3}}} we don't need to test that in content.

Does this need to block the release? If so, please explain and request blocking-basecamp. Thanks!

(In reply to Vicamo Yang [:vicamo][:vyang] from comment #3) > Comment on attachment 668612 [details] [diff] [review] > Part 1: Patch > > Review of attachment 668612 [details] [diff] [review]: > ----------------------------------------------------------------- > > That's bug 737202. Since there is several users of readHexOctetArray, and I > don't think we're going to fix them all, and I don't think fixing it in > ril_worker is a good idea. I would rather: > 1. Let readHexOctetArray returns javascript Array instead. One will have to > take care of all the readHexOctetArray users. > 2. Do as done in RadioInterfaceLayer.js, line 1081, function > handleSmsReceived. Hi, Vicamo I've tried your suggestion, Uint8Array will become normal object in content process, so I think my original patch is more appropriate.

(In reply to Dietrich Ayala (:dietrich) from comment #6) > Does this need to block the release? If so, please explain and request > blocking-basecamp. Thanks! Hi Dietrich This feature is already implemented and this bug is just for refactoring.

Comment on attachment 668612 [details] [diff] [review] Part 1: Patch Review of attachment 668612 [details] [diff] [review]: ----------------------------------------------------------------- Yoshi had a few experiments and it seems there are plenty problems with typed arrays.

(In reply to Yoshi Huang[:yoshi][:allstars.chh] from comment #7) > I've tried your suggestion, Uint8Array will become normal object in content > process, so I think my original patch is more appropriate. Please note the test case passes no matter the first patch is applied or not. And, please remove the empty line in line 396 but add one between line 398 and 399 in the second patch.

nit addressed, also add Array.isArray test suggested by Vicamo

Comment on attachment 671721 [details] [diff] [review] Part 2: xpcshell tests v2 Review of attachment 671721 [details] [diff] [review]: ----------------------------------------------------------------- Nice!

https://hg.mozilla.org/integration/mozilla-inbound/rev/67b12a268fbe https://hg.mozilla.org/integration/mozilla-inbound/rev/bf68f3cd1e66

https://hg.mozilla.org/mozilla-central/rev/67b12a268fbe https://hg.mozilla.org/mozilla-central/rev/bf68f3cd1e66