Closed
Bug 738131
Opened 12 years ago
Closed 12 years ago
implement device proximity
Categories
(Core :: DOM: Device Interfaces, defect)
Core
DOM: Device Interfaces
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.
Assignee | ||
Comment 1•12 years ago
|
||
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•12 years ago
|
||
Attachment #608439 -
Flags: review?(mwu)
Comment 4•12 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•12 years ago
|
||
Attachment #608219 -
Attachment is obsolete: true
Assignee | ||
Comment 6•12 years ago
|
||
Attachment #608439 -
Attachment is obsolete: true
Attachment #609928 -
Attachment is obsolete: true
Attachment #608439 -
Flags: review?(mwu)
Attachment #609998 -
Flags: review?(bugs)
Comment 7•12 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-
![]() |
||
Updated•12 years ago
|
Whiteboard: [secr:curtisk
Assignee | ||
Comment 8•12 years ago
|
||
Attachment #609998 -
Attachment is obsolete: true
Attachment #610426 -
Flags: review?(bugs)
Assignee | ||
Comment 9•12 years ago
|
||
needs a pref to disable. also need to determine if we should prefix this event.
Comment 10•12 years ago
|
||
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•12 years ago
|
||
Attachment #610426 -
Attachment is obsolete: true
![]() |
||
Updated•12 years ago
|
Keywords: sec-review-needed
Whiteboard: [secr:curtisk → [secr:curtisk]
Assignee | ||
Updated•12 years ago
|
Attachment #610472 -
Flags: review?(bugs)
Assignee | ||
Comment 12•12 years ago
|
||
Attachment #610472 -
Attachment is obsolete: true
Attachment #610472 -
Flags: review?(bugs)
Attachment #612652 -
Flags: review?(bugs)
Comment 13•12 years ago
|
||
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•12 years ago
|
||
Comment on attachment 612652 [details] [diff] [review] rebased mounir for the hal bits
Attachment #612652 -
Flags: review?(mounir)
Assignee | ||
Comment 15•12 years ago
|
||
Comment on attachment 612652 [details] [diff] [review] rebased blassey for the android change
Attachment #612652 -
Flags: review?(blassey.bugs)
Assignee | ||
Updated•12 years ago
|
Attachment #612652 -
Flags: review?(jonas)
Comment 16•12 years ago
|
||
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•12 years ago
|
||
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 18•12 years ago
|
||
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•12 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 20•12 years ago
|
||
Comment on attachment 612652 [details] [diff] [review] rebased Review of attachment 612652 [details] [diff] [review]: ----------------------------------------------------------------- r=me for hal changes.
Attachment #612652 -
Flags: review+
Updated•12 years ago
|
OS: Mac OS X → All
Hardware: x86 → All
Version: unspecified → Trunk
Updated•12 years ago
|
Attachment #612652 -
Flags: review?(blassey.bugs) → review+
Updated•12 years ago
|
Keywords: dev-doc-needed
![]() |
||
Updated•12 years ago
|
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•12 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•12 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•12 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•12 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•12 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•12 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•12 years ago
|
||
filed bug 751663.
Assignee | ||
Comment 34•12 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/1aa618c21bab
Comment 35•12 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/1aa618c21bab
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla15
Comment 36•12 years ago
|
||
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.
Description
•