Closed Bug 1046554 Opened 5 years ago Closed 5 years ago

B2G NFC: pass NFCPeerEvent in onpeerready

Categories

(Firefox OS Graveyard :: NFC, defect)

ARM
Gonk (Firefox OS)
defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
2.1 S2 (15aug)

People

(Reporter: allstars.chh, Assigned: allstars.chh)

References

Details

(Whiteboard: [p=3])

Attachments

(3 files, 6 obsolete files)

46 bytes, text/x-github-pull-request
arcturus
: review+
alive
: review+
Details | Review
6.93 KB, patch
allstars.chh
: review+
Details | Diff | Splinter Review
7.84 KB, patch
dimi
: review+
Details | Diff | Splinter Review
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.
Duplicate of this bug: 1040713
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 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
Attachment #8465315 - Flags: feedback?(bugs)
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.
Oh, and if w3c spec requires non-null, file a spec bug, please.
Attachment #8465315 - Flags: feedback?(bugs) → feedback+
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
Attachment #8465315 - Flags: review?(bugs)
Attached file Gaia PullRequest (obsolete) —
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 ~~
Attachment #8466059 - Flags: review?(francisco)
Attachment #8466059 - Flags: review?(dkuo)
Attachment #8466059 - Flags: review?(dflanagan)
Attachment #8466059 - Flags: review?(dale)
Comment on attachment 8466059 [details] [review]
Gaia PullRequest

Pretty elegant solution :), didn't try on the device but the contacts change is really simple.
Attachment #8466059 - Flags: review?(francisco) → review+
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.
Attachment #8465315 - Flags: review?(bugs) → review+
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
Attachment #8466059 - Flags: review?(pdahiya)
Attachment #8466059 - Flags: review?(dflanagan)
Attachment #8466059 - Flags: feedback+
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.
Attachment #8466059 - Flags: review?(pdahiya) → review+
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)
Attachment #8466059 - Flags: review?(im)
Attachment #8466059 - Flags: review?(dkuo)
Attachment #8466059 - Flags: review?(dale)
Attachment #8466059 - Flags: review+
Comment on attachment 8466059 [details] [review]
Gaia PullRequest

Looks good, thanks
Attachment #8466059 - Flags: review?(dale) → review+
Comment on attachment 8466059 [details] [review]
Gaia PullRequest

Thanks. The event.peer is better.
Attachment #8466059 - Flags: review?(im) → review+
Whiteboard: [p=3]
Target Milestone: --- → 2.1 S2 (15aug)
Attached patch Patch v2. (obsolete) — Splinter Review
Add HasNFCSupport in MozNFCPeerEvent.webidl.
Attachment #8465315 - Attachment is obsolete: true
Attachment #8467582 - Flags: review+
(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.
(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 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.
Attached patch Patch v3. (obsolete) — Splinter Review
Attachment #8467582 - Attachment is obsolete: true
Attachment #8467690 - Flags: review+
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.
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.
https://hg.mozilla.org/mozilla-central/rev/b2ef6a444640
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
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
Flags: needinfo?(allstars.chh)
Follow-up landed to fix linter: https://github.com/mozilla-b2g/gaia/commit/25701a4f9fc4bd1d1dc4a1d4bedfe66ee8c94502
Flags: needinfo?(allstars.chh)
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.
Status: RESOLVED → REOPENED
Flags: needinfo?(allstars.chh)
Resolution: FIXED → ---
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.
Flags: needinfo?(allstars.chh)
Attached file 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
Attachment #8466059 - Attachment is obsolete: true
Attachment #8469098 - Flags: review?(francisco)
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.
Attachment #8469098 - Flags: review?(francisco) → review?(jmcf)
Some tests are not updated.
Attachment #8469950 - Flags: review?(dlee)
Attachment #8469098 - Flags: review?(jmcf)
Attachment #8469098 - Flags: review?(sergi.mansilla)
handing over the review to Sergi, as he is more familar with the NFC code and functionality

thanks
rebase
Attachment #8467690 - Attachment is obsolete: true
Attachment #8470729 - Flags: review+
Attachment #8470729 - Attachment description: Patch. v4. → Part 1: Add MozNFCPeerEvent. v4.
Attachment #8470731 - Flags: review?(dlee) → review+
Comment on attachment 8469098 [details] [review]
Gaia PullRequest v2.

Forward r? back to Francisco
Attachment #8469098 - Flags: review?(sergi.mansilla) → review?(francisco)
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
Attachment #8469098 - Flags: review?(alive)
Comment on attachment 8469098 [details] [review]
Gaia PullRequest v2.

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

Thanks!
Attachment #8469098 - Flags: review?(francisco) → review+
(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 on attachment 8469098 [details] [review]
Gaia PullRequest v2.

Better having tests.
Attachment #8469098 - Flags: review?(alive) → review+
(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. :)
rebase
Attachment #8470731 - Attachment is obsolete: true
Attachment #8473474 - Flags: review+
Comment on attachment 8473474 [details] [diff] [review]
Part 2: update marionette test cases. v3

wrong one.
Attachment #8473474 - Attachment is obsolete: true
Attachment #8470731 - Attachment is obsolete: false
https://hg.mozilla.org/mozilla-central/rev/b24e74058201
https://hg.mozilla.org/mozilla-central/rev/37449b85186f
Status: REOPENED → RESOLVED
Closed: 5 years ago5 years ago
Resolution: --- → FIXED
No longer depends on: 1055375
You need to log in before you can comment on or make changes to this bug.