Note: There are a few cases of duplicates in user autocompletion which are being worked on.

implement device proximity

RESOLVED FIXED in mozilla15

Status

()

Core
DOM: Device Interfaces
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: dougt, Assigned: dougt)

Tracking

({dev-doc-complete, privacy-review-needed})

Trunk
mozilla15
dev-doc-complete, privacy-review-needed
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [sec-assigned:curtisk])

Attachments

(1 attachment, 6 obsolete attachments)

(Assignee)

Description

5 years ago
We need to expose the proximity sensor to content.
(Assignee)

Comment 1

5 years ago
Created attachment 608219 [details] [diff] [review]
patch v.1

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
Keywords: privacy-review-needed, sec-review-needed
(Assignee)

Comment 3

5 years ago
Created attachment 608439 [details] [diff] [review]
gonk proximity fixes
Attachment #608439 - Flags: review?(mwu)

Comment 4

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

Same problems as what bug 738465 has.
Attachment #608219 - Flags: review?(bugs) → review-
(Assignee)

Comment 5

5 years ago
Created attachment 609928 [details] [diff] [review]
patch v.2
Attachment #608219 - Attachment is obsolete: true
(Assignee)

Comment 6

5 years ago
Created attachment 609998 [details] [diff] [review]
patch v.3
Attachment #608439 - Attachment is obsolete: true
Attachment #609928 - Attachment is obsolete: true
Attachment #608439 - Flags: review?(mwu)
Attachment #609998 - Flags: review?(bugs)

Comment 7

5 years ago
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
(Assignee)

Comment 8

5 years ago
Created attachment 610426 [details] [diff] [review]
patch v.4
Attachment #609998 - Attachment is obsolete: true
Attachment #610426 - Flags: review?(bugs)
(Assignee)

Comment 9

5 years ago
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-
(Assignee)

Comment 11

5 years ago
Created attachment 610472 [details] [diff] [review]
Add DeviceProximity
Attachment #610426 - Attachment is obsolete: true
Keywords: sec-review-needed
Whiteboard: [secr:curtisk → [secr:curtisk]
(Assignee)

Updated

5 years ago
Attachment #610472 - Flags: review?(bugs)
(Assignee)

Comment 12

5 years ago
Created attachment 612652 [details] [diff] [review]
rebased
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+
(Assignee)

Comment 14

5 years ago
Comment on attachment 612652 [details] [diff] [review]
rebased

mounir for the hal bits
Attachment #612652 - Flags: review?(mounir)
(Assignee)

Comment 15

5 years ago
Comment on attachment 612652 [details] [diff] [review]
rebased

blassey for the android change
Attachment #612652 - Flags: review?(blassey.bugs)
(Assignee)

Updated

5 years ago
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?
(Assignee)

Comment 19

5 years ago
> 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+
Keywords: dev-doc-needed
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.
(Assignee)

Comment 22

5 years ago
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?
(Assignee)

Comment 26

5 years ago
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
  }
}
(Assignee)

Updated

5 years ago
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)
(Assignee)

Comment 28

5 years ago
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.
(Assignee)

Comment 30

5 years ago
> 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.
(Assignee)

Comment 31

5 years ago
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.
(Assignee)

Comment 33

5 years ago
filed bug 751663.
(Assignee)

Comment 34

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/1aa618c21bab
https://hg.mozilla.org/mozilla-central/rev/1aa618c21bab
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla15
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
Keywords: dev-doc-needed → dev-doc-complete
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.