B2G NFC: Add 'CheckPermission' and AvailableIn in NFC WebIDLs.

RESOLVED FIXED in 2.1 S3 (29aug)

Status

Firefox OS
NFC
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: allstars, Unassigned)

Tracking

unspecified
2.1 S3 (29aug)
ARM
Gonk (Firefox OS)

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [p=2])

Attachments

(3 attachments, 1 obsolete attachment)

Right now the function Navigator::HasNfcSupport will also check permissions, however we should do the permission check in the WebIDL part.

Also add AvaialbleIn for these NFC WebIDLs.
Created attachment 8475090 [details] [diff] [review]
Part 1: Add CheckPermission
Created attachment 8475091 [details] [diff] [review]
Part 2: Add AvailableIn
(Assignee)

Updated

4 years ago
Attachment #8475090 - Flags: review?(bugs)
(Assignee)

Updated

4 years ago
Attachment #8475091 - Flags: review?(bugs)

Updated

4 years ago
Summary: B2G NFC: Add 'CheckPermission' and AvaialbleIn in NFC WebIDLs. → B2G NFC: Add 'CheckPermission' and AvailableIn in NFC WebIDLs.

Comment 3

4 years ago
Comment on attachment 8475090 [details] [diff] [review]
Part 1: Add CheckPermission

!!win is a bit odd, and CheckPermissions in BindingUtils.cpp does similar check
already. So you could just return true there.
Attachment #8475090 - Flags: review?(bugs) → review+

Comment 4

4 years ago
Comment on attachment 8475091 [details] [diff] [review]
Part 2: Add AvailableIn

.webidl changes are fine to me, but I think ehsan or sicking should give the
opinion why nfc stuff needs to be for certifiedapps only
Attachment #8475091 - Flags: review?(bugs) → review+
(In reply to Olli Pettay [:smaug] from comment #4)
> Comment on attachment 8475091 [details] [diff] [review]
> Part 2: Add AvailableIn
> 
> .webidl changes are fine to me, but I think ehsan or sicking should give the
> opinion why nfc stuff needs to be for certifiedapps only

Hi smaug
Actually I am not sure when to use AvailableIn.

"nfc" permission is already certified now.
http://lxr.mozilla.org/mozilla-central/source/dom/apps/src/PermissionsTable.jsm#326

It seems to me CheckPermission is already enough.
If AvailableIn is not mandatory I'll remove it and land only Part 1 patch.
Comment on attachment 8475091 [details] [diff] [review]
Part 2: Add AvailableIn

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

Hi Ehsan
Can you help to review this patch for me?
Right now NFC is certified API, so I add AvailableIn in these WebIDLs.

This came to my mind base on your review on Bug 879861 (NFC Secure Element Support ) and Bug 951976 (API for Resource Statistics), you suggested to add AvailableIn on these WebIDLS.

However there's one thing I am not quite sure.
These WebIDLs already have CheckPermissions attribute, and why should we need to add "AvailableIn" to these?

For example, nfc-manager or resource-manager is already a certified permission, so CheckPermissions="resource-manager" implied the caller is already a certified app. Do we still need to add AvailableIn="CertifiedApps" in this case?

Thank you
Attachment #8475091 - Flags: review?(ehsan)

Comment 7

4 years ago
(In reply to Yoshi Huang[:allstars.chh] from comment #6)
> Comment on attachment 8475091 [details] [diff] [review]
> Part 2: Add AvailableIn
> 
> Review of attachment 8475091 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Hi Ehsan
> Can you help to review this patch for me?
> Right now NFC is certified API, so I add AvailableIn in these WebIDLs.
> 
> This came to my mind base on your review on Bug 879861 (NFC Secure Element
> Support ) and Bug 951976 (API for Resource Statistics), you suggested to add
> AvailableIn on these WebIDLS.
> 
> However there's one thing I am not quite sure.
> These WebIDLs already have CheckPermissions attribute, and why should we
> need to add "AvailableIn" to these?
> 
> For example, nfc-manager or resource-manager is already a certified
> permission, so CheckPermissions="resource-manager" implied the caller is
> already a certified app. Do we still need to add AvailableIn="CertifiedApps"
> in this case?

Our permissions table is the ultimate source of truth when it comes to what kinds of apps can request what types of permissions.  So for example, as you suggest, if the resource-manager permission can only be granted to the certified apps, adding the [AvailableIn=CertifiedApps] on top of [CheckPermissions="resource-manager"] won't change where the API is exposed.  It would, however, make it *much* easier to read the IDL and figure out where the API is exposed.  It also makes exposing the API to let's say privileged apps require a WebIDL change, which means it will require a DOM peer review.  Both of these are great results, and I would like to make more APIs use AvailableIn for this reason.  If you know of more, please keep these kinds of patches coming.  :-)  Much appreciated.

Updated

4 years ago
Attachment #8475091 - Flags: review?(ehsan) → review+
(In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from comment #7)
> (In reply to Yoshi Huang[:allstars.chh] from comment #6)
Thanks for the detail explaination. I got it now.
Thank you.
Created attachment 8476591 [details] [diff] [review]
Part 3: Update MozNFCPeerEvent and mochitests.

Hi, Smaug
I found I forgot to update MozNFCPeerEvent.webidl, and upload another patch for that. Also update test_interfaces.html.

Could you help to review this for me ?

Thanks
Attachment #8476591 - Flags: review?(bugs)
Comment on attachment 8475090 [details] [diff] [review]
Part 1: Add CheckPermission

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

::: dom/base/Navigator.cpp
@@ +2314,5 @@
>    if (!contentHelper) {
>      return false;
>    }
>  
> +  return !!win;

if return true directly is okay,
I'll update the patch to

nsCOMPtr<nsISupports> contentHelper =  do_GetService("@mozilla.org/nfc/content-helper;1");
return !!contentHelper;
Oh, sure that is fine.
Comment on attachment 8476591 [details] [diff] [review]
Part 3: Update MozNFCPeerEvent and mochitests.

Hmm, we don't have a way to test AvailableIn stuff in test_interfaces :/
Attachment #8476591 - Flags: review?(bugs) → review+
Created attachment 8477975 [details] [diff] [review]
Part 1. Add CheckPermission v2.

comment addressed.
Attachment #8475090 - Attachment is obsolete: true
Attachment #8477975 - Flags: review+
(Assignee)

Updated

4 years ago
Whiteboard: [p=2]
Target Milestone: --- → 2.1 S3 (29aug)
You need to log in before you can comment on or make changes to this bug.