738131 - implement device proximity

Status: RESOLVED FIXED

(Core :: DOM: Device Interfaces, defect)

Description

[Doug Turner (:dougt)]

We need to expose the proximity sensor to content.

Comment 1

[Doug Turner (:dougt)]

Attachment: patch v.1

we probably should prefix this new event with moz*

Comment 2

[Curtis Koenig [:curtisk-use curtis.koenig+bzATgmail.com]]]

per conversation with dougt marking this for priv and sec review
Privacy review started: https://wiki.mozilla.org/Privacy/Reviews/DeviceProximity
Sec-Review Triage to occur on 28-Mar-2012

Comment 3

[Doug Turner (:dougt)]

Attachment: gonk proximity fixes

Comment 4

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

Comment on attachment 608219 [details] [diff] [review]
patch v.1

Same problems as what bug 738465 has.

Comment 5

[Doug Turner (:dougt)]

Attachment: patch v.2

Comment 6

[Doug Turner (:dougt)]

Attachment: patch v.3

Comment 7

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

Comment on attachment 609998 [details] [diff] [review]
patch v.3

This is missing the InitFromCtor thing.
All these new events need some tests, at least to check that
new FooEvent("type", {...}); work

Comment 8

[Doug Turner (:dougt)]

Attachment: patch v.4

Comment 9

[Doug Turner (:dougt)]

needs a pref to disable.  also need to determine if we should prefix this event.

Comment 10

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

Comment on attachment 610426 [details] [diff] [review]
patch v.4


>+  virtual nsresult InitFromCtor(const nsAString& aType,
>+				JSContext* aCx,
>+				jsval* aVal);

You're using tabs, which is why the alignment is wrong here
Please use spaces. Check the whole patch whether there are tabs


>+  } else if (aTypeAtom == nsGkAtoms::ondeviceproximity) {
>+     nsPIDOMWindow* window = GetInnerWindowForTarget();
>+     if (window)
>+       window->EnableDeviceSensor(SENSOR_PROXIMITY);
Coding style is
if (expr) {
  stmt;
}

>@@ -356,16 +360,21 @@ nsEventListenerManager::RemoveEventListe
>           window->DisableDeviceSensor(SENSOR_ORIENTATION);
>       } else if (aType == NS_DEVICE_MOTION) {
>         nsPIDOMWindow* window = GetInnerWindowForTarget();
>         if (window) {
>           window->DisableDeviceSensor(SENSOR_ACCELERATION);
>           window->DisableDeviceSensor(SENSOR_LINEAR_ACCELERATION);
>           window->DisableDeviceSensor(SENSOR_GYROSCOPE);
>         }
>+      } else if (aType == NS_DEVICE_PROXIMITY) {
>+        nsPIDOMWindow* window = GetInnerWindowForTarget();
>+        if (window) {
>+          window->DisableDeviceSensor(SENSOR_PROXIMITY);
>+        }
>       }
This is wrong, and so is the other Disable* code in this method.

Comment 11

[Doug Turner (:dougt)]

Attachment: Add DeviceProximity

Comment 12

[Doug Turner (:dougt)]

Attachment: rebased

Comment 13

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

Comment on attachment 612652 [details] [diff] [review]
rebased

Someone else needs to review the gonk/android parts

Comment 14

[Doug Turner (:dougt)]

Comment on attachment 612652 [details] [diff] [review]
rebased

mounir for the hal bits

Comment 15

[Doug Turner (:dougt)]

Comment on attachment 612652 [details] [diff] [review]
rebased

blassey for the android change

Comment 16

[Mounir Lamouri (:mounir)]

Comment on attachment 612652 [details] [diff] [review]
rebased

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

::: hal/gonk/GonkSensor.cpp
@@ +102,5 @@
>        mSensorValues.AppendElement(radToDeg(data.data[1]));
>        mSensorValues.AppendElement(radToDeg(data.data[2]));
> +    } else if (mSensorData.sensor() == SENSOR_PROXIMITY) {
> +      mSensorValues.AppendElement(data.data[0]);
> +      mSensorValues.AppendElement(0);     

Why is min always 0?

@@ +106,5 @@
> +      mSensorValues.AppendElement(0);     
> +
> +      // Determine the maxRange for this sensor.
> +      const sensor_t* sensors = NULL;
> +      SensorDevice& device = SensorDevice::getInstance();

You could merge those two lines.

@@ +110,5 @@
> +      SensorDevice& device = SensorDevice::getInstance();
> +      size_t size = device.getSensorList(&sensors);
> +      for (size_t i = 0; i < size; i++) {
> +        if (sensors[i].type == SENSOR_TYPE_PROXIMITY) {
> +          mSensorValues.AppendElement(sensors[i].maxRange);     

Couldn't you pass the sensor directly to that runnable so you will not have to do that?

Comment 17

[Mounir Lamouri (:mounir)]

Comment on attachment 612652 [details] [diff] [review]
rebased

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

Canceling the review request because I need answers to the questions above to be able to do the review.
Feel free to re-assign.

Comment 18

[Brad Lassey [:blassey] (use needinfo?)]

Comment on attachment 612652 [details] [diff] [review]
rebased

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

::: mobile/android/base/GeckoAppShell.java
@@ +624,5 @@
>              Log.i(LOGTAG, "Enabling SENSOR_PROXIMITY");
>              if(gProximitySensor == null)
>                  gProximitySensor = sm.getDefaultSensor(Sensor.TYPE_PROXIMITY);
>              if (gProximitySensor != null)
> +                sm.registerListener(GeckoApp.mAppContext, gProximitySensor, SensorManager.SENSOR_DELAY_NORMAL);

shouldn't there be a corresponding unregisterListener call?

Comment 19

[Doug Turner (:dougt)]

> Why is min always 0?

it is a distance value reported by the sensor - it will always be 0 ... Max.  Other proximity sensors might have neg values, but I would find that odd.

> You could merge those two lines.

Yup.  Fixed locally.

> Couldn't you pass the sensor directly to that runnable so you will not have to do that?

All of the runnables take an array of values.  I don't think we should change that up in this bug.  Maybe a follow up.


> shouldn't there be a corresponding unregisterListener call?

The unregister call is in: public static void disableSensor(int aSensortype) {}

Comment 20

[Mounir Lamouri (:mounir)]

Comment on attachment 612652 [details] [diff] [review]
rebased

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

r=me for hal changes.

Comment 21

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

Comment on attachment 612652 [details] [diff] [review]
rebased

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

I'm not terribly excited about this API, but I agree it makes sense to be consistent with the orientation sensors.

However mark initDeviceProximityEvent as [noscript]. No need to expose that to the web.

Or better yet, remove the function from the interface completely and make NS_NewDOMDeviceProximityEvent use nsDOMDeviceProximityEvent** as out-argument.

I'm concerned about exposing min/max/value. How will the page judge "phone is close enough to face that we should just turn off the screen/touchevents which likely is the by far most common use case? Should the page consider value==min to be "held against face"? Since you set min to 0 that doesn't seem like that would work.

I think we need to at the very least also include a boolean value indicating "up against face". Not sure what the name of it would be, but that seems less important.

Comment 22

[Doug Turner (:dougt)]

I disagree with adding a boolean to indicate up-against-face.  You could imagine other use cases that do not have anything to do with the user's face.  Providing the bare minimum information that the sensor knows is what I want to do.

I'll fix up the constructor in both this bug and in the device light bug.

Thx!

Comment 23

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

What unit does this report, cm?

Comment 24

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

(In reply to Doug Turner (:dougt) from comment #22)
> You could
> imagine other use cases that do not have anything to do with the user's
> face.

qDot ^^

Comment 25

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

Doug: I'm not suggesting we remove .value/.min/.max, so other usecases will still be supported.

My question is, with the current API, what code do you suggest that the phone-app use to decide when to turn off the screen? My initial guess would have been to do something like:

window.ondeviceproximity = function(event) {
  if (screenIsOn() && event.value == event.min) {
    turnOffScreen();
  }
  else if (screenIsOff()) {
    turnOnScreen();
  }
}

but looking at the implementation that doesn't seem like it would work. So I'm not sure how pages are expected to use the API?

Comment 26

[Doug Turner (:dougt)]

cjones - yes, units are in cm.

jonas - 

That is exactly how it works.  Proximity sensors range is from 0 cm to the sensor defined max.  For example, the Galaxy Nexus has a Sharp gp2a proximity sensor.  It's range is 0cm to 5cm.  This sensor like many other cheap proximity sensors is binary.  That is to say, its resolution is also 5cm.  (Some are not as cheap and have a higher resolution)  So, if you wanted to know if the user's face is touching the screen/sensor, your code works fine.

window.ondeviceproximity = function(e) {
  if (e.value == e.min) {
    // person's face is directly on top of proximity sensor
  }
}

Comment 27

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

The problem is that the code would fail for a higher-resolution sensor. The user rarely has the face pressed up against the sensor, but rather a cm or two away with only the ear touching the phone speaker.

So the code would only work on a low precision sensor and would start failing once proximity sensors get better. So if pages write code like that, we'd be left with not exposing the full sensitivity of the sensor (which would likely break certain use cases) or breaking pages (which might not be an option if it breaks high-profile pages)

Comment 28

[Doug Turner (:dougt)]

It is a proximity sensor, not a "is the user's face next to the screen" sensor.  :)

If you want your app (the dialer) to detect when a user's face is reasonably close to the screen, pick your value.  Lets say we decide it's 2cm:

if (e.value < 2) {
 ...
}

Comment 29

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

The main use-case we have for this API is to detect if the user is holding up the phone against the face. We need to make that use-case convenient and not error-prone.

So it sounds like what you are recommending is something like:

window.ondeviceproximity = function(e) {
  if (e.value == e.min || e.value < 2) {
    turnOffScreen();
  }
}

Is there a reason we couldn't expose that computation as a boolean flag on the event? That way everyone doesn't have to repeat that same logic. It also means that we can tweak the logic if it's inappropriate for some sensor (for example we can apply a low-pass filter in case some low-quality sensor give bad readings on occasion).

But most importantly, it reduces the risk that pages will just check value==min which would lead to the situation in comment 27.

Comment 30

[Doug Turner (:dougt)]

> The main use-case we have for this API is to detect if the user is holding up the phone against the face.

main, not only.

> Is there a reason we couldn't expose that computation as a boolean flag on the event? 

No technical reason.  On a high resolution promimity sensor, you may get alot of events before this new flag '.isTheScreenCloseEnoughToTheUsersFace' is true.

I'd be happy with some specialized event - a separate event that gets fired only when the value changes.

Comment 31

[Doug Turner (:dougt)]

both my suggestion in comment 30 and sicking's suggestion in comment 29 can be done as follow-ups.

Comment 32

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

> I'd be happy with some specialized event - a separate event that gets fired
> only when the value changes.

That would be even better! I guess we can do that as a followup yes.

Comment 33

[Doug Turner (:dougt)]

filed bug 751663.

Comment 34

[Doug Turner (:dougt)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/1aa618c21bab

Comment 35

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/1aa618c21bab

Comment 36

[Florian Scholz (Open Web Docs)]

Mentioned on
https://developer.mozilla.org/en-US/docs/DOM/DOM_event_reference
https://developer.mozilla.org/en-US/docs/Firefox_15_for_developers
New pages:
https://developer.mozilla.org/en-US/docs/DOM/DeviceProximityEvent
https://developer.mozilla.org/en-US/docs/DOM/UserProximityEvent

Comment 37

[Curtis Koenig [:curtisk-use curtis.koenig+bzATgmail.com]]]

Can we get the privacy review https://wiki.mozilla.org/Privacy/Reviews/MobileSensors filled out so we can get the privacy flags off this bug please?