996426 - [NFC] Rename emulator command 'nfc ntf' to 'nfc nci'

Status: RESOLVED FIXED

(Firefox OS Graveyard :: NFC, defect)

Description

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

As soon as bug 980851 lands, NFC emulation will involve multiple protocols with individual emulator commands. The current naming scheme of console commands doesn't meet these requirements.

To distinguish individual protocols we should use the second argument for the protocol. For NCI this means to rename 'nfc ntf $PRIMITIVE' to 'nfc nci $PRIMITIVE_ntf'. For example

  nfc ntf rf_intf_activated 0

would become

  nfc nci rf_intf_activated_ntf 0

Comment 1

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

See 'external/qemu/android/console.c' for the console implementation.

Comment 2

[psiddh]

Attachment: 0001-Bug-996426-NFC-Rename-emulator-command-nfc-ntf-to-nf.patch

Comment 3

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

Comment on attachment 8409950 [details] [diff] [review]
0001-Bug-996426-NFC-Rename-emulator-command-nfc-ntf-to-nf.patch

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

Hi Sid,

Thanks for the patch. You should first rebase it on top of bugs 993330 and 996452. And there is a bit more to do. Besides the points listed in the review, there are test cases in Gecko's dom/nfc/tests/marionette/*.js that need to be adapted to the new console interface. Every file that ends with .js might be affected. Just look for the old command and replace it with the new one.

::: android/console.c
@@ +3502,5 @@
>  }
>  
>  static const CommandDefRec  nfc_commands[] =
>  {
>      { "ntf", "send NCI notification",

The first field is supposed to contain "nci".

@@ +3503,5 @@
>  
>  static const CommandDefRec  nfc_commands[] =
>  {
>      { "ntf", "send NCI notification",
>        "'nfc ntf rf_discover <i> <type>' send RC_DISCOVER_NTF for Remote Endpoint <i> with notification type <type>\r\n"

And all these commands need to be changed to 'nfc nci <command>_ntf.

@@ +3510,2 @@
>        NULL,
>        do_nfc_ntf, NULL },

This function should be renamed to 'do_nfc_nci'.

Comment 4

[psiddh]

Attachment: Part 1 : 0001-Bug-996426-NFC-Rename-emulator-commands-from-nfc-ntf.patch

Comment 5

[psiddh]

Attachment: Part 1 : 0001-Bug-996426-NFC-Rename-emulator-commands-from-nfc-ntf.patch

Comment 6

[psiddh]

Attachment: Part 2 : 0001-Bug-996426-Part-2-Rename-emulator-commands-from-nfc-.patch

Comment 7

[psiddh]

Hi Thomas, Thanks for your comments. I have now rebased my changes on top of the said bugs and updated the marionette JS files as well (in part 2). Ran the  marionette on emulator to ensure that T.Cs are passing

Comment 8

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

Thanks Sid, I'll review these patches tomorrow. Been busy today.

Comment 9

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

Comment on attachment 8411493 [details] [diff] [review]
Part 2 : 0001-Bug-996426-Part-2-Rename-emulator-commands-from-nfc-.patch

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

Just a small nit: you may drop the 'Part 2' from the patch description and add a note that this depends on a change in the emulator.

Comment 10

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

Comment on attachment 8411492 [details] [diff] [review]
Part 1 : 0001-Bug-996426-NFC-Rename-emulator-commands-from-nfc-ntf.patch

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

This patch needs another rebase on the latest version of the emulator. To land the emulator patch, you should create a pull request on Github, attach a link here and ask :vicamo for a review.

Thanks, Sid!

Comment 11

[psiddh]

Attachment: Part 1 : Rename nfc emulator commands from 'nfc ntf' to 'nfc nci'

Comment 12

[psiddh]

Attachment: Part 2: Rename emulator command in marionette tests

Comment 13

[psiddh]

Hi Vicamo, Please ignore my 'pull req' - https://github.com/mozilla-b2g/platform_external_qemu/pull/79 that I accidentally created. Not sure how I can cancel this request.

Comment 14

[Vicamo Yang [:vicamo][:vyang]]

Comment on attachment 8413089 [details] [review]
Part 1 : Rename nfc emulator commands from 'nfc ntf' to 'nfc nci'

Like what you've already known, the two patches depend on each other and we don't have a way to effectively land them both at the same time, at least I don't know.

However, in this case we can play some console command tricks to allow this.  Emulator console command parser allow alternatives subcommand names. [1] That is, you can assign "<name_1>|<name_2>｜..." to the first data member of a CommandDefRec.  There are two existing examples: "help|h|?" [2] and "quit|exit" [3].  So you can use "nci|ntf" to keep backward compatibility here.

Alternatively, you may also send two patches to Gecko or two pull requests to qemu.  For Gecko, the first patch disables NFC test cases completely and the second one re-enables them.  For qemu, the first PR provides both CommandDefRec and the second one removes that obsoleted "ntf" one.

Thomas's review is enough for me.  I'll r+ if the patch/pull request here can be landed safely.

[1]: https://github.com/mozilla-b2g/platform_external_qemu/blob/master/android/console.c#L368
[2]: https://github.com/mozilla-b2g/platform_external_qemu/blob/master/android/console.c#L4120
[3]: https://github.com/mozilla-b2g/platform_external_qemu/blob/master/android/console.c#L4150

Comment 15

[Vicamo Yang [:vicamo][:vyang]]

Comment on attachment 8413094 [details] [diff] [review]
Part 2: Rename emulator command in marionette tests

It's good to me.  The only problem is how to land them at the same time without breaking tbpl tests.

Comment 16

[psiddh]

Hi Vicamo, 

Thanks for your suggestions and time.

Can we remove 'test_nfc_manager_tech_discovered.js' from 'manifest.ini' @ gecko/dom/nfc/tests/marionette?
Since this is the only test case that actually sends nfc emulator commands,does it help if we remove the entry from the manifest ? (I will also raise another bug that adds back the entry into the manifest.)

If the above is not going to help, please let me know if this is how we can disable nfc tests
@ gecko/testing/marionette/client/marionette/tests/unit-tests.ini, remove the 'nfc entry'
[include:../../../../../dom/nfc/tests/marionette/manifest.ini]

Comment 17

[Vicamo Yang [:vicamo][:vyang]]

(In reply to Siddartha P from comment #16)
> Hi Vicamo, 
> 
> Thanks for your suggestions and time.
> 
> Can we remove 'test_nfc_manager_tech_discovered.js' from 'manifest.ini' @
> gecko/dom/nfc/tests/marionette?
> Since this is the only test case that actually sends nfc emulator
> commands,does it help if we remove the entry from the manifest ? (I will
> also raise another bug that adds back the entry into the manifest.)

You can put a "disabled = <reason string>" on the next line of a test case entry.  See http://mxr.mozilla.org/mozilla-central/source/dom/mobileconnection/tests/marionette/manifest.ini#27

So we'll need two Gecko patches:
1) place "disabled = bug 996426",
2) remove that "disabled = ..." and rename "ntf" to "nci".

We can land the first one, and land qemu pull request after first one is merged into m-c, and in the end, the second Gecko patch to correct all things right.

Comment 18

[psiddh]

Attachment: (v1.1) - Part 2 : Renaming Emulator commands + disabling certain marionette tests

Comment 19

[psiddh]

Attachment: (v1) Part 3: Re-enable marionette test cases

Comment 20

[psiddh]

Attachment: (v1.1) - Part 2 : Renaming nfc Emulator commands + disabling certain marionette tests

Comment 21

[psiddh]

Hi Vicamo,

Please review the patches (Needed to modify another test case that Yoshi added recently). We can stage these patches in the following order.
'Part 2' , then 'Part 1' and finally 'Part 3' that re-enables the test cases back.
Note that 'Part 2' renames the emu commands + disables it as well at the same time. Let me know if you need the patches to be fixed in any other order.

Also please note that when I run './mach marionette-webapi ./gecko/dom/nfc/tests/marionette/', it still tries to run the test cases despite having 'disabled' flag set in the manifest.ini. As a result the test case fails. Is this expected ? (or) did I miss something ?

Comment 22

[Vicamo Yang [:vicamo][:vyang]]

(In reply to Siddartha P from comment #21)
> Also please note that when I run './mach marionette-webapi
> ./gecko/dom/nfc/tests/marionette/', it still tries to run the test cases
> despite having 'disabled' flag set in the manifest.ini. As a result the test
> case fails. Is this expected ? (or) did I miss something ?

Hi, because you choose to run every test case under a certain folder.  `./mach <test-category> <directory>` means running all test files under the directory no matter they are enabled, disable, skipped or not even on the manifest.  `./mach <test-category> <manifest_file>` means running all test files specified by the manifest only.  `./mach <test-category> <test_file>` runs the specified test file anyway.

Comment 23

[Vicamo Yang [:vicamo][:vyang]]

Is there any dependency to bug 1000935?

Comment 24

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

Hi Vicamo

(In reply to Vicamo Yang [:vicamo][:vyang] from comment #23)
> Is there any dependency to bug 1000935?

The test case in bug 1000935 still uses the old interface. I'll take care of that with the next update. There are no strict dependencies in qemu, but maybe rebase conflicts.

Comment 25

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

I just noticed that you need to rename 'rf_discover' as well.

Comment 26

[psiddh]

Attachment: (v1.2) - Part 2 : Renaming nfc Emulator commands + disabling certain marionette tests

Updated the the rf_discover commands as well as per Thomas's comments.
Also updated the pull req with the respective change in 'Part 1' of the patch

Comment 27

[psiddh]

Requesting review for all the patches (just in case)

Comment 28

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

Comment on attachment 8413089 [details] [review]
Part 1 : Rename nfc emulator commands from 'nfc ntf' to 'nfc nci'

Looks good. :) Sorry, I should have noticed this earlier.

Comment 29

[psiddh]

Thanks Thomas.
Hi Vicamo,
  Could you pls help with the pull request and the patches ? I am not sure what the next steps should be.
Also please let me know if I need to do something on my side.

Comment 30

[Vicamo Yang [:vicamo][:vyang]]

(In reply to Siddartha P from comment #29)
> Thanks Thomas.
> Hi Vicamo,
>   Could you pls help with the pull request and the patches ? I am not sure
> what the next steps should be.
> Also please let me know if I need to do something on my side.

Hi, like what you've already had in comment 21, I'm going to land part 2 to b2g-inbound first, and after it's merged to m-c, I'll land part 1, and then part 3.

Comment 31

[Vicamo Yang [:vicamo][:vyang]]

(In reply to Siddartha P from comment #29)
> Also please let me know if I need to do something on my side.

Hi, Please refer to https://developer.mozilla.org/en-US/docs/Mercurial_FAQ#How_can_I_generate_a_patch_for_somebody_else_to_check-in_for_me.3F to generate patches next time.

Comment 32

[Vicamo Yang [:vicamo][:vyang]]

https://hg.mozilla.org/integration/b2g-inbound/rev/15fbe6ab7334

Comment 33

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/15fbe6ab7334

Comment 34

[Vicamo Yang [:vicamo][:vyang]]

https://github.com/mozilla-b2g/platform_external_qemu/commit/24e0e7016bbc22096b28e8bfdd6362b250ac78b7

https://hg.mozilla.org/integration/b2g-inbound/rev/4cce089da9bc

Comment 35

[Vicamo Yang [:vicamo][:vyang]]

http://hg.mozilla.org/mozilla-central/rev/a8602e656d86

Comment 36

[Vicamo Yang [:vicamo][:vyang]]

https://hg.mozilla.org/integration/b2g-inbound/rev/14132a1a89dd

Comment 37

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/14132a1a89dd