Closed
Bug 959437
Opened 11 years ago
Closed 11 years ago
Refactor NfcManager APIs and implementation details to support sendFile , notifyUserAcceptedP2P and other privileged Nfc operations
Categories
(Firefox OS Graveyard :: NFC, defect)
Firefox OS Graveyard
NFC
Tracking
(feature-b2g:2.0, tracking-b2g:backlog)
RESOLVED
FIXED
People
(Reporter: psiddh, Assigned: psiddh)
References
()
Details
(Whiteboard: [FT:RIL] sharing video/audio/image)
Attachments
(2 files, 4 obsolete files)
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.
Part 2 (DOM code), Part 3 (system - Gaia changes) of this bug will follow soon
Assignee: nobody → psiddh
Attachment #8360723 -
Flags: review?(allstars.chh)
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.
Attachment #8360723 -
Flags: review?(allstars.chh)
Comment 3•11 years ago
|
||
(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?
Updated•11 years ago
|
Flags: needinfo?(psiddh)
Updated•11 years ago
|
blocking-b2g: --- → backlog
Whiteboard: [FT:RIL] sharing video/audio/image
Attachment #8362034 -
Flags: review?(bugs)
Flags: needinfo?(psiddh)
Attachment #8360723 -
Flags: review?(allstars.chh)
Comment 5•11 years ago
|
||
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/
Attachment #8362034 -
Flags: review?(bugs) → review+
Attachment #8362034 -
Attachment is obsolete: true
Attachment #8360723 -
Flags: review?(allstars.chh) → review+
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).
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.
Flags: needinfo?(bugs)
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.
Hi, Smaug
Can you check the comment 10 for the usage of requestId ?
iIf you think it's okay then we will push it.
Flags: needinfo?(bugs)
Assignee | ||
Comment 12•11 years ago
|
||
Fixed typo. Just asking a review just to be sure.
Attachment #8362158 -
Attachment is obsolete: true
Attachment #8362158 -
Flags: review?(bugs)
Attachment #8362969 -
Flags: superreview?(bugs)
Attachment #8360723 -
Attachment description: (v1) Part 1 : Add NotifySendFileStatus interface r=allstars.chh → (v1) Part 2 : Add NotifySendFileStatus interface r=allstars.chh
Attachment #8362969 -
Attachment description: (v1c) Part 2 : Add new / refactor interfaces to MozNfcManager DOM sr, r=smaug → (v1c) Part 1 : Add new / refactor interfaces to MozNfcManager DOM sr, r=smaug
Assignee | ||
Comment 13•11 years ago
|
||
Fixed subject line
Attachment #8362969 -
Attachment is obsolete: true
Attachment #8362969 -
Flags: superreview?(bugs)
Assignee | ||
Comment 14•11 years ago
|
||
Fixed subject line
Attachment #8360723 -
Attachment is obsolete: true
Comment 15•11 years ago
|
||
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.
Attachment #8362158 -
Flags: review?(bugs)
Comment 16•11 years ago
|
||
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).
Flags: needinfo?(bugs)
Assignee | ||
Comment 17•11 years ago
|
||
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 (stephouillon@mozilla.com) 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•11 years ago
|
||
Ah, it is exposed only to system app, then it sounds ok to me.
Flags: needinfo?(bugs)
Assignee | ||
Comment 19•11 years ago
|
||
Latest Try Results: https://tbpl.mozilla.org/?tree=Try&rev=a7c4fd2e2c8f
Keywords: checkin-needed
Comment 20•11 years ago
|
||
https://hg.mozilla.org/integration/b2g-inbound/rev/a504dfc43d3f
https://hg.mozilla.org/integration/b2g-inbound/rev/1d4c70a7296e
Keywords: checkin-needed
Comment 21•11 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/a504dfc43d3f
https://hg.mozilla.org/mozilla-central/rev/1d4c70a7296e
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Assignee | ||
Comment 22•11 years ago
|
||
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
Updated•11 years ago
|
Blocks: b2g-nfc
Updated•10 years ago
|
feature-b2g: --- → 2.0
Updated•10 years ago
|
Flags: in-moztrap-
Updated•10 years ago
|
blocking-b2g: backlog → ---
tracking-b2g:
--- → backlog
You need to log in
before you can comment on or make changes to this bug.
Description
•