734855 - Device Orientation - Make EnableDeviceMotion finer grain

Status: RESOLVED FIXED

(Core :: DOM: Geolocation, defect)

Description

[Doug Turner (:dougt)]

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.

Comment 1

[Doug Turner (:dougt)]

Attachment: patch 1/2. This makes EnableDeviceMotion finer grain.

Comment 2

[Doug Turner (:dougt)]

Attachment: patch 2/2. This renames device motion to device sensors

since this class is really does more than device motion, lets rename the fucker.

Comment 3

[Michael Wu [:mwu]]

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

Comment 4

[Doug Turner (:dougt)]

Attachment: patch 1/2. This makes EnableDeviceMotion finer grain. v.2

Comment 5

[Michael Wu [: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

Comment 6

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/246d43704fc7
https://hg.mozilla.org/mozilla-central/rev/df108e67c87a
https://hg.mozilla.org/mozilla-central/rev/b3b3928b2c0f