Closed Bug 903250 Opened 7 years ago Closed 7 years ago

[Gaia] Support sharing of URLs

Categories

(Firefox OS Graveyard :: Gaia::Browser, defect)

x86
macOS
defect
Not set
normal

Tracking

(blocking-b2g:-)

RESOLVED FIXED
1.3 Sprint 5 - 11/22
blocking-b2g -

People

(Reporter: frlee, Assigned: s.x.veerapandian)

References

Details

(Keywords: productwanted, Whiteboard: [systemsfe])

Attachments

(3 files, 4 obsolete files)

for NFC, Browser application needs to be able to create NDEF message which contains URL info. so that users can share URL via NFC.
Depends on: 894678
The user can - from the browser - share the URL of the page that
the browser is currently pointing to. The URL can be shared with another
NFC enabled device by tapping the devices together.
a) The user selects “Share Web Page” from the browser menu.
b) The user taps the device with another NFC enabled device.
c) On the receiving mobile, the device will get a prompt saying “Another device wants to share an URL with you – Accept/Reject” or something similar.
d) Upon accepting, the receiving mobile opens up the browser and goes to the URL.
Summary: [Gaia] Browser application: create NDEF message contains URL info → [Gaia] Support sharing of URLs
Blocks: 894678
No longer depends on: 894678
Hi Ben, are you available to help this bug?
Flags: needinfo?(bfrancis)
Hi Ken,

The browser app already has a share URL feature in the master branch, it fires a "share URL" web activity which any app can consume.

From the user requirements described here it sounds like the best way to implement this would be to create an NFC sharing app which can consume a "share URL" web activity from any app, including the browser, and has access to the NFC API to generate the NFC message.

If you think there are any new user requirements that need to be implemented in the browser app, the browser team (https://wiki.mozilla.org/FirefoxOS/SprintStatus#Browser) can assess those and prioritise them in the product backlog during the next sprint planning meeting along with all other feature requests.
Flags: needinfo?(bfrancis)
Candice, can you find someone from Browser team to own this bug? Thank you.
blocking-b2g: --- → 1.3+
Flags: needinfo?(cserran)
Whiteboard: [1.3:p1]
Whiteboard: [1.3:p1] → [1.3:p1][systemsfe]
This is an example of how to modify an application to get a nfc-ndef-discovered message. It basically just opens the web page any other incoming activity in this particular instance.
Answering email comments:
> Why "nfc-ndef-discovered" instead of "view".

Unlike "view"
"nfc-ndef-discovered' will also return the entire NDEF Record, see WebIDL here: https://bugzilla.mozilla.org/page.cgi?id=splinter.html&bug=674741&attachment=823735

A few definitions:
NFC stands for Near Field Communications.
NDEF stands for: NFC Data Exchange Format.

So, if the application has full knowledge of what an NDEF Record is, or even has a desire for very special handling for NFC tags, the distinction between "view and nfc-ndef-discovered is potentially important, as the NDEF message is delivered as a array of one or more NDEFRecords.

Sending NDEF messages:
In the case of Browser NFC Peer to Peer communications, the next API to be exposed is the "onpeerfound" and "onpeerlost". Next patch, after NFC DOM landing.
Blocks: NFC-Gaia
Ben, are you the one who could be the assignee on this bug? If you're not, I am sorry and please feel free to clear your name on the assignee. Thank you.
Assignee: nobody → bfrancis
We don't have time for this in the 1.3 time frame. We also need a device for this.
I can look into this further if it gets into the Systems FE backlog and planned for a sprint.
Assignee: bfrancis → nobody
It seems better to finish this bug before sprint5. If you think it isn't reasonable, please update target milestone.
Target Milestone: --- → 1.3 Sprint 5 - 11/22
Whiteboard: [1.3:p1][systemsfe] → [systemsfe]
NFC is not a committed feature for 1.3, so this should not block the release.
blocking-b2g: 1.3+ → 1.3?
Component: NFC → Gaia::Browser
Moving this out of NFC into browser as the code changes here would happen in the browser app, not the NFC codebase.
Gregor & I discussed this offline - we can't block on this at this point, as this is not a committed feature for 1.3. I do wonder if bug 920882 might have fixed this issue, so I'm putting needinfo on Greg to see what he thinks.
blocking-b2g: 1.3? → -
Flags: needinfo?(gweng)
I believe that Shrinking UI doesn't and shouldn't directly communicate with any applications, according to the discussions before. Shrinking UI should only accept some limited events from NFC manager. So the path would be

    NFC manager -> (peer found)
    Browser -> (decide to send or not)
    sending|not sending -> (call sending method)
    NFC manager -> (notify the shrinking ui)
    Shrinking UI -> (notify that user has tapped and swiped)
    NFC manager -> (do the sending thing)
    ...
Flags: needinfo?(gweng)
(If I'm wrong please correct me)
Ben, are you the best one to handle this bug? Thanks.
Assignee: nobody → bfrancis
Kevin, I have already un-assigned myself once. Please do not assign bugs to me.

Peter for prioritisation.
Flags: needinfo?(pdolanjski)
Keywords: productwanted
Assignee: bfrancis → nobody
Peter, partner would like to have a NFC demo in MWC. I wonder if you could find someone to handle this bug.
Thanks very much.
Attached patch NfcUri.patch (obsolete) — Splinter Review
Attachment #8355163 - Flags: review?(bfrancis)
Attached patch NfcUri.patch (obsolete) — Splinter Review
Please find the NFC URI implementation patch. 

Implementation Details:
1.	nfcURIPeerHandler() , get the URL from currently opened TAB in the Browser.
2.	Create NDEF record based on the {Url, tnf,id and rtd} inputs.
3.	Transmit the NDEF record via NFC peer connectivity.
4.	handleVisibilityChange(), Provides the Browser Hidden and Visible status. Based on that nfcURIPeerHandler can be added and removed.
5.	createUriNdefRecord() , creates the NDEF records, It Looks up the URL record type and create the Payload data and Payload Identifier.
6.	sendNdefRecords(), Create the Nfc Peer connectivity and Transmit the NDEF records.

OnGoing:
7.	Currently, I could not able to locate where exactly I have remove the handler (mozNfc.onpeerready = null).
a.	Browser visibility handled in handleVisibilityChange(). But during shrinking URI inside Browser,  handleVisibilityChange() has invoked, the value of document.hidden becomes true.
Attachment #8355164 - Flags: review?(bfrancis)
Comment on attachment 8355164 [details] [diff] [review]
NfcUri.patch

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

::: apps/browser/js/browser.js
@@ +602,5 @@
> +    records.push(record);
> +    res = NfcURI.sendNdefRecords(records,event);
> +
> +    if (!res)
> +      debug('URL transfer failed');;

It should have only one ";", right?
Here with I attached the Patch for review.
Also Find the PR https://github.com/mozilla-b2g/gaia/pull/15012
Attachment #8355163 - Attachment is obsolete: true
Attachment #8355164 - Attachment is obsolete: true
Attachment #8355163 - Flags: review?(bfrancis)
Attachment #8355164 - Flags: review?(bfrancis)
Attachment #8355536 - Flags: review?(khu)
Attachment #8355536 - Flags: review?(bfrancis)
Comment on attachment 8355536 [details]
0001-Bug-903245-Support-sharing-of-Contact-via-NFC-r-revi.patch

Review of attachment 8355536 [details]:
-----------------------------------------------------------------

::: apps/communications/contacts/js/contacts.js
@@ +264,5 @@
> +    var contact = {};
> +    var str = '';
> +    var count = 0;
> +    var tnf_mime_media = 0x02;
> +    var VCARD_VERSION = '2.1'

Should we add a ';' at the end of this line? Also, we should use version 2.1 here? There has a shared 'contact2vcard.js' file that defines 4.0 as the VCARD version. Maybe we can use that one? Or, we are using 2.1 for other reasons?

@@ +288,5 @@
> +
> +    LazyLoader.load('/shared/js/contact2vcard.js', function() {
> +    ContactToVcard([contact], VCARD_VERSION, function append(vcards, nCards) {
> +    str += vcards;
> +    count += nCards;

I don't understand what's the purpose for the local variable 'count'.

@@ +293,5 @@
> +    }, function success() {
> +        var data;
> +
> +        data = contains(str)
> +        if (data){

Looks like 'data' is used to check the return value from contains() only. Would it be simpler to write it as: 

if(contains(str)){
  payload = str;
}
Attached file 15012[2] (obsolete) —
Please find the NFC URI implementation:

Implementation Details:
1. nfcURIPeerHandler() is the peer handler function. It gets the URL from currently opened TAB in the Browser.
2. To Create NDEF record based on the {UrlData, tnf,id and rtd,Id} inputs.
3. To transmit the NDEF record via NFC peer connectivity.
4. handleVisibilityChange(), Provides the Browser Hidden and Visibility status. Based on that nfcURIPeerHandler can be added and removed.
5.createUriNdefRecord() , creates the NDEF records, It Looks up the URL record type and create the Payload data and Payload Identifier.
6. sendNdefRecords(), Create the Nfc Peer connectivity and Transmit the NDEF records.
7. Browser Exit Handle in Normal and Shrink mode, using the ‘NFC-Manager-Tech-Discovered’ message handler. If ‘NFC-Manager’ is enabled and ‘Browser Status’ is hidden. I am considering the NFC Paring is in progress, other-wise, Browser is Exit by User (mozNfc.onpeerready = null).
Attachment #8356034 - Flags: review?(kchang)
Comment on attachment 8356034 [details]
15012[2]

I am not a proper reviewer for this patch...:-(.
Attachment #8356034 - Flags: review?(kchang) → review?(bfrancis)
Comment on attachment 8355536 [details]
0001-Bug-903245-Support-sharing-of-Contact-via-NFC-r-revi.patch

Is this patch attached to the wrong bug? It doesn't seem related to URL sharing in the browser app.

You would need someone from the communications team to review this I'm afraid.

Looking at the other patch.
Attachment #8355536 - Flags: review?(khu)
Attachment #8355536 - Flags: review?(bfrancis)
Comment on attachment 8356034 [details]
15012[2]

Thanks for the patch!

See my comments and questions on the pull request https://github.com/mozilla-b2g/gaia/pull/15012

Please note that while we can land this in the browser app for demo purposes, this is unlikely to ever ship in a release because the browser app is being replace by the new system browser in the system app.
Attachment #8356034 - Flags: review?(bfrancis) → review-
(In reply to Ben Francis [:benfrancis] from comment #26)
> Comment on attachment 8355536 [details]
> 0001-Bug-903245-Support-sharing-of-Contact-via-NFC-r-revi.patch
> 
> Is this patch attached to the wrong bug? It doesn't seem related to URL
> sharing in the browser app.
> 
> You would need someone from the communications team to review this I'm
> afraid.
> 
> Looking at the other patch.

Yes, I believe that this one was attached to the wrong bug.
Comment on attachment 8355536 [details]
0001-Bug-903245-Support-sharing-of-Contact-via-NFC-r-revi.patch

Obsolete 0001-Bug-903245-Support-sharing-of-Contact-via-NFC-r-revi.patch to avoid confusion.
Attachment #8355536 - Attachment is obsolete: true
Attachment #8355536 - Attachment is patch: false
Attached file 15106[1]
1. The constant are URI Record Type Definition. These are mapped to URI string prefixes. URI Identifier code and URL data transfer via NFC
2. All Handler functions are changed from Browser to NFC and Code kept in the nfc.js 
3. String utility moved into utilities.js 
4. gjslint build error is verified 
5. Variables are camelCase
Attachment #8356034 - Attachment is obsolete: true
Attachment #8357136 - Flags: review?(bfrancis)
Comment on attachment 8357136 [details]
15106[1]

Great, thanks for the contribution!
Attachment #8357136 - Flags: review?(bfrancis) → review+
Please fix the remaining lint errors so we can ensure Travis is green before merging

https://github.com/mozilla-b2g/gaia/pull/15106

----- FILE : /home/travis/build/mozilla-b2g/gaia/apps/browser/js/browser.js -----

Line 600, E:0005: Illegal tab in whitespace before "}"

Line 602, E:0110: Line too long (97 characters).

----- FILE : /home/travis/build/mozilla-b2g/gaia/apps/browser/js/nfc.js -----

Line 64, E:0110: Line too long (89 characters).

Line 120, E:0001: Extra space at end of line

Line 142, E:0110: Line too long (81 characters).

Line 151, E:0121: Illegal comma at end of object literal

----- FILE : /home/travis/build/mozilla-b2g/gaia/apps/browser/js/utilities.js -----

Line 125, E:0121: Illegal comma at end of object literal

Found 7 errors, including 0 new errors, in 3 files (1477 files OK).
Flags: needinfo?(s.x.veerapandian)
Attached file 15146[1]
I Fixed the Errors. Travis CI build passed
Attachment #8357725 - Flags: review?(bfrancis)
Assignee: nobody → s.x.veerapandian
Attachment #8357725 - Flags: review?(bfrancis)
Flags: needinfo?(s.x.veerapandian)
Flags: needinfo?(pdolanjski)
Flags: needinfo?(cserran)
You need to log in before you can comment on or make changes to this bug.