Closed Bug 1096897 Opened 7 years ago Closed 7 years ago

replace powerLevel to rfState.

Categories

(Firefox OS Graveyard :: NFC, defect)

ARM
Gonk (Firefox OS)
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
2.1 S9 (21Nov)

People

(Reporter: allstars.chh, Assigned: allstars.chh)

References

Details

(Whiteboard: [p=2])

Attachments

(3 files, 7 obsolete files)

59 bytes, text/x-github-pull-request
dimi
: review+
Details | Review
17.01 KB, patch
dimi
: review+
Details | Diff | Splinter Review
11.52 KB, patch
allstars.chh
: review+
Details | Diff | Splinter Review
'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 on attachment 8521168 [details] [review]
nfcd Pull Request

Found something is missing.
Attachment #8521168 - Flags: review?(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;
Attachment #8521165 - Flags: review?(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.
Attachment #8521211 - Flags: review?(dlee) → review+
Attachment #8521213 - Flags: review?(dlee) → review+
Attachment #8521168 - Flags: review?(dlee) → review+
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.
Attachment #8521211 - Flags: review?(bugs)
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.)
Attachment #8521211 - Flags: review?(bugs) → review-
Use enum for RFState in WebIDL.
Attachment #8521211 - Attachment is obsolete: true
remove consts defined in nsINfcContentHelper.idl
Attachment #8521213 - Attachment is obsolete: true
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..
Attachment #8522041 - Flags: review?(dlee)
Attached patch Part 1: v2-v3 diff. (obsolete) — Splinter Review
This is the inter-diff generated from v2 to v3 in Part1.
Attached patch Part 2: v2-v3 diff (obsolete) — Splinter Review
This is the inter-diff for v2 and v3 in Part 2.
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.
Attachment #8522042 - Flags: review?(dlee)
Attachment #8522041 - Flags: review?(dlee) → review+
Attachment #8522042 - Flags: review?(dlee) → review+
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
Attachment #8522041 - Flags: review?(bugs)
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)
Attachment #8522041 - Flags: review?(bugs) → review+
(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.
Add MOZ_ASSERT on mRfState.
Attachment #8522041 - Attachment is obsolete: true
Attachment #8522043 - Attachment is obsolete: true
Attachment #8522045 - Attachment is obsolete: true
Attachment #8522634 - Flags: review+
Whiteboard: [p=2]
Target Milestone: --- → 2.1 S9 (21Nov)
You need to log in before you can comment on or make changes to this bug.