Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi] Assignee
	Description • 10 years ago

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.

	Krzysztof Mioduszewski[:tauzen]
	Comment 2 • 10 years ago

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.

	Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi] Assignee
	Comment 4 • 10 years ago

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

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 5 • 10 years ago

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.

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 6 • 10 years ago

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

	Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi] Assignee
	Comment 7 • 10 years ago

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

	Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi] Assignee
	Comment 8 • 10 years ago

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 ~~

	Francisco Jordano [:arcturus] [:francisco]
	Comment 9 • 10 years ago

Comment on attachment 8466059 [details]
Gaia PullRequest

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

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 10 • 10 years ago

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.

	David Flanagan [:djf]
	Comment 11 • 10 years ago

Comment on attachment 8466059 [details]
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

	Punam Dahiya [:pdahiya]
	Comment 12 • 10 years ago

Comment on attachment 8466059 [details]
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.

	Dominic Kuo [:dkuo]
	Comment 13 • 10 years ago

Comment on attachment 8466059 [details]
Gaia PullRequest

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

	Dale Harvey (:daleharvey)
	Comment 14 • 10 years ago

Comment on attachment 8466059 [details]
Gaia PullRequest

Looks good, thanks

	John Hu [:johnhu][:johu][:醬糊小弟]
	Comment 15 • 10 years ago

Comment on attachment 8466059 [details]
Gaia PullRequest

Thanks. The event.peer is better.

	Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi] Assignee
	Comment 16 • 10 years ago

Add HasNFCSupport in MozNFCPeerEvent.webidl.

	Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi] Assignee
	Comment 17 • 10 years ago

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

	Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi] Assignee
	Comment 18 • 10 years ago

(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

	Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi] Assignee
	Comment 19 • 10 years ago

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.

	Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi] Assignee
	Comment 21 • 10 years ago

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.

	Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi] Assignee
	Comment 22 • 10 years ago

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.

	Ed Morley [:emorley]
	Comment 23 • 10 years ago

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

	Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi] Assignee
	Comment 24 • 10 years ago

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

	Ed Morley [:emorley]
	Comment 25 • 10 years ago

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

	Kevin Grandon :kgrandon
	Comment 26 • 10 years ago

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

	Kevin Grandon :kgrandon
	Comment 27 • 10 years ago

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.

	Ed Morley [:emorley]
	Comment 28 • 10 years ago

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

	Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi] Assignee
	Comment 29 • 10 years ago

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.

	Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi] Assignee
	Comment 30 • 10 years ago

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

	Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi] Assignee
	Comment 31 • 10 years ago

Comment on attachment 8469098 [details]
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.

	Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi] Assignee
	Comment 32 • 10 years ago

Some tests are not updated.

	Jose Manuel Cantera
	Comment 33 • 10 years ago

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

thanks

	Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi] Assignee
	Comment 34 • 10 years ago

rebase

	Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi] Assignee
	Comment 36 • 10 years ago

Comment on attachment 8469098 [details]
Gaia PullRequest v2.

Forward r? back to Francisco

	Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi] Assignee
	Comment 37 • 10 years ago

Comment on attachment 8469098 [details]
Gaia PullRequest v2.

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

thanks

	Francisco Jordano [:arcturus] [:francisco]
	Comment 38 • 10 years ago

Comment on attachment 8469098 [details]
Gaia PullRequest v2.

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

Thanks!

	Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi] Assignee
	Comment 39 • 10 years ago

(In reply to Francisco Jordano [:arcturus] [:francisco] from comment #38)
> Comment on attachment 8469098 [details]
> 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.

	(inactive after 6/18) Alive Kuo [:alive]
	Comment 40 • 10 years ago

Comment on attachment 8469098 [details]
Gaia PullRequest v2.

Better having tests.

	Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi] Assignee
	Comment 41 • 10 years ago

(In reply to Alive Kuo [:alive][NEEDINFO!] from comment #40)
> Comment on attachment 8469098 [details]
> 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. :)

	Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi] Assignee
	Comment 42 • 10 years ago

rebase

	Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi] Assignee
	Comment 43 • 10 years ago

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

wrong one.

	Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi] Assignee
	Comment 44 • 10 years ago

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

	Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi] Assignee
	Comment 45 • 10 years ago

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

	Ed Morley [:emorley]
	Comment 46 • 10 years ago

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