Closed
Bug 813596
Opened 12 years ago
Closed 12 years ago
B2G RIL: add Marionette test cases for Cell Broadcast
Categories
(Core :: DOM: Device Interfaces, defect)
Tracking
()
RESOLVED
FIXED
mozilla21
People
(Reporter: vicamo, Assigned: vicamo)
References
Details
Attachments
(3 files, 4 obsolete files)
Emulator parts[1][2] of Cell Broadcast functions is still not reviewed. Let's land core functions first. [1]: https://github.com/mozilla-b2g/platform_external_qemu/pull/5 [2]: https://github.com/mozilla-b2g/platform_hardware_ril/pull/3
Assignee | ||
Comment 1•12 years ago
|
||
Assignee | ||
Comment 2•12 years ago
|
||
1) Rebase 2) Comment out event constructor checking because it crashes content process. 3) Correct message class values 4) Correct test cases for GSM CBS messages with UCS2 encoded body string.
Attachment #684421 -
Attachment is obsolete: true
Assignee | ||
Comment 3•12 years ago
|
||
Hi Marshall, feel free to cancel this review if you're busy on other stuff, I'll find somebody else.
Attachment #685011 -
Flags: review?(marshall)
Assignee | ||
Comment 4•12 years ago
|
||
Attachment #685012 -
Flags: review?(marshall)
Assignee | ||
Updated•12 years ago
|
Assignee: nobody → vyang
Assignee | ||
Updated•12 years ago
|
Attachment #684950 -
Flags: review?(htsai)
Comment 5•12 years ago
|
||
Comment on attachment 684950 [details] [diff] [review] Part 1: add marionette test cases Review of attachment 684950 [details] [diff] [review]: ----------------------------------------------------------------- Nice! ::: dom/cellbroadcast/tests/marionette/test_cellbroadcast_etws.js @@ +31,5 @@ > +function sendCellBroadcastMessage(pdu, callback) { > + pendingEmulatorCmdCount++; > + > + let cmd = "cbs pdu " + pdu; > + //log("Sending emulator command: " + cmd); Please remove it if you think it's unnecessary. @@ +95,5 @@ > + cbs.removeEventListener("received", onreceived); > + > + // TODO: this check causes b2g crash > + //ok(event instanceof MozCellBroadcastEvent, > + // "event is instanceof " + event.constructor); TODO comment should be followed by a filed bug number, right? I think we could add 'ok(event, "event is valid")' here. @@ +191,5 @@ > + > + // Here we use a simple ETWS message for test. > + let pdu = buildHexStr(0, 12); // 6 octets > + doTestHelper(pdu, testReceiving_ETWS_WarningType, function (message) { > + // Cell Broadcast messages do not contains a timestamp field (however, ETWS nit: s/contains/contain @@ +192,5 @@ > + // Here we use a simple ETWS message for test. > + let pdu = buildHexStr(0, 12); // 6 octets > + doTestHelper(pdu, testReceiving_ETWS_WarningType, function (message) { > + // Cell Broadcast messages do not contains a timestamp field (however, ETWS > + // does). We only check the timestamp doesn't go to far (60 seconds) here. Seems grammatical error here? Did you mean 'We only check the timestamp that doesn't go too far...'? Anyway, please check this sentence again. :) ::: dom/cellbroadcast/tests/marionette/test_cellbroadcast_gsm.js @@ +28,5 @@ > + > +const CB_MESSAGE_SIZE_GSM = 88; > + > +const CB_GSM_MESSAGEID_ETWS_BEGIN = 0x1100; > +const CB_GSM_MESSAGEID_ETWS_END = 0x1108; In your final patch, you already changed the value to '0x1107.' Please make it consistent. @@ +86,5 @@ > +function sendCellBroadcastMessage(pdu, callback) { > + pendingEmulatorCmdCount++; > + > + let cmd = "cbs pdu " + pdu; > + //log("Sending emulator command: " + cmd); ditto. @@ +149,5 @@ > + cbs.addEventListener("received", function onreceived(event) { > + cbs.removeEventListener("received", onreceived); > + > + // TODO: this check causes b2g crash > + //ok(event instanceof MozCellBroadcastEvent, ditto. @@ +150,5 @@ > + cbs.removeEventListener("received", onreceived); > + > + // TODO: this check causes b2g crash > + //ok(event instanceof MozCellBroadcastEvent, > + // "event is instanceof " + event.constructor); How about adding 'ok(event, "event is valid");' ? @@ +356,5 @@ > + log("Test receiving GSM Cell Broadcast - Timestamp"); > + > + let pdu = buildHexStr(0, CB_MESSAGE_SIZE_GSM * 2); > + doTestHelper(pdu, testReceiving_GSM_WarningType, function (message) { > + // Cell Broadcast messages do not contains a timestamp field (however, ETWS nit: s/contains/contain @@ +357,5 @@ > + > + let pdu = buildHexStr(0, CB_MESSAGE_SIZE_GSM * 2); > + doTestHelper(pdu, testReceiving_GSM_WarningType, function (message) { > + // Cell Broadcast messages do not contains a timestamp field (however, ETWS > + // does). We only check the timestamp doesn't go to far (60 seconds) here. ditto. @@ +368,5 @@ > +function testReceiving_GSM_WarningType() { > + log("Test receiving GSM Cell Broadcast - Warning Type"); > + > + let messageIdsToTest = []; > + for (let i = CB_GSM_MESSAGEID_ETWS_BEGIN; i < CB_GSM_MESSAGEID_ETWS_END; i++) { Please make the condition consistent with your implementation patch.
Attachment #684950 -
Flags: review?(htsai) → review+
Assignee | ||
Updated•12 years ago
|
Attachment #685011 -
Flags: review?(marshall) → review?(htsai)
Assignee | ||
Updated•12 years ago
|
Attachment #685012 -
Flags: review?(marshall) → review?(htsai)
Comment 6•12 years ago
|
||
Hey Vicamo, I saw you closed the pull requests in github. What's the status now? Where's the stuff I should help review?
Assignee | ||
Comment 7•12 years ago
|
||
Moved to new pull request
Attachment #685011 -
Attachment is obsolete: true
Attachment #685011 -
Flags: review?(htsai)
Attachment #696929 -
Flags: review?(htsai)
Assignee | ||
Comment 8•12 years ago
|
||
Moved to new pull request
Attachment #685012 -
Attachment is obsolete: true
Attachment #685012 -
Flags: review?(htsai)
Attachment #696930 -
Flags: review?(htsai)
Comment 9•12 years ago
|
||
Comment on attachment 696929 [details] [review] Github Pull Request for external/qemu Looks great!
Attachment #696929 -
Flags: review?(htsai) → review+
Comment 10•12 years ago
|
||
Comment on attachment 696929 [details] [review] Github Pull Request for external/qemu Looks great, only a nit there. Please see my comment in github :)
Comment 11•12 years ago
|
||
Comment on attachment 696930 [details] [review] Github Pull Request for hardware/ril Looks good to me!
Attachment #696930 -
Flags: review?(htsai) → review+
Assignee | ||
Comment 12•12 years ago
|
||
Address review comments.
Attachment #684950 -
Attachment is obsolete: true
Attachment #701690 -
Flags: review+
Assignee | ||
Comment 13•12 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/830177b40e86
Comment 14•12 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/830177b40e86
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla21
Updated•11 years ago
|
Attachment mime type: text/plain text/plain text/plain text/plain → text/x-github-pull-request text/x-github-pull-request text/x-github-pull-request text/x-github-pull-request
You need to log in
before you can comment on or make changes to this bug.
Description
•