Last Comment Bug 734855 - Device Orientation - Make EnableDeviceMotion finer grain
: Device Orientation - Make EnableDeviceMotion finer grain
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Geolocation (show other bugs)
: unspecified
: x86 Mac OS X
: -- normal (vote)
: mozilla14
Assigned To: Doug Turner (:dougt)
:
Mentors:
Depends on: 740252
Blocks:
  Show dependency treegraph
 
Reported: 2012-03-12 06:13 PDT by Doug Turner (:dougt)
Modified: 2012-03-28 22:01 PDT (History)
1 user (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch 1/2. This makes EnableDeviceMotion finer grain. (25.75 KB, patch)
2012-03-23 14:31 PDT, Doug Turner (:dougt)
no flags Details | Diff | Review
patch 2/2. This renames device motion to device sensors (21.67 KB, patch)
2012-03-23 14:32 PDT, Doug Turner (:dougt)
mwu.code: review+
Details | Diff | Review
patch 1/2. This makes EnableDeviceMotion finer grain. v.2 (26.57 KB, patch)
2012-03-23 17:16 PDT, Doug Turner (:dougt)
mwu.code: review+
Details | Diff | Review

Description Doug Turner (:dougt) 2012-03-12 06:13:52 PDT
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) 2012-03-23 14:31:38 PDT
Created attachment 608867 [details] [diff] [review]
patch 1/2.  This makes EnableDeviceMotion finer grain.
Comment 2 Doug Turner (:dougt) 2012-03-23 14:32:41 PDT
Created attachment 608869 [details] [diff] [review]
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] 2012-03-23 15:01:21 PDT
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) 2012-03-23 17:16:20 PDT
Created attachment 608921 [details] [diff] [review]
patch 1/2.  This makes EnableDeviceMotion finer grain. v.2
Comment 5 Michael Wu [:mwu] 2012-03-23 17:50:22 PDT
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

Note You need to log in before you can comment on or make changes to this bug.