Vicamo Yang [:vicamo][:vyang] Assignee
	Description • 12 years ago

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

	Vicamo Yang [:vicamo][:vyang] 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.

	Vicamo Yang [:vicamo][:vyang] 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.

	Hsin-Yi Tsai (she/her) [:hsinyi]
	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.

	Hsin-Yi Tsai (she/her) [:hsinyi]
	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?

	Vicamo Yang [:vicamo][:vyang] Assignee
	Comment 7 • 12 years ago

Moved to new pull request

	Vicamo Yang [:vicamo][:vyang] Assignee
	Comment 8 • 12 years ago

Moved to new pull request

	Hsin-Yi Tsai (she/her) [:hsinyi]
	Comment 9 • 12 years ago

Comment on attachment 696929 [details]
Github Pull Request for external/qemu

Looks great!

	Hsin-Yi Tsai (she/her) [:hsinyi]
	Comment 10 • 12 years ago

Comment on attachment 696929 [details]
Github Pull Request for external/qemu

Looks great, only a nit there. Please see my comment in github :)

	Hsin-Yi Tsai (she/her) [:hsinyi]
	Comment 11 • 12 years ago

Comment on attachment 696930 [details]
Github Pull Request for hardware/ril

Looks good to me!

	Vicamo Yang [:vicamo][:vyang] Assignee
	Comment 12 • 12 years ago

Address review comments.

	Vicamo Yang [:vicamo][:vyang] Assignee
	Comment 13 • 12 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/830177b40e86

	Ed Morley [:emorley]
	Comment 14 • 12 years ago

https://hg.mozilla.org/mozilla-central/rev/830177b40e86