Closed Bug 996426 Opened 6 years ago Closed 6 years ago

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

Categories

(Firefox OS Graveyard :: NFC, defect)

ARM
Gonk (Firefox OS)
defect
Not set

Tracking

(feature-b2g:2.0, tracking-b2g:backlog)

RESOLVED FIXED
2.0 S1 (9may)
feature-b2g 2.0
tracking-b2g backlog

People

(Reporter: tzimmermann, Assigned: psiddh)

References

Details

Attachments

(3 files, 7 obsolete files)

61 bytes, text/x-github-pull-request
tzimmermann
: review+
Details | Review
783 bytes, patch
vicamo
: review+
Details | Diff | Splinter Review
2.97 KB, patch
vicamo
: review+
Details | Diff | Splinter Review
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
See 'external/qemu/android/console.c' for the console implementation.
blocking-b2g: --- → backlog
Assignee: nobody → psiddh
Status: NEW → ASSIGNED
Attachment #8409950 - Flags: review?(tzimmermann)
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'.
Attachment #8409950 - Flags: review?(tzimmermann) → review-
Attachment #8409950 - Attachment is obsolete: true
Attachment #8411489 - Flags: review?(tzimmermann)
Attachment #8411489 - Attachment is obsolete: true
Attachment #8411489 - Flags: review?(tzimmermann)
Attachment #8411492 - Flags: review?(tzimmermann)
Attachment #8411493 - Flags: review?(tzimmermann)
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
Thanks Sid, I'll review these patches tomorrow. Been busy today.
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.
Attachment #8411493 - Flags: review?(tzimmermann) → review+
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!
Attachment #8411492 - Flags: review?(tzimmermann) → review+
Attachment #8411492 - Attachment is obsolete: true
Attachment #8413089 - Flags: review?(vyang)
Attachment #8411493 - Attachment is obsolete: true
Attachment #8413094 - Flags: review?(vyang)
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 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
Attachment #8413089 - Flags: review?(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.
Attachment #8413094 - Flags: review?(vyang) → review+
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]
Flags: needinfo?(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.
Flags: needinfo?(vyang)
Attachment #8413094 - Attachment is obsolete: true
Attachment #8414901 - Flags: review?(vyang)
Attachment #8414907 - Flags: review?(vyang)
Attachment #8414901 - Attachment is obsolete: true
Attachment #8414901 - Flags: review?(vyang)
Attachment #8414909 - Flags: review?(vyang)
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 ?
(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.
Attachment #8414907 - Flags: review?(vyang) → review+
Attachment #8414909 - Flags: review?(vyang) → review+
Is there any dependency to bug 1000935?
Flags: needinfo?
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.
Flags: needinfo?
I just noticed that you need to rename 'rf_discover' as well.
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
Attachment #8414909 - Attachment is obsolete: true
Attachment #8417730 - Flags: review?(vyang)
Attachment #8413089 - Flags: review?(tzimmermann)
Attachment #8414907 - Flags: review+ → review?(vyang)
Requesting review for all the patches (just in case)
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.
Attachment #8413089 - Flags: review?(tzimmermann) → review+
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.
Flags: needinfo?(vyang)
Attachment #8414907 - Flags: review?(vyang) → review+
Attachment #8417730 - Flags: review?(vyang) → review+
(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.
Flags: needinfo?(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.
Flags: needinfo?(vyang)
https://hg.mozilla.org/mozilla-central/rev/14132a1a89dd
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → 2.0 S1 (9may)
feature-b2g: --- → 2.0
Flags: in-moztrap-
blocking-b2g: backlog → ---
You need to log in before you can comment on or make changes to this bug.