1035606 - [NFC] Testcase cleanup

Status: RESOLVED FIXED

(Firefox OS Graveyard :: NFC, defect)

Comment 1

[Dimi Lee [:dimi][:dlee]]

Attachment: bug 1035606 patch v1

Comment 2

[Thomas Zimmermann [:tzimmermann] [:tdz]]

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?

Comment 3

[Dimi Lee [:dimi][:dlee]]

> ::: 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.

Comment 4

[Dimi Lee [:dimi][:dlee]]

Attachment: bug 1035606 v2 patch

Hi Thomas,
  I did other clean up according to your suggestion.
could you help review again? thanks!

Comment 6

[Krzysztof Mioduszewski[:tauzen]]

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).

Comment 7

[Thomas Zimmermann [:tzimmermann] [:tdz]]

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?

Comment 8

[Dimi Lee [:dimi][:dlee]]

(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

Comment 9

[Dimi Lee [:dimi][:dlee]]

Attachment: bug 1035606 v3.patch

Comment 10

[Ryan VanderMeulen [:RyanVM]]

Is there a Try link handy for this? :)

Comment 11

[Dimi Lee [:dimi][:dlee]]

https://tbpl.mozilla.org/?tree=Try&rev=4bb02b0257b5

Comment 12

[Ryan VanderMeulen [:RyanVM]]

Slight overkill there, but thanks ;)
https://wiki.mozilla.org/Sheriffing/How:To:Recommended_Try_Practices

https://hg.mozilla.org/integration/b2g-inbound/rev/97ed5d7ce52f

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/97ed5d7ce52f