Closed Bug 1138886 Opened 5 years ago Closed 5 years ago

Structured Clone for MozNDEFRecord

Categories

(Firefox OS Graveyard :: NFC, defect)

ARM
Gonk (Firefox OS)
defect
Not set

Tracking

(firefox39 fixed)

RESOLVED FIXED
2.2 S12 (15may)
Tracking Status
firefox39 --- fixed

People

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

References

Details

(Whiteboard: [p=5])

Attachments

(1 file, 1 obsolete file)

To let MozNDEFRecord can be sent with message manager, I think we have to implement Structured Clone Algorithm for MozNDEFRecord.

The use case will be System app could fire nfc-ndef-discovered MozActivity, with the array of MozNDEFRecords as the data.
Right now it works (records in nfc-ndef-discovered could pass process boundary) because System app use plain JS object. 
{
  tnf: ..,
  type:...
  id:...,
  payload
}

But Bug 1127181 will remove the nfc-manager-tech-discovered system message, so the NDEF messages won't be of plain JS object, so we still need this bug to pass NDEF to other apps.
Attached patch WIP - Patch (obsolete) — Splinter Review
Hi, Smaug

I'd like to ask feedback from you first, this is still a WIP patch

(1). This is the follow up to add Init* methods in MozNDEFRecord since you asked in https://bugzilla.mozilla.org/show_bug.cgi?id=1121298#c19 before.
Although I really didn't reuse them in this bug, but I'd like to get feedback from you the idea to seperate type/id/payload from the cstor.

(2). What's the easy way to the get the length of Uint8Array from JS::Heap<JSObject*> ?  Right now I use js::GetUint8ArrayLengthAndData.

(3). Suggestions for handling null in MozNDEFRecord::WriteStructuredClone,
For example if the id is null, then it will assert in the line of 
JS::Rooted<JS::Value> idValue(aCx, JS::ObjectValue(*id));

Otherwise I might use (2) to use the length to guard those objects.


Thanks for your feedback.
Attachment #8571911 - Flags: feedback?(bugs)
(In reply to Yoshi Huang[:allstars.chh] from comment #1)

> (1). This is the follow up to add Init* methods in MozNDEFRecord since you
> asked in https://bugzilla.mozilla.org/show_bug.cgi?id=1121298#c19 before.
> Although I really didn't reuse them in this bug, but I'd like to get
> feedback from you the idea to seperate type/id/payload from the cstor.
I guess it is fine to hide the typed array handling to a method.


> (2). What's the easy way to the get the length of Uint8Array from
> JS::Heap<JSObject*> ?  Right now I use js::GetUint8ArrayLengthAndData.
I don't know any other way. You might want to ask #jsapi


> (3). Suggestions for handling null in MozNDEFRecord::WriteStructuredClone,
> For example if the id is null, then it will assert in the line of 
> JS::Rooted<JS::Value> idValue(aCx, JS::ObjectValue(*id));
Could you write a boolean flag, and when false, the next thing isn't an array but the flag for the next thing?
Attachment #8571911 - Flags: feedback?(bugs) → feedback+
Attached patch Patch.Splinter Review
Hi Smaug
Thanks for the great feedback.

I've made the patch ready for r?.

One small question is I am not sure if I should also use JSAutoCompartment in MozNDEFRecord::ReadStructuredClone.
I took dom/bindings/StructuredClone.cpp as example but in the file it doesn't use JSAutoCompartment in ReadStructuredClone.

Thanks.
Attachment #8571911 - Attachment is obsolete: true
Attachment #8572548 - Flags: review?(bugs)
Comment on attachment 8572548 [details] [diff] [review]
Patch.


>+  } else if (tag == SCTAG_DOM_NFC_NDEF) {
>+#ifdef MOZ_NFC
>+    nsIGlobalObject *global = xpc::NativeGlobal(JS::CurrentGlobalOrNull(cx));
nsIGlobalObject* glogal


You shouldn't need JSAutoCompartment there.
Attachment #8572548 - Flags: review?(bugs) → review+
(In reply to Olli Pettay [:smaug] (no new review request before March 8, please) from comment #4)
> Comment on attachment 8572548 [details] [diff] [review]
> Patch.
> 
> 
> >+  } else if (tag == SCTAG_DOM_NFC_NDEF) {
> >+#ifdef MOZ_NFC
> >+    nsIGlobalObject *global = xpc::NativeGlobal(JS::CurrentGlobalOrNull(cx));
> nsIGlobalObject* glogal
> 
> 
> You shouldn't need JSAutoCompartment there.

Hi Smaug
sorry I am a little confused here.
My previous question is should I also need 'JSAutoCompartment' in *MozNDEFRecord::ReadStructuredClone* ?

But your comment is commented in NS_DOMReadStructuredClone.

Thanks
You need to log in before you can comment on or make changes to this bug.