1000935 - [NFC] Support sending NDEF messages on Emulator

Status: RESOLVED FIXED

(Firefox OS Graveyard :: NFC, defect)

Description

[Thomas Zimmermann [:tzimmermann] [:tdz]]

This is the send part of bug 980851.

Comment 1

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Attachment: Github pull request

This implements the whole protocol stack for sending NDEF messages over LLCP, and most of the primitives for receiving.

Comment 2

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Attachment: [01] Bug 1000935: Added NfcUtils

Copied from Gaia.

Comment 3

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Attachment: [02] Bug 1000935: Added Marionette test for NfcPeer.sendNDEF

This test case uses most of the protocol stack in the pull request.

Comment 4

[Vicamo Yang [:vicamo][:vyang]]

Comment on attachment 8411880 [details] [diff] [review]
[01] Bug 1000935: Added NfcUtils

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

::: dom/nfc/tests/marionette/head.js
@@ +87,5 @@
> +
> +/* -*- Mode: js; js-indent-level: 2; indent-tabs-mode: nil -*- */
> +/* vim: set shiftwidth=2 tabstop=2 autoindent cindent expandtab: */
> +
> +/* Copyright © 2013, Deutsche Telekom, Inc. */

Hi, I don't think we can merge a patch that explicitly says its copyright is not owned by Mozilla or not being MPL/CC compatible.  Actually, this file has already copyright declaration right on the first line. http://mxr.mozilla.org/mozilla-central/source/dom/nfc/tests/marionette/head.js#1

Comment 5

[Thomas Zimmermann [:tzimmermann] [:tdz]]

(In reply to Vicamo Yang [:vicamo][:vyang] from comment #4)
> Comment on attachment 8411880 [details] [diff] [review]
> [01] Bug 1000935: Added NfcUtils
> 
> Review of attachment 8411880 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/nfc/tests/marionette/head.js
> @@ +87,5 @@
> > +
> > +/* -*- Mode: js; js-indent-level: 2; indent-tabs-mode: nil -*- */
> > +/* vim: set shiftwidth=2 tabstop=2 autoindent cindent expandtab: */
> > +
> > +/* Copyright © 2013, Deutsche Telekom, Inc. */
> 
> Hi, I don't think we can merge a patch that explicitly says its copyright is
> not owned by Mozilla or not being MPL/CC compatible.

That is not strictly a problem, I think. I copied the license statement and code below from directly Gaia. [1] But I'm not really happy with the situation either. Maybe we can add the code under Public Domain here if the DT guys don't mind.

Garner, are you OK with me adding the code of [1] as Public Domain here?

[1] https://github.com/mozilla-b2g/gaia/blob/master/shared/js/nfc_utils.js

Comment 6

[Garner Lee]

I believe Mozilla Public License can be added explicitly (and probably made consistent elsewhere as well), if MPL is not already inherited from the associated parent repository license. I'll go check through the proper channels if it can be even more explicitly open for Utility files and test cases (full Public Domain).

Comment 7

[Vicamo Yang [:vicamo][:vyang]]

(In reply to Thomas Zimmermann [:tzimmermann] [:tdz] from comment #5)
> (In reply to Vicamo Yang [:vicamo][:vyang] from comment #4)
> > Hi, I don't think we can merge a patch that explicitly says its copyright is
> > not owned by Mozilla or not being MPL/CC compatible.
> 
> That is not strictly a problem, I think. I copied the license statement and
> code below from directly Gaia. [1] But I'm not really happy with the
> situation either.

That only means the committer/reviewer of the pull request doesn't pay enough attention to software license agreements.

This file has been released under CC Public Domain and I don't feel it necessary to change.  Adding a line to claim DT owns the copyright is really ambiguous here.  How can you dedicate the rights you're holding to public domain while owning them?

Comment 8

[Thomas Zimmermann [:tzimmermann] [:tdz]]

(In reply to Vicamo Yang [:vicamo][:vyang] from comment #7)
> (In reply to Thomas Zimmermann [:tzimmermann] [:tdz] from comment #5)
> > (In reply to Vicamo Yang [:vicamo][:vyang] from comment #4)
> > > Hi, I don't think we can merge a patch that explicitly says its copyright is
> > > not owned by Mozilla or not being MPL/CC compatible.
> > 
> > That is not strictly a problem, I think. I copied the license statement and
> > code below from directly Gaia. [1] But I'm not really happy with the
> > situation either.
> 
> That only means the committer/reviewer of the pull request doesn't pay
> enough attention to software license agreements.

No. The license discussion came up when the NFC code landed in the first place in bug 674741. Apparently it was decided that Mozilla doesn't need to hold copyright.

> 
> This file has been released under CC Public Domain and I don't feel it
> necessary to change.  Adding a line to claim DT owns the copyright is really
> ambiguous here.  How can you dedicate the rights you're holding to public
> domain while owning them?

If you're the owner of the code, you can license it in any way you what. Dual-licensing is nothing uncommon.

Comment 9

[Thomas Zimmermann [:tzimmermann] [:tdz]]

BTW, is there a way to specify more than one file in MARIONETTE_HEAD_JS? Having multiple files with individual licenses could ease such license mess in the future.

Comment 10

[Dimi Lee [ooo 16.04 - 21.04］]

Comment on attachment 8411883 [details] [diff] [review]
[02] Bug 1000935: Added Marionette test for NfcPeer.sendNDEF

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

Pass local test ! r+ with nits fixed

::: dom/nfc/tests/marionette/test_nfc_peer_sendndef.js
@@ +3,5 @@
> +
> +MARIONETTE_TIMEOUT = 30000;
> +MARIONETTE_HEAD_JS = 'head.js';
> +
> +let url = 'https://www.example.com';

From MDN coding style and checked other test cases, double-quoted strings are preferred.
To sync our coding style, could you help modify here and below to use double-quoted literals, thanks

@@ +46,5 @@
> +  return ndef;
> +}
> +
> +function sendNDEF(techType, sessionToken) {
> +  let tnf = 1;

Could use TNF_WELL_KNOWN defined in head.js

@@ +52,5 @@
> +  let id = new Uint8Array(NfcUtils.fromUTF8(''));
> +  let payload = new Uint8Array(NfcUtils.fromUTF8(url));
> +  let ndef = [new MozNDEFRecord(tnf, type, id, payload)];
> +
> +  let peer = window.navigator.mozNfc.getNFCPeer(sessionToken);

After bug 987596 is land, we could use nfc directly instead of window.navigator.mozNfc

@@ +103,5 @@
> +SpecialPowers.pushPermissions(
> +  [{'type': 'nfc', 'allow': true,
> +                   'read': true, 'write': true, context: document},
> +   {'type': 'nfc-manager', 'allow': true, context: document},
> +   {'type': 'settings', 'allow': true, context: document}], runTests);

settings permission is not required now.

Comment 11

[Vicamo Yang [:vicamo][:vyang]]

(In reply to Thomas Zimmermann [:tzimmermann] [:tdz] from comment #9)
> BTW, is there a way to specify more than one file in MARIONETTE_HEAD_JS?
> Having multiple files with individual licenses could ease such license mess
> in the future.

That was landed in bug 805838.  One can always send a patch to allow multiple, even nested MARIONETTE_HEAD_JS labels if necessary.

Comment 12

[Vicamo Yang [:vicamo][:vyang]]

(In reply to Thomas Zimmermann [:tzimmermann] [:tdz] from comment #8)
> (In reply to Vicamo Yang [:vicamo][:vyang] from comment #7)
> > This file has been released under CC Public Domain and I don't feel it
> > necessary to change.  Adding a line to claim DT owns the copyright is really
> > ambiguous here.  How can you dedicate the rights you're holding to public
> > domain while owning them?
> 
> If you're the owner of the code, you can license it in any way you what.
> Dual-licensing is nothing uncommon.

I mean the word "dedicate" used in CC Public Domain literally disallow owning copyright by the author at the same time.  Wikipedia has a more clearer definition: "Works in the public domain are those whose intellectual property rights have expired, have been forfeited, or are inapplicable."  The original license term also has "A certifier, moreover, dedicates any copyright interest he may have in the associated work, and for these purposes."  One may claim the authorship of a piece of Public Domain work, but he no longer owns copyright because, by Public Domain terms, he has given up the copyright related to the work.

http://creativecommons.org/licenses/publicdomain/

Comment 13

[Vicamo Yang [:vicamo][:vyang]]

Comment on attachment 8411877 [details]
Github pull request

Hi, a few questions, typo on Github.  That's really an awesome job!  Thank you.

Comment 14

[Ken Chang[:kenkai]]

This bug blocks NFC 2.0 release.

Comment 15

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Comment on attachment 8411877 [details]
Github pull request

Updated github pull request

  - rebased on top of bug 996426
  - added white spaces around operators
  - fixed typo 'nsep'

Comment 16

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Attachment: [02] Bug 1000935: Added Marionette test for NfcPeer.sendNDEF (v2)

Changes since v1:

  - replaced single quotes by double quotes
  - use NDEF.TNF_WELL_KNOWN
  - don't acquire settings permission
  - replace remaining 'var' by 'let'

I didn't change |window.navigator.mozNfc| to |nfc|. IMHO that variable mostly adds confusion and needs a cleanup first to be more discoverable.

Comment 17

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Attachment: [01] Bug 1000935: Added NDEF helpers for NFC test cases (v2)

I reimplemented the required utility functions from scratch to solve the licensing problems for now. It's not much extra code.

Comment 18

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Attachment: [02] Bug 1000935: Added Marionette test for NfcPeer.sendNDEF (v3)

Changes since v2:

  - rebased onto bug 996426

Comment 19

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Comment on attachment 8411877 [details]
Github pull request

Hi Vicamo,

I updated the pull request as reported in comment 15 and rebased once more, now that bug 996426 has landed. If there's nothing else, I'd like to get the patches merged.

Comment 20

[Vicamo Yang [:vicamo][:vyang]]

Comment on attachment 8411877 [details]
Github pull request

I'll merge it after https://tbpl.mozilla.org/?tree=B2g-Inbound&rev=14132a1a89dd

Comment 21

[Vicamo Yang [:vicamo][:vyang]]

Jellybean:
https://github.com/mozilla-b2g/platform_external_qemu/commit/e16b5c17131d2296101b24c87887c9ccf057649f

KitKat:
https://github.com/mozilla-b2g/platform_external_qemu/commit/265db49431fed74e1887f8dcf083a3b3f3550031

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

Comment 22

[Vicamo Yang [:vicamo][:vyang]]

http://hg.mozilla.org/mozilla-central/rev/420f4c65a67f

Comment 23

[Vicamo Yang [:vicamo][:vyang]]

Attachment: [02] Bug 1000935: Added Marionette test for NfcPeer.sendNDEF (v4)

Rebase after bug 996426.

Comment 24

[Vicamo Yang [:vicamo][:vyang]]

https://hg.mozilla.org/integration/b2g-inbound/rev/bf6874a80bbf
https://hg.mozilla.org/integration/b2g-inbound/rev/1548cd4a5895

Comment 25

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Thanks a lot, Vicamo!

Comment 26

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/bf6874a80bbf
https://hg.mozilla.org/mozilla-central/rev/1548cd4a5895