793728 - Ambient Light API: OS X backend

Status: RESOLVED FIXED

(Core :: Widget: Cocoa, defect)

Comment 1

[Reuben Morais [:reuben]]

Relevant links:
https://mxr.mozilla.org/mozilla-central/source/hal/cocoa/CocoaSensor.cpp - light sensor code goes here :)
https://mxr.mozilla.org/mozilla-central/source/hal/gonk/GonkSensor.cpp - existing implementation of this API for B2G

Comment 2

[Reuben Morais [:reuben]]

Attachment: Simple command-line program to query the light sensor data

Comment 3

[Steven Downum]

I can start working on this unless someone has other plans.

Comment 4

[Reuben Morais [:reuben]]

(In reply to Steven Downum from comment #3)
> I can start working on this unless someone has other plans.

Great! Feel free to attach work-in-progress patches, and to contact me via email or IRC if you have any questions.

Comment 5

[Steven Downum]

(In reply to Reuben Morais [:reuben] from comment #1)
Didn't find the first link, but this one seems to fit the bill:
https://mxr.mozilla.org/mozilla-central/source/hal/cocoa/CocoaSensor.mm

The command line example works for me on 10.8. I'll go through GonkSensor plus the spec and start planning the IOKit/CF implementation, sending WIP patches as things come together on my end. Thanks!

Comment 6

[Reuben Morais [:reuben]]

I contacted Steven via email about this bug a few days ago and haven't received an answer, so it's OK to take this bug and work on it.

Comment 7

[Jorge Luis Mendez [:jlmendezbonini]]

I'll adopt this bug if that's ok with you Reuben.

Comment 8

[Reuben Morais [:reuben]]

(In reply to Jorge Luis Mendez from comment #7)
> I'll adopt this bug if that's ok with you Reuben.

Great! Ping me if you have any questions, either here or via email on reuben.morais at gmail.

Comment 9

[Reuben Morais [:reuben]]

There's clearly more to this bug than I thought, since this is the second contributor that never replied back. Let's find out.

Comment 10

[Jorge Luis Mendez [:jlmendezbonini]]

Hi Reuben,

I've been out for the last 3 weeks but I'm finally back home. I'll get back to you next week with an update and/or questions. I apologize for the delay.

Comment 11

[Reuben Morais [:reuben]]

(In reply to Jorge Luis Mendez from comment #10)
> Hi Reuben,
> 
> I've been out for the last 3 weeks but I'm finally back home. I'll get back
> to you next week with an update and/or questions. I apologize for the delay.

Awesome! :D

Comment 12

[Jorge Luis Mendez [:jlmendezbonini]]

Hi Reuben, 

Please see the attachment for a very work-in-progress patch.

You'll notice that I'm using InitWithFuncCallback() void* argument to pass the SensorType to the UpdateHandler(), however, this is not working as I expected. How can we differentiate between SensorTypes inside UpdateHandler()?

Also, I briefly mentioned to you that I had found the units of the reported values, sorry I think I did but I can't find it. Anyone have some extra information about this?

Comment 13

[Jorge Luis Mendez [:jlmendezbonini]]

Attachment: wip AmbientLight API implementation.

Comment 14

[Jorge Luis Mendez [:jlmendezbonini]]

(In reply to Jorge Luis Mendez from comment #13)
> Created attachment 704634 [details] [diff] [review]
> wip AmbientLight API implementation.

Test html page courtesy of Doug Turner (http://dougturner.wordpress.com/2012/03/26/device-light-sensor/)

http://dl.dropbox.com/u/8727858/mozilla/light/light.html

Comment 15

[Reuben Morais [:reuben]]

(In reply to Jorge Luis Mendez from comment #12)
> You'll notice that I'm using InitWithFuncCallback() void* argument to pass
> the SensorType to the UpdateHandler(), however, this is not working as I
> expected. How can we differentiate between SensorTypes inside
> UpdateHandler()?

You'll have to keep track of all enabled sensors, since more than one can be enabled at the same time, and we don't need one timer for each sensor type. You can look at how the B2G implementation does it at hal/gonk/GonkSensor.cpp.

> Also, I briefly mentioned to you that I had found the units of the reported
> values, sorry I think I did but I can't find it. Anyone have some extra
> information about this?

I looked into this when I first filed the bug, and couldn't find anything very useful. The spec requires a value in lux, but if we can map the values reported by OS X to the correspondent values in lux (e.g. using an Android app that shows the light level in lux), that should be fine.

Comment 16

[Jorge Luis Mendez [:jlmendezbonini]]

See the new patch. The patch implements a static InfallibleArray<SensorType> that tracks the active sensors and that we loop over in UpdateHandler. Is this what you had in mind?

> I looked into this when I first filed the bug, and couldn't find anything
> very useful. The spec requires a value in lux, but if we can map the values
> reported by OS X to the correspondent values in lux (e.g. using an Android
> app that shows the light level in lux), that should be fine.

I think this is the best we can do since the reported units are not documented at all AFAIK. Do you have a device that reports lux values? (I had one but there's an __unresolved__ firmware bug that disabled the sensor on my old phone...geez)

If the patch is in the right track, I think that we should refactor the light sensor code into a class. Though this might seem unnecessary given that the current code it's simple enough, I noticed that the specification now includes an additional attribute (http://www.w3.org/TR/ambient-light/#light-level). What do you think?

Finally, the example code, lmutracker.mm, was implemented to return two light values -- although they are identical in my MacBook. Are you aware if any differences with other Mac's? Also, since the specs only allow one attribute for this event, should we consider returning the value that meets a certain criteria? (i.e. highest one)

Comment 17

[Jorge Luis Mendez [:jlmendezbonini]]

Attachment: Tracks active sensors and uses one timer.

Comment 18

[Jorge Luis Mendez [:jlmendezbonini]]

Reuben,

Does the Acceleration sensor works on your Mac? It doesn't in my end, see Bug 833038

Comment 19

[Reuben Morais [:reuben]]

(In reply to Jorge Luis Mendez from comment #16)
> See the new patch. The patch implements a static InfallibleArray<SensorType>
> that tracks the active sensors and that we loop over in UpdateHandler. Is
> this what you had in mind?

Yes, that's what I had in mind.

> > I looked into this when I first filed the bug, and couldn't find anything
> > very useful. The spec requires a value in lux, but if we can map the values
> > reported by OS X to the correspondent values in lux (e.g. using an Android
> > app that shows the light level in lux), that should be fine.
> 
> I think this is the best we can do since the reported units are not
> documented at all AFAIK. Do you have a device that reports lux values? (I
> had one but there's an __unresolved__ firmware bug that disabled the sensor
> on my old phone...geez)

Yes, I do. I'll gather data for some common light settings.

> If the patch is in the right track, I think that we should refactor the
> light sensor code into a class. Though this might seem unnecessary given
> that the current code it's simple enough, I noticed that the specification
> now includes an additional attribute
> (http://www.w3.org/TR/ambient-light/#light-level). What do you think?

Interesting! That actually makes it easier for us to estimate the light level value to be passed. We don't support LightLevelEvent yet, though, so I don't think you should worry about it.

> Finally, the example code, lmutracker.mm, was implemented to return two
> light values -- although they are identical in my MacBook. Are you aware if
> any differences with other Mac's? Also, since the specs only allow one
> attribute for this event, should we consider returning the value that meets
> a certain criteria? (i.e. highest one)

I'm pretty sure at some point it was a positional thing (i.e. light level on the left and right sides of the notebook), but I've never seen different values in my hardware either. We can just pass the medium value.

Thanks for your work! I'll respond with some data for the LUX levels so we can experiment with the values we report and how to best express that in the code.

Comment 20

[Jorge Luis Mendez [:jlmendezbonini]]

(In reply to Reuben Morais [:reuben] from comment #19)
> Interesting! That actually makes it easier for us to estimate the light
> level value to be passed. We don't support LightLevelEvent yet, though, so I
> don't think you should worry about it.
> 

Bug 738465 is the implementation of the device interface for this bug so I thought I could use that one as a guide to implement this new event. Once we are done with this bug that is.


>Thanks for your work! I'll respond with some data for the LUX levels so we can experiment with the values we report and >how to best express that in the code.

Thank you for all your help and this is quite fun actually.

Comment 21

[Reuben Morais [:reuben]]

Sorry for the delay, I was busy with a nasty B2G bug over the weekend :P

I collected a few data points around the office, but I still want to get data in more varied light configurations. Here are the initial results: 

LMU tracker    Lux      Description

12000000       390      Office, flat under light
5000000        136      Bright white light, flat
3000000        100      Bright white light, 130˚
2750000        86       Office, 130˚ under light
2500000        76       Office, flat on desk
1200000        46       Office, 130˚ on desk
800000         26       Office, 90˚
180000         6        Office, shadow

Linear regression shows lux ≈ 3*lmu/100000 - 1.5.

Comment 22

[Jorge Luis Mendez [:jlmendezbonini]]

"The values of the LightLevelEvent event may be "normal", "dim", or "bright". "bright" is supposed to mean "direct sunlight, or similarly bright conditions that make it hard to see things that aren't high-contrast". "dim" is supposed to mean "dark enough that the light produced by a white background is eye-straining or distracting". The lux values for "dim" typical begin below 50, and the values for "bright" begin above 10000."  http://www.w3.org/TR/ambient-light/#introduction

dim < 50 lx
50 lx < normal < 10000 lx
bright > 10000 lx

However, the specification also states that the actual ranges are left to the UA so we don't _have_ to use those ranges. 

Can you get some measurements within the "bright" range?. Good excuse hang out at SF office deck ;)

Comment 23

[Reuben Morais [:reuben]]

(In reply to Jorge Luis Mendez [:jlmendezbonini] from comment #22)
Interesting stuff, and definitely more useful to web devs than raw lux values. I'll do some measurements.

Comment 24

[Reuben Morais [:reuben]]

It's been cloudy, so I couldn't get past the 2500 lux range (which gives me ~50000000 in the LMU tracker).

Comment 25

[Jorge Luis Mendez [:jlmendezbonini]]

In addition of a function to convert the value to LUX, we are properly closing the connection handle opened with IOServiceOpen. The example code didn't do this but it's necessary to properly destroy the handle. From IOServiceClose docs:

"A connection created with the IOServiceOpen should be closed when the connection is no longer to be used with IOServiceClose.....It will be destroyed by this function, and should not be released with IOObjectRelease."

The patch complies, to the best of my knowledge, to Mozilla's coding style. What else should we do to get the patch committed?

Comment 26

[Jorge Luis Mendez [:jlmendezbonini]]

Attachment: Properly closes the connection handle

Comment 27

[Reuben Morais [:reuben]]

I got readings above the 10k lux level today. Turns out the sensor on my machine doesn't read values over ~6300 lux, and 67090000 seems to be the highest number it'll ever report. I don't know if this applies to all LMU trackers, but it should be good enough for deciding whether the light level is "normal", "dim" or "bright".

LMU tracker    Lux      Description
67090000       6300     Outside, bright day
28790000       2500     Outside, cloudy
12500000       390      Office, flat under light
5000000        136      Bright white light, flat
3000000        100      Bright white light, 130˚
2750000        86       Office, 130˚ under light
2500000        76       Office, flat on desk
1200000        46       Office, 130˚ on desk
800000         26       Office, 90˚
180000         6        Office, shadow

The new readings show that this isn't a linear scale, and I was only able to get a meaningful regression as a polynomial of degree 4, giving the ugly (but functional) equation:

lux = (-3*(10^-27))*x^4 + (2.6*(10^-19))*x^3 - (3.4*(10^-12))*x^2 + (3.9*(10^-5))*x - 0.19

Comment 28

[Reuben Morais [:reuben]]

(In reply to Jorge Luis Mendez [:jlmendezbonini] from comment #25)
> In addition of a function to convert the value to LUX, we are properly
> closing the connection handle opened with IOServiceOpen. The example code
> didn't do this but it's necessary to properly destroy the handle. From
> IOServiceClose docs:
> 
> "A connection created with the IOServiceOpen should be closed when the
> connection is no longer to be used with IOServiceClose.....It will be
> destroyed by this function, and should not be released with IOObjectRelease."
>
> The patch complies, to the best of my knowledge, to Mozilla's coding style.
> What else should we do to get the patch committed?

Nice! After changing the conversion function to work with higher values from the sensor, we need review from a HAL peer, and a Widget peer (for the Cocoa API usage).

Comment 29

[Reuben Morais [:reuben]]

Comment on attachment 713430 [details] [diff] [review]
Properly closes the connection handle

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

::: hal/cocoa/CocoaSensor.mm
@@ +35,2 @@
>  {
> +  for (PRUint32 i = 0; i < sActiveSensors.Length(); ++i) {

We don't use PR types anymore, this should be uint32_t or simply int.

@@ +38,3 @@
>  
> +    if (sensor != SENSOR_ACCELERATION && sensor != SENSOR_LIGHT)
> +      return;

This check is not needed, since you do that in EnableSensorNotifications.

@@ +85,5 @@
> +    }
> +
> +    if (!smsLoadCalibration()) {
> +      return;
> +    }

You should remove the braces here to match the rest of the file.

@@ +125,4 @@
>      return;
>  
>    if (sUpdateTimer) {
>      sUpdateTimer->Cancel();

The timer should only be cancelled if all sensors are disabled. You probably want |sActiveSensors.RemoveElement(aSensor);| and then only cancel the timer if the array is empty.

Comment 30

[Jorge Luis Mendez [:jlmendezbonini]]

All the changes are included in the newest patch, however, I need help with the new polynomial. The big input values from the LMU tracker together with the exponents produce some _huge numbers and I'm not sure how to implement it properly. E.g. 67090000^4 ~= 2E+31.

>LMU tracker    Lux      Description
>67090000       6300     Outside, bright day

I think very it's strange that the _highest_ measurement doesn't come close to the "bright" range (10000lx). I'm sure you could hardly see the screen at 6300lx because of the glare. Maybe it's a typo in the document and should be 1000lx? It might be worth verifying this.

Comment 31

[Jorge Luis Mendez [:jlmendezbonini]]

Attachment: Ambient Light API: OS X backend

Still missing a proper implementation of the polynomial.

Comment 32

[Reuben Morais [:reuben]]

(In reply to Jorge Luis Mendez [:jlmendezbonini] from comment #30)
> All the changes are included in the newest patch, however, I need help with
> the new polynomial. The big input values from the LMU tracker together with
> the exponents produce some _huge numbers and I'm not sure how to implement
> it properly. E.g. 67090000^4 ~= 2E+31.
> 
> >LMU tracker    Lux      Description
> >67090000       6300     Outside, bright day
> 
> I think very it's strange that the _highest_ measurement doesn't come close
> to the "bright" range (10000lx). I'm sure you could hardly see the screen at
> 6300lx because of the glare. Maybe it's a typo in the document and should be
> 1000lx? It might be worth verifying this.

I don't understand what you meant here. What is 100001x and 63001x? And yes, the glare at 6300+ lux is enough to make it hard to read the screen, which matches the definition of "bright" in the spec.

The values match what I saw earlier: any lux value over 6300 gives me what seems to be the highest value the LMU tracker reports, 670 millions. Does it help if you divide the value by 10000 and then multiply the result by that at the end?

Comment 33

[Jorge Luis Mendez [:jlmendezbonini]]

> I don't understand what you meant here. What is 100001x and 63001x? And yes,
> 

The font is the culprit here...100001x = 10000 1x = 10000 1ux. The symbol for lux is lx. Sorry for the confusion.

> the glare at 6300+ lux is enough to make it hard to read the screen, which
> matches the definition of "bright" in the spec.

No, that value still falls within "normal". The spec says "dim" < 50 lux and "bright" > 10,000 lux. The values you were able to get only reached 6,300 lux, that's why I think there might be an extra 0 in the draft.

Comment 34

[Reuben Morais [:reuben]]

(In reply to Jorge Luis Mendez [:jlmendezbonini] from comment #33)
> > I don't understand what you meant here. What is 100001x and 63001x? And yes,
> > 
> 
> The font is the culprit here...100001x = 10000 1x = 10000 1ux. The symbol
> for lux is lx. Sorry for the confusion.

Hahaha, that's… embarrassing.

> > the glare at 6300+ lux is enough to make it hard to read the screen, which
> > matches the definition of "bright" in the spec.
> 
> No, that value still falls within "normal". The spec says "dim" < 50 lux and
> "bright" > 10,000 lux. The values you were able to get only reached 6,300
> lux, that's why I think there might be an extra 0 in the draft.

What I meant is that it's bright enough to make it hard to tell things apart in the screen ("direct sunlight, or similarly bright conditions that make it hard to see things that aren't high-contrast"). Also, I was able to get values higher than that, up to 200000 lux, but the LMU tracker still reports 67090000. I think it's closely tuned for the automatic display brightness feature, so it doesn't need to report anything brighter than full brightness.

Comment 35

[Jorge Luis Mendez [:jlmendezbonini]]

Attachment: Ambient Light API: OS X backend

This solution seems to be working fine. What do you think?

Comment 36

[Jorge Luis Mendez [:jlmendezbonini]]

Attachment: Ambient Light API: OS X backend

Submitted the wrong patch file, this is the right one. Sorry for that.

Comment 37

[Reuben Morais [:reuben]]

Attachment: Ambient Light API: OS X backend

(In reply to Jorge Luis Mendez [:jlmendezbonini] from comment #36)
> Created attachment 713941 [details] [diff] [review]
> Ambient Light API: OS X backend
> 
> Submitted the wrong patch file, this is the right one. Sorry for that.

This looks good! I noticed when testing your patch that it was causing a warning because of sActiveSensors being a XPCOM object initialized in a static constructor :( I fixed that by using a boolean array instead of a nsTArray.

Adding mounir and BenWa for HAL/DOM and Cocoa API reviews, respectively.

Comment 38

[Mounir Lamouri (:mounir)]

Comment on attachment 714135 [details] [diff] [review]
Ambient Light API: OS X backend

I think Doug Turner knows more about the Senser API than I do and Benoit knows about MacOS X APIs. Those reviews should be enough.

Comment 39

[Benoit Girard (:BenWa)]

Comment on attachment 714135 [details] [diff] [review]
Ambient Light API: OS X backend

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

Please brace all single line if. Mac API usage seems good.

::: hal/cocoa/CocoaSensor.mm
@@ +58,5 @@
> +
> +      values.AppendElement(accel.x * MEAN_GRAVITY);
> +      values.AppendElement(accel.y * MEAN_GRAVITY);
> +      values.AppendElement(accel.z * MEAN_GRAVITY);
> +    }

Nit: else if

Comment 40

[Doug Turner (:dougt)]

Comment on attachment 714135 [details] [diff] [review]
Ambient Light API: OS X backend

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

thanks for the patch. pretty close.  lets see one more patch. :)

::: hal/cocoa/CocoaSensor.mm
@@ +7,5 @@
>  
>  #include "smslib.h"
> +
> +#include <mach/mach.h>
> +#include <cmath> // for std::pow()

no need for the comment.  please remove.

@@ +20,5 @@
>  namespace hal_impl {
>  
>  static nsITimer* sUpdateTimer = nullptr;
> +static bool sActiveSensors[NUM_SENSOR_TYPE];
> +static io_connect_t sDataPort = 0;

shouldn't this be IO_OBJECT_NULL.

@@ +29,3 @@
>  {
> +  //Conversion formula from regression. See Bug 793728.
> +  // -3*(10^-27)*x^4 + 2.6*(10^-19)*x^3 + -3.4*(10^-12)*x^2 + 3.9*(10^-5)*x - 0.19

My brain hurts.  Ask bz to review this part.  He is suppose to be good at math.

@@ +63,5 @@
> +
> +    if (sensor == SENSOR_LIGHT) {
> +      kern_return_t kr;
> +      uint32_t outputs = 2;
> +      uint64_t LMUvalues[outputs];

variables must begin with lower case, right?

@@ +65,5 @@
> +      kern_return_t kr;
> +      uint32_t outputs = 2;
> +      uint64_t LMUvalues[outputs];
> +
> +      kr = IOConnectCallMethod(sDataPort, 0, nil, 0, nil, 0, LMUvalues, &outputs, nil, 0);

Should we check sDataPort != IO_OBJECT_NULL?  I am not sure what happens if you don't.

@@ +74,5 @@
> +        sLastMean = mean;
> +        values.AppendElement(LMUvalueToLux(mean));
> +      }
> +
> +      if (kr == kIOReturnBusy)

else if

@@ +104,3 @@
>    }
>  
> + if (aSensor == SENSOR_LIGHT) {

else if?

@@ +113,5 @@
> +      return;
> +
> +    kr = IOServiceOpen(serviceObject, mach_task_self(), 0, &sDataPort);
> +    IOObjectRelease(serviceObject);
> +    if (kr != KERN_SUCCESS)

Do we also need to test to see if sDataPort != IO_OBJECT_NULL

@@ +123,5 @@
> + if (!sUpdateTimer) {
> +   CallCreateInstance("@mozilla.org/timer;1", &sUpdateTimer);
> +    if (sUpdateTimer)
> +        sUpdateTimer->InitWithFuncCallback(UpdateHandler,
> +                                       nullptr,

the white space looks off on this.

@@ +137,5 @@
>      return;
>  
> +  sActiveSensors[aSensor] = false;
> +
> +  if (aSensor == SENSOR_ACCELERATION)

Lets make the braces consist.  Probably not consist in this file yet.

if () {
  stmt;
}

@@ +140,5 @@
> +
> +  if (aSensor == SENSOR_ACCELERATION)
> +    smsShutdown();
> +
> +  if (aSensor == SENSOR_LIGHT)

else if

@@ +148,2 @@
>    if (sUpdateTimer) {
> +    for (int i = 0; i < NUM_SENSOR_TYPE; ++i) {

why not i++? like the other places.

Comment 41

[Jorge Luis Mendez [:jlmendezbonini]]

Thanks Reuben, Benoit, and Doug for reviewing the patch and your feedback. 

> thanks for the patch. pretty close.  lets see one more patch. :)

Thanks, happy to keep improving it.

> >  static nsITimer* sUpdateTimer = nullptr;
> > +static bool sActiveSensors[NUM_SENSOR_TYPE];
> > +static io_connect_t sDataPort = 0;
> 
> shouldn't this be IO_OBJECT_NULL.
> 

You are right. It's probably better than using a 0

> > +      kern_return_t kr;
> > +      uint32_t outputs = 2;
> > +      uint64_t LMUvalues[outputs];
> > +
> > +      kr = IOConnectCallMethod(sDataPort, 0, nil, 0, nil, 0, LMUvalues, &outputs, nil, 0);
> 
> Should we check sDataPort != IO_OBJECT_NULL?  I am not sure what happens if
> you don't.
> 

The new patch includes that check. 

By the way, IOConnectCallMethod would fail with error: "invalid destination port error"  if sDataPort == IO_OBJECT_NULL

> > +    kr = IOServiceOpen(serviceObject, mach_task_self(), 0, &sDataPort);
> > +    IOObjectRelease(serviceObject);
> > +    if (kr != KERN_SUCCESS)
> 
> Do we also need to test to see if sDataPort != IO_OBJECT_NULL

This would never evaluate to true since we are initializing sDataPort to IO_OBJECT_NULL and we have to use IOServiceOpen() to modify it.

Maybe you were thinking of a null pointer? That segfaults but I don't see how it could happen here since sDataPort is not a pointer.

> >    if (sUpdateTimer) {
> > +    for (int i = 0; i < NUM_SENSOR_TYPE; ++i) {
> 
> why not i++? like the other places.

Because I used CocoaBattery.cpp as a code style reference :)

Comment 42

[Jorge Luis Mendez [:jlmendezbonini]]

Attachment: Ambient Light API: OS X backend

Apart from the requested changes I noticed that LMUvalueToLux() would return negative values when AppleLMUTracker values approached zero. We now make sure that the lux values are always > 0.

Comment 43

[Doug Turner (:dougt)]

Comment on attachment 715201 [details] [diff] [review]
Ambient Light API: OS X backend

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

::: hal/cocoa/CocoaSensor.mm
@@ +26,2 @@
>  
> +float

static float

@@ +39,5 @@
> +  long double term2 = -3.4 * powerC2 * pow(aValue,2);
> +  long double term1 =  3.9 * powerC1 * aValue;
> +
> +  float lux = static_cast<float>(term4 + term3 + term2 + term1 - 0.19);
> +  return lux > 0 ? lux : 0;

bz, can you review this piece.

@@ +60,5 @@
> +
> +      values.AppendElement(accel.x * MEAN_GRAVITY);
> +      values.AppendElement(accel.y * MEAN_GRAVITY);
> +      values.AppendElement(accel.z * MEAN_GRAVITY);
> +    } else if (sensor == SENSOR_LIGHT  && sDataPort != IO_OBJECT_NULL) {

+    } else if (sensor == SENSOR_LIGHT  && sDataPort != IO_OBJECT_NULL) {

Please only one spaces between _LIGHT and the &&

@@ +94,2 @@
>      return;
>    }

Please remove this test -- since we are doing it below.

don't worry about returning early -- instead -- just add a final else{}

@@ +116,5 @@
> +    IOObjectRelease(serviceObject);
> +    if (kr != KERN_SUCCESS) {
> +      return;
> +    }
> +  }

} else {
   NS_WARNING("EnableSensorNotifications called on an unknown sensor type...
   return;
 }

Comment 44

[Doug Turner (:dougt)]

another thing I think we should consider.  We may want to round the value up to the closest Lux whole number.  I am seeing numbers like:

55.20669174194336

What do you think?

Comment 45

[Jorge Luis Mendez [:jlmendezbonini]]

Good point.  Most Lux values I've seen are given as whole numbers so, even though the developer can round it up himself, it seems more logical to report a whole number in the first place.

Unless anybody disagrees, I'll create a new patch to round the reported value and your latest code review feedback.

Comment 46

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 715201 [details] [diff] [review]
Ambient Light API: OS X backend

> bz, can you review this piece.

Seems fine as far as it goes, if we're not overfitting the curve....

Comment 47

[Jorge Luis Mendez [:jlmendezbonini]]

Attachment: Ambient Light API: OS X backend

Includes Doug's feedback and rounds up the reported Lux value.

Comment 48

[Reuben Morais [:reuben]]

\o/

Comment 49

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/e952b94e1f78

Thanks for the patch!

Comment 50

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/e952b94e1f78

Comment 51

[Florian Scholz (Open Web Docs)]

Documented:
https://developer.mozilla.org/en-US/docs/Firefox_22_for_developers#DOM
https://developer.mozilla.org/en-US/docs/DOM/DeviceLightEvent#Browser_compatibility

Comment 52

[Alex Keybl [:akeybl]]

(In reply to Florian Scholz [:fs] from comment #51)
> Documented:
> https://developer.mozilla.org/en-US/docs/Firefox_22_for_developers#DOM
> https://developer.mozilla.org/en-US/docs/DOM/
> DeviceLightEvent#Browser_compatibility

Since this is well documented on MDN for Mac OS X developers, we're going to leave this off of the user release notes.