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

Status: RESOLVED FIXED

(Firefox OS Graveyard :: NFC, defect)

Description

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

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.

Comment 1

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

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.

Comment 2

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Attachment: Part 1: Make type, id and payload optional

Comment 3

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Attachment: Part 2: marionette tests for MozNDEFRecord.

Comment 4

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Attachment: Gaia PullRequest

Comment 5

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

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.

Comment 6

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Attachment: Part 1: Make type, id and payload optional v2.

Thanks for the review, Kyle.
It did crashed. :P

updated the patch and ask for r? again.

Comment 7

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Attachment: Part 2-2 : test for accessing members and checking for null.

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.

Comment 8

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Thanks for the review,
given the new policy I'll push these patches after 2/28.
ni? to myself in case I forgot.

Comment 9

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Attachment: Part 2: marionette tests for MozNDEFRecord. v2

Merged Part 2 patches into one.

Comment 10

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

https://hg.mozilla.org/integration/b2g-inbound/rev/fbc9b3cba3fe
https://hg.mozilla.org/integration/b2g-inbound/rev/82e2e931b427

Comment 11

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Gaia patch merged.
https://github.com/mozilla-b2g/gaia/commit/2c46348849db3dc39b25010aad334252ba9c5e59

Comment 12

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/fbc9b3cba3fe
https://hg.mozilla.org/mozilla-central/rev/82e2e931b427