983502 - Implement and expose the feature detection API to privileged apps

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[(no longer active)]

Please see <https://wiki.mozilla.org/WebAPI/Navigator.hasFeature> for a description of the API.

Note to the implementer: The hardware.memory feature name should probably be implemented as a special case without any generic machinery at this point.

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.  Boris to fill in the details there.

If something doesn't make sense here, please ni? me and I'll be happy to clarify.

Comment 1

[(no longer active)]

Boris, can you please sanity check comment 0?

Comment 2

[(no longer active)]

Oh, and like I mentioned in the wiki page, this should be hidden behind a permission (let's call it "feature-detection" or something).

Comment 3

[Boris Zbarsky [:bzbarsky]]

Hmm.  I forgot that the resolve hook stuff is not declared in WebIDL at all.  That makes it hard to generate anything for it.  :(

We should just try to move away from having that resolve hook...

The rest of comment 0 seems sensible.

Comment 4

[(no longer active)]

(In reply to Boris Zbarsky [:bz] from comment #3)
> Hmm.  I forgot that the resolve hook stuff is not declared in WebIDL at all.
> That makes it hard to generate anything for it.  :(
> 
> We should just try to move away from having that resolve hook...

Agreed, adding getters to Navigator.webidl for that stuff should be easy.

Comment 5

[Boris Zbarsky [:bzbarsky]]

The only reason we haven't so far is I was given to understand it was a hard requirement that people be able to add these things without modifying C++ code.  There was some talk of codegenning a superclass for Navigator which would implement the relevant getters...

Comment 6

[Boris Zbarsky [:bzbarsky]]

E.g. using something similar to the existing NavigatorProperty setup.

Comment 7

[Boris Zbarsky [:bzbarsky]]

I guess we could in fact use that already, _if_ we also move the relevant things from being XPCOM components to being JS-implemented WebIDL.  Which we want to do anyway....

Comment 8

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

For the initial release, lets just do a hardcoded list. I.e. nothing that gets information from WebIDL files. We only have a few days to get this thing implemented, reviewed and landed.

Comment 9

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

We need this to enable the marketplace to not display apps that won't run well on the Tarako.

Comment 10

[Jason Smith [:jsmith]]

See https://bugzilla.mozilla.org/show_bug.cgi?id=983880#c8.

Comment 11

[Jason Smith [:jsmith]]

(In reply to Jason Smith [:jsmith] from comment #10)
> See https://bugzilla.mozilla.org/show_bug.cgi?id=983880#c8.

See https://bugzilla.mozilla.org/show_bug.cgi?id=983880#c15.

Comment 12

[(no longer active)]

So based on the recent discussion, I'm not sure if we're still considering this for 1.3t or not.  The implementation strategy (the proper implementation versus the hardcoded list).  Jonas, what's your take on how we should proceed on the implementation here?

Comment 13

[Eric Chou [:ericchou] [:echou]]

Hi Ehsan,

> So based on the recent discussion, I'm not sure if we're still considering
> this for 1.3t or not.  The implementation strategy (the proper
> implementation versus the hardcoded list).  Jonas, what's your take on how
> we should proceed on the implementation here?

Alphan Chen and Alan Huang in my team were about to raise a topic about implementing "DeviceInformation" API, which was inspired from Cordova's Device API, to solve this kind of problem. After an offline discussion with Jonas, we decided to take over the implementation and target to finish this by Firefox OS v1.5.

Comment 14

[Eric Chou [:ericchou] [:echou]]

(In reply to Eric Chou [:ericchou] [:echou] from comment #13)
> Hi Ehsan,
> 
> > So based on the recent discussion, I'm not sure if we're still considering
> > this for 1.3t or not.  The implementation strategy (the proper
> > implementation versus the hardcoded list).  Jonas, what's your take on how
> > we should proceed on the implementation here?
> 
> Alphan Chen and Alan Huang in my team were about to raise a topic about
> implementing "DeviceInformation" API, which was inspired from Cordova's
> Device API, to solve this kind of problem. After an offline discussion with
> Jonas, we decided to take over the implementation and target to finish this
> by Firefox OS v1.5.

I forgot to mention more details. We think Feature Detection API can cover the functionality of our Device Information API, and discussion of Feature Detection API has been done for a while, so we decided to go for your solution. 

One more thing I'd like to discuss is, we should try to make this API compatible with Cordova's Device API (http://cordova.apache.org/docs/en/3.0.0/cordova_device_device.md.html#Device). Therefore I'd like to propose having a new namespace called 'device', which includes features such as 'platform', 'version' and 'model'.

Comment 15

[Jose Manuel Cantera]

(In reply to Eric Chou [:ericchou] [:echou] from comment #14)
> (In reply to Eric Chou [:ericchou] [:echou] from comment #13)
> > Hi Ehsan,
> > 
> > > So based on the recent discussion, I'm not sure if we're still considering
> > > this for 1.3t or not.  The implementation strategy (the proper
> > > implementation versus the hardcoded list).  Jonas, what's your take on how
> > > we should proceed on the implementation here?
> > 
> > Alphan Chen and Alan Huang in my team were about to raise a topic about
> > implementing "DeviceInformation" API, which was inspired from Cordova's
> > Device API, to solve this kind of problem. After an offline discussion with
> > Jonas, we decided to take over the implementation and target to finish this
> > by Firefox OS v1.5.
> 
> I forgot to mention more details. We think Feature Detection API can cover
> the functionality of our Device Information API, and discussion of Feature
> Detection API has been done for a while, so we decided to go for your
> solution. 
> 
> One more thing I'd like to discuss is, we should try to make this API
> compatible with Cordova's Device API
> (http://cordova.apache.org/docs/en/3.0.0/cordova_device_device.md.
> html#Device). Therefore I'd like to propose having a new namespace called
> 'device', which includes features such as 'platform', 'version' and 'model'.

Hi guys,

There is an oldie but a goodie piece of standards work I participated in that can also be useful to define the scope and vocabulary for this API

http://www.w3.org/TR/ddr-core-vocabulary/

Also another piece of work I've edited long time ago

http://www.w3.org/TR/2009/WD-dcontology-20090616/ 

Also it is interesting to read the introduction and conceptualization, particularly:

http://www.w3.org/TR/DDR-Simple-API/#sec-vocabularies

and the associated API:

http://www.w3.org/TR/DDR-Simple-API

Comment 16

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

(In reply to Eric Chou [:ericchou] [:echou] from comment #14)
> I forgot to mention more details. We think Feature Detection API can cover
> the functionality of our Device Information API, and discussion of Feature
> Detection API has been done for a while, so we decided to go for your
> solution. 

Awesome!

> One more thing I'd like to discuss is, we should try to make this API
> compatible with Cordova's Device API
> (http://cordova.apache.org/docs/en/3.0.0/cordova_device_device.md.
> html#Device). Therefore I'd like to propose having a new namespace called
> 'device', which includes features such as 'platform', 'version' and 'model'.

That would be fine with me. So your proposal is that we'd have "device.platform", "device.version" etc? This sounds ok to me, but since this information can be used for fingerprinting (device.model in particular) we should definitely stick this behind a permission for now.

If we do this, we should probably change "hardware.memory" to "device.memory" as well.

Comment 17

[Wil Clouser [:clouserw]]

(In reply to Jonas Sicking (:sicking) from comment #16)
> That would be fine with me. So your proposal is that we'd have
> "device.platform", "device.version" etc? This sounds ok to me, but since
> this information can be used for fingerprinting (device.model in particular)
> we should definitely stick this behind a permission for now.

Since we have a few more weeks to finish this for 1.5 I'd like to enter the discussion with this being a public API.  Being behind a permission means it won't work for android, desktop, or any other marketplace and isn't very webby.  I've mentioned this in the privacy review bug as well.

Comment 18

[(no longer active)]

I'd *really* prefer if we did not scope creep this bug to cover all of the "detection" scenarios that people want.  Specifically, I'm not exactly sure what extensions to the current proposal comment 14 is suggesting.

Can you please start a thread on dev-webapi about what you're trying to achieve?  Also can you please let me know if you're planning to do something other than what I suggested in comment 0?

In the mean time, I don't think we should necessarily block this bug on those possible future extensions.  Most of comment 0 at least can be implemented in a generic way.

Comment 19

[(no longer active)]

(In reply to Wil Clouser [:clouserw] from comment #17)
> (In reply to Jonas Sicking (:sicking) from comment #16)
> > That would be fine with me. So your proposal is that we'd have
> > "device.platform", "device.version" etc? This sounds ok to me, but since
> > this information can be used for fingerprinting (device.model in particular)
> > we should definitely stick this behind a permission for now.
> 
> Since we have a few more weeks to finish this for 1.5 I'd like to enter the
> discussion with this being a public API.  Being behind a permission means it
> won't work for android, desktop, or any other marketplace and isn't very
> webby.  I've mentioned this in the privacy review bug as well.

(In reply to Wil Clouser [:clouserw] from comment #6)
> It sounds like this is being bumped from 1.3T+ which means we have a few
> more weeks to discuss the proposal.  If that is happening I'd like us to
> consider it *not* being a privileged API which would let us use it for
> desktop and android as well as FxOS.

Part of that decision would be based on what the privacy team thinks about this proposal.  But exposing things to normal Web content also entails other requirements, in particular we would need to take the proposal to other browser vendors, wait for their feedback (which may take a while as we suspect that they won't be interested in solving this use case for now), write a proper specification for it, etc.  See <https://wiki.mozilla.org/WebAPI/ExposureGuidelines> for the full details.  But I just want to be clear that doing all of that work before we start to implement this API _may_ mean that we end up missing the 1.5 train.  Also, I'm interested to get some feedback from the usage of this API from the MarketPlace app.  Therefore I think we want to implement this as a privileged API in the beginning and then work on our way towards exposing it to all Web content.

Comment 20

[Alphan Chen [:alchen]]

(In reply to :Ehsan Akhgari (lagging on bugmail, needinfo? me!) from comment #18)
> I'd *really* prefer if we did not scope creep this bug to cover all of the
> "detection" scenarios that people want.  Specifically, I'm not exactly sure
> what extensions to the current proposal comment 14 is suggesting.
> 
> Can you please start a thread on dev-webapi about what you're trying to
> achieve?  Also can you please let me know if you're planning to do something
> other than what I suggested in comment 0?
> 
> In the mean time, I don't think we should necessarily block this bug on
> those possible future extensions.  Most of comment 0 at least can be
> implemented in a generic way.

Hi Ehsan,
I understand your concern.
We will start the implementation from "hardware.memory"(or"device.memory").
For other part, we will have other bug or mail thread to discuss.

Comment 21

[Alphan Chen [:alchen]]

Attachment: (WIP-0326) navigator.getFeature and navigator.hasFeature implementation

Hi Ehsan,
this is a WIP patch as first version.

Summarize the patch and future work:
1. We will read ro.device.memory to know the true memory of this device.
   ro.device.memory need to be set by vendor.
   For now, I set the prop by myself.
2. Navigator::GetFeature return a string.
   Navigator::HasFeature return a bool.
3. For this version, we do all the logic in Navigator.cpp
   We will move the logic out of Navigator.cpp to avoid too many modification on this file.

Comment 22

[Alphan Chen [:alchen]]

Attachment: Test sample in bluetooth setting

I add the test code in gaia/apps/settings/js/bluetooth.js.
With patched gecko(attachment 8396988 [details] [diff] [review]) and patched gaia(this file),we can see the following output when entering bluetooth setting and enable/disable Discoverable.

(logcat)
I/GeckoDump(  659): [Alphan-testing] hasFeature(hardware.memory):true
I/GeckoDump(  659): [Alphan-testing] hasFeature(hardware.tv):false
I/GeckoDump(  659): We are in a memory constrained world!
I/GeckoDump(  659): [Alphan-testing] getFeature(hardware.memory):128

Comment 23

[Alphan Chen [:alchen]]

Attachment: Set ro.device.memory as 128

I use this patch to set ro.device.memory as 256.
The property will be set when adjusting the time.

Comment 24

[Alphan Chen [:alchen]]

Comment on attachment 8396990 [details]
Set ro.device.memory as 128

typo

Comment 25

[(no longer active)]

Comment on attachment 8396988 [details] [diff] [review]
(WIP-0326) navigator.getFeature and navigator.hasFeature implementation

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

::: dom/base/Navigator.cpp
@@ +91,4 @@
>  
>  #include "nsIUploadChannel2.h"
>  #include "nsFormData.h"
> +#include <cutils/properties.h>

This and the code using it won't compile on all of our platforms.  You need to hide it behind a MOZ_WIDGET_GONK #ifdef or something.  Same for the code using property_get() below.

@@ +1430,5 @@
> +
> +  char val[64];
> +  bool feature = false;
> +
> +  if (aName.EqualsLiteral("hardware.memory")) {

hasFeature() should not attempt to handle hardware.memory, please see the wiki page.

@@ +1445,5 @@
> +
> +already_AddRefed<Promise>
> +Navigator::GetFeature(const nsAString& aName, ErrorResult& aRv)
> +{
> +  if (!mWindow || !mWindow->GetDocShell()) {

How can we have a null mWindow here?  The API is not exposed to workers yet, and even once we do that we still don't need a window here.  I think you should remove this check and also remove the [Throws] annotation in WebIDL.

@@ +1454,5 @@
> +  char val[64];
> +  nsAutoString featureValue(NS_LITERAL_STRING("false"));
> +
> +  if (aName.EqualsLiteral("hardware.memory")) {
> +    property_get("ro.device.memory", val, "not support");

What is the advantage of using the Android property APIs over just using /proc/meminfo (similar to <http://mxr.mozilla.org/mozilla-central/source/xpcom/base/SystemMemoryReporter.cpp#198>)?  The latter will work in all Linux flavors including b2g.

Also, it's not clear what unit ro.device.memory returns its result in.

Also, I couldn't find any documentation regarding where ro.device.memory is supported.

Also, looking at the implementation of property_get(), it seems that it potentially blocks on the property server for reading the property.  That means that calling into navigator.hasFeature() will block the main thread which is very bad, we should make sure to read this either in a background thread or using non-blocking IO (the latter doesn't seem to be supported by the properties API.)

The other thing is that this property would not change during the lifetime of the system, so once we retrieve it, we can just cache it for the future usages.

@@ +1460,5 @@
> +
> +  nsCOMPtr<nsIGlobalObject> go = do_QueryInterface(mWindow);
> +  nsRefPtr<Promise> p = new Promise(go);
> +  featureValue.AssignLiteral(val);
> +  p->MaybeResolve(featureValue);

You need to convert this value to an integer.  I think if we encounter an error retrieving this, we should reject the promise.

::: dom/webidl/Navigator.webidl
@@ +81,5 @@
>  };
>  
> +[NoInterfaceObject]
> +interface NavigatorFeatures {
> +  [Throws]

You need to hide these two methods behind a permission named "feature-detection", which means that you need to add a Func annotation here pointing it to a function which performs the check.  See other functions on Navigator with names such as HasFooSupport() for other places where we do this.

Comment 26

[Alphan Chen [:alchen]]

(In reply to :Ehsan Akhgari (lagging on bugmail, needinfo? me!) from comment #25)
> > +  char val[64];
> > +  nsAutoString featureValue(NS_LITERAL_STRING("false"));
> > +
> > +  if (aName.EqualsLiteral("hardware.memory")) {
> > +    property_get("ro.device.memory", val, "not support");
> 
> What is the advantage of using the Android property APIs over just using
> /proc/meminfo (similar to
> <http://mxr.mozilla.org/mozilla-central/source/xpcom/base/
> SystemMemoryReporter.cpp#198>)?  The latter will work in all Linux flavors
> including b2g.
> 
for /proc/meminfo, we can only know the memory which belong to framework.
Take hamachi for example, it is a 256MB device.
But we can only see "184764 kB" in meminfo.
MemTotal:         184764 kB
MemFree:           21032 kB
> Also, it's not clear what unit ro.device.memory returns its result in.
> 
> Also, I couldn't find any documentation regarding where ro.device.memory is
> supported.
> 
This property is not defined by anyone now.
In our internal discussion, we think use property which is set by vendor to get a real number of whole device should be reasonable and easy to implement.

So do you think we should read meminfo and show the total memory belong to framework?

Comment 27

[Alphan Chen [:alchen]]

(In reply to :Ehsan Akhgari (lagging on bugmail, needinfo? me!) from comment #25)
> Comment on attachment 8396988 [details] [diff] [review]

> 
> @@ +1460,5 @@
> > +
> > +  nsCOMPtr<nsIGlobalObject> go = do_QueryInterface(mWindow);
> > +  nsRefPtr<Promise> p = new Promise(go);
> > +  featureValue.AssignLiteral(val);
> > +  p->MaybeResolve(featureValue);
> 
> You need to convert this value to an integer.  I think if we encounter an
> error retrieving this, we should reject the promise.
> 

So all the getFeature should return integer?
Or it just for hardware.memory case?

Comment 28

[(no longer active)]

(In reply to Alphan Chen[:Alphan] from comment #26)
> (In reply to :Ehsan Akhgari (lagging on bugmail, needinfo? me!) from comment
> #25)
> > > +  char val[64];
> > > +  nsAutoString featureValue(NS_LITERAL_STRING("false"));
> > > +
> > > +  if (aName.EqualsLiteral("hardware.memory")) {
> > > +    property_get("ro.device.memory", val, "not support");
> > 
> > What is the advantage of using the Android property APIs over just using
> > /proc/meminfo (similar to
> > <http://mxr.mozilla.org/mozilla-central/source/xpcom/base/
> > SystemMemoryReporter.cpp#198>)?  The latter will work in all Linux flavors
> > including b2g.
> > 
> for /proc/meminfo, we can only know the memory which belong to framework.
> Take hamachi for example, it is a 256MB device.
> But we can only see "184764 kB" in meminfo.
> MemTotal:         184764 kB
> MemFree:           21032 kB
> > Also, it's not clear what unit ro.device.memory returns its result in.
> > 
> > Also, I couldn't find any documentation regarding where ro.device.memory is
> > supported.
> > 
> This property is not defined by anyone now.
> In our internal discussion, we think use property which is set by vendor to
> get a real number of whole device should be reasonable and easy to implement.
> 
> So do you think we should read meminfo and show the total memory belong to
> framework?

Hmm...  That's an interesting question, not sure if I know a good answer.  Michael, do you know what MemTotal is supposed to be?  Just the amount of memory exposed to the user space or something?  Is there a better way for us to get our hands on the amount of physical memory?

Comment 29

[(no longer active)]

(In reply to Alphan Chen[:Alphan] from comment #27)
> (In reply to :Ehsan Akhgari (lagging on bugmail, needinfo? me!) from comment
> #25)
> > Comment on attachment 8396988 [details] [diff] [review]
> 
> > 
> > @@ +1460,5 @@
> > > +
> > > +  nsCOMPtr<nsIGlobalObject> go = do_QueryInterface(mWindow);
> > > +  nsRefPtr<Promise> p = new Promise(go);
> > > +  featureValue.AssignLiteral(val);
> > > +  p->MaybeResolve(featureValue);
> > 
> > You need to convert this value to an integer.  I think if we encounter an
> > error retrieving this, we should reject the promise.
> > 
> 
> So all the getFeature should return integer?
> Or it just for hardware.memory case?

Well, hardware.memory is the only thing we're planning to support in getFeature() for now, but yes, the idea is that hasFeature will be used for booleans and getFeature() for other things.

Comment 30

[(no longer active)]

Another idea for figuring out the amount of actual physical memory on the device is to read MemTotal from /proc/meminfo and bump it up to the next power of two, and report that as the result.

Comment 31

[Alphan Chen [:alchen]]

(In reply to :Ehsan Akhgari (lagging on bugmail, needinfo? me!) from comment #30)
> Another idea for figuring out the amount of actual physical memory on the
> device is to read MemTotal from /proc/meminfo and bump it up to the next
> power of two, and report that as the result.

I don't think this idea is reasonable.
The memory occupied by kernel and radio depend on vendor.
The difference between two vendors maybe huge.
We can not figure out the actual physical memory by "MemTotal from /proc/meminfo".

Comment 32

[Alphan Chen [:alchen]]

Attachment: (WIP-0402) navigator.getFeature and navigator.hasFeature implementationBug-983502-WIP-0402.patch

Summarize this patch.
1. Add "#ifdef MOZ_WIDGET_GONK" and "#endif" for specific B2G code.
2. Remove hardware.memory support from HasFeature()
3. Remove the mWindow checking from HasFeature() and GetFeature
4. Keep the way to get the size of memory. "getprop(ro.device.memory)"
   I can change the design if we have the conclusion of this part.
   Queue the value of memory once we got it.
5. For the return type of GetFeature, I still keep it as string type.
   String type can contain both integer and string.
   Developer should know the expected type of return value.
   For example, when using GetFeature(hardware.memory),
     developer know the return value would be an integer.
   We can use "parseInt" to convert the type from string to integer.
6. Add Func annotation for hasFeature and getFeature in Navigator.webidl.
[NoInterfaceObject]
interface NavigatorFeatures {
  [Func="Navigator::HasFeatureDetectionSupport"]
  Promise hasFeature(DOMString name);
  [Func="Navigator::HasFeatureDetectionSupport"]
  Promise getFeature(DOMString name);
};
7. Add "HasFeatureDetectionSupport" function and "feature-detection" permission

Comment 33

[(no longer active)]

(In reply to Alphan Chen[:Alphan] from comment #31)
> (In reply to :Ehsan Akhgari (lagging on bugmail, needinfo? me!) from comment
> #30)
> > Another idea for figuring out the amount of actual physical memory on the
> > device is to read MemTotal from /proc/meminfo and bump it up to the next
> > power of two, and report that as the result.
> 
> I don't think this idea is reasonable.
> The memory occupied by kernel and radio depend on vendor.
> The difference between two vendors maybe huge.

Does that mean that you expect some vendors to use more than half of the physical memory for the kernel and radio?

But even if that is the case, I don't understand why that matters.  As far as an application running in Gecko is concerned, if you have two devices, both with 180MB of RAM dedicated to userspace, but one with 256MB physical memory and the other with 512MB physical memory, we should treat both as a 256MB device, because they both give the userspace around the same ballpark of physical memory.  What do you think?

Comment 34

[(no longer active)]

Comment on attachment 8400458 [details] [diff] [review]
(WIP-0402) navigator.getFeature and navigator.hasFeature implementationBug-983502-WIP-0402.patch

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

Looks mostly good, but please see my comments below.

::: dom/apps/src/PermissionsTable.jsm
@@ +327,5 @@
>                               certified: PROMPT_ACTION
>                             },
> +                           "feature-detection": {
> +                             app: DENY_ACTION,
> +                             privileged: DENY_ACTION,

We want to allow privileged apps to have this permission.

::: dom/base/Navigator.cpp
@@ +1444,5 @@
> +
> +  nsCOMPtr<nsIGlobalObject> go = do_QueryInterface(mWindow);
> +  nsRefPtr<Promise> p = new Promise(go);
> +  p->MaybeResolve(feature);
> +  return p.forget();

So, there is nothing wrong with the implementation of this function, but I don't think it's a good idea to land what we have here with a hasFeature() implementation which doesn't do anything.  Are you planning to work on something along the lines of what I described in comment 0 in this bug for hasFeature()? If yes, great!  If not, please file a follow-up for that, and let's not add the hasFeature() function in this patch.

@@ +1470,5 @@
> +#endif
> +
> +  nsCOMPtr<nsIGlobalObject> go = do_QueryInterface(mWindow);
> +  nsRefPtr<Promise> p = new Promise(go);
> +  p->MaybeResolve(featureValue);

getFeature() should reject the promise for unknown arguments.

Comment 35

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

> getFeature() should reject the promise for unknown arguments.

I think getFeature should resolve the promise with the <undefined> value if it doesn't recognize the feature-name. Otherwise callers have to add both success and error callbacks even to handle the basic case. We should think of error callbacks as exceptions, and exceptions should only be used in exceptional cases.

Comment 36

[(no longer active)]

(In reply to comment #35)
> > getFeature() should reject the promise for unknown arguments.
> 
> I think getFeature should resolve the promise with the <undefined> value if it
> doesn't recognize the feature-name. Otherwise callers have to add both success
> and error callbacks even to handle the basic case. We should think of error
> callbacks as exceptions, and exceptions should only be used in exceptional
> cases.

That sounds good to me!  Edited the wiki page accordingly.

Comment 37

[Alphan Chen [:alchen]]

(In reply to :Ehsan Akhgari (lagging on bugmail, needinfo? me!) from comment #33)
> (In reply to Alphan Chen[:Alphan] from comment #31)
> > (In reply to :Ehsan Akhgari (lagging on bugmail, needinfo? me!) from comment
> > #30)
> > > Another idea for figuring out the amount of actual physical memory on the
> > > device is to read MemTotal from /proc/meminfo and bump it up to the next
> > > power of two, and report that as the result.
> > 
> > I don't think this idea is reasonable.
> > The memory occupied by kernel and radio depend on vendor.
> > The difference between two vendors maybe huge.
> 
> Does that mean that you expect some vendors to use more than half of the
> physical memory for the kernel and radio?
No, I just think one vendor may use 100mb for their kernel and radio, another may use 60mb.
So if we just bump the total memory up to the next power of two as real physical memory, it is not reasonable.
> 
> But even if that is the case, I don't understand why that matters.  As far
> as an application running in Gecko is concerned, if you have two devices,
> both with 180MB of RAM dedicated to userspace, but one with 256MB physical
> memory and the other with 512MB physical memory, we should treat both as a
> 256MB device, because they both give the userspace around the same ballpark
> of physical memory.  What do you think?
I think we should report either memory owned by usersapce or total physical memory.
The following are pro and cons for each case:
1. report memory owned by userspace:
   pros: It is what we can use in userspace. 
         It work in all Linux flavors including b2g.
         No change needed from our vendor.
   cons: If we show this memory as total memory, user may confuse about the value.
         In hamachi case, user think they buy a 256mb device.
         However, they only see 184764 kB on device info.
   conclusion: If we use this choice, we should not show this value.
2. total physical memory
   pros: We can show this information in device info page. No confusion.
   cons: Need to set property(ro.device.memory) by vendor.

Comment 38

[Alphan Chen [:alchen]]

(In reply to :Ehsan Akhgari (lagging on bugmail, needinfo? me!) from comment #34)
> Comment on attachment 8400458 [details] [diff] [review]
> (WIP-0402) navigator.getFeature and navigator.hasFeature
> implementationBug-983502-WIP-0402.patch
> 
> Review of attachment 8400458 [details] [diff] [review]:
> -----------------------------------------------------------------

> ::: dom/base/Navigator.cpp
> @@ +1444,5 @@
> > +
> > +  nsCOMPtr<nsIGlobalObject> go = do_QueryInterface(mWindow);
> > +  nsRefPtr<Promise> p = new Promise(go);
> > +  p->MaybeResolve(feature);
> > +  return p.forget();
> 
> So, there is nothing wrong with the implementation of this function, but I
> don't think it's a good idea to land what we have here with a hasFeature()
> implementation which doesn't do anything.  Are you planning to work on
> something along the lines of what I described in comment 0 in this bug for
> hasFeature()? If yes, great!  If not, please file a follow-up for that, and
> let's not add the hasFeature() function in this patch.

Could you explain what's your plan on hasFeature()?
I read comment 0.But I don't get the point you want.

Also we need to make a choice about how to get the value of hardware.memory.
Thanks.

Comment 39

[(no longer active)]

(In reply to Alphan Chen[:Alphan] from comment #37)
> > But even if that is the case, I don't understand why that matters.  As far
> > as an application running in Gecko is concerned, if you have two devices,
> > both with 180MB of RAM dedicated to userspace, but one with 256MB physical
> > memory and the other with 512MB physical memory, we should treat both as a
> > 256MB device, because they both give the userspace around the same ballpark
> > of physical memory.  What do you think?
> I think we should report either memory owned by usersapce or total physical
> memory.
> The following are pro and cons for each case:
> 1. report memory owned by userspace:
>    pros: It is what we can use in userspace. 
>          It work in all Linux flavors including b2g.
>          No change needed from our vendor.
>    cons: If we show this memory as total memory, user may confuse about the
> value.
>          In hamachi case, user think they buy a 256mb device.
>          However, they only see 184764 kB on device info.
>    conclusion: If we use this choice, we should not show this value.
> 2. total physical memory
>    pros: We can show this information in device info page. No confusion.
>    cons: Need to set property(ro.device.memory) by vendor.

I think the important thing to keep in mind is _why_ we want to expose this value.  The reason is to give app developers a ballpark figure around the amount of memory that the device has available for running apps.  To give an example of what I mean, consider the case of the current Tarako devices versus a hypothetical future device that has 1GB of physical memory, 900MB of which has been reserved for things outside of userspace.  From the perspective of an app developer targeting various devices, these two devices have around the same amount of memory, and if they have a memory intensive app (such as a large 3D game) they can expect neither of the devices to be able to run that app.  In other words, #2 above is a non-goal.

Also, note that we absolutely do not want to expose the exact amount of available memory to the userspace, since that will probably be a unique value which will impose a fingerprinting issue as it will allow people to uniquely identify individual devices.

Therefore, I still think my suggestion of reading MemTotal and rounding it up to the next power of two is a good solution here.

Comment 40

[(no longer active)]

(In reply to Alphan Chen[:Alphan] from comment #38)
> (In reply to :Ehsan Akhgari (lagging on bugmail, needinfo? me!) from comment
> #34)
> > Comment on attachment 8400458 [details] [diff] [review]
> > (WIP-0402) navigator.getFeature and navigator.hasFeature
> > implementationBug-983502-WIP-0402.patch
> > 
> > Review of attachment 8400458 [details] [diff] [review]:
> > -----------------------------------------------------------------
> 
> > ::: dom/base/Navigator.cpp
> > @@ +1444,5 @@
> > > +
> > > +  nsCOMPtr<nsIGlobalObject> go = do_QueryInterface(mWindow);
> > > +  nsRefPtr<Promise> p = new Promise(go);
> > > +  p->MaybeResolve(feature);
> > > +  return p.forget();
> > 
> > So, there is nothing wrong with the implementation of this function, but I
> > don't think it's a good idea to land what we have here with a hasFeature()
> > implementation which doesn't do anything.  Are you planning to work on
> > something along the lines of what I described in comment 0 in this bug for
> > hasFeature()? If yes, great!  If not, please file a follow-up for that, and
> > let's not add the hasFeature() function in this patch.
> 
> Could you explain what's your plan on hasFeature()?
> I read comment 0.But I don't get the point you want.

I was talking about the WebIDL machinery that will allow us to mark an interface or method as [FeatureDetectible] in the WebIDL and generate code in http://mxr.mozilla.org/mozilla-central/source/dom/bindings/Codegen.py to implement hasFeature() based on that.  I'd be glad to provide more details, but I need to know more about which parts of that plan you're not clear on.

Comment 41

[Alphan Chen [:alchen]]

Attachment: (0408 WIP) navigator.getFeature implementation (support "hardware.memory")

1. Use "round the value of MemToal up to the next power of two" to be the return value of getFeature(hardware.memory)

2. resolve the promise with <undefined> if it doesn't recognize the feature-name.

3. Remove hasFeature() and will file a follow-up for it.

Comment 42

[Alphan Chen [:alchen]]

Attachment: Test sample in bluetooth setting

Output Message:
I/GeckoDump(  512): [Alphan-testing] getFeature(hardware.memory):256
I/GeckoDump(  512): We are on hamachi!
I/GeckoDump(  512): [Alphan] resolve promise:undefined
I/GeckoDump(  512): [Alphan] tv is undefined

Comment 43

[(no longer active)]

Comment on attachment 8403109 [details] [diff] [review]
(0408 WIP) navigator.getFeature implementation (support "hardware.memory")

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

Please request review from a DOM peer when you address the comment below.  Please also add some tests for this.

::: dom/base/Navigator.cpp
@@ +1445,5 @@
> +
> +  if (aName.EqualsLiteral("hardware.memory")) {
> +    static int memLevel = 1;
> +    if (memLevel == 1) {
> +      FILE* f = fopen("/proc/meminfo", "r");

This code should be Linux only.

Comment 44

[Alphan Chen [:alchen]]

Attachment: (0409) navigator.getFeature implementation (support hardware.memory)

Hi Jonas,
could you review the latest patch of getFeature implementation?

Now we only support "hardware.memory".
The value is based on the suggestion from Ehasn. (round the value of MemToal up to the next power of two)

For unrecognized feature-name, we will resolve the promise with <undefined> value.

Comment 45

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 8403775 [details] [diff] [review]
(0409) navigator.getFeature implementation (support hardware.memory)

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

Ehsan has been following this more than I have.

Comment 46

[Alphan Chen [:alchen]]

Hi Ehsan,
I'm not sure who is the appropriate dom peer for this patch.
Could you review the patch or dispatch to appropriate dom peer?
Thanks.

Comment 47

[Alphan Chen [:alchen]]

Attachment: (ver 0409) Testcase for navigator.getFeature

Hi Ehasn,
here is the test case for getFeature.
Please take a look.
Thanks a lot.

Comment 48

[(no longer active)]

Comment on attachment 8403775 [details] [diff] [review]
(0409) navigator.getFeature implementation (support hardware.memory)

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

Looks mostly good.  Please ask :bz for review on the final version of the patch.

::: dom/base/Navigator.cpp
@@ +1442,5 @@
> +  ThreadsafeAutoJSContext cx;
> +  JS::Rooted<JS::Value> val(cx);
> +  val.setUndefined();
> +
> +#if defined(ANDROID) || defined(LINUX)

I think you can just check XP_LINUX which should encompass all of the Linux variants we care about.

@@ +1453,5 @@
> +        fscanf(f, "MemTotal: %d kB\n", &memTotal);
> +        fclose(f);
> +        memTotal /= 1024;
> +
> +        while(memLevel <= memTotal) {

Please add a comment stating what this code is doing.

@@ +1458,5 @@
> +          memLevel *= 2;
> +        }
> +        val.setInt32(memLevel);
> +      }
> +    } else { // memLevel != 1

This comment is unnecessary.

Comment 49

[(no longer active)]

Comment on attachment 8403908 [details] [diff] [review]
(ver 0409) Testcase for navigator.getFeature

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

::: dom/base/test/test_bug983502.html
@@ +12,5 @@
> +<body>
> +<a target="_blank" href="https://bugzilla.mozilla.org/show_bug.cgi?id=983502">Mozilla Bug 983502</a>
> +<script type="application/javascript">
> +
> +  SpecialPowers.addPermission("feature-detection", true, document);

You want to call SimpleTest.waitForExplicitFinish() and SimpleTest.finish() in this test.  It's not good to assume that the promises are immediately resolved.

@@ +15,5 @@
> +
> +  SpecialPowers.addPermission("feature-detection", true, document);
> +
> +  var mem;
> +  navigator.getFeature("hardware.memory").then(function(mem) {

Does this even work?  AFAIK the permission needs to be set before the navigator object is resolved, which means you need to use something like SpecialPowers.pushPermissions() and in the callback, load a new iframe where you call navigator.getFeature().  Otherwise I would expect navigator.getFeature("...") to throw because getFeature should not be available on navigator.

And in fact, please add another test to ensure that navigator.getFeature doesn't exist if you don't have the permission.

@@ +19,5 @@
> +  navigator.getFeature("hardware.memory").then(function(mem) {
> +    if (typeof mem === 'undefined') {
> +      ok(true, "Resolve the Promise with undefined value (hardware.memory)");
> +    } else {
> +      ok(mem, "Resolve the memory Promise. mem=" + mem + "MB");

I'd like us to have this check a bit more strict.  Please do something like this:

  var isLinux = !!navigator.platform.contains("Linux");
  var isAndroid = !!navigator.userAgent.contains("Android");
  var isB2g = !isAndroid && /Mobile|Tablet/.test(navigator.userAgent);

  if (isLinux || isAndroid || isB2g) {
    // expect an integer > 0
  } else {
    // expect undefined
  }

Of course, this code is untested.  :-)

Comment 50

[(no longer active)]

Alphan, did you file the follow-up bug for hasFeature()?  There are a number of dependencies set on this bug which we can only fix once we have hasFeature() to give the MarketPlace app a good way of feature detecting stuff; it would be nice to make those bugs depend on the hasFeature() bug if this bug is not going to be it.  Thanks!

Comment 51

[Alphan Chen [:alchen]]

Attachment: (0410) navigator.getFeature implementation (support hardware.memory)

Hi Boris,
could you review patch of getFeature implementation?
I included the suggestion of comment 48.
Thanks.

Comment 52

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8404403 [details] [diff] [review]
(0410) navigator.getFeature implementation (support hardware.memory)

>+++ b/dom/base/Navigator.cpp
>+  JS::Rooted<JS::Value> val(cx);

This is OK or now because Promise.h doesn't have an ArgumentToJSValue that takes int32_t.  Bug 994453 will add such a beastie, after which we should use it here.  Please file a followup bug depending on bug 994453?

>+  val.setUndefined();

Not needed; Rooted<Value> preinitializes to undefined.

>+      FILE* f = fopen("/proc/meminfo", "r");

So the first time this is asked for we'll do mainthread I/O?  I can probably live with that in the interests of getting something landed as long as we have a followup to fix that, but we should get that followup.  The whole point of using a Promise here is to be able to do this in the background.

>+        int memTotal;
>+        fscanf(f, "MemTotal: %d kB\n", &memTotal);

If this returns 0, we should probably reject the promise instead of using the uninitialized value?

>+        while(memLevel <= memTotal) {

Space before '(', please.

>+        val.setInt32(memLevel);

So the point of having two copies of that line is that if the fopen() failed we resolve with undefined?  Should we reject instead?


r=me with the above addressed.

Comment 53

[Boris Zbarsky [:bzbarsky]]

Oh, also, if the feature name is not supported, is the intent to resolve with undefined, or to reject?

Comment 54

[Alphan Chen [:alchen]]

For hasFeature(), I am wondering whether we really need it or not.

<Reason>
In present design of getFeature, we can resolve the promise with any type.
Since user should know the expected type of the feature name they use, I think we can use "Resove with boolean" to replace "hasFeature()".

<proposal>
Resolve the promise of getFeature by the following:
1. Resolve with <undefined> (unrecognized feature-name)
2. Resolve with interger (hardware.memory)
3. Resolve with boolean  (for example: userdefined.keyboard)
4. Resolve with string   (for example: device.version or device.platform)

<Discussion>
Is there any other usage that "getFeature" cannot cover?
What advantage can we get from "hasFeaure"?

In my opinion, we need both hasFeature() and getFeature() in one situation:
There are some features which are supported by both hasFeature() and getFeature().
However, we don't have this kind of use case, do we?

Comment 55

[Alphan Chen [:alchen]]

(In reply to Boris Zbarsky [:bz] from comment #53)
> Oh, also, if the feature name is not supported, is the intent to resolve
> with undefined, or to reject?

Based on comment 35 and comment 36, we will resolve with undefined.

Comment 56

[Alphan Chen [:alchen]]

(In reply to Boris Zbarsky [:bz] from comment #52)
> Comment on attachment 8404403 [details] [diff] [review]
> (0410) navigator.getFeature implementation (support hardware.memory)
> 
> >+++ b/dom/base/Navigator.cpp
> >+  JS::Rooted<JS::Value> val(cx);
> 
> This is OK or now because Promise.h doesn't have an ArgumentToJSValue that
> takes int32_t.  Bug 994453 will add such a beastie, after which we should
> use it here.  Please file a followup bug depending on bug 994453?
> 
> >+  val.setUndefined();
> 
> Not needed; Rooted<Value> preinitializes to undefined.
> 

Based on comment 35 and comment 36, we need to resolve the promise with undefined when feature name is not supported. I still need to use "JS::Rooted<JS::Value> val(cx);" for this purpose.
Is there anything I can refine when bug 994453 is landed?
If yes, I will file a followup bug depending on bug 994453.

Comment 57

[(no longer active)]

(In reply to Boris Zbarsky [:bz] from comment #53)
> Oh, also, if the feature name is not supported, is the intent to resolve
> with undefined, or to reject?

Resolve with undefined.

Comment 58

[(no longer active)]

(In reply to Alphan Chen[:Alphan] from comment #54)
> For hasFeature(), I am wondering whether we really need it or not.
> 
> <Reason>
> In present design of getFeature, we can resolve the promise with any type.
> Since user should know the expected type of the feature name they use, I
> think we can use "Resove with boolean" to replace "hasFeature()".
> 
> <proposal>
> Resolve the promise of getFeature by the following:
> 1. Resolve with <undefined> (unrecognized feature-name)
> 2. Resolve with interger (hardware.memory)
> 3. Resolve with boolean  (for example: userdefined.keyboard)
> 4. Resolve with string   (for example: device.version or device.platform)
> 
> <Discussion>
> Is there any other usage that "getFeature" cannot cover?
> What advantage can we get from "hasFeaure"?
> 
> In my opinion, we need both hasFeature() and getFeature() in one situation:
> There are some features which are supported by both hasFeature() and
> getFeature().
> However, we don't have this kind of use case, do we?

That's actually a good point.  I'm fine with just having getFeature() and moving the list of stuff under hasFeature() to that function.  Jonas, how does that sound?

If Jonas agrees to, I'll edit the wiki page.

Comment 59

[Boris Zbarsky [:bzbarsky]]

> Based on comment 35 and comment 36, we need to resolve the promise with undefined

Sure.  My point is you don't need the setUndefined() call.

> Is there anything I can refine when bug 994453 is landed?

It's landed.  So yes.  You can now do things like p->MaybeResolve(memLevel).

Whether that's useful depends on the behavior you want when the fopen() fails, actually,  Right now you end up resolving with undefined, but I suspect rejecting would make more sense there.

Comment 60

[Boris Zbarsky [:bzbarsky]]

> as long as we have a followup to fix that

Which we need anyway to make this work in the seccomp sandbox, no?  Have to do the open() in a non-sandboxed process...

Comment 61

[(no longer active)]

(In reply to comment #60)
> > as long as we have a followup to fix that
> 
> Which we need anyway to make this work in the seccomp sandbox, no?  Have to do
> the open() in a non-sandboxed process...

Yeah that's a great point actually.

BTW on the question of main-thread IO, I managed to convince myself that this should be fine since the read will be answered by a function in the kernel (https://github.com/torvalds/linux/blob/master/fs/proc/meminfo.c#L23) once it goes through the vfs layer and will never hit the disk.  The only place where we might hit the disk is resolving the dirent for /, but hopefully that would be in the page cache.

Comment 62

[Alphan Chen [:alchen]]

Attachment: (0414) navigator.getFeature implementation (support hardware.memory)

Hi Boris,
I based on the latest code base to implement this feature. (included bug 994453)
I will file a follow up bug for fopen case.
Could you take a look again and see if there is any needed improvement?

Here is the try server result.
https://tbpl.mozilla.org/?tree=Try&rev=a567697c55c5

Comment 63

[Alphan Chen [:alchen]]

Attachment: (ver 0414) Testcase for navigator.getFeature

Hi Ehsan,
I did the following test as you mentioned in comment 49.
1. with/without permission
2. Parameter as recognize feature-name and unrecognized feature-name
3. check the result on different platform (Linux based v.s. non-Linux based)

Comment 64

[Alphan Chen [:alchen]]

This try server result is based on Attachment #8406035 [details] [diff] and Attachment #8406040 [details] [diff].
https://tbpl.mozilla.org/?tree=Try&rev=a567697c55c5

Comment 65

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8406035 [details] [diff] [review]
(0414) navigator.getFeature implementation (support hardware.memory)

> +    // resolve with <undefined> for not support feature name

How about:

  // resolve with <undefined> because the feature name is not supported

?

r=me with that, though we should really be rejecting with Error instances, not strings.  A followup for having a sane way to do that would be good.

Comment 66

[(no longer active)]

Comment on attachment 8406040 [details] [diff] [review]
(ver 0414) Testcase for navigator.getFeature

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

Thanks, this looks great!  I have a few minor comments below.  r=me with those fixed.

::: dom/base/test/test_hasFeature_with_perm.html
@@ +12,5 @@
> +<body>
> +<a target="_blank" href="https://bugzilla.mozilla.org/show_bug.cgi?id=983502">Mozilla Bug 983502</a>
> +<script type="application/javascript">
> +
> +function testSupport() {

Nit: testSupported.

@@ +28,5 @@
> +      ok(true, "It is Android version");
> +    }
> +    if (isB2G) {
> +      ok(true, "It is B2G version");
> +    }

Please use info("str") instead of ok(true, "str").

@@ +30,5 @@
> +    if (isB2G) {
> +      ok(true, "It is B2G version");
> +    }
> +
> +    if (isLinux || isAndroid || isB2G) {

Please add a typeof check on mem too, to make sure that we're receiving an integer as expected.

@@ +34,5 @@
> +    if (isLinux || isAndroid || isB2G) {
> +      if (mem > 0)
> +        ok(mem, "hardware.memory is support on this platform. mem=" + mem + "MB");
> +      else
> +        ok(false, "The amount of memory should bigger than 0!");

Instead of this, you could just do:

ok(mem > 0, "hardware.memory is supported on this platform. mem=" + mem + "MiB");

The ok() function will fail the test if its first argument is false, so you don't need the else branch above.

@@ +46,5 @@
> +    ok(false, "The Promise should not be rejected");
> +  });
> +}
> +  
> +function testNotSupport() {

Nit: testNotSupported

::: dom/base/test/test_hasFeature_without_perm.html
@@ +15,5 @@
> +
> +
> +is("getFeature" in navigator, false, "getFeature should not exist without permission");
> +SimpleTest.finish();
> +SimpleTest.waitForExplicitFinish();

You don't need either waitForExplicitFinish or finish for synrhonous tests such as this one.  Please remove the above two lines!

Comment 67

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8406035 [details] [diff] [review]
(0414) navigator.getFeature implementation (support hardware.memory)

Actually, just realized one more thing.  This bit:

>+    ThreadsafeAutoJSContext cx;
>+    JS::Rooted<JS::Value> val(cx);
>+    p->MaybeResolve(cx,val);

can be replaced with:

  p->MaybeResolve(nullptr, JS::UndefinedHandleValue);

and a followup bug to add a ToJSValue overload for JS::Handle<JS::Value> so we can drop the nullptr bit there.

Comment 68

[Alphan Chen [:alchen]]

Attachment: (0415) navigator.getFeature implementation (support hardware.memory), r=bz

latest revision

Comment 69

[Alphan Chen [:alchen]]

Attachment: (0415) Testcase for navigator.getFeature, r=ehsan

Comment 70

[Alphan Chen [:alchen]]

(In reply to Boris Zbarsky [:bz] from comment #67)
> Comment on attachment 8406035 [details] [diff] [review]
> (0414) navigator.getFeature implementation (support hardware.memory)
> 
> Actually, just realized one more thing.  This bit:
> 
> >+    ThreadsafeAutoJSContext cx;
> >+    JS::Rooted<JS::Value> val(cx);
> >+    p->MaybeResolve(cx,val);
> 
> can be replaced with:
> 
>   p->MaybeResolve(nullptr, JS::UndefinedHandleValue);
> 
> and a followup bug to add a ToJSValue overload for JS::Handle<JS::Value> so
> we can drop the nullptr bit there.

I file a bug for this support. (BUG 996474)

Comment 71

[(no longer active)]

(In reply to :Ehsan Akhgari (lagging on bugmail, needinfo? me!) from comment #61)
> (In reply to comment #60)
> > > as long as we have a followup to fix that
> > 
> > Which we need anyway to make this work in the seccomp sandbox, no?  Have to do
> > the open() in a non-sandboxed process...
> 
> Yeah that's a great point actually.

please file a follow-up on this as well.  Thanks!

Comment 72

[Alphan Chen [:alchen]]

(In reply to :Ehsan Akhgari (lagging on bugmail, needinfo? me!) from comment #71)
> (In reply to :Ehsan Akhgari (lagging on bugmail, needinfo? me!) from comment
> #61)
> > (In reply to comment #60)
> > > > as long as we have a followup to fix that
> > > 
> > > Which we need anyway to make this work in the seccomp sandbox, no?  Have to do
> > > the open() in a non-sandboxed process...
> > 
> > Yeah that's a great point actually.
> 
> please file a follow-up on this as well.  Thanks!

Bug 996996 is filed.

Comment 73

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

I guess I don't care terribly strongly either way. I did sort of like the fact that hasFeature returns "false" both when Gecko knows about feature but knows that the necessary hardware is missing, and when Gecko is too old and doesn't know about the feature.

But I guess returning "undefined" in both those cases are also ok since it's falsy.

Comment 74

[(no longer active)]

OK, so let's drop hasFeature() and use getFeature() instead.  I'll edit the wiki page accordingly.

Comment 75

[Alphan Chen [:alchen]]

Try server result looks fine. (attachment 8406681 [details] [diff] [review] and attachment 8406682 [details] [diff] [review]) 
https://tbpl.mozilla.org/?tree=Try&rev=925c6a206753

Comment 76

[Jason Smith [:jsmith]]

Clearing the blocking flag - we're not using the blocking b2g flag to track features for a release.

Comment 77

[(no longer active)]

Alphan, please proceed with landing your work here, thanks!

Comment 78

[Alphan Chen [:alchen]]

Attachment: (0512) navigator.getFeature implementation (support hardware.memory), r=bz

The patch based on attachment 8406681 [details] [diff] [review] and latest code base.
The only change is revise "p->MaybeResolve(nullptr, JS::UndefinedHandleValue)" to "p->MaybeResolve(JS::UndefinedHandleValue)" by BUG 996474.

Here is the try server result.
https://tbpl.mozilla.org/?tree=Try&rev=a9a31b1f555e

Comment 79

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/53ac928e5e43
https://hg.mozilla.org/integration/mozilla-inbound/rev/2f7a4bba1dac

Comment 80

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/53ac928e5e43
https://hg.mozilla.org/mozilla-central/rev/2f7a4bba1dac

Comment 81

[(no longer active)]

Alphan, sorry I sort of lost track of the work here.  Did you file a follow-up bug for the rest of the work outlined in comment 0?  Thanks!

Comment 82

[(no longer active)]

Oh, and thanks a lot for your work on this so far!  :-)

Comment 83

[Alphan Chen [:alchen]]

(In reply to :Ehsan Akhgari (@work week, needinfo? me!) from comment #81)
> Alphan, sorry I sort of lost track of the work here.  Did you file a
> follow-up bug for the rest of the work outlined in comment 0?  Thanks!

Actually no.
The only follow-up bug now is bug 996996.
I'm still working on it.
I checked comment 0. But it seems not fully match this work now.
Could you point out the rest of the work you would like to highlight?

In my mind, the next step of feature detection is trying to add more supports.
At that time, I may add 'FeatureDetectionService' component for this API implementation. 
What's your opinion?

Comment 84

[(no longer active)]

(In reply to Alphan Chen[:Alphan] from comment #83)
> (In reply to :Ehsan Akhgari (@work week, needinfo? me!) from comment #81)
> > Alphan, sorry I sort of lost track of the work here.  Did you file a
> > follow-up bug for the rest of the work outlined in comment 0?  Thanks!
> 
> Actually no.
> The only follow-up bug now is bug 996996.
> I'm still working on it.
> I checked comment 0. But it seems not fully match this work now.
> Could you point out the rest of the work you would like to highlight?

I think we should still do the work outlined in comment 0 for the api.* arguments to getFeature().  Filed bug 1009645 for that.  I don't think that anything changed since I wrote comment 0 (except for renaming hasFeature to getFeature which the current wiki page reflects.)

> In my mind, the next step of feature detection is trying to add more
> supports.
> At that time, I may add 'FeatureDetectionService' component for this API
> implementation. 
> What's your opinion?

Not sure what FeatureDetectionService is supposed to do.  I think all of the information we need to implement the rest of this API is already available in WebIDL, so please let's move the discussion to bug 1009645.  If you have any questions, please ask me there.

Comment 85

[Eric Shepherd [:sheppy]]

Bug 1054389 filed for tracking documentation work on this topic. We won't prioritize it, by any stretch, but it's on the (rather lengthy) list.