Closed Bug 738131 Opened 12 years ago Closed 12 years ago

implement device proximity

Categories

(Core :: DOM: Device Interfaces, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla15

People

(Reporter: dougt, Assigned: dougt)

Details

(Keywords: dev-doc-complete, privacy-review-needed, Whiteboard: [sec-assigned:curtisk])

Attachments

(1 file, 6 obsolete files)

We need to expose the proximity sensor to content.
Attached patch patch v.1 (obsolete) — Splinter Review
we probably should prefix this new event with moz*
Attachment #608219 - Flags: review?(bugs)
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
Attached patch gonk proximity fixes (obsolete) — Splinter Review
Attachment #608439 - Flags: review?(mwu)
Comment on attachment 608219 [details] [diff] [review]
patch v.1

Same problems as what bug 738465 has.
Attachment #608219 - Flags: review?(bugs) → review-
Attached patch patch v.2 (obsolete) — Splinter Review
Attachment #608219 - Attachment is obsolete: true
Attached patch patch v.3 (obsolete) — Splinter Review
Attachment #608439 - Attachment is obsolete: true
Attachment #609928 - Attachment is obsolete: true
Attachment #608439 - Flags: review?(mwu)
Attachment #609998 - Flags: review?(bugs)
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
Attachment #609998 - Flags: review?(bugs) → review-
Whiteboard: [secr:curtisk
Attached patch patch v.4 (obsolete) — Splinter Review
Attachment #609998 - Attachment is obsolete: true
Attachment #610426 - Flags: review?(bugs)
needs a pref to disable.  also need to determine if we should prefix this event.
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.
Attachment #610426 - Flags: review?(bugs) → review-
Attached patch Add DeviceProximity (obsolete) — Splinter Review
Attachment #610426 - Attachment is obsolete: true
Whiteboard: [secr:curtisk → [secr:curtisk]
Attachment #610472 - Flags: review?(bugs)
Attached patch rebasedSplinter Review
Attachment #610472 - Attachment is obsolete: true
Attachment #610472 - Flags: review?(bugs)
Attachment #612652 - Flags: review?(bugs)
Comment on attachment 612652 [details] [diff] [review]
rebased

Someone else needs to review the gonk/android parts
Attachment #612652 - Flags: review?(bugs) → review+
Comment on attachment 612652 [details] [diff] [review]
rebased

mounir for the hal bits
Attachment #612652 - Flags: review?(mounir)
Comment on attachment 612652 [details] [diff] [review]
rebased

blassey for the android change
Attachment #612652 - Flags: review?(blassey.bugs)
Attachment #612652 - Flags: review?(jonas)
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 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.
Attachment #612652 - Flags: review?(mounir)
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?
> 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 on attachment 612652 [details] [diff] [review]
rebased

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

r=me for hal changes.
Attachment #612652 - Flags: review+
OS: Mac OS X → All
Hardware: x86 → All
Version: unspecified → Trunk
Attachment #612652 - Flags: review?(blassey.bugs) → review+
Whiteboard: [secr:curtisk] → [sec-assigned:curtisk]
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.
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!
What unit does this report, cm?
(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 ^^
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?
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
  }
}
Attachment #612652 - Flags: review?(jonas)
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)
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) {
 ...
}
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.
> 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.
both my suggestion in comment 30 and sicking's suggestion in comment 29 can be done as follow-ups.
> 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.
filed bug 751663.
https://hg.mozilla.org/mozilla-central/rev/1aa618c21bab
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla15
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?
You need to log in before you can comment on or make changes to this bug.