1138886 - Structured Clone for MozNDEFRecord

Status: RESOLVED FIXED

(Firefox OS Graveyard :: NFC, defect)

Description

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

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.

Comment 1

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

Attachment: WIP - Patch

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.

Comment 2

[Olli Pettay [:smaug][[e-mail]]]

(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?

Comment 3

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

Attachment: Patch.

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.

Comment 4

[Olli Pettay [:smaug][[e-mail]]]

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.

Comment 5

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

(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

Comment 6

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

https://hg.mozilla.org/integration/b2g-inbound/rev/adab48bc6a47

Comment 7

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

https://hg.mozilla.org/mozilla-central/rev/adab48bc6a47