B2G STK: make eventList in MozStkEventList as an array

RESOLVED FIXED in mozilla19

Status

()

defect
RESOLVED FIXED
7 years ago
7 years ago

People

(Reporter: allstars.chh, Assigned: allstars.chh)

Tracking

Trunk
mozilla19
ARM
Gonk (Firefox OS)
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 1 obsolete attachment)

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.
Attachment #668612 - Flags: review?(vyang) → review-
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.
Attachment #668613 - Flags: review?(vyang) → review-
(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.
Attachment #668612 - Flags: review- → review+
(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
Attachment #668613 - Attachment is obsolete: true
Attachment #671721 - Flags: review?(vyang)
Comment on attachment 671721 [details] [diff] [review]
Part 2: xpcshell tests v2

Review of attachment 671721 [details] [diff] [review]:
-----------------------------------------------------------------

Nice!
Attachment #671721 - Flags: review?(vyang) → review+
https://hg.mozilla.org/mozilla-central/rev/67b12a268fbe
https://hg.mozilla.org/mozilla-central/rev/bf68f3cd1e66
Status: NEW → RESOLVED
Closed: 7 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.