959437 - Refactor NfcManager APIs and implementation details to support sendFile , notifyUserAcceptedP2P and other privileged Nfc operations

Status: RESOLVED FIXED

(Firefox OS Graveyard :: NFC, defect)

Description

[psiddh]

User Agent: Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.36 (KHTML, like Gecko) Ubuntu Chromium/31.0.1650.63 Chrome/31.0.1650.63 Safari/537.36

Steps to reproduce:

This bug is to track the changes of NfcManager dom APIs and its implementation.

Comment 1

[psiddh]

Attachment: (v1) Part 2 : Add NotifySendFileStatus interface r=allstars.chh

Part 2 (DOM code), Part 3 (system - Gaia changes) of this bug will follow soon

Comment 2

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

Comment on attachment 8360723 [details] [diff] [review]
(v1) Part 2 : Add NotifySendFileStatus interface r=allstars.chh

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

I'll review when DOM patch is ready.

Comment 3

[Ken Chang[:kenkai]]

(In reply to Yoshi Huang[:allstars.chh][:yoshi] from comment #2)
> 
> I'll review when DOM patch is ready.
Sid, when will you provide the patch for DOM?

Comment 4

[psiddh]

Attachment: (v1) Part 2 : Add new / refactor interfaces to MozNfcManager DOM sr, r=smaug

Comment 5

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

Comment on attachment 8362034 [details] [diff] [review]
(v1) Part 2 : Add new / refactor interfaces to MozNfcManager DOM sr, r=smaug

>+  notifyUserAcceptedP2P: function notifyUserAcceptedP2P(manifestUrl) {
>+    let appID = appsService.getAppLocalIdByManifestURL(manifestUrl);
>+    // Notify Chrome process of User's acknowledgement
s/Chrome/chrome/
s/User/user/

Comment 6

[psiddh]

Attachment: (v1b) Part 2 : Add new / refactor interfaces to MozNfcManager DOM sr, r=smaug

Comment 7

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

The patch order is wrong here.

Ususally we put patches for higher API first, in this bug Part 1 should be DOM, and Part 2 should be Impl.

If you do it like right now, DOM patch is in Part 2.
Once DOM patch needs to be updated, you mostly like will have to modify Part 1(Impl) patch as well.

Also for the DOM patch, you need to ask r? for a DOM peer, not a sr?.
And we need to ask smaug if we need to ask sr? from another reviewer here. 
(reviewer and super-reviewer cannot be the same one).

Comment 8

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

I'll like to ask smaug if it's okay we expose 'requestId' from a DOMRequest to the web app, even it's a certified app like nfc_manager in this case.

Comment 9

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

Expose requestId where?

Comment 10

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

Comment on attachment 8362158 [details] [diff] [review]
(v1b) Part 2 : Add new / refactor interfaces to MozNfcManager DOM sr, r=smaug

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

::: dom/webidl/MozNfc.webidl
@@ +23,5 @@
> +
> +   /**
> +    * Notify the status of sendFile operation
> +    */
> +   void notifySendFileStatus(octet status, DOMString requestId);

Hi Smaug,
Sorry for not being clear before.

Also see https://bug933093.bugzilla.mozilla.org/attachment.cgi?id=8349166
Step 5, 6.

RequestId is notified to Gaia side in Step 5,
and Gaia app calls this requestId to Gecko in step 6.

Comment 11

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

Hi, Smaug

Can you check the comment 10 for the usage of requestId ?
iIf you think it's okay then we will push it.

Comment 12

[psiddh]

Attachment: (v1c) Part 1 : Add new / refactor interfaces to MozNfcManager DOM sr, r=smaug

Fixed typo. Just asking a review just to be sure.

Comment 13

[psiddh]

Attachment: (v1d) Part 1 : Add new / refactor interfaces to MozNfcManager DOM sr, r=smaug

Fixed subject line

Comment 14

[psiddh]

Attachment: (v1b) Part 2 : Add NotifySendFileStatus interface r=allstars.chh

Fixed subject line

Comment 15

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

Comment on attachment 8362158 [details] [diff] [review]
(v1b) Part 2 : Add new / refactor interfaces to MozNfcManager DOM sr, r=smaug

This was marked obsolete, so I assume no review is needed.

Comment 16

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

Why we need to expose any requestID to non-privileged code? Couldn't we use some transparent token
(just some random JS object), and do token<->requestID mapping in the privileged js (frame scripts and such).

Comment 17

[psiddh]

Hi Olli,
We are exposing the 'requestID' only to System app (Privileged Content Process) and no other app would be able to get this information. Yes agreed we could expose some token instead of 'requestID'.

During the recently concluded work week (Yoshi was also present), we also had this bug reviewed by Security team member, Stephanie ([e-mail]) and the general consensus was that exposing 'requestID' to system app is not a security risk. However that said, I will make the change if you would like a 'token' to be exposed instead of 'requestID'. What do you suggest ?

Comment 18

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

Ah, it is exposed only to system app, then it sounds ok to me.

Comment 19

[psiddh]

Latest Try Results: https://tbpl.mozilla.org/?tree=Try&rev=a7c4fd2e2c8f

Comment 20

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/b2g-inbound/rev/a504dfc43d3f
https://hg.mozilla.org/integration/b2g-inbound/rev/1d4c70a7296e

Comment 21

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/a504dfc43d3f
https://hg.mozilla.org/mozilla-central/rev/1d4c70a7296e

Comment 22

[psiddh]

Since this bug has already landed, opened another bug to track corresponding changes to Gaia (Nfc Manager)

https://bugzilla.mozilla.org/show_bug.cgi?id=966009