970251 - B2G NFC: Use API to enable/disable NFC hardware

Status: RESOLVED FIXED

(Firefox OS Graveyard :: NFC, defect)

Description

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi] OOO ~ Feb.7]

I think this is mentioned by Alive and Evelyn before.
Also I mentioned this in https://bugzilla.mozilla.org/show_bug.cgi?id=866907#c5, 
https://bugzilla.mozilla.org/show_bug.cgi?id=866907#c7,
and https://bugzilla.mozilla.org/show_bug.cgi?id=866907#c9

Right now we use Settings API to turn on/off NFC HW.

This bug is filed to discuss should we use API for this.

Comment 1

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

What's wrong with using the Settings API?

Comment 2

[Garner Lee]

There's 2 halves.

1) The UI sets the user preference
2)
  a) Settings restore startup time: nsISettingsServiceCallback (Nfc.js) in system/gonk watches that setting. This then fires off the Config message to the nfcd system daemon.
  b) nsIObserver (Nfc.js): watches in system/gonk to fire off the UI changes. Fires off Config message to nfcd.

What improvements are we discussing? Move it up from Gonk to something higher in the stack?

Comment 3

[Ken Chang[:kenkai]]

The question is whether we need to temporarily turn off NFC. Like the working mode between WIFI and data call, although the setting of data call is turned on, the data call should not be established temporarily when wifi is connected.

Comment 4

[Garner Lee]

Okay. The bug is to look at improving the NFC suspend feature to be more API based, and/or less ambiguous? Suspend currently available as a feature only for airplane mode, as with wifi and bluetooth, via the "nfc.suspend" setting.

Comment 5

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi] OOO ~ Feb.7]

I'll add three interfaces into NfcManager

DOMRequest startPoll();
DOMRequest stopPoll();
DOMRequest powerOff();

Which are from W3C NFC, except here we return DOMRequest, also powerOn is removed.

Comment 6

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi] OOO ~ Feb.7]

This bug will focus on Gecko part first, I'll handle Gaia changes in Bug 995101.

Comment 7

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi] OOO ~ Feb.7]

Attachment: Part 1: WebIDL change

Comment 8

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi] OOO ~ Feb.7]

Attachment: Part 2: Impl

Comment 9

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi] OOO ~ Feb.7]

Attachment: Part 3: Marionette tests.

Comment 10

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi] OOO ~ Feb.7]

Attachment: Part 3: Marionette tests. v2.

update the sequence of tests.

Comment 11

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi] OOO ~ Feb.7]

Attachment: Part 3: Marionette tests. v2.

Comment 12

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi] OOO ~ Feb.7]

Comment on attachment 8405233 [details] [diff] [review]
Part 1: WebIDL change

Hi smaug

The three interfaces added here are referenced from W3C NFC API. [1]
I didn't add powerOn because I think startPoll can cover it.

Thanks

[1]: http://www.w3.org/TR/nfc/#nfcmanager-interface

Comment 13

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8405233 [details] [diff] [review]
Part 1: WebIDL change

Could you file a followup bug to investigate if the API shouldn't use 
DOMRequests (which it is already using), but Promises.

Comment 14

[Dimi Lee [:dimi]]

Comment on attachment 8405234 [details] [diff] [review]
Part 2: Impl

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

::: dom/system/gonk/nsINfcContentHelper.idl
@@ +135,5 @@
>    void notifySendFileStatus(in nsIDOMWindow window,
>                              in octet status,
>                              in DOMString requestId);
> +
> +

Nit.
Extra empty line here, it will also be good if we add comment here
since startPoll menas turn on nfc and start polling

Comment 15

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi] OOO ~ Feb.7]

(In reply to Olli Pettay [:smaug] from comment #13)
> Comment on attachment 8405233 [details] [diff] [review]
> Part 1: WebIDL change
> 
> Could you file a followup bug to investigate if the API shouldn't use 
> DOMRequests (which it is already using), but Promises.

Thanks for the review.
Filed Bug 996397 for this.

Comment 16

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi] OOO ~ Feb.7]

Attachment: Part 2: Impl. v2

Address Dimi's comments:
remove extra line,
add comments in nsINfcContentHelper.idl

Comment 17

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi] OOO ~ Feb.7]

https://hg.mozilla.org/integration/b2g-inbound/rev/1fd7913a1b3f
https://hg.mozilla.org/integration/b2g-inbound/rev/640f5ac75700
https://hg.mozilla.org/integration/b2g-inbound/rev/e5e03c701f7a

Comment 18

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/1fd7913a1b3f
https://hg.mozilla.org/mozilla-central/rev/640f5ac75700
https://hg.mozilla.org/mozilla-central/rev/e5e03c701f7a

Comment 19

[Ryan VanderMeulen [:RyanVM]]

Please fix your Mercurial configuration so that your patches contain your full name.
https://developer.mozilla.org/en-US/docs/Mercurial_FAQ#How_can_I_generate_a_patch_for_somebody_else_to_check-in_for_me.3F

Comment 20

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi] OOO ~ Feb.7]

fixed now, sorry for the confusion.