Closed Bug 960510 Opened 6 years ago Closed 6 years ago

B2G NFC: type, id and payload should be optional in MozNDEFRecord

Categories

(Firefox OS Graveyard :: NFC, defect)

ARM
Gonk (Firefox OS)
defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
1.4 S4 (28mar)

People

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

References

Details

Attachments

(3 files, 3 obsolete files)

From NDEF spec, type and id could be optional.

However if we call new MozNdefRecord() with id or type is null, error will happen.
This is also the reason why nfc_manager in Gaia has some many new Uint8Array([]) calls.
Summary: B2G NFC: type and id should be optional in MozNdefRecord → B2G NFC: type and id should be optional in MozNDEFRecord
Assignee: nobody → allstars.chh
From NDEF Spec,
3.2.9 Payload Length
"A payload length of 0 is allowed in which case the PAYLOAD field is omitted from the NDEF record."

Payload could also be optional.
Summary: B2G NFC: type and id should be optional in MozNDEFRecord → B2G NFC: type, id and payload should be optional in MozNDEFRecord
Attachment #8376962 - Flags: review?(dlee) → review+
Attached file Gaia PullRequest
Attachment #8377409 - Flags: review?(alive)
Attachment #8377409 - Flags: review?(alive) → review+
Comment on attachment 8376961 [details] [diff] [review]
Part 1: Make type, id and payload optional

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

You can't call JS::ExposeObjectToActiveJS on things that are null.  Try to get some of the properties you didn't pass in and you'll see the crash.

Please add this to your testcase.
Attachment #8376961 - Flags: review?(khuey) → review-
Thanks for the review, Kyle.
It did crashed. :P

updated the patch and ask for r? again.
Attachment #8376961 - Attachment is obsolete: true
Attachment #8378836 - Flags: review?(khuey)
Test case for accessing members and checking for null.

Given that Part 2 is already r+, so I make it another patch for Kyle to review.
Once it's r+ I will merge two tests into one patch.
Attachment #8378840 - Flags: review?(khuey)
Thanks for the review,
given the new policy I'll push these patches after 2/28.
ni? to myself in case I forgot.
Flags: needinfo?(allstars.chh)
Merged Part 2 patches into one.
Attachment #8376962 - Attachment is obsolete: true
Attachment #8378840 - Attachment is obsolete: true
Attachment #8392655 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/fbc9b3cba3fe
https://hg.mozilla.org/mozilla-central/rev/82e2e931b427
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → 1.4 S4 (28mar)
You need to log in before you can comment on or make changes to this bug.