Closed Bug 1035606 Opened 5 years ago Closed 5 years ago

[NFC] Testcase cleanup

Categories

(Firefox OS Graveyard :: NFC, defect)

ARM
Gonk (Firefox OS)
defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
2.0 S6 (18july)

People

(Reporter: dimi, Assigned: dimi)

References

Details

(Whiteboard: [p=2])

Attachments

(1 file, 2 obsolete files)

No description provided.
Attached patch bug 1035606 patch v1 (obsolete) — Splinter Review
Attachment #8452198 - Flags: review?(tzimmermann)
Comment on attachment 8452198 [details] [diff] [review]
bug 1035606 patch v1

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

Nice cleanup! :) I have a few points, you might consider cleaning up as well, but this looks good already.

::: dom/nfc/tests/marionette/head.js
@@ +14,5 @@
> +const P2P_RE_INDEX_1 = 1;
> +const T1T_RE_INDEX   = 2;
> +const T2T_RE_INDEX   = 3;
> +const T3T_RE_INDEX   = 4;
> +const T4T_RE_INDEX   = 5;

Would it make sense to move these constants into the emulator object?

@@ +119,5 @@
> +  };
> +}());
> +
> +let SNEP = (function() {
> +  function putNdef(dsap, ssap, flags, tnf, type, payload, id) {

Maybe just |put|, like the protocol operation?

@@ +173,5 @@
>      window.navigator.mozSetMessageHandler(type, null);
>    });
>  }
>  
>  function enableRE0() {

This function should be removed and users should be converted to |NCI.activateRE|.

::: dom/nfc/tests/marionette/test_nfc_read_tag.js
@@ +94,5 @@
>      testUrlT4TDiscover,
>      testEmptyT1TDiscover,
>      testEmptyT2TDiscover,
>      testEmptyT3TDiscover,
> +    testEmptyT4TDiscover,

Just out of curiosity, is there a difference between having the comma and not having it?
Attachment #8452198 - Flags: review?(tzimmermann) → review+
> ::: dom/nfc/tests/marionette/test_nfc_read_tag.js
> @@ +94,5 @@
> >      testUrlT4TDiscover,
> >      testEmptyT1TDiscover,
> >      testEmptyT2TDiscover,
> >      testEmptyT3TDiscover,
> > +    testEmptyT4TDiscover,
> 
> Just out of curiosity, is there a difference between having the comma and
> not having it?
No, it works both way. Just forget to remove it after removing some debug code.
I'll remove it in next patch.
Attached patch bug 1035606 v2 patch (obsolete) — Splinter Review
Hi Thomas,
  I did other clean up according to your suggestion.
could you help review again? thanks!
Attachment #8452198 - Attachment is obsolete: true
Attachment #8452846 - Flags: review?(tzimmermann)
Duplicate of this bug: 1035837
Dimi you will also need to rebase and fix test_nfc_error_messages.js once Bug 1033218 will land. I think it will be conflicted with the current patch because it uses emulator.activateRE and adds emulator.deactivate usage instead of toggleNFC(false).
Depends on: 1033218
Comment on attachment 8452846 [details] [diff] [review]
bug 1035606 v2 patch

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

Thanks!

::: dom/nfc/tests/marionette/test_nfc_manager_tech_discovered.js
@@ +31,5 @@
>    window.navigator.mozSetMessageHandler(
>      'nfc-manager-tech-discovered', handleTechnologyDiscoveredRE0);
>  
>    toggleNFC(true)
> +  .then(() => NCI.notifyDiscoverRE(emulator.P2P_RE_INDEX_0, NCI_MORE_NOTIFICATIONS))

I just noticed that these constants could be moved into NCI without the prefix. Are there any other constants anywhere?
Attachment #8452846 - Flags: review?(tzimmermann) → review+
(In reply to Krzysztof Mioduszewski[:kmioduszewski][:tauzen] from comment #6)
> Dimi you will also need to rebase and fix test_nfc_error_messages.js once
> Bug 1033218 will land. I think it will be conflicted with the current patch
> because it uses emulator.activateRE and adds emulator.deactivate usage
> instead of toggleNFC(false).

Thanks for the info, I will rebase it once bug 1033218 merges to the central
Attachment #8452846 - Attachment is obsolete: true
Attachment #8453520 - Flags: review+
Whiteboard: [p=2]
Keywords: checkin-needed
Is there a Try link handy for this? :)
Keywords: checkin-needed
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/97ed5d7ce52f
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.