1046554 - B2G NFC: pass NFCPeerEvent in onpeerready

Status: RESOLVED FIXED

(Firefox OS Graveyard :: NFC, defect)

Description

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

Right now in onpeerready() callback, the event is CustomEvent, with the 'detail' property is the sessionToken. We should pass a NFCPeerEvent in onpeerready, and we can get MozNFCPeer object in the 'peer' property.

Comment 2

[Krzysztof Mioduszewski[:tauzen]]

This is a really good solution. It's similar to what W3C onpeerfound is doing. Once this will be ready I can fix the gaia apps to use this instead of getNFCPeer.

Comment 3

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

Attachment: 0001-Add-MozNFCPeerEvent.patch

Comment 4

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

Comment on attachment 8465315 [details] [diff] [review]
0001-Add-MozNFCPeerEvent.patch

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

Hi, Smaug
I'd like to ask how should I make the 'peer' property *not* nullable in MozNFCPeerEvent.webidl?

If I try 

[Constructor(DOMString type, optional MozNFCPeerEventInit eventInitDict)]
interface MozNFCPeerEvent : Event
{
  /**
   * The detected NFCPeer.
   */
  readonly attribute MozNFCPeer peer;
};

dictionary MozNFCPeerEventInit : EventInit
{
  MozNFCPeer peer;
};

I'll get compilation error 
no match for 'operator=' in 'e.nsRefPtr<T>::operator-><mozilla::dom::MozNFCPeerEvent>()->mozilla::dom::MozNFCPeerEvent::mPeer = aEventInitDict.mozilla::dom::MozNFCPeerEventInit::mPeer'

I guess I should add some CopyConstructor in MozNFCPeer.webidl?

Sorry I cannot find MDN for WebIDL event generator, so ask your feedback for this.

Thanks

Comment 5

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

non-nullable peer property sounds like a bug.
If it is not nullable, what could peer value be in case
the event was created using
var e = new MozNFCPeerEvent("foobar");
Note, passing a dictionary to event ctor is optional.

So, as far as I see, non-nullable just doesn't make sense.

Comment 6

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

Oh, and if w3c spec requires non-null, file a spec bug, please.

Comment 7

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

Comment on attachment 8465315 [details] [diff] [review]
0001-Add-MozNFCPeerEvent.patch

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

Hi, smaug
The filed bug is Bug 1047187.
Given I didn't update my patch at all, so send r? directly.

And try looks green.
https://tbpl.mozilla.org/?tree=Try&rev=9fc2be6a5f18

I'll update the subject in next version of my patch.

Thanks

Comment 8

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

Attachment: Gaia PullRequest

There 5 commits in this Pull Request, one for each app.

@Dale, could you help to review the Browser part for me?

@Francisco, could you help to review the Contacts part for me?

@David, could you help to review the Video and Gallery parts for me ?

and 
@Dominic, coud you help to review the Music part ?

Thank you all ~~

Comment 9

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

Comment on attachment 8466059 [details] [review]
Gaia PullRequest

Pretty elegant solution :), didn't try on the device but the contacts change is really simple.

Comment 10

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

Comment on attachment 8465315 [details] [diff] [review]
0001-Add-MozNFCPeerEvent.patch

We should expose MozNFCPeerEvent only if Navigator::HasNFCSupport returns true,
so add Func="Navigator::HasNFCSupport" to interface MozNFCPeerEvent.

Comment 11

[David Flanagan [:djf]]

Comment on attachment 8466059 [details] [review]
Gaia PullRequest

Looks good to me, and I'm setting feedback+ but I'm transferring the review request to Punam who is our NFC expert.  Punam: note that this API change will affect the patch you investigated to gallery/js/open.js

Comment 12

[Punam Dahiya [:pdahiya]]

Comment on attachment 8466059 [details] [review]
Gaia PullRequest

Thanks David, will update open.js once gecko fix lands and event.peer is available. 

The gaia NFC changes looks simple and good, however I am not able to test on device as the respective gecko patch (attachment 8465315 [details] [diff] [review]) hasn't  landed. On Flame latest master build,  gaia patch gives error 'JavaScript Error: "TypeError: event.peer is undefined"' .

It has my r+ , assuming developer has tested it with gecko patch.

Comment 13

[Dominic Kuo [:dkuo]]

Comment on attachment 8466059 [details] [review]
Gaia PullRequest

Simple patch and looks good to me!
(Redirect the video patch to the correct peer, John Hu)

Comment 14

[Dale Harvey (:daleharvey)]

Comment on attachment 8466059 [details] [review]
Gaia PullRequest

Looks good, thanks

Comment 15

[John Hu [:johnhu][:johu][:醬糊小弟]]

Comment on attachment 8466059 [details] [review]
Gaia PullRequest

Thanks. The event.peer is better.

Comment 16

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

Attachment: Patch v2.

Add HasNFCSupport in MozNFCPeerEvent.webidl.

Comment 17

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

(In reply to Punam Dahiya from comment #12)
> It has my r+ , assuming developer has tested it with gecko patch.
Yes, I've verified this manually in my local build.

Comment 18

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

(In reply to Olli Pettay [:smaug] from comment #6)
> Oh, and if w3c spec requires non-null, file a spec bug, please.
I can't find their bug system, or it's not working now, so I asked it here
http://lists.w3.org/Archives/Public/public-nfc/2014Aug/0000.html

Comment 19

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

Comment on attachment 8467582 [details] [diff] [review]
Patch v2.

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

::: dom/tests/mochitest/general/test_interfaces.html
@@ +687,5 @@
>      {name: "MozNDEFRecord", b2g: true},
>  // IMPORTANT: Do not change this list without review from a DOM peer!
>      {name: "MozNFCPeer", b2g: true},
>  // IMPORTANT: Do not change this list without review from a DOM peer!
> +    {name: "MozNFCPeerEvent", b2g: true},

need to add permission:"nfc-write" here.

Comment 20

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

Attachment: Patch v3.

Comment 21

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

Try on B2G emulator
https://tbpl.mozilla.org/?tree=Try&rev=be499e0fe287

Will run a full try again to make sure this doesn't break.

Comment 22

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

Gecko patch,
https://hg.mozilla.org/integration/b2g-inbound/rev/b2ef6a444640

I'll try to land Gaia patches as close as possible when gecko patch is landed on m-c.

Comment 23

[Ed Morley [:emorley]]

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

Comment 24

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

Gaia patch.
https://github.com/mozilla-b2g/gaia/commit/4b82145c4859427e0689e01e107a1ab5f0f97f67

Comment 25

[Ed Morley [:emorley]]

Please can you land a followup for:
https://tbpl.mozilla.org/php/getParsedLog.php?id=45331132&tree=B2g-Inbound

07:15:59     INFO -  Running jshint...
07:18:46    ERROR - TEST-UNEXPECTED-FAIL | apps/browser/js/nfc.js | line 93, col 15, 'nfcdom' is defined but never used.
07:18:46     INFO -  apps/browser/js/nfc.js: line 93, col 15, 'nfcdom' is defined but never used. (ERROR)
07:18:46     INFO -  1 error (12254 xfailed)
07:18:46     INFO -  Please consult https://github.com/mozilla-b2g/gaia/tree/master/build/jshint/README.md to get some information about how to fix jshint issues.
07:18:46     INFO - TEST-UNEXPECTED-FAIL | make lint | make[1]: *** [hint] Error 1

Comment 26

[Kevin Grandon :kgrandon]

Follow-up landed to fix linter: https://github.com/mozilla-b2g/gaia/commit/25701a4f9fc4bd1d1dc4a1d4bedfe66ee8c94502

Comment 27

[Kevin Grandon :kgrandon]

Reverted for Gu failures: https://tbpl.mozilla.org/?tree=B2g-Inbound&rev=794d3a271fc4

Linter follow-up revert: https://github.com/mozilla-b2g/gaia/commit/703004ee2f7c803a29545f949acea2c05468ba33
Original PR revert: https://github.com/mozilla-b2g/gaia/commit/2e2d3263b39ffcef8e1d2d200ce461245b97ebec

These failures are also in the original PR's gaia-try run: https://tbpl.mozilla.org/?rev=d7d9839a9f573649e8a80cd15f9c2d8553a1b3cf&tree=Gaia-Try

Your reviews should stand, and if you need a new one it would only be for the contacts app. Please make sure you have a green gaia-try run before landing.

Comment 28

[Ed Morley [:emorley]]

Backout of gecko part:
remote:   https://hg.mozilla.org/mozilla-central/rev/f41a267983c1

Comment 29

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

Sorry for the gaia-try failure, I didn't notice the error.
When we were still using Travis-ci before it will have some red warnings when some tests failed. Today I saw all are green on the github so I just merged it.
I think I haven't pushed a gaia commit for a while.

Comment 30

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

Attachment: Gaia PullRequest v2.

Another Pull Request is created since the original one is closed.

Already addressed the lint failure in Comment 25 and the Gaia Unit test failure in Comment 27.

@Francisco
Sorry I didn't update the unit tests for Contacts app before, could you help to review the Part 5: Contacts app change for me again?

Thank you

Gaia-Try
https://tbpl.mozilla.org/?rev=2570be20f7a37f92f0f02d4e5d8bdc744120d628&tree=Gaia-Try

Comment 31

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

Comment on attachment 8469098 [details] [review]
Gaia PullRequest v2.

switch r? to Jose as I found Francisco is PTO now.

Jose, could you help to review the Contacts app part for me ?

Gracias.

Comment 32

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

Attachment: Part 2: update marionette test cases.

Some tests are not updated.

Comment 33

[Jose Manuel Cantera]

handing over the review to Sergi, as he is more familar with the NFC code and functionality

thanks

Comment 34

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

Attachment: Part 1: Add MozNFCPeerEvent. v4.

rebase

Comment 35

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

Attachment: Part 2: update marionette test cases. v2

Comment 36

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

Comment on attachment 8469098 [details] [review]
Gaia PullRequest v2.

Forward r? back to Francisco

Comment 37

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

Comment on attachment 8469098 [details] [review]
Gaia PullRequest v2.

Update Pull Request since Bug 1039935 is landed.
Alive, can you review the System app part for me ?

thanks

Comment 38

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

Comment on attachment 8469098 [details] [review]
Gaia PullRequest v2.

Contacts part LGTM, take into account that gaia-try is still red.

Thanks!

Comment 39

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

(In reply to Francisco Jordano [:arcturus] [:francisco] from comment #38)
> Comment on attachment 8469098 [details] [review]
> Gaia PullRequest v2.
> 
> Contacts part LGTM, take into account that gaia-try is still red.
> 
> Thanks!
Re-Run and it's green.
https://tbpl.mozilla.org/?rev=b393e0be68046f1a22a22653f7df2d4d2c34f598&tree=Gaia-Try

Or you looked into the first try result? The first one did have failures.

Comment 40

[(inactive after 6/18) Alive Kuo [:alive]]

Comment on attachment 8469098 [details] [review]
Gaia PullRequest v2.

Better having tests.

Comment 41

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

(In reply to Alive Kuo [:alive][NEEDINFO!] from comment #40)
> Comment on attachment 8469098 [details] [review]
> Gaia PullRequest v2.
> 
> Better having tests.

I already have Gecko tests (marionette-webapi and mochitest) and updated Gaia unit tests in case I break it.

Or if you mean add more tests on Gaia side I'd file follow-up bugs because I'd like to land this bug first, otherwise I will keep rebasing on Gecko/Gaia side and ask for more review on this. :)

Comment 42

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

Attachment: Part 2: update marionette test cases. v3

rebase

Comment 43

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

Comment on attachment 8473474 [details] [diff] [review]
Part 2: update marionette test cases. v3

wrong one.

Comment 44

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

https://hg.mozilla.org/integration/b2g-inbound/rev/b24e74058201
https://hg.mozilla.org/integration/b2g-inbound/rev/37449b85186f

Comment 45

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

Gaia part.
https://github.com/mozilla-b2g/gaia/commit/60a5a13c5d34d847f60c0984201eaa9dfd7299e4

Comment 46

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/b24e74058201
https://hg.mozilla.org/mozilla-central/rev/37449b85186f