1053732 - B2G NFC: Use dictionary in MozNDEFRecord Constructor

Status: RESOLVED FIXED

(Firefox OS Graveyard :: NFC, defect)

Description

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi] OOO ~ Feb.7]

In Bug 960510 we make the type, id and payload parameters optional in MozNDEFRecord Constructor, part of this also because W3C NFC API also make them optional [1].
So we could simply call 'new MozNDEFRecord(tnf)' for the the EMPTY NDEFREcord.

However in most cases the 'id' paramter is null, so many developers will do 
new MozNDEFRecord(tnf, typeArray, undefined, payloadArray) or 
new MozNDEFREcord(tnf, typeArray, new Uint8Array(0), payloadArray).

But the better way is to make it nullable, so we can do
new MozNDEFRecord(tnf, typeArray, null, payloadArray);

So I am wondering should we make these parameters Optional and Nullable? 
[Constructor(octet tnf, optional Uint8Array? type, optional Uint8Array? id, optional Uint8Array? payload)]
interface MozNDEFRecord


Or simply Nullable is enough?
[Constructor(octet tnf, Uint8Array? type, Uint8Array? id, Uint8Array? payload)]
interface MozNDEFRecord


Request feedback from smuag for this, 
@smaug, can you provide some suggestions for us?

Thank you. 


[1]: http://w3c.github.io/nfc/proposals/common/nfc.html#ndefrecord-interface

Comment 1

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

I would put all the optional stuff to a dictionary.

dictionary NDEFRecordOptions {
  optional Uint8Array? type,
  optional Uint8Array? id,
  optional Uint8Array? payload 
}

[Constructor(TNF tfn, optional NDEFRecordOptions options)]
interface MozNDEFRecord {
...
}

then one could say: new MozNDEFRecord(some_tnf);
or: new MozNDEFRecord(some_tnf, { palyload: some_array})
etc.

Comment 2

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

Oh, those properties in the dictionary don't need to be nullable, I think, so drop '?'.
Properties in the dictionary are optional by default.

Comment 3

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi] OOO ~ Feb.7]

(In reply to Olli Pettay [:smaug] from comment #1)
> I would put all the optional stuff to a dictionary.
> 
> dictionary NDEFRecordOptions {
>   optional Uint8Array? type,
>   optional Uint8Array? id,
>   optional Uint8Array? payload 
> }
> 
> [Constructor(TNF tfn, optional NDEFRecordOptions options)]
> interface MozNDEFRecord {
> ...
> }
> 
> then one could say: new MozNDEFRecord(some_tnf);
> or: new MozNDEFRecord(some_tnf, { palyload: some_array})
> etc.

Thanks smaug, this is even better.
I'd work on this proposal.

But maybe I will move tnf into the dict as well, and let it have default value.

dictionary NDEFRecordOptions {
  octet tnf = 0; // default to tnf_empty
  optional Uint8Array type;
  optional Uint8Array id;
  optional Uint8Array payload;
};

Also I noticed you used 'TNF' in the constructor, I'd check if we should use use enum for 'tnf' and file another bug for this.

Comment 4

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi] OOO ~ Feb.7]

Attachment: Part 1: DOM Change

Comment 5

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi] OOO ~ Feb.7]

Attachment: Part 2: NfcService update

Comment 6

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi] OOO ~ Feb.7]

Attachment: Part 3: Impl and test cases update

Comment 7

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi] OOO ~ Feb.7]

https://tbpl.mozilla.org/?tree=Try&rev=e40b11ba22e7

Will send Gaia PR once Gecko patches are r+.

Comment 8

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi] OOO ~ Feb.7]

(In reply to Yoshi Huang[:allstars.chh] from comment #3)
> Also I noticed you used 'TNF' in the constructor, I'd check if we should use
> use enum for 'tnf' and file another bug for this.
Filed Bug 1055560

Comment 9

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

Comment on attachment 8475727 [details] [diff] [review]
Part 2: NfcService update

Oh, wait, why we need both
MozNDEFRecordOptions and NDEFRecord dictionaries?
which both have the same properties, just a bit different types
(some nullable)

We should be able to have just one, which works in all the needed cases.

Comment 10

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi] OOO ~ Feb.7]

(In reply to Olli Pettay [:smaug] from comment #9)
> We should be able to have just one, which works in all the needed cases.
Hi Smaug,
Yeah, Kyle also asked this when he reviewed it.
https://bugzilla.mozilla.org/show_bug.cgi?id=933588#c32

=== Quoted from Kyle ===
::: dom/system/gonk/NfcOptions.h
@@ +8,5 @@
> +#include "mozilla/dom/NfcOptionsBinding.h"
> +
> +namespace mozilla {
> +
> +struct NDEFRecordStruct

Why can't we just use the generated DOM dictionaries here?  Is it because of the JS values?

=== End of Quote. ===

However I tried in https://bugzilla.mozilla.org/show_bug.cgi?id=933588#c37
Because the constructor of generated DOM dict is private, to pass it to nsRunnable I have to construct it manually again.

Or there's any suggestion to pass it to nsRunnable?

Thanks

Comment 11

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi] OOO ~ Feb.7]

Kyle is asking in the header file but Smaug is asking on the WebIDL.
I'll file another bug for removing NDEFRecord from NfcOptions.webidl.

Comment 12

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi] OOO ~ Feb.7]

Attachment: Part 2: NfcService update v2.

Also updated NfcContentHelper as record.id could be null, and the value in our dict cannot be nullable.

Comment 13

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi] OOO ~ Feb.7]

Comment on attachment 8479695 [details] [diff] [review]
Part 2: NfcService update v2.

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

Hi, Smaug
Sorry for asking wrong question to you before.
Now this patch has addressed your Comment 9.

Could you help to review this again for me ?

Thank you.

Comment 14

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi] OOO ~ Feb.7]

Attachment: Gaia Pull Request

Comment 15

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi] OOO ~ Feb.7]

Comment on attachment 8480419 [details] [review]
Gaia Pull Request

Gaia PR for updating MozNDEFRecord.

@Ben, could you help review Part 1: Browser app change for me?

@Francisco, could you help to review Part 2: Contacts app change ?

And
@Alive, could you help to review Part 3: System app change ?

Thank you

Comment 16

[Ben Francis [:benfrancis]]

Comment on attachment 8480419 [details] [review]
Gaia Pull Request

Is this for 2.0? The browser app is being deleted in 2.1.

Comment 17

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi] OOO ~ Feb.7]

(In reply to Ben Francis [:benfrancis] from comment #16)
> Comment on attachment 8480419 [details] [review]
> Gaia Pull Request
> 
> Is this for 2.0? The browser app is being deleted in 2.1.
No, not for 2.0. It's should be landed on the master(currently it's 2.1)
I don't know when the browser app will be removed, if it's removed when I try to merge this, I'll remove this commit.

Thanks

Comment 18

[Francisco Jordano [:arcturus] [:francisco]]

Comment on attachment 8480419 [details] [review]
Gaia Pull Request

Contact change looking good, thx!

Comment 19

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi] OOO ~ Feb.7]

Attachment: Part 2: NfcService update v3

rebase

Comment 20

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi] OOO ~ Feb.7]

https://hg.mozilla.org/integration/b2g-inbound/rev/f7dd432d3702
https://hg.mozilla.org/integration/b2g-inbound/rev/524bf2f003b0
https://hg.mozilla.org/integration/b2g-inbound/rev/443b75b59926

Try:
https://tbpl.mozilla.org/?tree=Try&rev=e44b66fad770

Comment 21

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi] OOO ~ Feb.7]

Update Gaia PR
Gaia Try result
https://tbpl.mozilla.org/?rev=52f361805243167c3b79838074e0d7448b063f68&tree=Gaia-Try

Comment 22

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi] OOO ~ Feb.7]

Add checkin-needed for Gaia commit since I'd like to land Gaia part only when Gecko part lands in m-c.

Thanks

Comment 23

[Ryan VanderMeulen [:RyanVM]]

Master: https://github.com/mozilla-b2g/gaia/commit/b1f8dc3c759dcc2c9fde4619b85aee8da97601a9

Comment 24

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/f7dd432d3702
https://hg.mozilla.org/mozilla-central/rev/524bf2f003b0
https://hg.mozilla.org/mozilla-central/rev/443b75b59926