1096897 - replace powerLevel to rfState.

Status: RESOLVED FIXED

(Firefox OS Graveyard :: NFC, defect)

Description

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh] OOO ~ end of July]

'PowerLevel' isn't an appropriate term for what we do in NFC, for example, enable P2P Listening, enable CE Listening, ... etc.

We'd rename it to "rfState" to make it more appropriate to fit the state diagram mentioned in NCI documentation.

Comment 1

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh] OOO ~ end of July]

Attachment: Part 1: rename powerLevel to rfState.

Comment 2

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh] OOO ~ end of July]

Attachment: Part 2: rename to changeRFState

Comment 3

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh] OOO ~ end of July]

Attachment: nfcd Pull Request

Comment 4

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh] OOO ~ end of July]

Comment on attachment 8521168 [details] [review]
nfcd Pull Request

Found something is missing.

Comment 5

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

Comment on attachment 8521165 [details] [diff] [review]
Part 1: rename powerLevel to rfState.

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

::: dom/nfc/gonk/Nfc.js
@@ +601,5 @@
>      let isPowerAPI = message.name == "NFC:StartPoll" ||
>                       message.name == "NFC:StopPoll"  ||
>                       message.name == "NFC:PowerOff";
>  
>      if (!isPowerAPI) {

rename isPowerAPI

::: dom/nfc/gonk/nfc_consts.js
@@ +39,5 @@
>  
> +this.NFC_RF_STATE_UNKNOWN        = -1;
> +this.NFC_RF_STATE_DISABLED       = 0;
> +this.NFC_RF_STATE_LOW            = 1;
> +this.NFC_RF_STATE_ENABLED        = 2;

Since we are using rf state now, suggest to use
this.NFC_RF_STATE_UNKNOWN = -1;
this.NFC_RF_STATE_IDLE = 0;
this.NFC_RF_STATE_LISTEN = 1;
this.NFC_RF_STATE_DISCOVERY = 2;

Comment 6

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

(In reply to Dimi Lee[:dimi][:dlee] from comment #5)
> Comment on attachment 8521165 [details] [diff] [review]
> Part 1: rename powerLevel to rfState.
> 
> Review of attachment 8521165 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/nfc/gonk/Nfc.js
> @@ +601,5 @@
> >      let isPowerAPI = message.name == "NFC:StartPoll" ||
> >                       message.name == "NFC:StopPoll"  ||
> >                       message.name == "NFC:PowerOff";
> >  
> >      if (!isPowerAPI) {
> 
> rename isPowerAPI
> 
I saw this is renamed in part2.

Comment 7

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh] OOO ~ end of July]

Attachment: Part 1: rename powerLevel to rfState. v2.

Comment 8

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh] OOO ~ end of July]

Attachment: Part 2: rename to changeRFState v2

Comment 9

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh] OOO ~ end of July]

Comment on attachment 8521211 [details] [diff] [review]
Part 1: rename powerLevel to rfState. v2.

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

Add r? to smaug for the WebIDL change in NfcOptions.webidl.

Comment 10

[Olli Pettay [:smaug]]

Comment on attachment 8521211 [details] [diff] [review]
Part 1: rename powerLevel to rfState. v2.

rfState has just a few possible values so it behaves like an enum.
I think more webby API would be to use an enum.

enum RFState
{
  "unknown",
  "idle",
  "listen",
  "discovery"
};
or something like that.

Any reason to not use enum? (If there is, ask a review again for this patch.)

Comment 11

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh] OOO ~ end of July]

Attachment: Part 1: rename powerLevel to rfState. v3.

Use enum for RFState in WebIDL.

Comment 12

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh] OOO ~ end of July]

Attachment: Part 2: rename to changeRFState v3.

remove consts defined in nsINfcContentHelper.idl

Comment 13

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh] OOO ~ end of July]

Comment on attachment 8522041 [details] [diff] [review]
Part 1: rename powerLevel to rfState. v3.

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

r? to Dimi again since use enum for RFState..

Comment 14

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh] OOO ~ end of July]

Attachment: Part 1: v2-v3 diff.

This is the inter-diff generated from v2 to v3 in Part1.

Comment 15

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh] OOO ~ end of July]

Attachment: Part 2: v2-v3 diff

This is the inter-diff for v2 and v3 in Part 2.

Comment 16

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh] OOO ~ end of July]

Comment on attachment 8522042 [details] [diff] [review]
Part 2: rename to changeRFState v3.

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

r? to Dimi again since using enum.

The diff between v2 and v3 is also attached.

Comment 17

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh] OOO ~ end of July]

Comment on attachment 8522041 [details] [diff] [review]
Part 1: rename powerLevel to rfState. v3.

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

Hi Smaug
Thanks for your suggestion.
Now I've changed to use enum now, could you help to review this again ? 
(I also attached inter-diff with previous version v2 if you need it.)

Thanks

Comment 18

[Olli Pettay [:smaug]]

Comment on attachment 8522041 [details] [diff] [review]
Part 1: rename powerLevel to rfState. v3.

>+    if (mEvent.mRfState != -1) {
Would it be possible to assert here that
mRfState < RFState::EndGuard_

(There should be some sort of end value generated for enum for C++. See <objdir>/dist/include/mozilla/dom/yourwebidlfileBinding.h)

Comment 19

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh] OOO ~ end of July]

merged nfcd PR, since it only involves renaming.

https://github.com/mozilla-b2g/platform_system_nfcd/commit/9663d24afcda9f96146185972ade780714727beb

Comment 20

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh] OOO ~ end of July]

(In reply to Olli Pettay [:smaug] from comment #18)
> Comment on attachment 8522041 [details] [diff] [review]
> Part 1: rename powerLevel to rfState. v3.
> 
> >+    if (mEvent.mRfState != -1) {
> Would it be possible to assert here that
> mRfState < RFState::EndGuard_

Sure, I'll add the assert. 
Thanks for the review.

Comment 21

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh] OOO ~ end of July]

Attachment: Part 1: rename powerLevel to rfState. v4.

Add MOZ_ASSERT on mRfState.

Comment 22

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh] OOO ~ end of July]

https://hg.mozilla.org/integration/b2g-inbound/rev/65257e4d29bb
https://hg.mozilla.org/integration/b2g-inbound/rev/d889fed1d819

Comment 23

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/65257e4d29bb
https://hg.mozilla.org/mozilla-central/rev/d889fed1d819