Spin-off from bug 983502 comment 0. Please see <https://wiki.mozilla.org/WebAPI/Navigator.hasFeature> for a description of the API. The api feature names should be implemented through WebIDL where possible to make the implementation easier to maintain. We can use the WebIDL code generator in order to generate the necessary code automatically. I'll describe how this setup would work below. Upon receiving the feature name, we will tokenize it into parts separated by dots. If the first and second parts are respectively not "api" or "worker", we resolve the promise to false. Otherwise, we hook into the generated WebIDL code. We should probably add a new WebIDL extended attribute, let's call it [FeatureDetectible] for now. For things that are implemented through WebIDL, we spray that annotation over each one of the properties we want to implement. Note that [FeatureDetectible] will only make sense if the attribute is somehow hidden from non-privileged contexts. One such way would be through [AvailableIn]. The other might be through [Func]. [Pref] should however be disregarded here because if something is only hidden behind a pref, it cannot be visible to privileged apps but not non-privileged apps by definition. If [FeatureDetectible] is specified without either [Func] or [AvailableIn], then the WebIDL compiler should emit an error (note that for some cases such as XHR.mozSystem, we incorrectly expose the attribute everywhere. We should fix that here.) For the names which are exposed through the dynamic resolve hooks (such as mozPay <http://mxr.mozilla.org/mozilla-central/source/dom/payment/Payment.manifest#3>), we probably need to code gen something similar to what xpc::IsPermitted calls on dom::WindowBinding.

Hi Ehsan, What is the main purpose of the API detection part? Can't we just check if "navigator.mozBluetooth" exist or not?

(In reply to Alphan Chen[:Alphan] from comment #1) > Hi Ehsan, > What is the main purpose of the API detection part? > Can't we just check if "navigator.mozBluetooth" exist or not? The entire purpose of this API is so that we can hide properties such as navigator.mozBluetooth for contexts where the web page lacks the required permissions in order to be able to call into the API. The fact that these properties are currently visible in non-privileged contexts but are set to null is a bug which we would like to fix after the getFeature() API is implemented. Comment 0 explains a way to implement this API through our WebIDL generated bindings layer. Please let me know if I can clarify things further.

So the presence of any of {AvailableIn,CheckPermissions,Func} marks something as being feature-detectible, and we don't care about the semantics these annotations might have, right? In other words, anything with [FeatureDetectible] and one or more of those extended attributes will resolve to true. I have an initial (very hacky) patch working along those lines, so I want to make sure I'm on the right track before I make it reviewable :)

(In reply to Reuben Morais [:reuben] from comment #3) > So the presence of any of {AvailableIn,CheckPermissions,Func} marks > something as being feature-detectible, and we don't care about the semantics > these annotations might have, right? Yes. The assumption (which I believe is a sensible one, but please let me know if you disagree!) is that AvailableIn, CheckPermissions and Func are only used in order to hide a name from specific permission contexts. Which is why we won't use Pref in this way (because it is used to enable/disable APIs unconditionally). Of course it would be worth changing what the Func for FeatureDetectible names are doing while working on this to make sure that the assumption above really holds. > In other words, anything with > [FeatureDetectible] and one or more of those extended attributes will > resolve to true. Yes, exactly! > I have an initial (very hacky) patch working along those lines, so I want to > make sure I'm on the right track before I make it reviewable :) I think you are, thanks for working on this!

Cool, thanks! > Of course it would be worth changing what the Func for FeatureDetectible > names are doing while working on this to make sure that the assumption above > really holds. Yeah, we have stuff like this today which would need to be fixed: bool Navigator::HasMobileMessageSupport(JSContext* /* unused */, JSObject* aGlobal) { #ifndef MOZ_WEBSMS_BACKEND return false; #endif

(In reply to comment #5) > Cool, thanks! > > > Of course it would be worth changing what the Func for FeatureDetectible > > names are doing while working on this to make sure that the assumption above > > really holds. > > Yeah, we have stuff like this today which would need to be fixed: > > bool > Navigator::HasMobileMessageSupport(JSContext* /* unused */, JSObject* > aGlobal) > { > #ifndef MOZ_WEBSMS_BACKEND > return false; > #endif Ah that's not ideal... FWIW, I would like to end up where specifying [Pref] and [FeatureDetectible] would be a WebIDL compiler error, but it doesn't need to happen right when we check this in for the first time.

(In reply to comment #6) > (In reply to comment #5) > > Cool, thanks! > > > > > Of course it would be worth changing what the Func for FeatureDetectible > > > names are doing while working on this to make sure that the assumption above > > > really holds. > > > > Yeah, we have stuff like this today which would need to be fixed: > > > > bool > > Navigator::HasMobileMessageSupport(JSContext* /* unused */, JSObject* > > aGlobal) > > { > > #ifndef MOZ_WEBSMS_BACKEND > > return false; > > #endif > > Ah that's not ideal... > > FWIW, I would like to end up where specifying [Pref] and [FeatureDetectible] > would be a WebIDL compiler error, but it doesn't need to happen right when we > check this in for the first time. The other thing to keep in mind is that now that we have proper review for WebIDL changes enforced through the push hook, we might just be able to rely on reviewers to enforce sanity here in the future, so it might be OK to not add that check to the compiler.

(In reply to :Ehsan Akhgari (lagging on bugmail, needinfo? me!) from comment #6) > FWIW, I would like to end up where specifying [Pref] and [FeatureDetectible] > would be a WebIDL compiler error, but it doesn't need to happen right when > we check this in for the first time. Do you mean _only_ [Pref] and [FeatureDetectible]? Should things like this be allowed, and the pref checked? [Throws, CheckPermissions="idle", Pref="dom.idle-observers-api.enabled", FeatureDetectible] void addIdleObserver(MozIdleObserver aIdleObserver);

So out of the interfaces listed in https://wiki.mozilla.org/WebAPI/Navigator.hasFeature#.22api.22_namespace, 3 are still in XPIDL land: navigator.mozAlarms, navigator.mozNetworkStats, and navigator.mozTCPSocket. All three have bugs on file for moving to WebIDL, all blocked by this bug, so I'll finish this bug without worrying about any interfaces using component registration.

(In reply to Reuben Morais [:reuben] from comment #9) > (In reply to :Ehsan Akhgari (lagging on bugmail, needinfo? me!) from comment > #6) > > FWIW, I would like to end up where specifying [Pref] and [FeatureDetectible] > > would be a WebIDL compiler error, but it doesn't need to happen right when > > we check this in for the first time. > > Do you mean _only_ [Pref] and [FeatureDetectible]? Yes. And like I said in comment 7, perhaps we don't need to worry about that... > Should things like this be allowed, and the pref checked? > > [Throws, CheckPermissions="idle", Pref="dom.idle-observers-api.enabled", > FeatureDetectible] > void addIdleObserver(MozIdleObserver aIdleObserver); Hmm, in the case of that API, the pref is set to true in all.js so it doesn't really matter. The reason why I'm not comfortable with [Pref] and [FeatureDetectible] to play with each other is that [Pref] can be used to disable APIs on desktop for example but not on b2g, which would mean that the MarketPlace app running on desktop could not detect the functionality on the same Gecko version, but since the current API is hidden behind a permisson, that is mostly a theoretical concern, hence comment 7. We should still audit anywhere that [Pref] and [FeatureDetectible] appear together to make sure that we have some kind of a sane behavior, probably on a case by case basis.

(In reply to Reuben Morais [:reuben] from comment #10) > So out of the interfaces listed in > https://wiki.mozilla.org/WebAPI/Navigator.hasFeature#.22api.22_namespace, 3 > are still in XPIDL land: navigator.mozAlarms, navigator.mozNetworkStats, and > navigator.mozTCPSocket. All three have bugs on file for moving to WebIDL, > all blocked by this bug, so I'll finish this bug without worrying about any > interfaces using component registration. I think we need to add hand-written code to feature detect them here. One reason why we cannot move those APIs to WebIDL right now is that doing so will break the MarketPlace feature detection on trunk immediately (since WebIDL will properly hide the navigator property from the contexts without the permissions) so we should do the hand-written getFeature() support for those APIs here, get the MarketPlace app migrated to use getFeature(), and then move those APIs to WebIDL while marking them [FeatureDetectible] and remove the hand-written special case to handle them in one go. Does that make sense?

(In reply to :Ehsan Akhgari (lagging on bugmail, needinfo? me!) from comment #15) > (In reply to Reuben Morais [:reuben] from comment #10) > > So out of the interfaces listed in > > https://wiki.mozilla.org/WebAPI/Navigator.hasFeature#.22api.22_namespace, 3 > > are still in XPIDL land: navigator.mozAlarms, navigator.mozNetworkStats, and > > navigator.mozTCPSocket. All three have bugs on file for moving to WebIDL, > > all blocked by this bug, so I'll finish this bug without worrying about any > > interfaces using component registration. > > I think we need to add hand-written code to feature detect them here. One > reason why we cannot move those APIs to WebIDL right now is that doing so > will break the MarketPlace feature detection on trunk immediately (since > WebIDL will properly hide the navigator property from the contexts without > the permissions) so we should do the hand-written getFeature() support for > those APIs here, get the MarketPlace app migrated to use getFeature(), and > then move those APIs to WebIDL while marking them [FeatureDetectible] and > remove the hand-written special case to handle them in one go. Does that > make sense? Sure, but it seems just as good as: land [FeatureDetectible], move APIs to WebIDL, get Marketplace app to use getFeature().

(In reply to Reuben Morais [:reuben] from comment #16) > (In reply to :Ehsan Akhgari (lagging on bugmail, needinfo? me!) from comment > #15) > > (In reply to Reuben Morais [:reuben] from comment #10) > > > So out of the interfaces listed in > > > https://wiki.mozilla.org/WebAPI/Navigator.hasFeature#.22api.22_namespace, 3 > > > are still in XPIDL land: navigator.mozAlarms, navigator.mozNetworkStats, and > > > navigator.mozTCPSocket. All three have bugs on file for moving to WebIDL, > > > all blocked by this bug, so I'll finish this bug without worrying about any > > > interfaces using component registration. > > > > I think we need to add hand-written code to feature detect them here. One > > reason why we cannot move those APIs to WebIDL right now is that doing so > > will break the MarketPlace feature detection on trunk immediately (since > > WebIDL will properly hide the navigator property from the contexts without > > the permissions) so we should do the hand-written getFeature() support for > > those APIs here, get the MarketPlace app migrated to use getFeature(), and > > then move those APIs to WebIDL while marking them [FeatureDetectible] and > > remove the hand-written special case to handle them in one go. Does that > > make sense? > > Sure, but it seems just as good as: land [FeatureDetectible], move APIs to > WebIDL, get Marketplace app to use getFeature(). Assuming all of these parts happen very soon after each other, yes. But I'm worried that some bits and pieces may get stalled so I'd rather us doing things in the right order. :-)

Comment on attachment 8429559 [details] [diff] [review] Implement the [FeatureDetectible] extended attribute, WIP Review of attachment 8429559 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/bindings/Configuration.py @@ +450,5 @@ > > + self.featureDetectibleThings = [] > + if self.interface.getExtendedAttribute("FeatureDetectible") is not None: > + assert(self.interface.getNavigatorProperty() is not None) > + self.featureDetectibleThings.append("Navigator.%s" % self.interface.getNavigatorProperty()) This is mostly great, thanks a lot! One comment that I have on your patches so far is that you should not assume Navigator here, but of course you probably know that already, because of systemXHR being on the list. :-)

(In reply to :Ehsan Akhgari (lagging on bugmail, needinfo? me!) from comment #18) > One comment that I have on your patches so far is that you should not assume > Navigator here, but of course you probably know that already, because of > systemXHR being on the list. :-) I'm assuming Navigator when [FeatureDetectible] is used on interfaces. You'll notice that I also assert the presence of [NavigatorProperty]. Also, I figured systemXHR was in that list as a future item or to be implemented manually in getFeature(), since the property is not conditionally exposed currently. (And I don't think it'd make sense for it to be conditionally exposed, since it's just a readonly flag matching what was passed in the ctor.)

(In reply to Reuben Morais [:reuben] from comment #19) > (In reply to :Ehsan Akhgari (lagging on bugmail, needinfo? me!) from comment > #18) > > One comment that I have on your patches so far is that you should not assume > > Navigator here, but of course you probably know that already, because of > > systemXHR being on the list. :-) > > I'm assuming Navigator when [FeatureDetectible] is used on interfaces. > You'll notice that I also assert the presence of [NavigatorProperty]. Fair enough. But please check with Boris or whoever else you're going to ask review from whether they're fine with this. I'm fine with it at least for now. :-) > Also, > I figured systemXHR was in that list as a future item or to be implemented > manually in getFeature(), since the property is not conditionally exposed > currently. (And I don't think it'd make sense for it to be conditionally > exposed, since it's just a readonly flag matching what was passed in the > ctor.) No, I'd like us to not expose the the systemXHR attribute at all in non-privileged contexts. The fact that we expose XMLHttpRequest.mozSystem is Web-visible, and I'd like to fix that.

(Note that the argument passed to the ctor is fine, since that is a dictionary member, and therefore invisible to Web content.)

Comment on attachment 8436067 [details] [diff] [review] Implement the [FeatureDetectible] extended attribute, v1 >+ nsCString name = NS_LossyConvertUTF16toASCII(aName); NS_LossyConvertUTF16toASCII name(aName); >+ p->MaybeResolve(mozilla::dom::IsFeatureDetectible(featureName)); Why do you need the mozilla::dom:: prefix? >+ things = CGList([CGGeneric(declare='"%s",' % t) for t in things], joiner="\n") sorted(things), I assume, since you don't want this header file to randomly be different every compile. Also, you can pass a generator as the first arg to CGList, so just drop those square brackets. >+ return true; So this will return true if the thing exists in IDL at all. Even if it's disabled (and specifically, if it's preffed off), right? Is that the behavior we're looking for here? It seems odd to me to expose preffed-off things. >+++ b/dom/bindings/parser/WebIDL.py >+ if self.getNavigatorProperty() is None: It looks like this: [FeatureDetectible,NavigatorProperty="whatever"] will cause an error, because we'll hit the FeatureDetectable bit before we've seen the NavigatorProperty. You probably want to do this check in finish() instead.

Comment on attachment 8436069 [details] [diff] [review] Use [FeatureDetectible] in some interfaces, v1 Review of attachment 8436069 [details] [diff] [review]: ----------------------------------------------------------------- Looks mostly good from an API perspective except for the comment below. Also, please note that there are a number of API entry points in <https://wiki.mozilla.org/WebAPI/Navigator.hasFeature#.22api.22_namespace> which are not ported to WebIDL yet, so initially you probably want to add a hardcoded list for those items too. :/ ::: dom/webidl/Navigator.webidl @@ +235,5 @@ > [Throws] > boolean mozIsLocallyAvailable(DOMString uri, boolean whenOffline); > }; > > +#ifdef MOZ_WEBSMS_BACKEND Hmm, we can't #ifdef this out, because then the compiler won't even see [FeatureDetectible] if MOZ_WEBSMS_BACKEND is not defined. We need to make this a runtime check instead.

Wondering if we should add MozMobileNetworkInfo to this list - which would be needed for privileged apps that need to get the mnc/mcc to determine the network?

(In reply to comment #26) > Wondering if we should add MozMobileNetworkInfo to this list - which would be > needed for privileged apps that need to get the mnc/mcc to determine the > network? MozMobileNetworkInfo is not usable where navigator.mozMobileConnections is not available, so the existing "api.window.Navigator.mozMobileConnections" should help cover that I think.

(In reply to :Ehsan Akhgari (lagging on bugmail, needinfo? me!) from comment #27) > MozMobileNetworkInfo is not usable where navigator.mozMobileConnections is > not available, so the existing "api.window.Navigator.mozMobileConnections" > should help cover that I think. api.window.Navigator.mozMobileConnections is certified, but MozMobileNetworkInfo is privileged (right?)

Comment on attachment 8436067 [details] [diff] [review] Implement the [FeatureDetectible] extended attribute, v1 Pending an answer to my question from comment 24...

(In reply to Boris Zbarsky [:bz] from comment #24) > So this will return true if the thing exists in IDL at all. Even if it's > disabled (and specifically, if it's preffed off), right? Is that the > behavior we're looking for here? It seems odd to me to expose preffed-off > things. Yes. [Pref] and [FeatureDetectible] should not be used together, and this should be enforced by reviewers. See comment 0 and comment 4. I guess I should go ahead and add the code to make that an error.

> [Pref] and [FeatureDetectible] should not be used together, and this should be enforced > by reviewers. Let's enforce it in code, please. That seems much more likely to succeed. For the Func case, do we want to require that FeatureDetectible be provided with a separate function name to call to get the boolean? Or are we going to assume that we'll never have a Func that just returns false always in a given build configuration?

(In reply to Boris Zbarsky [:bz] from comment #31) > For the Func case, do we want to require that FeatureDetectible be provided > with a separate function name to call to get the boolean? Or are we going > to assume that we'll never have a Func that just returns false always in a > given build configuration? The latter. (:Ehsan Akhgari (lagging on bugmail, needinfo? me!) from comment #4) > (In reply to Reuben Morais [:reuben] from comment #3) > > So the presence of any of {AvailableIn,CheckPermissions,Func} marks > > something as being feature-detectible, and we don't care about the semantics > > these annotations might have, right? > > Yes. The assumption (which I believe is a sensible one, but please let me > know if you disagree!) is that AvailableIn, CheckPermissions and Func are > only used in order to hide a name from specific permission contexts. > > Of course it would be worth changing what the Func for FeatureDetectible > names are doing while working on this to make sure that the assumption above > really holds.

(Please let Ehsan know if you disagree :-)

I guess now that we can combine Pref with other stuff, having a Func that checks a pref and also does other things is pointless; it should be refactored as Pref plus the other things (which may still be a Func). Alright, that works for me. Reviewers will just need to be careful, and we should document this clearly somewhere.

(In reply to Andrew Williamson [:eviljeff] from comment #28) > (In reply to :Ehsan Akhgari (lagging on bugmail, needinfo? me!) from comment > #27) > > MozMobileNetworkInfo is not usable where navigator.mozMobileConnections is > > not available, so the existing "api.window.Navigator.mozMobileConnections" > > should help cover that I think. > > api.window.Navigator.mozMobileConnections is certified, but > MozMobileNetworkInfo is privileged (right?) Not according to what I can see in the IDL... They both seem to be hidden behind the dom.mobileconnection.enabled pref.

(In reply to :Ehsan Akhgari (lagging on bugmail, needinfo? me!) from comment #35) > (In reply to Andrew Williamson [:eviljeff] from comment #28) > > (In reply to :Ehsan Akhgari (lagging on bugmail, needinfo? me!) from comment > > #27) > > > MozMobileNetworkInfo is not usable where navigator.mozMobileConnections is > > > not available, so the existing "api.window.Navigator.mozMobileConnections" > > > should help cover that I think. > > > > api.window.Navigator.mozMobileConnections is certified, but > > MozMobileNetworkInfo is privileged (right?) > > Not according to what I can see in the IDL... They both seem to be hidden > behind the dom.mobileconnection.enabled pref. I'm not sure what the pref does, but the 'mobilenetwork' permission is privileged [1] and according to MDN [2] provides access to MozMobileNetworkInfo [3] whereas the 'mobileconnection' permission is certified and provides access to MozMobileConnection [4]. I know MozMobileNetworkInfo has been used by some apps to do mnc/mcc detection, but I don't know about MozMobileConnection. [1] https://mxr.mozilla.org/mozilla-central/source/dom/apps/src/PermissionsTable.jsm#122 [2] https://developer.mozilla.org/en-US/Apps/Build/App_permissions [3] https://developer.mozilla.org/en-US/docs/Web/API/MozMobileNetworkInfo [4] https://developer.mozilla.org/en-US/docs/Web/API/MozMobileConnection

(In reply to Andrew Williamson [:eviljeff] from comment #36) > (In reply to :Ehsan Akhgari (lagging on bugmail, needinfo? me!) from comment > #35) > > (In reply to Andrew Williamson [:eviljeff] from comment #28) > > > (In reply to :Ehsan Akhgari (lagging on bugmail, needinfo? me!) from comment > > > #27) > > > > MozMobileNetworkInfo is not usable where navigator.mozMobileConnections is > > > > not available, so the existing "api.window.Navigator.mozMobileConnections" > > > > should help cover that I think. > > > > > > api.window.Navigator.mozMobileConnections is certified, but > > > MozMobileNetworkInfo is privileged (right?) > > > > Not according to what I can see in the IDL... They both seem to be hidden > > behind the dom.mobileconnection.enabled pref. > > I'm not sure what the pref does, but the 'mobilenetwork' permission is > privileged [1] and according to MDN [2] provides access to > MozMobileNetworkInfo [3] whereas the 'mobileconnection' permission is > certified and provides access to MozMobileConnection [4]. I know > MozMobileNetworkInfo has been used by some apps to do mnc/mcc detection, but > I don't know about MozMobileConnection. Oh, you're completely right! Added it to the wiki page: <https://wiki.mozilla.org/index.php?title=WebAPI%2FNavigator.hasFeature&diff=988210&oldid=968159>. Thanks for convincing me. :-)

(In reply to Boris Zbarsky [:bz] from comment #34) > I guess now that we can combine Pref with other stuff, having a Func that > checks a pref and also does other things is pointless; it should be > refactored as Pref plus the other things (which may still be a Func). Yes. Please note that as part of this bug, we should probably move towards using [CheckPermissions] and [AvailableIn] on a bunch of APIs. > Alright, that works for me. Reviewers will just need to be careful, and we > should document this clearly somewhere. Agreed. Can you think of a good place to document this? I'd be happy to do it.

> Can you think of a good place to document this? No. :(

Reuben, please make sure to mark the mobileid API (bug 1023266) as feature detectible.

Is this going to make gecko 32 (which is what will be used by FxOS 2.0, is now on aurora, and goes to beta on July 21)?

This is not a blocker since this is a feature request for 2.0, which we are not accepting into the release at this time.

Comment on attachment 8451832 [details] [diff] [review] Implement the [FeatureDetectible] extended attribute, v2 Sorry for the spam, this still needs a bit of work to handle constructors.

Slight changes to the validation, since we want to allow [FeatureDetectible] on interfaces that don't have [NavigatorProperty] too.

This covers: MozMobileNetworkInfo Navigator.addIdleObserver Navigator.getMobileIdAssertion Navigator.mozBluetooth Navigator.mozCameras Navigator.mozFMRadio Navigator.mozTime XMLHttpRequest.mozSystem This doesn't cover: Navigator.getDeviceStorage -> uses [Pref] Navigator.mozAlarms -> uses [Pref] Navigator.mozContacts -> not conditionally exposed (yet) Navigator.mozInputMethod -> [Func] that checks prefs Navigator.mozMobileConnections -> uses [Pref] Navigator.mozNetworkStats -> XPIDL Navigator.mozSms -> what is this? Navigator.mozTCPSocket -> XPIDL Navigator.push -> uses [Pref] I'll look at these individually and figure out what to do. Maybe hard coding for now is OK so we can land this quickly? Although it would be very ugly :(

Navigator.getDeviceStorage -> Looks like it shouldn't be on this list. It's always exposed, but checks a permission before actually doing work depending on the storage type. Navigator.mozAlarms -> Uses [Pref] and [CheckPermission], looks like the Pref can be removed. Bug 1036220 Navigator.mozContacts -> Not conditionally exposed (yet). We can add [AvailableIn] and [FeatureDetectible] together with this bug, and not land bug 971766 (after all that effort :/). Navigator.mozInputMethod -> [Func] that checks prefs. Used to skip permission checks during tests, tests should just set the perms themselves. Bug 1036222. Navigator.mozMobileConnections -> [Pref], removable. Bug 1036227. Navigator.mozNetworkStats -> Looks like bug 993311 can land shortly after this one lands. Navigator.push -> Uses [Pref]. Probably removable. Bug 1036229. Navigator.mozSms -> what is this? Navigator.mozTCPSocket -> XPIDL. Looks like this one will need hard coding after all. Not a lot of movement in bug 885982.

(In reply to Reuben Morais [:reuben] from comment #47) > Navigator.mozSms -> what is this? https://developer.mozilla.org/en-US/docs/Web/API/WebSMS_API afaik - but its certified so wouldn't be in the scope of this feature.

(In reply to Andrew Williamson [:eviljeff] from comment #48) > (In reply to Reuben Morais [:reuben] from comment #47) > > Navigator.mozSms -> what is this? > > https://developer.mozilla.org/en-US/docs/Web/API/WebSMS_API afaik - but its > certified so wouldn't be in the scope of this feature. Looks like mozSms was removed since I wrote that page. The equivalent API right now is navigator.mozMobileMessage, which is certified-only as Andrew mentions. Based on this, I will remove mozSms from the list.

(In reply to Reuben Morais [:reuben] from comment #47) > Navigator.getDeviceStorage -> Looks like it shouldn't be on this list. It's > always exposed, but checks a permission before actually doing work depending > on the storage type. It _should_ be on this list. That way we can hide the API entry points from the contexts where you don't have any of these permissions (such as normal web pages on b2g.) > Navigator.mozAlarms -> Uses [Pref] and [CheckPermission], looks like the > Pref can be removed. Bug 1036220 > Navigator.mozContacts -> Not conditionally exposed (yet). We can add > [AvailableIn] and [FeatureDetectible] together with this bug, and not land > bug 971766 (after all that effort :/). Yes, let's do that! Sorry for the wasted effort there... :( > Navigator.mozInputMethod -> [Func] that checks prefs. Used to skip > permission checks during tests, tests should just set the perms themselves. > Bug 1036222. > Navigator.mozMobileConnections -> [Pref], removable. Bug 1036227. > Navigator.mozNetworkStats -> Looks like bug 993311 can land shortly after > this one lands. Yes indeed. But we need to hardcode the string here and remove it as part of bug 993311 in order to keep things working during the transition. > Navigator.push -> Uses [Pref]. Probably removable. Bug 1036229. Yeah, we shouldn't need [Pref] for hiding that API. > Navigator.mozSms -> what is this? > Navigator.mozTCPSocket -> XPIDL. Looks like this one will need hard coding > after all. Not a lot of movement in bug 885982. Yep.

Comment on attachment 8452822 [details] [diff] [review] Use [FeatureDetectible] in some interfaces, v2 Review of attachment 8452822 [details] [diff] [review]: ----------------------------------------------------------------- This looks really good! Marking the patch feedback+ though because I'm not sure about your landing strategy with regards to the bug cited in comment 47. Please let me know if you're planning on landing those dependencies first and providing an updated patch here or if you'd like to hardcode the strings for now. Either would be fine for me, of course, but this patch as it currently stands is not complete yet... ::: dom/base/test/test_getFeature_with_perm.html @@ +55,5 @@ > }); > } > > +function testDetectibleAPI() { > + navigator.getFeature("api.window.Navigator.addIdleObserver").then(function(available) { I'd prefer to extend the tests to cover all of the values currently documented on the wiki. ::: dom/webidl/Navigator.webidl @@ +281,5 @@ > > #ifdef MOZ_B2G_RIL > partial interface Navigator { > + [Throws, Pref="dom.mobileconnection.enabled", > + CheckPermissions="mobileconnection mobilenetwork"] Are you going to land bug 1036227 first? If not, I'm OK with hardcoding this for now. ::: dom/webidl/XMLHttpRequest.webidl @@ +146,5 @@ > any getInterface(IID iid); > > readonly attribute boolean mozAnon; > + > + [AvailableIn=PrivilegedApps, FeatureDetectible] Hmm, don't you want "PrivilegedApps" here? This is a bit confusing to read.

(In reply to :Ehsan Akhgari (lagging on bugmail, needinfo? me!) from comment #51) > This looks really good! Marking the patch feedback+ though because I'm not > sure about your landing strategy with regards to the bug cited in comment > 47. Please let me know if you're planning on landing those dependencies > first and providing an updated patch here or if you'd like to hardcode the > strings for now. Either would be fine for me, of course, but this patch as > it currently stands is not complete yet... Yes, I think landing those first works better, they're all simple. > ::: dom/webidl/XMLHttpRequest.webidl > @@ +146,5 @@ > > any getInterface(IID iid); > > > > readonly attribute boolean mozAnon; > > + > > + [AvailableIn=PrivilegedApps, FeatureDetectible] > > Hmm, don't you want "PrivilegedApps" here? This is a bit confusing to read. I don't understand this comment.

Actually, should that be [CheckPermissions="systemXHR"] instead? Using [AvailableIn=PrivilegedApps] makes fixing all the tests quite tricky, since they expect that permission to be enough.

(In reply to :Ehsan Akhgari (lagging on bugmail, needinfo? me!) from comment #4) > (In reply to Reuben Morais [:reuben] from comment #3) > > So the presence of any of {AvailableIn,CheckPermissions,Func} marks > > something as being feature-detectible, and we don't care about the semantics > > these annotations might have, right? > > Yes. The assumption (which I believe is a sensible one, but please let me > know if you disagree!) is that AvailableIn, CheckPermissions and Func are > only used in order to hide a name from specific permission contexts. And device variants that do not have that feature. For example, flatfish, B2G on tablet, does not have RIL function and all RIL API nodes have to be hidden. So far the mechanism controlling RIL availability has to address following conditions: 1. platforms has no RIL: disabled (e.g. desktop firefox) 2. platforms has RIL but explicitly disabled: disabled (e.g. fennec) 3. platforms usually has RIL and target device has it: enabled (e.g. generic B2G phone devices) 4. platforms usually has RIL but target device has not: disabled (e.g. B2G flatfish) 5. from bug 947116, https://groups.google.com/forum/#!msg/mozilla.dev.b2g/EUK5cZp8KJo/i6ZcbgRYJzUJ , MOZ_B2G_RIL should be removed in the future. From 5., RIL components, when converted to IPDL, their DOM/IPC implementations are no longer guarded by MOZ_B2G_RIL. From 3. and 4., only their backend parts and corresponding "dom.foo.enabled" are guarded by MOZ_B2G_RIL. From 1. and 2., platforms without RIL or being explicitly turned off have MOZ_B2G_RIL undefined and "dom.foo.enabled=false". > Which > is why we won't use Pref in this way (because it is used to enable/disable > APIs unconditionally). Yes, Flatfish, Firefox OS tablet and probably Firefox OS TV, has no RIL. So, is there a way that addresses the five conditions? Actually we just need a run-time way to turn off RIL functions, and that's the purpose of "dom.foo.enabled". BTW, in addition to mozMobileConnections and mozMobileMessage, we have also "navigator.{mozCellBroadcast, mozVoicemail, mozIccManager, mozTelephony}".

(In reply to Reuben Morais [:reuben] from comment #52) > > ::: dom/webidl/XMLHttpRequest.webidl > > @@ +146,5 @@ > > > any getInterface(IID iid); > > > > > > readonly attribute boolean mozAnon; > > > + > > > + [AvailableIn=PrivilegedApps, FeatureDetectible] > > > > Hmm, don't you want "PrivilegedApps" here? This is a bit confusing to read. > > I don't understand this comment. I meant wrapping PrivilegedApps in quotes. :-) (In reply to Reuben Morais [:reuben] from comment #53) > Actually, should that be [CheckPermissions="systemXHR"] instead? Using > [AvailableIn=PrivilegedApps] makes fixing all the tests quite tricky, since > they expect that permission to be enough. Sounds good.

(In reply to Vicamo Yang [:vicamo][:vyang] from comment #54) > (In reply to :Ehsan Akhgari (lagging on bugmail, needinfo? me!) from comment > #4) > > (In reply to Reuben Morais [:reuben] from comment #3) > > > So the presence of any of {AvailableIn,CheckPermissions,Func} marks > > > something as being feature-detectible, and we don't care about the semantics > > > these annotations might have, right? > > > > Yes. The assumption (which I believe is a sensible one, but please let me > > know if you disagree!) is that AvailableIn, CheckPermissions and Func are > > only used in order to hide a name from specific permission contexts. > > And device variants that do not have that feature. For example, flatfish, > B2G on tablet, does not have RIL function and all RIL API nodes have to be > hidden. > > So far the mechanism controlling RIL availability has to address following > conditions: > > 1. platforms has no RIL: disabled (e.g. desktop firefox) > 2. platforms has RIL but explicitly disabled: disabled (e.g. fennec) > 3. platforms usually has RIL and target device has it: enabled (e.g. generic > B2G phone devices) > 4. platforms usually has RIL but target device has not: disabled (e.g. B2G > flatfish) > 5. from bug 947116, > https://groups.google.com/forum/#!msg/mozilla.dev.b2g/EUK5cZp8KJo/ > i6ZcbgRYJzUJ , MOZ_B2G_RIL should be removed in the future. > > From 5., RIL components, when converted to IPDL, their DOM/IPC > implementations are no longer guarded by MOZ_B2G_RIL. > From 3. and 4., only their backend parts and corresponding "dom.foo.enabled" > are guarded by MOZ_B2G_RIL. > From 1. and 2., platforms without RIL or being explicitly turned off have > MOZ_B2G_RIL undefined and "dom.foo.enabled=false". > > > Which > > is why we won't use Pref in this way (because it is used to enable/disable > > APIs unconditionally). > > Yes, Flatfish, Firefox OS tablet and probably Firefox OS TV, has no RIL. Hmm, OK, this is actually a great point. On platforms such as Flatfish, we need to make getFeature() resolve the promise with an undefined value because no matter what permissions your app may have, it will not have access to these APIs. But [Pref] is not going to be enough for our needs here, since it tells us nothing about whether the API is disabled because of lack of hardware support or for other reasons. I think we need another WebIDL annotation that basically works the same way as a [Pref], but have [FeatureDetecible] treat it differently. Here's a proposal: [HardwareSupport="pref.name"] will hide the API is the "pref.name" pref is disabled. If [FeatureDetecible] is specified alongside it, then getFeature() will resolve the promise to true if "pref.name" is set to true. How does that sound? > So, is there a way that addresses the five conditions? Actually we just need > a run-time way to turn off RIL functions, and that's the purpose of > "dom.foo.enabled". > > BTW, in addition to mozMobileConnections and mozMobileMessage, we have also > "navigator.{mozCellBroadcast, mozVoicemail, mozIccManager, mozTelephony}". Yeah, we don't intend to support every possible API entry point in getFeature(), we have a fixed list of supported APIs <https://wiki.mozilla.org/WebAPI/Navigator.hasFeature#.22api.22_namespace>.

(In reply to :Ehsan Akhgari (lagging on bugmail, needinfo? me!) from comment #56) > [HardwareSupport="pref.name"] will hide the API is the "pref.name" pref is > disabled. If [FeatureDetecible] is specified alongside it, then > getFeature() will resolve the promise to true if "pref.name" is set to true. > > How does that sound? That sounds good to me.

(In reply to :Ehsan Akhgari (lagging on bugmail, needinfo? me!) from comment #55) > (In reply to Reuben Morais [:reuben] from comment #52) > > > ::: dom/webidl/XMLHttpRequest.webidl > > > @@ +146,5 @@ > > > > any getInterface(IID iid); > > > > > > > > readonly attribute boolean mozAnon; > > > > + > > > > + [AvailableIn=PrivilegedApps, FeatureDetectible] > > > > > > Hmm, don't you want "PrivilegedApps" here? This is a bit confusing to read. > > > > I don't understand this comment. > > I meant wrapping PrivilegedApps in quotes. :-) Ah. I just did what I remembered from the docs: https://developer.mozilla.org/en-US/docs/Mozilla/WebIDL_bindings#AvailableIn I'll update the patch to quote that string. As for the mobileconnection stuff, I'll hardcode it for now and file bugs.

Comment on attachment 8452622 [details] [diff] [review] Implement the [FeatureDetectible] extended attribute, v3 >+++ b/dom/base/Navigator.cpp >+ // WebIDL identifiers are ASCII only Yes, but aName might not be, now that I think about it. Please use NS_ConvertUTF16toUTF8 here. >+ const char* featureName = name.get() + strlen("api.window."); If you use a single NS_NAMED_LITERAL_STRING for "api.window." you can use its .Length() here. >@@ -2957,16 +2974,23 @@ class IDLAttribute(IDLInterfaceMember): >+ elif identifier == "FeatureDetectible": >+ if not (self.getExtendedAttribute("Func") or This has the problem I mentioned in my last review: we might not have added teh Func extended attribute yet. This should be done in finish(). Similar issue in IDLMethod. r=me with those fixed.

https://hg.mozilla.org/mozilla-central/rev/3f6a695b80a4 https://hg.mozilla.org/mozilla-central/rev/f9bb67be4abd

Was this block supposed to be removed: https://hg.mozilla.org/mozilla-central/annotate/3f6a695b80a4/dom/bindings/parser/WebIDL.py#l3034 ? Please add spaces around the minus in |Substring(aName, apiWindowPrefix.Length(), aName.Length()-apiWindowPrefix.Length())|, although I personally find |StringTail(aName, aName.Length() - apiWindowPrefix.Length())| slightly more readable.

In fact, you can just do: Substring(aName, apiWindowPrefix.Length()) as well to get "all of the string starting at apiWindowPrefix.Length()". In addition to the issue Peter asked about: 1) Please use self.hasInterfaceObject() instead of self.getExtendedAttribute("NoInterfaceObject"). 2) And for the IDLInterface case, is there a reason you're not checking the "has one of Func/AvailableIn/CheckPermissions" thing? 3) In IDLAttribute.finish(), your error-reporting code will throw because it uses "attr.location" and "identifier", which do not exist in that code. 4) The code in the WebIDLError constructor invocation in the "Pref" case in IDLMethod.finish is misindented.

(In reply to Boris Zbarsky [:bz] from comment #63) > In fact, you can just do: > > Substring(aName, apiWindowPrefix.Length()) > > as well to get "all of the string starting at apiWindowPrefix.Length()". Nice, I changed it. > 2) And for the IDLInterface case, is there a reason you're not checking the > "has one of Func/AvailableIn/CheckPermissions" thing? I had misunderstood one of the use cases (MozMobileNetworkInfo). I included the check there as well. > 3) In IDLAttribute.finish(), your error-reporting code will throw because it > uses "attr.location" and "identifier", which do not exist in that code. Copy-paste fail, fixed. Sorry for the mess, and thanks for double checking the commit. https://hg.mozilla.org/integration/b2g-inbound/rev/9d9e97d42eb8

Forgot to clear NI with comment 64.

I wrote some tests for things that should behave differently in privileged/certified app contexts. Currently the worker parts are failing due to bug 1040333.

Adding [FeatureDetectible] to some of the interfaces in the list turned out to be quite tricky, so the list of hardcoded endpoints is quite long, but this lets us land and then transition things to using the extended attribute. I also extended the test to cover all but two endpoints. I'm hoping bug 1040333 won't be too hard, or else XMLHttpRequest.mozSystem will need to be hardcoded too.

Comment on attachment 8459030 [details] [diff] [review] Use [FeatureDetectible] where possible, hardcode tricky cases Review of attachment 8459030 [details] [diff] [review]: ----------------------------------------------------------------- ::: content/base/test/test_XHR_parameters.html @@ +52,5 @@ > return; > } > ok(xhr instanceof XMLHttpRequest, "passed " + JSON.stringify(value)); > > + let expectedAnon = Boolean(value && value.mozAnon); Not sure why you're moving this line... ::: content/base/test/test_bug927196.html @@ +53,5 @@ > + SpecialPowers.pushPermissions([{'type': 'systemXHR', 'allow': true, 'context': document}], function() { > + SpecialPowers.pushPrefEnv({ > + set: [["dom.ignore_webidl_scope_checks", true]], > + }, function() { > + window.location.reload(); I think this will confuse our test harness. Can we do it without reloading the test page? ::: dom/base/Navigator.cpp @@ +1541,5 @@ > const nsAString& featureName = Substring(aName, apiWindowPrefix.Length()); > + > + // Temporary hardcoded entry points due to technical constraints > + if (featureName.EqualsLiteral("Navigator.mozTCPSocket")) { > + p->MaybeResolve(Preferences::GetBool("dom.mozTCPSocket.enabled")); Shouldn't you resolve all of these to true? ::: dom/base/test/test_getFeature_with_perm.html @@ +107,5 @@ > + promises.push(navigator.getFeature("api.window." + v.name)); > + }); > + Promise.all(promises).then(function(values) { > + for (var i = 0; i < values.length; ++i) { > + is(values[i], APIEndPoints[i].enabled, Can you please test true versus undefined explicitly here? I think that requires writing the check in terms of ok() and using ===. ::: dom/permission/tests/test_systemXHR.html @@ +27,5 @@ > > var gData = [ > { > perm: ["systemXHR"], > + settings: [["dom.ignore_webidl_scope_checks", true]], Note that I think some of these hunks such as this one will need to go away once you switch to using [CheckPermissions] for systemXHR. ::: dom/webidl/XMLHttpRequest.webidl @@ +146,5 @@ > any getInterface(IID iid); > > readonly attribute boolean mozAnon; > + > + [AvailableIn="PrivilegedApps", FeatureDetectible] The last time we discussed this, we agreed that this should use [CheckPermission] (that is what the XHR implementation actually check.) See comment 55 please. ::: dom/workers/test/test_xhr_parameters.js @@ +46,2 @@ > var expectedAnon = false; > + var expectedSystem = undefined; Hmm, it would be much better if we could test |"mozSystem" in xhr| instead...

(In reply to :Ehsan Akhgari (lagging on bugmail, needinfo? me!) from comment #69) > ::: content/base/test/test_XHR_parameters.html > @@ +52,5 @@ > > return; > > } > > ok(xhr instanceof XMLHttpRequest, "passed " + JSON.stringify(value)); > > > > + let expectedAnon = Boolean(value && value.mozAnon); > > Not sure why you're moving this line... Because the comment is talking about `expectedSystem', not `expectedAnon'. A bit confusing. > ::: content/base/test/test_bug927196.html > @@ +53,5 @@ > > + SpecialPowers.pushPermissions([{'type': 'systemXHR', 'allow': true, 'context': document}], function() { > > + SpecialPowers.pushPrefEnv({ > > + set: [["dom.ignore_webidl_scope_checks", true]], > > + }, function() { > > + window.location.reload(); > > I think this will confuse our test harness. Can we do it without reloading > the test page? Yeah, I can use an iframe. > ::: dom/base/Navigator.cpp > @@ +1541,5 @@ > > const nsAString& featureName = Substring(aName, apiWindowPrefix.Length()); > > + > > + // Temporary hardcoded entry points due to technical constraints > > + if (featureName.EqualsLiteral("Navigator.mozTCPSocket")) { > > + p->MaybeResolve(Preferences::GetBool("dom.mozTCPSocket.enabled")); > > Shouldn't you resolve all of these to true? These prefs are being used to disable APIs on specific platforms. We don't want to resolve with true if someone does |navigator.getFeature("api.window.Navigator.mozTCPSocket");| on desktop, for example. > ::: dom/permission/tests/test_systemXHR.html > @@ +27,5 @@ > > > > var gData = [ > > { > > perm: ["systemXHR"], > > + settings: [["dom.ignore_webidl_scope_checks", true]], > > Note that I think some of these hunks such as this one will need to go away > once you switch to using [CheckPermissions] for systemXHR. > > ::: dom/webidl/XMLHttpRequest.webidl > @@ +146,5 @@ > > any getInterface(IID iid); > > > > readonly attribute boolean mozAnon; > > + > > + [AvailableIn="PrivilegedApps", FeatureDetectible] > > The last time we discussed this, we agreed that this should use > [CheckPermission] (that is what the XHR implementation actually check.) See > comment 55 please. Yes, I got invested in fixing the [AvailableIn] failures and forgot to talk to you about that. The problem is that [CheckPermission] doesn't work on workers, and unlike [AvailableIn], bug 1040333, it's complicated to fix. I guess I'll hardcode XMLHttpRequest too :(

(In reply to Reuben Morais [:reuben] from comment #70) > > ::: dom/base/Navigator.cpp > > @@ +1541,5 @@ > > > const nsAString& featureName = Substring(aName, apiWindowPrefix.Length()); > > > + > > > + // Temporary hardcoded entry points due to technical constraints > > > + if (featureName.EqualsLiteral("Navigator.mozTCPSocket")) { > > > + p->MaybeResolve(Preferences::GetBool("dom.mozTCPSocket.enabled")); > > > > Shouldn't you resolve all of these to true? > > These prefs are being used to disable APIs on specific platforms. We don't > want to resolve with true if someone does > |navigator.getFeature("api.window.Navigator.mozTCPSocket");| on desktop, for > example. Well, that's only true for dom.mobileconnection.enabled though, right? mozTCPSocket, for example, doesn't have anything b2g/gonk specific. > > ::: dom/permission/tests/test_systemXHR.html > > @@ +27,5 @@ > > > > > > var gData = [ > > > { > > > perm: ["systemXHR"], > > > + settings: [["dom.ignore_webidl_scope_checks", true]], > > > > Note that I think some of these hunks such as this one will need to go away > > once you switch to using [CheckPermissions] for systemXHR. > > > > ::: dom/webidl/XMLHttpRequest.webidl > > @@ +146,5 @@ > > > any getInterface(IID iid); > > > > > > readonly attribute boolean mozAnon; > > > + > > > + [AvailableIn="PrivilegedApps", FeatureDetectible] > > > > The last time we discussed this, we agreed that this should use > > [CheckPermission] (that is what the XHR implementation actually check.) See > > comment 55 please. > > Yes, I got invested in fixing the [AvailableIn] failures and forgot to talk > to you about that. The problem is that [CheckPermission] doesn't work on > workers, and unlike [AvailableIn], bug 1040333, it's complicated to fix. I > guess I'll hardcode XMLHttpRequest too :( OK, no problem!

Comment on attachment 8461558 [details] [diff] [review] Use [FeatureDetectible] where possible, hardcode tricky cases, v2 Review of attachment 8461558 [details] [diff] [review]: ----------------------------------------------------------------- Thanks for sticking with me thus far! I think this is good as is, let's get it landed. I'm going to ping the Market Place team to ask them to not use this API on places other than Firefox OS for now. ::: dom/base/Navigator.cpp @@ +1557,5 @@ > + return p.forget(); > + } > + > + if (featureName.EqualsLiteral("Navigator.mozContacts")) { > + p->MaybeResolve(Preferences::GetBool("dom.mozContacts.enabled")); This pref is a lie, see bug 1043562, please resolve to true unconditionally.

Comment on attachment 8461558 [details] [diff] [review] Use [FeatureDetectible] where possible, hardcode tricky cases, v2 Olli, I need a review from a DOM peer for the Navigator.webidl changes.

So do we still believe this is going to remain anywhere close to accurate? The review comments here sure don't inspire confidence.

So this is error prone. Why would anyone use FeatureDetectible annotation when adding a new API? I mean, all the tests etc probably set the right pref on and the API is just there. Why aren't all the CheckPermissions="foo" and AvailableIn things automatically FeatureDetectible? (Func is too generic, it is not for permission stuff only) and then perhaps have FeatureDetectible for special cases where we want always-existing API to show up in hasFeature too? (I don't understand why the name is getFeature and not hasFeature)

(In reply to Olli Pettay [:smaug] from comment #76) > So this is error prone. Why would anyone use FeatureDetectible annotation > when > adding a new API? I mean, all the tests etc probably set the right pref on > and the > API is just there. Yes, it is definitely error prone, but there is no silver bullets here. The only alternative is for Market Place to maintain a huge table of UA strings for Firefox OS devices to the APIs available on those devices. [FeatureDetectible] is supposed to be used for APIs that are hidden behind a privilege requirement (such as [AvailableIn] or [CheckPermission]) or hardware requirements ([Pref] for now, but hopefully [HardwarePref] or some such going forward). It is supposed to be enforced through DOM peer reviews. That was one of my goals for writing the WebIDL review push hook. > Why aren't all the CheckPermissions="foo" and AvailableIn things > automatically FeatureDetectible? Because Market Place doesn't need to feature detect everything in this way, and I prefer to keep the number of strings we support limited to what we absolutely need. > (Func is too generic, it is not for permission stuff only) and then perhaps > have > FeatureDetectible for special cases where we want > always-existing API to show up in hasFeature too? > > (I don't understand why the name is getFeature and not hasFeature) Because it needs to return both boolean and numeric values.

(In reply to :Ehsan Akhgari (lagging on bugmail, needinfo? me!) from comment #77) > > Why aren't all the CheckPermissions="foo" and AvailableIn things > > automatically FeatureDetectible? > > Because Market Place doesn't need to feature detect everything in this way, > and I prefer to keep the number of strings we support limited to what we > absolutely need. How can then a random DOM peer know whether FeatureDetectible should be there or not? Some consistency here, pretty please. > > (I don't understand why the name is getFeature and not hasFeature) > > Because it needs to return both boolean and numeric values. but it still doesn't return any feature. So get* sounds wrong.

(In reply to comment #78) > (In reply to :Ehsan Akhgari (lagging on bugmail, needinfo? me!) from comment > #77) > > > Why aren't all the CheckPermissions="foo" and AvailableIn things > > > automatically FeatureDetectible? > > > > Because Market Place doesn't need to feature detect everything in this way, > > and I prefer to keep the number of strings we support limited to what we > > absolutely need. > > How can then a random DOM peer know whether FeatureDetectible should be there > or not? > Some consistency here, pretty please. Based on the criteria in comment 76. If you can think of a way to make it more obvious I'd be happy to. Code comments perhaps? > > > (I don't understand why the name is getFeature and not hasFeature) > > > > Because it needs to return both boolean and numeric values. > but it still doesn't return any feature. So get* sounds wrong. Do you think there is really any value to have a getFeature() that works for hardware.memory and a hasFeature that works for api.* and manifest.*?

(In reply to :Ehsan Akhgari (lagging on bugmail, needinfo? me!) from comment #79) > > Based on the criteria in comment 76. If you can think of a way to make it > more obvious I'd be happy to. Code comments perhaps? As I said, I'd make all the CheckPermission and AvailableIn things feature detectable, at least by default. Or what is the criteria what should be exposed via has/getFeature and what shouldn't be? > Do you think there is really any value to have a getFeature() that works for > hardware.memory and a hasFeature that works for api.* and manifest.*? Yes. API sanity. Somewhat similar reason why many programming languages have boolean, not only int type, even though int would work just fine.

(In reply to Olli Pettay [:smaug] from comment #80) > As I said, I'd make all the CheckPermission and AvailableIn things feature > detectable, at least by default. That sounds good to me, AFAIK the only reason the list is curated right now is to shorten the scope of the bug/patch. > > Do you think there is really any value to have a getFeature() that works for > > hardware.memory and a hasFeature that works for api.* and manifest.*? > Yes. API sanity. Somewhat similar reason why many programming languages have > boolean, not > only int type, even though int would work just fine. Can we do that in a follow up or something? The name was reviewed by a DOM peer in bug 983502, and then again in attachment 8452622 [details] [diff] [review].

(In reply to comment #80) > (In reply to :Ehsan Akhgari (lagging on bugmail, needinfo? me!) from comment > #79) > > > > Based on the criteria in comment 76. If you can think of a way to make it > > more obvious I'd be happy to. Code comments perhaps? > As I said, I'd make all the CheckPermission and AvailableIn things feature > detectable, at least by default. > Or what is the criteria what should be exposed via has/getFeature and what > shouldn't be? It's arbitrary, Market Place has a list of things that they need to detect, and I just used that as the list. > > Do you think there is really any value to have a getFeature() that works for > > hardware.memory and a hasFeature that works for api.* and manifest.*? > Yes. API sanity. Somewhat similar reason why many programming languages have > boolean, not > only int type, even though int would work just fine. :( I don't think that will make this API any saner, but I don't feel strongly either way. Over to Jonas since IIRC he suggested to get rid of hasFeature().

Note that the reason I do not want to add anything that has [AvailableIn] or [CheckPermissions] to be detectible through this API because 1) nobody will be using them, and 2) it sends the wrong signal to the Market Place team since we have had cases where they needed to detect third party keyboards for example, and they used mozInputMethod as a proxy for that, and it didn't work out because third party keyboards were disabled in Gaia. I would like to avoid this potential confusion by having the Market Place team come to us with use cases of which functionality they want to detect so that we can ensure that whatever getFeature() retuns is the right proxy for their use case. Making getFeature() handle all of these API names makes that process completely break down.

(In reply to :Ehsan Akhgari (lagging on bugmail, needinfo? me!) from comment #82) > It's arbitrary, Market Place has a list of things that they need to detect, > and I just used that as the list. > arbitrary sounds rather bad. If we aim for some API which will be exposed, eventually, to web, "arbitrary" certainly isn't something I'd expect to see in the API spec. > > > Do you think there is really any value to have a getFeature() that works for > > > hardware.memory and a hasFeature that works for api.* and manifest.*? > > Yes. API sanity. Somewhat similar reason why many programming languages have > > boolean, not > > only int type, even though int would work just fine. > > :( I don't think that will make this API any saner, but I don't feel > strongly either way. Over to Jonas since IIRC he suggested to get rid of > hasFeature(). (I don't feel very strongly about this, although getFeature returning information whether certain API is supported sure sounds wrong)

(In reply to comment #84) > (In reply to :Ehsan Akhgari (lagging on bugmail, needinfo? me!) from comment > #82) > > It's arbitrary, Market Place has a list of things that they need to detect, > > and I just used that as the list. > > > arbitrary sounds rather bad. If we aim for some API which will be exposed, > eventually, to web, > "arbitrary" certainly isn't something I'd expect to see in the API spec. I am now less confident that we can expose this to the Web than I was when I first proposed it. It's not entirely clear how we can expose it unless if we are willing to do so without getting anybody else's agreement. And the implementation difficulties that we have faced so far makes me much less comfortable with this than what I had imagined before. > > > > Do you think there is really any value to have a getFeature() that works for > > > > hardware.memory and a hasFeature that works for api.* and manifest.*? > > > Yes. API sanity. Somewhat similar reason why many programming languages have > > > boolean, not > > > only int type, even though int would work just fine. > > > > :( I don't think that will make this API any saner, but I don't feel > > strongly either way. Over to Jonas since IIRC he suggested to get rid of > > hasFeature(). > (I don't feel very strongly about this, although getFeature returning > information whether certain API is supported sure sounds wrong) I don't feel strongly either, and am happy to use whatever name you suggest instead. The name is the least of my worries here. :-)

It was not my suggestion to get rid of hasFeature(). I believe I suggested keeping it. I can live with either solution though, I don't feel strongly.

Comment on attachment 8461558 [details] [diff] [review] Use [FeatureDetectible] where possible, hardcode tricky cases, v2 So how did we then end up to this API design? Also, I'm not really happy to add API which requires DOM peers to know whatever random requirements marketplace team has.

(In reply to Olli Pettay [:smaug] from comment #87) > Comment on attachment 8461558 [details] [diff] [review] > Use [FeatureDetectible] where possible, hardcode tricky cases, v2 > > So how did we then end up to this API design? Digging up the history here, it seems like Alphan suggested this (bug 983502 comment 54), sorry for remembering incorrectly. And like I said, I have 0 objections to having two functions instead of one. > Also, I'm not really happy to add API which requires DOM peers to know > whatever random requirements marketplace team has. I assume this means that you do not agree with the criteria in comment 77? Can you please tell me what would be acceptable to you? I am not really sure if I understand your position, do you prefer us to recognize all possible strings that can be interpreted as "interface.member" in getFeature()?

(In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from comment #88) > I assume this means that you do not agree with the criteria in comment 77? > Can you please tell me what would be acceptable to you? why we can't just map CheckPermission/AvailableIn to be FeatureDetectible. In comment 77 you said you don't prefer to do that, but didn't explain why.

I and Olli discussed this on IRC, and Olli is requesting the following changes to the API: 1. Split getFeature() back into getFeature() and hasFeature(). I did that, please see the wiki page: <https://wiki.mozilla.org/WebAPI/Navigator.hasFeature> 2. Make anything which has either [CheckPermissions] or [AvailableIn] supported by hasFeature(). That means for cases such as: interface Foo { [CheckPermission] void f(); }; We would make hasFeature() recognize "api.window.Foo.f". For cases such as: [CheckPermission] interface Foo { void f(); boolean attribute m; }; We would make hasFeature() recognize the following: "api.window.Foo", "api.window.Foo.f" and "api.window.Foo.m". This is done so that if someone starts to depend on "api.window.Foo.f", for example, their code won't break if we move the [CheckPermission] to f() itself in the future. Reuben, can you please change your patch to implement the above? I'm so sorry that you have to abandon your existing work here. :(

Does this correctly deal with NavigatorProperty BTW?

Comment on attachment 8463676 [details] [diff] [review] Remove FeatureDetectible, add things with CheckPermissions or AvailableIn to the feature list instead, and move code to Navigator.hasFeature Could you please test some navigator properties like mozChromeNotifications. In fact I think we should test all the properties you have in IsFeatureDetectible, and those hard coded cases too. One thing came to my mind... does MarketPlace care about only certain AvailableIn values? (I don't know what all apps can be installed from MarketPlace)

(In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from comment #93) > Does this correctly deal with NavigatorProperty BTW? Yes, see Navigator.push being included in the codegen, for example. (In reply to Olli Pettay [:smaug] from comment #94) > Could you please test some navigator properties like mozChromeNotifications. > In fact I think we should test all the properties you have in > IsFeatureDetectible, > and those hard coded cases too. Testing everything is complicated because I need to duplicate whatever logic exists in the build system for those interfaces in the test. If the test was preprocessed, this would be easier, but I can't easily figure out if MOZ_B2G_FM is enabled in a mochitest, for example. > One thing came to my mind... does MarketPlace care about only certain > AvailableIn values? (I don't know what all apps can be installed from > MarketPlace) Hmm, good point, Marketplace likely only cares about [AvailableIn=PrivilegedApps], since we don't allow installing certified apps yet.

See comment 95.

So should hasFeature return only AvailableIn=PrivilegedApps stuff, not also Certified (or whatever the right term is.)

(In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from comment #90) > 2. Make anything which has either [CheckPermissions] or [AvailableIn] > supported by hasFeature(). Does that mean that if we make an API more widely available (eg by removing the AvailableIn annotation) then hasFeature will stop working for that API?

(In reply to Peter Van der Beken [:peterv] from comment #98) > (In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from > comment #90) > > 2. Make anything which has either [CheckPermissions] or [AvailableIn] > > supported by hasFeature(). > > Does that mean that if we make an API more widely available (eg by removing > the AvailableIn annotation) then hasFeature will stop working for that API? Yes, that's what would happen with the current approach.

What is the notification process for APIs becoming more widely available? I'm not sure there is one and I'm concerned this would result in a broken experience for marketplace users each time that happened.

(In reply to Wil Clouser [:clouserw] from comment #100) > What is the notification process for APIs becoming more widely available? > I'm not sure there is one and I'm concerned this would result in a broken > experience for marketplace users each time that happened. There is no formal notification process. But we are trying to get better at sending out the intent emails <https://wiki.mozilla.org/WebAPI/ExposureGuidelines#Email_templates> so I think the best thing that you can do for now is to subscribe to dev-platform and watch the intent emails. Also, we will add specific tests for everything that Market Place depends on, so that we won't accidentally break getFeature() with one of those.

I think our original plan was to have getFeature() just work regardless of API permission level. However, I can see why you'd want to encourage the regular detection methods for non-privileged APIs but I am worried about getting notified about those changes and having the Marketplace break. Writing unit tests that have a big message that says to email us sounds about as reasonable as it gets though. Anyway, carry on, just voicing my concerns. :) While I'm here - is there a reason you wouldn't include the certified APIs in this as well as privileged?

(In reply to Wil Clouser [:clouserw] from comment #102) > While I'm here - is there a reason you wouldn't include the certified APIs > in this as well as privileged? Because certified apps can't be installed via Marketplace, and we want to leak as few proprietary bits to the Web as possible, and limit the scope of this API. Does that make sense?

(In reply to Wil Clouser [:clouserw] from comment #102) > I am worried about getting notified about those changes and having the Marketplace break. > Writing unit tests that have a big message that says to email us sounds > about as reasonable as it gets though. Test failures and/or the review process are not particularly good at this type of thing (see test_interfaces.html), but at the same time it's the best we've got (see test_interfaces.html). I'll make the appropriate changes to codegen and to the test.

(In reply to Reuben Morais [:reuben] from comment #103) > (In reply to Wil Clouser [:clouserw] from comment #102) > > While I'm here - is there a reason you wouldn't include the certified APIs > > in this as well as privileged? > > Because certified apps can't be installed via Marketplace, and we want to > leak as few proprietary bits to the Web as possible, and limit the scope of > this API. Does that make sense? Yep, fair enough.

https://hg.mozilla.org/mozilla-central/rev/e5a8a5b86bbd https://hg.mozilla.org/mozilla-central/rev/edcb55e950a3

Thanks *a lot* Reuben!