Closed Bug 996452 Opened 6 years ago Closed 6 years ago

[NFC] Add emulator rf_discover test case

Categories

(Firefox OS Graveyard :: NFC, defect)

ARM
Gonk (Firefox OS)
defect
Not set

Tracking

(tracking-b2g:backlog)

RESOLVED FIXED
1.4 S6 (25apr)
tracking-b2g backlog

People

(Reporter: dimi, Assigned: dimi)

Details

(Whiteboard: [p=2])

Attachments

(1 file, 2 obsolete files)

After landing Bug 984397 , we will need a test case for emulator rf_discover
Whiteboard: [p=2]
Summary: [NDC] Add emulator rf_discover test case → [NFC] Add emulator rf_discover test case
Attached patch WIP Patch (obsolete) — Splinter Review
blocking-b2g: --- → backlog
Attached patch rf_discover test case patch V1 (obsolete) — Splinter Review
Attachment #8406680 - Attachment is obsolete: true
Attachment #8408878 - Flags: review?(tzimmermann)
Comment on attachment 8408878 [details] [diff] [review]
rf_discover test case patch V1

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

In general this looks good and the test passed locally. You can r+ me with this one question addressed and the style cleanups.

I ran gjslint, which complained about a few style errors. I'd like to seem them fixed, if they were introduced by the patch and the fix doesn't violate any other policies. For test_nfc_manager_tech_discovered.js it is

Line 7, E:0131: Single-quoted string preferred over double-quoted string.
Line 7, E:0110: Line too long (84 characters).
Line 10, E:0001: Extra space after "NCI_LAST_NOTIFICATION"

and for head.js it's

Line 6, E:0131: Single-quoted string preferred over double-quoted string.
Line 17, E:0131: Single-quoted string preferred over double-quoted string.
Line 24, E:0131: Single-quoted string preferred over double-quoted string.
Line 32, E:0010: Missing semicolon at end of line

::: dom/nfc/tests/marionette/test_nfc_manager_tech_discovered.js
@@ +56,5 @@
> +// DISCOVERY -> W4_ALL_DISCOVERIES -> W4_HOST_SELECT -> POLL_ACTIVE
> +function testRfDiscover() {
> +  log('Running \'testRfDiscover\'');
> +  window.navigator.mozSetMessageHandler(
> +    'nfc-manager-tech-discovered', handleTechnologyDiscoveredRE0);

Will this callback run for every discovered RE, or just after activating one of them? Because the handler turns off NFC, and if that happens on the first detected RE, the results for the second RE are undefined.
Attachment #8408878 - Flags: review?(tzimmermann) → review+
(In reply to Thomas Zimmermann [:tzimmermann] [:tdz] from comment #3)
> Comment on attachment 8408878 [details] [diff] [review]
> rf_discover test case patch V1
> 
> Review of attachment 8408878 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> In general this looks good and the test passed locally. You can r+ me with
> this one question addressed and the style cleanups.
> 
> I ran gjslint, which complained about a few style errors. I'd like to seem
> them fixed, if they were introduced by the patch and the fix doesn't violate
> any other policies. For test_nfc_manager_tech_discovered.js it is
> 
> Line 7, E:0131: Single-quoted string preferred over double-quoted string.
> Line 7, E:0110: Line too long (84 characters).
> Line 10, E:0001: Extra space after "NCI_LAST_NOTIFICATION"
> 
> and for head.js it's
> 
> Line 6, E:0131: Single-quoted string preferred over double-quoted string.
> Line 17, E:0131: Single-quoted string preferred over double-quoted string.
> Line 24, E:0131: Single-quoted string preferred over double-quoted string.
> Line 32, E:0010: Missing semicolon at end of line
> 
> ::: dom/nfc/tests/marionette/test_nfc_manager_tech_discovered.js
> @@ +56,5 @@
> > +// DISCOVERY -> W4_ALL_DISCOVERIES -> W4_HOST_SELECT -> POLL_ACTIVE
> > +function testRfDiscover() {
> > +  log('Running \'testRfDiscover\'');
> > +  window.navigator.mozSetMessageHandler(
> > +    'nfc-manager-tech-discovered', handleTechnologyDiscoveredRE0);
> 
> Will this callback run for every discovered RE, or just after activating one
> of them? Because the handler turns off NFC, and if that happens on the first
> detected RE, the results for the second RE are undefined.

This callback will only run after activating , :)
Attached patch bug_996452.patchSplinter Review
Attachment #8408878 - Attachment is obsolete: true
(In reply to Dimi Lee[:dimi][:dlee] from comment #4)
> (In reply to Thomas Zimmermann [:tzimmermann] [:tdz] from comment #3)
> > Comment on attachment 8408878 [details] [diff] [review]
> > rf_discover test case patch V1
> > 
> > Review of attachment 8408878 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > In general this looks good and the test passed locally. You can r+ me with
> > this one question addressed and the style cleanups.
> > 
> > I ran gjslint, which complained about a few style errors. I'd like to seem
> > them fixed, if they were introduced by the patch and the fix doesn't violate
> > any other policies. For test_nfc_manager_tech_discovered.js it is
> > 
> > Line 7, E:0131: Single-quoted string preferred over double-quoted string.
> > Line 7, E:0110: Line too long (84 characters).
> > Line 10, E:0001: Extra space after "NCI_LAST_NOTIFICATION"
> > 
> > and for head.js it's
> > 
> > Line 6, E:0131: Single-quoted string preferred over double-quoted string.
> > Line 17, E:0131: Single-quoted string preferred over double-quoted string.
> > Line 24, E:0131: Single-quoted string preferred over double-quoted string.
> > Line 32, E:0010: Missing semicolon at end of line

From MDN coding style, i think we prefer using double-quoted string for js.
So i will not modify this part , are you fine with this ?
Flags: needinfo?(tzimmermann)
Sure.
Flags: needinfo?(tzimmermann)
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/509937940bc6
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
blocking-b2g: backlog → ---
You need to log in before you can comment on or make changes to this bug.