Closed Bug 1055560 Opened 6 years ago Closed 6 years ago

B2G NFC: enum TNF in MozNDEFRecord.webidl

Categories

(Firefox OS Graveyard :: NFC, defect)

ARM
Gonk (Firefox OS)
defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
2.1 S4 (12sep)

People

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

References

Details

(Whiteboard: [p=3])

Attachments

(4 files, 4 obsolete files)

TNF byte right now is 0x00 ~ 0x07, if it's reasonable we could try to make it as enum.
Whiteboard: [good first bug]
Whiteboard: [good first bug]
Assignee: nobody → allstars.chh
Blocks: 1007724
Comment on attachment 8475850 [details] [diff] [review]
Part 1: DOM Change

Hi Smaug
I use different style as opposed to W3C NFC API, http://w3c.github.io/nfc/proposals/common/nfc.html#idl-def-TNF, 
and the reason is in our dom/webidl, most enums start with lower case.

Also for the 'uri'(all lower case, W3C NFC API uses all upper case), current;y in our WebIDL there are 'document-uri', and 'blocked-uri' from CSPReport.webidl, so I use the same naming rule.

Could you help to review this for me ?

Thank you
Attachment #8475850 - Flags: review?(bugs)
Attachment #8475850 - Flags: review?(bugs) → review+
Comment on attachment 8479820 [details] [diff] [review]
Part 2: NfcService change v2

>+    record.mTnf = static_cast<TNF>(tnf);
Could you assert that tnf is < TNF::EndGuard_, if possible.
(I'm not sure what kind of code MOZ_BEGIN_ENUM_CLASS ends up creating for Webidl enums.)
Attachment #8479820 - Flags: review?(bugs) → review+
Attachment #8476586 - Flags: review?(dlee) → review+
Attached patch Part 2: NfcService change v3 (obsolete) — Splinter Review
add assert for tnf.
Attachment #8479820 - Attachment is obsolete: true
Attachment #8480317 - Flags: review+
Attached file Gaia Pull Request
Hi, Alive
This patch is base on Bug 1053732, so this PR contains 3 commits from Bug 1053732.

Send r? to you to review the commit 'Bug 1055560 - enum TNF in MozNDEFRecord.'

Thanks
Attachment #8480428 - Flags: review?(alive)
Comment on attachment 8475850 [details] [diff] [review]
Part 1: DOM Change

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

::: dom/webidl/MozNDEFRecord.webidl
@@ +12,5 @@
> +  "absolute-uri",
> +  "external",
> +  "unknown",
> +  "unchanged",
> +  "reserved"

Just start working on Bug 1007724, From clause 3.3, NDEF spec 1.0.
there's a requirement that states:

"The TNF value MUST NOT be 0x07.(reserved)"

So I'll remove 'reserved' from this enum.
Comment on attachment 8480428 [details] [review]
Gaia Pull Request

Defer review..
Attachment #8480428 - Flags: review?(alive) → review?(gweng)
removed 'reserved'
Attachment #8475850 - Attachment is obsolete: true
Attachment #8481144 - Flags: review+
Comment on attachment 8480428 [details] [review]
Gaia Pull Request

Thanks, bug I've some questions left on GitHub page. So set review again if the patch get ready again. And please remember to squash those commits to one commit before you land the patch.
Attachment #8480428 - Flags: review?(gweng)
Comment on attachment 8480428 [details] [review]
Gaia Pull Request

Sorry Greg,
I have mentioned in Comment 8 to ask review the commit 'Bug 1055560 - enum TNF in MozNDEFRecord.'

Your review is on Bug 1053732, which is not this bug.

Can you review the commit again?
(https://github.com/allstarschh/gaia/commit/0a3a1a6748835d576608a2e939e06202306afe50)

Thank you
Attachment #8480428 - Flags: review?(gweng)
Comment on attachment 8480428 [details] [review]
Gaia Pull Request

Oh okay. The patch comes without issues I think.
Attachment #8480428 - Flags: review?(gweng) → review+
Comment on attachment 8480317 [details] [diff] [review]
Part 2: NfcService change v3

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

::: dom/nfc/gonk/NfcMessageHandler.cpp
@@ +308,5 @@
>    for (int i = 0; i < recordCount; i++) {
>      int32_t tnf = aParcel.readInt32();
>      NDEFRecordStruct record;
> +    record.mTnf = static_cast<TNF>(tnf);
> +    MOZ_ASSERT(record.mTnf < TNF::EndGuard_);

Will move this assert to NfcService since in Bug 1059168 will do the assert check in NfcService because the techList is contained in the byte array and not checkable in NfcMessageHandler.
Move MOZ_ASSSERT to NfcService
Attachment #8480317 - Attachment is obsolete: true
Attachment #8487004 - Flags: review+
You need to log in before you can comment on or make changes to this bug.