Closed Bug 734855 Opened 12 years ago Closed 12 years ago

Device Orientation - Make EnableDeviceMotion finer grain

Categories

(Core :: DOM: Geolocation, defect)

x86
macOS
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla14

People

(Reporter: dougt, Assigned: dougt)

References

Details

Attachments

(2 files, 1 obsolete file)

EnableDeviceMotion currently enables all sensors:

        EnableSensor(hal::SENSOR_ORIENTATION);                                             EnableSensor(hal::SENSOR_ACCELERATION);                                            EnableSensor(hal::SENSOR_LINEAR_ACCELERATION);                                     EnableSensor(hal::SENSOR_GYROSCOPE); 


We can do better by only requiring SENSOR_ORIENTATION for device orientation and the other 3 for device motion.
Assignee: nobody → doug.turner
Attachment #608867 - Flags: review?(mwu)
since this class is really does more than device motion, lets rename the fucker.
Attachment #608869 - Flags: review?(mwu)
Attachment #608869 - Flags: review?(mwu) → review+
Comment on attachment 608867 [details] [diff] [review]
patch 1/2.  This makes EnableDeviceMotion finer grain.

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

Not entirely sure if I'm qualified to review this, but I think I understand what's going on.

::: content/events/src/nsEventListenerManager.cpp
@@ +84,5 @@
>  #include "nsIContentSecurityPolicy.h"
>  #include "nsJSEnvironment.h"
>  #include "xpcpublic.h"
>  #include "sampler.h"
> +#

?

::: dom/base/nsGlobalWindow.cpp
@@ +10060,5 @@
>  
>    if (!suspended) {
> +    nsCOMPtr<nsIDeviceMotion> ac = do_GetService(NS_DEVICE_MOTION_CONTRACTID);
> +    if (ac)
> +      ac->PauseAllSensors();

Shouldn't we disable just the sensors for this window instead all sensors?

::: dom/system/nsDeviceMotion.cpp
@@ +134,5 @@
>      if (NS_SUCCEEDED(rv) && bvalue == false)
>        mEnabled = false;
>    }
> +
> +  for (int i=0; i<NUM_SENSOR_TYPE; i++) {

This file appears to use spaces around operators like = and <.

@@ +181,3 @@
>  {
> +  for (int i=0; i<NUM_SENSOR_TYPE; i++) {
> +    if (IsSensorEnabled(i))

This check seems unnecessary
Attachment #608867 - Attachment is obsolete: true
Attachment #608921 - Flags: review?(mwu)
Attachment #608867 - Flags: review?(mwu)
Comment on attachment 608921 [details] [diff] [review]
patch 1/2.  This makes EnableDeviceMotion finer grain. v.2

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

Looks good to me.

::: dom/base/nsGlobalWindow.h
@@ +539,5 @@
>    virtual nsresult DispatchSyncPopState();
>  
> +  virtual void EnableDeviceSensor(PRUint32 aType);
> +  virtual void DisableDeviceSensor(PRUint32 aType);
> +  nsTArray<PRUint32> mEnabledSensors;

This could probably be private/protected.

::: dom/system/nsDeviceMotion.h
@@ +74,5 @@
> +  // sensor -> window listener
> +  nsTArray<nsTArray<nsIDOMWindow*>* > mWindowListeners;
> +  
> +  // window -> sensortype enabled
> +  nsDataHashtable<nsUint32HashKey, nsTArray<PRUint32 > > mSensorsEnabled;

Remove
Attachment #608921 - Flags: review?(mwu) → review+
You need to log in before you can comment on or make changes to this bug.