Closed Bug 1115465 Opened 5 years ago Closed 5 years ago

Add id attribute to NFCTag

Categories

(Firefox OS Graveyard :: NFC, defect)

ARM
Gonk (Firefox OS)
defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
2.2 S3 (9jan)

People

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

References

Details

(Whiteboard: [p=2])

Attachments

(2 files, 2 obsolete files)

Every NFC tag should have an unique identifier (UID), although for some technology the UID is generated randomly at discovery (for example, NFCID-1 of length 4 used by NFC_A), and for some technology the naming is different, like Felica it's called IDm.

To make it consistent in WebAPI, I am thinking about add an 'id' attribute into NFCTag.
Blocks: b2g-nfcd
No longer blocks: 1082445
Attached patch Patch. (obsolete) — Splinter Review
Attachment #8541623 - Flags: review?(dlee)
Comment on attachment 8541623 [details] [diff] [review]
Patch.

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

forgot updating minor version.
Attachment #8541623 - Flags: review?(dlee)
Attached patch Patch. v2. (obsolete) — Splinter Review
Attachment #8541623 - Attachment is obsolete: true
Attachment #8541697 - Flags: review?(dlee)
Attachment #8541699 - Flags: review?(dlee) → review+
Attachment #8541697 - Flags: review?(dlee) → review+
Comment on attachment 8541697 [details] [diff] [review]
Patch. v2.

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

Forward r? to smaug for WebIDL changes. (MozNFCTag.webidl, NfcOptions.webidl)
Attachment #8541697 - Flags: review?(bugs)
Comment on attachment 8541697 [details] [diff] [review]
Patch. v2.

>+  int32_t idCount = aParcel.readInt32();
>+  aOptions.mTagId.AppendElements(
>+      static_cast<const uint8_t*>(aParcel.readInplace(idCount)), idCount);
Nit, 2 spaces for indentation
Attachment #8541697 - Flags: review?(bugs) → review+
Attached patch Patch. v3.Splinter Review
I also noticed some places doesn't use 2 spaces, so changed them as well.
Attachment #8541697 - Attachment is obsolete: true
Attachment #8542777 - Flags: review+
https://hg.mozilla.org/integration/b2g-inbound/rev/c776f112102f
Whiteboard: [p=2]
Target Milestone: --- → 2.2 S3 (9jan)
The IDL added in this bug:

  [Pure, Constant] readonly attribute Uint8Array? id;

is nonsense.  Is it Pure or Constant?  Those are mutually exclusive things (and I wrote a path that fails codegen when both are specified, but landing that is blocked on the IDL this bug added).
Flags: needinfo?(bugs)
Flags: needinfo?(allstars.chh)
Sorry, should be [Constant]. I'll file another bug to fix this.
Flags: needinfo?(bugs)
Flags: needinfo?(allstars.chh)
(In reply to Boris Zbarsky [:bz] from comment #11)
> The IDL added in this bug:
> 
>   [Pure, Constant] readonly attribute Uint8Array? id;
> 
> is nonsense.  Is it Pure or Constant? 
Bah, sorry. Constant it should be


> Those are mutually exclusive things
How so. Even the documentation for [Constant] says
"This extended attribute implies [Pure] as far as the JIT is concerned." 
So per that [Pure] in this case is just not needed, but shouldn't cause any harm.
> So per that [Pure] in this case is just not needed, but shouldn't cause any harm.

That depends on how the codegen is structured.  It happens to be the case that we check for "Constant" before checking for "Pure", but if we checked in the opposite order you'd get different behavior.

"Constant" means not affecting other things and not depending on other things.  "Pure" means not affecting other things but depending on other things. When both are specified, do we depend on other things?

Anyway, the DependsOn/Affects stuff I was trying to land makes that all much clearer, and I wanted to make sure people didn't specify DependsOn in combination with Pure and such, precisely because the results end up ambiguous.
Might be good if MDN documentation about [Constant] wouldn't say anything about 'implies [Pure]'.
Yes, once I land bug 1118978 I'll update the documentation to be clearer.
Thanks.
You need to log in before you can comment on or make changes to this bug.