Closed Bug 1073014 Opened 7 years ago Closed 7 years ago

[B2G][NFC] Use Promise constructor instead of Promise.defer() in HCIEventTransactionSystemMessageConfigurator

Categories

(Firefox OS Graveyard :: NFC, defect)

x86
Gonk (Firefox OS)
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
2.1 S8 (7Nov)

People

(Reporter: tauzen, Assigned: tauzen)

References

Details

Attachments

(3 files, 9 obsolete files)

4.36 KB, patch
tauzen
: review+
Details | Diff | Splinter Review
2.51 KB, patch
allstars.chh
: review+
Details | Diff | Splinter Review
5.17 KB, patch
tauzen
: review+
Details | Diff | Splinter Review
Promise.defer() is currently obsolete, see [1]. HCIEventTransactionSystemMessageConfigurator should use Promise constructor instead.

[1] - https://developer.mozilla.org/en-US/docs/Mozilla/JavaScript_code_modules/Promise.jsm#Method_overview
Two initial tests with mocked up appsService, need to add more test with more complicated filtering rules.
Attachment #8496053 - Attachment is obsolete: true
Attachment #8498263 - Flags: review?(allstars.chh)
Comment on attachment 8498264 [details] [diff] [review]
Bug 1073014 Part 2: Using Promise constructor instead of Promise.defer()

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

This should be Part 1.
Attachment #8498264 - Flags: review?(allstars.chh) → review+
Commit message change only, setting r+ since Yoshi reviewed this in comment 4.
Attachment #8498264 - Attachment is obsolete: true
Attachment #8502294 - Flags: review+
Attached patch Part 2: xpcshell tests (obsolete) — Splinter Review
Attachment #8498263 - Attachment is obsolete: true
Attachment #8498263 - Flags: review?(allstars.chh)
Attachment #8502295 - Flags: review?(allstars.chh)
Bug 1066735 landed couple of hours ago and xpcshell_b2g.ini is not needed now.
Attachment #8502295 - Attachment is obsolete: true
Attachment #8502295 - Flags: review?(allstars.chh)
Attachment #8502333 - Flags: review?(allstars.chh)
Comment on attachment 8502333 [details] [diff] [review]
(v1.1 rebased) Part 2: xpcshell tests

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

::: dom/nfc/moz.build
@@ +4,5 @@
>  # file, You can obtain one at http://mozilla.org/MPL/2.0/.
>  #
>  # Copyright © 2013 Deutsche Telekom, Inc.
>  
> +XPCSHELL_TESTS_MANIFESTS += ['tests/unit/xpcshell.ini']

So far the tests should be only available when MOZ_NFC and B2G_GONK are enabled, it should be moved to below.

::: dom/nfc/tests/unit/test_HCIEventTransactionSystemMessageConfigurator.js
@@ +7,5 @@
> +/* exported run_test */
> +
> +const {classes: Cc, interfaces: Ci, utils: Cu} = Components;
> +
> +Cu.import('resource://gre/modules/Promise.jsm');

Gecko part use double quotes. "
Same for below.

@@ +11,5 @@
> +Cu.import('resource://gre/modules/Promise.jsm');
> +Cu.import('resource://gre/modules/XPCOMUtils.jsm');
> +
> +const MANIFEST_URL = 'app://system.gaiamobile.org/manifest.webapp';
> +const MANIFEST = {secure_element_access: ['SIM1/01', 'ESE/*', '*/A*']};

should be 'eSE' ?
Attachment #8502333 - Flags: review?(allstars.chh)
Comment on attachment 8502333 [details] [diff] [review]
(v1.1 rebased) Part 2: xpcshell tests

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

::: dom/nfc/tests/unit/test_HCIEventTransactionSystemMessageConfigurator.js
@@ +11,5 @@
> +Cu.import('resource://gre/modules/Promise.jsm');
> +Cu.import('resource://gre/modules/XPCOMUtils.jsm');
> +
> +const MANIFEST_URL = 'app://system.gaiamobile.org/manifest.webapp';
> +const MANIFEST = {secure_element_access: ['SIM1/01', 'ESE/*', '*/A*']};

and A* means ASSD?
(In reply to Yoshi Huang[:allstars.chh] from comment #10)
> Comment on attachment 8502333 [details] [diff] [review]
> (v1.1 rebased) Part 2: xpcshell tests
> 
> Review of attachment 8502333 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/nfc/tests/unit/test_HCIEventTransactionSystemMessageConfigurator.js
> @@ +11,5 @@
> > +Cu.import('resource://gre/modules/Promise.jsm');
> > +Cu.import('resource://gre/modules/XPCOMUtils.jsm');
> > +
> > +const MANIFEST_URL = 'app://system.gaiamobile.org/manifest.webapp';
> > +const MANIFEST = {secure_element_access: ['SIM1/01', 'ESE/*', '*/A*']};
> 
> and A* means ASSD?

A* is after '/' so this means every AID which starts with 'A' in hex string.
Attached patch (v1.2) Part 2: xpcshell tests (obsolete) — Splinter Review
Fixed all problems pointed out by Yoshi.
Attachment #8502333 - Attachment is obsolete: true
Attachment #8506167 - Flags: review?(allstars.chh)
Hello Yoshi, after switching to 'eSE' (as you suggested in comment 9) the test started to fail because |_checkAppManifest| compares SE name from the system message with uppercased SE name from manifest rule. I'm uppercasing the SE name for comparison in this version of the patch. Please take a look at this change and let me know if it's ok for you. Thank you.
Attachment #8502294 - Attachment is obsolete: true
Attachment #8506170 - Flags: review?(allstars.chh)
Sorry for the long wait, however I am not familiar with the code here so I need time to check this. But I have to finish Bug 1042851 before December and fix blockers if any, so I don't have too much bandwidth to do this, if you think you need this patch to be reviewed soon please find another reviewer like Fabrice (he reviewed hci event patch).
Comment on attachment 8506170 [details] [diff] [review]
(v1.1) Part 1: Using Promise constructor instead of Promise.defer()

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

::: dom/nfc/messages/HCIEventTransactionSystemMessageConfigurator.js
@@ +43,4 @@
>  
> +    return new Promise((resolve, reject) => {
> +      let aid = this._byteAIDToHex(aMessage.aid);
> +      let seName = aMessage.seName.toUpperCase();

This line should be moved to checkAppManifest.
Attachment #8506170 - Flags: review?(allstars.chh) → review+
Comment on attachment 8506170 [details] [diff] [review]
(v1.1) Part 1: Using Promise constructor instead of Promise.defer()

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

::: dom/nfc/messages/HCIEventTransactionSystemMessageConfigurator.js
@@ +43,4 @@
>  
> +    return new Promise((resolve, reject) => {
> +      let aid = this._byteAIDToHex(aMessage.aid);
> +      let seName = aMessage.seName.toUpperCase();

I mean toUpperCase.
Comment on attachment 8506167 [details] [diff] [review]
(v1.2) Part 2: xpcshell tests

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

::: dom/nfc/tests/unit/test_HCIEventTransactionSystemMessageConfigurator.js
@@ +11,5 @@
> +Cu.import("resource://gre/modules/Promise.jsm");
> +Cu.import("resource://gre/modules/XPCOMUtils.jsm");
> +
> +const MANIFEST_URL = "app://system.gaiamobile.org/manifest.webapp";
> +const MANIFEST = {secure_element_access: ["SIM1/01", "eSE/*", "*/A*"]};

A* should be a0*
one is to test toUpperCase, and the other reason is your test data is 160 (0xa0)
unless there's some reason to only verify the high byte.

@@ +17,5 @@
> +const TYPE = "nfc-hci-event-transaction";
> +
> +let HCIEvtTransactionConfigurator = null;
> +
> +function setMockManifest(manifest) {

This function looks to me is setMockAppService.

@@ +85,5 @@
> +  })
> +  .catch(handleRejectedPromise)
> +  .then(run_next_test);
> +});
> +

why test_shouldDispatch_rule_matching and test_shouldDispatch_rule_not_matching cannnot be merged here(test_shouldDispatch_general_rule_validation)?

@@ +88,5 @@
> +});
> +
> +add_test(function test_shouldDispatch_general_rule_validation() {
> +  let msgs = [
> +    { aid: new Uint8Array([223]), seName: "eSE" },

0xa0

@@ +89,5 @@
> +
> +add_test(function test_shouldDispatch_general_rule_validation() {
> +  let msgs = [
> +    { aid: new Uint8Array([223]), seName: "eSE" },
> +    { aid: new Uint8Array([160, 1, 2, 3]), seName: "SIM2" },

The Uint8Array here is dummy data, right ? if so, add a comment for this.

@@ +90,5 @@
> +add_test(function test_shouldDispatch_general_rule_validation() {
> +  let msgs = [
> +    { aid: new Uint8Array([223]), seName: "eSE" },
> +    { aid: new Uint8Array([160, 1, 2, 3]), seName: "SIM2" },
> +    { aid: new Uint8Array([2]), seName: "SIM3" },

There should at least have a valid AID here, like 0x1234567890ab, the test aids here are too short.
Attachment #8506167 - Flags: review?(allstars.chh)
R+ by Yoshi in comment 15. Moved the two lines which do conversion of data received from nfcd/system-message to checkAppManifest since this is specific for comparison with manifest contents. Added comment explaining the reason of conversion.
Attachment #8506170 - Attachment is obsolete: true
Attachment #8516618 - Flags: review+
Fix for bug found during on device testing, NfcService sends secure element name in origin property not seName.
Attachment #8516623 - Flags: review?(allstars.chh)
Attachment #8516623 - Flags: review?(allstars.chh) → review+
Attached patch (v1.3) Part 3: xpcshell tests (obsolete) — Splinter Review
Fixed issues pointed out in comment 17.
Attachment #8506167 - Attachment is obsolete: true
Attachment #8516625 - Flags: review?(allstars.chh)
Comment on attachment 8516625 [details] [diff] [review]
(v1.3) Part 3: xpcshell tests

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

::: dom/nfc/tests/unit/test_HCIEventTransactionSystemMessageConfigurator.js
@@ +16,5 @@
> +const MANIFEST = {
> +  secure_element_access: [
> +    // rule0, full AID, SIM1 Secure Element
> +    "SIM1/000102030405060708090A0B0C0D0E",
> +    // rule1, every AID from embeded Secure Element

embedded

@@ +18,5 @@
> +    // rule0, full AID, SIM1 Secure Element
> +    "SIM1/000102030405060708090A0B0C0D0E",
> +    // rule1, every AID from embeded Secure Element
> +    "eSE/*",
> +    // rule2, every AID wich starts with 0xA0, from every Secure Element

which
Attachment #8516625 - Flags: review?(allstars.chh) → review+
fixed nits from comment 21
Attachment #8516625 - Attachment is obsolete: true
Attachment #8516633 - Flags: review+
Keywords: checkin-needed
You need to log in before you can comment on or make changes to this bug.