969217 - [Gaia] [NFC] Improvement for the function of sharing of URLs

Status: RESOLVED FIXED

(Firefox OS Graveyard :: NFC, defect)

Description

[Ken Chang[:kenkai]]

After fixing Bug 903250, there are a lot of bugs coming. We should improve the function of sharing of URLs by using the library provided by bug 963556.

Comment 1

[saminath]

Hi Ken,

I will do modification on [Bug 903250] according with NFC Shared Library [Bug 963556].

Comment 2

[saminath]

Attachment: 0001-Bug-969217-Improvement-for-the-function-of-sharing-o.patch

New patch for NFC URL content sharing.  It uses NFC Shared Library [https://github.com/svic/gaia/blob/Bug_963556/shared/js/nfc_util.js]. I integrated and Tested in Nexus-4 devices.
PR: https://github.com/mozilla-b2g/gaia/pull/16120

For removing ‘nfc-manager’ permission from browser [https://bugzilla.mozilla.org/show_bug.cgi?id=963488#c3], I need info for implementation.

In browser js file, HandleVisibilityChange() provides the Browser exit information. I could not differentiate normal exit and Shrink UI Exit.  For this purpose only I am using ‘nfc-manager-tech-state’.

Alternate Solution may be:
1. How do I get the Current NFC Discover state?
2. In browser.js where else I can check the Exit operation?

Comment 3

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

Why do you needs to know it's shrinking or not?

Comment 4

[saminath]

In browser.js 'document.hidden' provide the browser exit state. Both Normal Exit and Shrink UI(NFC Discover time) becomes TRUE.

How to differentiate it?

Comment 5

[Ben Francis [:benfrancis]]

Comment on attachment 8373215 [details] [diff] [review]
0001-Bug-969217-Improvement-for-the-function-of-sharing-o.patch

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

This patch seems to mainly just simplify code which has already landed, so I gave it feedback+, but bug 963488 indicates that there may be a problem with the underlying implementation.

I would prefer Alive to review this to make sure he is happy with the implementation.

I don't understand why we are landing this code on master if it is only for a demo and will never ship. Would it be better to create your own feature branch?

Comment 6

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

Sorry, I still don't understand...
My question is just why do you need to differentiate shrinking and background?

nfc-manage permission shouldn't be used for this purpose, if it's a normal use case we need to reconsider the API design again.

Comment 7

[Garner Lee]

(In reply to Alive Kuo [:alive][NEEDINFO!][:艾莉芙] from comment #6)
> nfc-manage permission shouldn't be used for this purpose, if it's a normal
> use case we need to reconsider the API design again.

saminath:

NFC manager permissions is not needed for normal applications, it is needed for the System App only, as documented by the WebNFC wiki: https://wiki.mozilla.org/WebAPI/WebNFC#Application_Permissions

For example, the current browser incorrectly opens whenever a technology is found, because nfc-manager targeted messages are solely intended for the certified System app that handles incoming NFC messages.

In the activity handler of the app, implement the following to handle tech-discovered (and possibly, tech-lost but this has UX concerns).

  case 'nfc-tech-discovered':
    handleTechnologyDiscoveredMessages(data); // data includes the NDEF record, usually.
    break;

There is a limitation, similar to Android actually, where Techlost should not handled (or even sent). The reason being, if it handles it, it usually launches an action. When the tag leaves, UX wise, who needs message besides System App? The background app would need to wake up, or be brought forward on top to handle it, away from the newly launched app (a callback won't have this issue, but it won't be symmetric with how the data is delivered via activity).

Comment 8

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

Comment on attachment 8373215 [details] [diff] [review]
0001-Bug-969217-Improvement-for-the-function-of-sharing-o.patch

Cancel review until I am conveyed nfc-manage permission is necessary for this patch.

Comment 9

[saminath]

Attachment: 0001-Bug-969217-Improvement-for-the-function-of-sharing-o.patch

I have discussion with alive in offline, I modified the code accordingly.
1. Get rid of Nfc-manager accessing from nfc.js and corresponding handler binding in browser.js
2. Manifest has only ‘nfc’ access permission.
PR:  https://github.com/mozilla-b2g/gaia/pull/16211

Thanks. Alive

Comment 10

[Ken Chang[:kenkai]]

Comment on attachment 8374794 [details] [diff] [review]
0001-Bug-969217-Improvement-for-the-function-of-sharing-o.patch

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

Garner, please also have a joint review.

Comment 11

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

Comment on attachment 8374794 [details] [diff] [review]
0001-Bug-969217-Improvement-for-the-function-of-sharing-o.patch

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

r+ if you move onpeerready registration to Browser.init.

::: apps/browser/js/browser.js
@@ -616,1 @@
>        if (window.navigator.mozNfc) {

I think you should just move this part to Browser.init

Comment 12

[Garner Lee]

Comment on attachment 8374794 [details] [diff] [review]
0001-Bug-969217-Improvement-for-the-function-of-sharing-o.patch

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

::: apps/browser/js/browser.js
@@ +607,2 @@
>        if (window.navigator.mozNfc) {
>          window.navigator.mozNfc.onpeerready = NfcURI.handlePeerConnectivity;

I don't see the rest of the browser code here, but does UX have a case where this is set and unset? (Possible use case: the user is entering data on a long and involved HTML form, and won't want to potentially lose focus to it behind another tab somewhere).

::: apps/browser/js/nfc.js
@@ +7,3 @@
>    lookupUrlRecordType: function nfc_lookupUrlRecordType(uri) {
> +    for (var i = 1; i < NDEF.URIS.length; i++) {
> +    var len = NDEF.URIS[i].length;

General nits: 2 column indenting/code beautification for checkin.

@@ +34,2 @@
>        if (split.identifier == 0) {
> +          urlPayload = currentUrl;

Nit: indenting.

Comment 13

[Garner Lee]

I'll try to land 963556 soon for MWC, but if not, please change the shared/js/nfc_util.js include to be a local app include.

Comment 14

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

(In reply to Garner Lee from comment #12)
> Comment on attachment 8374794 [details] [diff] [review]
> 0001-Bug-969217-Improvement-for-the-function-of-sharing-o.patch
> 
> Review of attachment 8374794 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: apps/browser/js/browser.js
> @@ +607,2 @@
> >        if (window.navigator.mozNfc) {
> >          window.navigator.mozNfc.onpeerready = NfcURI.handlePeerConnectivity;
> 
> I don't see the rest of the browser code here, but does UX have a case where
> this is set and unset? (Possible use case: the user is entering data on a
> long and involved HTML form, and won't want to potentially lose focus to it
> behind another tab somewhere).

Don't think it's in the scope of this bug.
Also we will never know what's the current page is doing (form entering) right now from browser app. Security risk!

Comment 15

[saminath]

Attachment: 0001-Bug-969217-Improvement-for-the-function-of-sharing-o.patch

PR: https://github.com/mozilla-b2g/gaia/pull/16292

Modification details:
1. Openready is only present in Browser.init(), nowhere else.
2. Indenting problem resolved, TI build is success.
3. Temporarily created the nfc_util.js in locally (Remove after shared nfc library integrated- Bug 963556 )
4. Index.html change the js/nfc_util.js -> shared/js/nfc_util.js (Change after nfc shared library integrated – Bug 963556)

Thanks.

Comment 16

[Jan Jongboom [:janjongboom] (inactive)]

Comment on attachment 8376226 [details] [diff] [review]
0001-Bug-969217-Improvement-for-the-function-of-sharing-o.patch

Carrying over r+.

Comment 17

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Alive, could you merge the patch? Thanks.

Comment 18

[Ken Chang[:kenkai]]

(In reply to Tim Guan-tin Chien [:timdream] (MoCo-TPE) (please ni?) from comment #17)
> Alive, could you merge the patch? Thanks.
Greg is working on this. Thanks for Greg's help.

Comment 19

[Greg Weng [:snowmantw][:gweng][:λ]]

master: https://github.com/mozilla-b2g/gaia/commit/a075c52367e7a9c10a9c72704a29e6e178d70bb4