Last Comment Bug 738131 - implement device proximity
: implement device proximity
Status: RESOLVED FIXED
[sec-assigned:curtisk]
: dev-doc-complete, privacy-review-needed
Product: Core
Classification: Components
Component: DOM: Device Interfaces (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla15
Assigned To: Doug Turner (:dougt)
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-03-21 20:09 PDT by Doug Turner (:dougt)
Modified: 2012-11-15 06:25 PST (History)
13 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch v.1 (27.19 KB, patch)
2012-03-21 21:14 PDT, Doug Turner (:dougt)
bugs: review-
Details | Diff | Splinter Review
gonk proximity fixes (2.64 KB, patch)
2012-03-22 13:17 PDT, Doug Turner (:dougt)
no flags Details | Diff | Splinter Review
patch v.2 (26.26 KB, patch)
2012-03-27 15:47 PDT, Doug Turner (:dougt)
no flags Details | Diff | Splinter Review
patch v.3 (30.21 KB, patch)
2012-03-27 20:55 PDT, Doug Turner (:dougt)
bugs: review-
Details | Diff | Splinter Review
patch v.4 (30.25 KB, patch)
2012-03-28 21:35 PDT, Doug Turner (:dougt)
bugs: review-
Details | Diff | Splinter Review
Add DeviceProximity (30.20 KB, patch)
2012-03-29 00:26 PDT, Doug Turner (:dougt)
no flags Details | Diff | Splinter Review
rebased (30.73 KB, patch)
2012-04-05 12:39 PDT, Doug Turner (:dougt)
bugs: review+
blassey.bugs: review+
mounir: review+
Details | Diff | Splinter Review

Description Doug Turner (:dougt) 2012-03-21 20:09:29 PDT
We need to expose the proximity sensor to content.
Comment 1 Doug Turner (:dougt) 2012-03-21 21:14:44 PDT
Created attachment 608219 [details] [diff] [review]
patch v.1

we probably should prefix this new event with moz*
Comment 2 Curtis Koenig [:curtisk-use curtis.koenig+bzATgmail.com]] 2012-03-22 09:47:08 PDT
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) 2012-03-22 13:17:22 PDT
Created attachment 608439 [details] [diff] [review]
gonk proximity fixes
Comment 4 Olli Pettay [:smaug] 2012-03-26 14:45:46 PDT
Comment on attachment 608219 [details] [diff] [review]
patch v.1

Same problems as what bug 738465 has.
Comment 5 Doug Turner (:dougt) 2012-03-27 15:47:27 PDT
Created attachment 609928 [details] [diff] [review]
patch v.2
Comment 6 Doug Turner (:dougt) 2012-03-27 20:55:12 PDT
Created attachment 609998 [details] [diff] [review]
patch v.3
Comment 7 Olli Pettay [:smaug] 2012-03-27 21:50:44 PDT
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) 2012-03-28 21:35:56 PDT
Created attachment 610426 [details] [diff] [review]
patch v.4
Comment 9 Doug Turner (:dougt) 2012-03-28 21:36:34 PDT
needs a pref to disable.  also need to determine if we should prefix this event.
Comment 10 Olli Pettay [:smaug] 2012-03-28 21:52:25 PDT
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) 2012-03-29 00:26:38 PDT
Created attachment 610472 [details] [diff] [review]
Add DeviceProximity
Comment 12 Doug Turner (:dougt) 2012-04-05 12:39:26 PDT
Created attachment 612652 [details] [diff] [review]
rebased
Comment 13 Olli Pettay [:smaug] 2012-04-10 08:43:36 PDT
Comment on attachment 612652 [details] [diff] [review]
rebased

Someone else needs to review the gonk/android parts
Comment 14 Doug Turner (:dougt) 2012-04-15 21:13:40 PDT
Comment on attachment 612652 [details] [diff] [review]
rebased

mounir for the hal bits
Comment 15 Doug Turner (:dougt) 2012-04-15 21:14:47 PDT
Comment on attachment 612652 [details] [diff] [review]
rebased

blassey for the android change
Comment 16 Mounir Lamouri (:mounir) 2012-04-16 00:55:16 PDT
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) 2012-04-16 00:56:55 PDT
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?) 2012-04-16 09:02:29 PDT
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) 2012-04-16 09:07:36 PDT
> 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) 2012-04-16 09:16:58 PDT
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) PTO Until July 5th 2012-05-01 19:06:20 PDT
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) 2012-05-01 21:26:05 PDT
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 2012-05-01 22:29:45 PDT
What unit does this report, cm?
Comment 24 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-05-01 22:30:18 PDT
(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) PTO Until July 5th 2012-05-02 10:08:54 PDT
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) 2012-05-02 10:19:51 PDT
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) PTO Until July 5th 2012-05-02 15:50:07 PDT
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) 2012-05-02 20:12:40 PDT
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) PTO Until July 5th 2012-05-03 00:21:18 PDT
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) 2012-05-03 08:30:33 PDT
> 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) 2012-05-03 08:34:21 PDT
both my suggestion in comment 30 and sicking's suggestion in comment 29 can be done as follow-ups.
Comment 32 Jonas Sicking (:sicking) PTO Until July 5th 2012-05-03 11:47:52 PDT
> 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) 2012-05-03 11:55:27 PDT
filed bug 751663.
Comment 35 Ed Morley [:emorley] 2012-05-04 11:44:05 PDT
https://hg.mozilla.org/mozilla-central/rev/1aa618c21bab
Comment 37 Curtis Koenig [:curtisk-use curtis.koenig+bzATgmail.com]] 2012-10-16 09:52:12 PDT
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?

Note You need to log in before you can comment on or make changes to this bug.