Closed
Bug 1096897
Opened 8 years ago
Closed 8 years ago
replace powerLevel to rfState.
Categories
(Firefox OS Graveyard :: NFC, defect)
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)
'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.
Assignee | ||
Comment 1•8 years ago
|
||
Assignee | ||
Comment 2•8 years ago
|
||
Assignee | ||
Updated•8 years ago
|
Attachment #8521165 -
Flags: review?(dlee)
Assignee | ||
Updated•8 years ago
|
Attachment #8521166 -
Flags: review?(dlee)
Assignee | ||
Comment 3•8 years ago
|
||
Attachment #8521168 -
Flags: review?(dlee)
Assignee | ||
Comment 4•8 years ago
|
||
Comment on attachment 8521168 [details] [review] nfcd Pull Request Found something is missing.
Attachment #8521168 -
Flags: review?(dlee)
Comment 5•8 years ago
|
||
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)
Comment 6•8 years ago
|
||
(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.
Assignee | ||
Updated•8 years ago
|
Attachment #8521166 -
Flags: review?(dlee)
Assignee | ||
Comment 7•8 years ago
|
||
Attachment #8521165 -
Attachment is obsolete: true
Assignee | ||
Comment 8•8 years ago
|
||
Attachment #8521166 -
Attachment is obsolete: true
Assignee | ||
Updated•8 years ago
|
Attachment #8521211 -
Flags: review?(dlee)
Assignee | ||
Updated•8 years ago
|
Attachment #8521213 -
Flags: review?(dlee)
Assignee | ||
Updated•8 years ago
|
Attachment #8521168 -
Flags: review?(dlee)
Updated•8 years ago
|
Attachment #8521211 -
Flags: review?(dlee) → review+
Updated•8 years ago
|
Attachment #8521213 -
Flags: review?(dlee) → review+
Updated•8 years ago
|
Attachment #8521168 -
Flags: review?(dlee) → review+
Assignee | ||
Comment 9•8 years ago
|
||
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 10•8 years ago
|
||
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-
Assignee | ||
Comment 11•8 years ago
|
||
Use enum for RFState in WebIDL.
Attachment #8521211 -
Attachment is obsolete: true
Assignee | ||
Comment 12•8 years ago
|
||
remove consts defined in nsINfcContentHelper.idl
Attachment #8521213 -
Attachment is obsolete: true
Assignee | ||
Comment 13•8 years ago
|
||
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)
Assignee | ||
Comment 14•8 years ago
|
||
This is the inter-diff generated from v2 to v3 in Part1.
Assignee | ||
Comment 15•8 years ago
|
||
This is the inter-diff for v2 and v3 in Part 2.
Assignee | ||
Comment 16•8 years ago
|
||
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)
Updated•8 years ago
|
Attachment #8522041 -
Flags: review?(dlee) → review+
Updated•8 years ago
|
Attachment #8522042 -
Flags: review?(dlee) → review+
Assignee | ||
Comment 17•8 years ago
|
||
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 18•8 years ago
|
||
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+
Assignee | ||
Comment 19•8 years ago
|
||
merged nfcd PR, since it only involves renaming. https://github.com/mozilla-b2g/platform_system_nfcd/commit/9663d24afcda9f96146185972ade780714727beb
Assignee | ||
Comment 20•8 years ago
|
||
(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.
Assignee | ||
Comment 21•8 years ago
|
||
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+
Assignee | ||
Comment 22•8 years ago
|
||
https://hg.mozilla.org/integration/b2g-inbound/rev/65257e4d29bb https://hg.mozilla.org/integration/b2g-inbound/rev/d889fed1d819
Assignee | ||
Updated•8 years ago
|
Whiteboard: [p=2]
Target Milestone: --- → 2.1 S9 (21Nov)
Comment 23•8 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/65257e4d29bb https://hg.mozilla.org/mozilla-central/rev/d889fed1d819
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•