Last Comment Bug 735330 - move device motion from dom/system to hal
: move device motion from dom/system to hal
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:
: 734874 (view as bug list)
Depends on: 738102
Blocks: 735337 735345 735346
  Show dependency treegraph
 
Reported: 2012-03-13 11:05 PDT by Doug Turner (:dougt)
Modified: 2012-03-27 00:49 PDT (History)
7 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
core and android (40.67 KB, patch)
2012-03-13 11:06 PDT, Doug Turner (:dougt)
josh: review+
Details | Diff | Splinter Review
Cocoa (14.04 KB, patch)
2012-03-13 11:06 PDT, Doug Turner (:dougt)
no flags Details | Diff | Splinter Review
others (108 bytes, patch)
2012-03-13 11:07 PDT, Doug Turner (:dougt)
no flags Details | Diff | Splinter Review
others (28.29 KB, patch)
2012-03-13 11:08 PDT, Doug Turner (:dougt)
no flags Details | Diff | Splinter Review
support compassneedscalibration event via hal (15.18 KB, patch)
2012-03-14 20:28 PDT, Doug Turner (:dougt)
josh: review+
Details | Diff | Splinter Review
rollup v.2 (96.53 KB, patch)
2012-03-19 13:27 PDT, Doug Turner (:dougt)
josh: review+
Details | Diff | Splinter Review
gonk build bustage and cleanup/bug fixes (13.03 KB, patch)
2012-03-19 13:28 PDT, Doug Turner (:dougt)
josh: review+
Details | Diff | Splinter Review
remove static nsCOMPtr from cocoa hal sensor implementation (9.71 KB, patch)
2012-03-19 21:40 PDT, Doug Turner (:dougt)
no flags Details | Diff | Splinter Review
remove static nsCOMPtr from cocoa hal sensor implementation (1.34 KB, patch)
2012-03-19 21:52 PDT, Doug Turner (:dougt)
josh: review+
Details | Diff | Splinter Review

Description Doug Turner (:dougt) 2012-03-13 11:05:11 PDT
Move device motion/orientation code from dom/system into Hal.

This is for Android and Cocoa.  The Qt/Linux/Window are returning incorrect units.  I will file follow up bugs to have those fixed up.
Comment 1 Doug Turner (:dougt) 2012-03-13 11:06:23 PDT
Created attachment 605453 [details] [diff] [review]
core and android
Comment 2 Doug Turner (:dougt) 2012-03-13 11:06:49 PDT
Created attachment 605456 [details] [diff] [review]
Cocoa
Comment 3 Doug Turner (:dougt) 2012-03-13 11:07:08 PDT
Created attachment 605457 [details] [diff] [review]
others
Comment 4 Doug Turner (:dougt) 2012-03-13 11:08:49 PDT
Created attachment 605458 [details] [diff] [review]
others
Comment 5 Doug Turner (:dougt) 2012-03-14 20:28:37 PDT
Created attachment 606099 [details] [diff] [review]
support compassneedscalibration event via hal
Comment 6 :Ms2ger 2012-03-15 05:27:54 PDT
Comment on attachment 606099 [details] [diff] [review]
support compassneedscalibration event via hal

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

::: dom/system/nsDeviceMotion.cpp
@@ +242,5 @@
>  
> +  SensorAccuracyType accuracy = aSensorData.accuracy();
> +
> +  if (accuracy <= SENSOR_ACCURACY_LOW && mLastAccuracy >= SENSOR_ACCURACY_MED) {
> +      FireNeedsCalibration();

Two spaces
Comment 7 :Ms2ger 2012-03-15 05:35:16 PDT
Comment on attachment 605453 [details] [diff] [review]
core and android

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

::: dom/system/nsDeviceMotion.cpp
@@ +35,5 @@
>   * ***** END LICENSE BLOCK ***** */
>  
> +#include "mozilla/Hal.h"
> +#include "mozilla/HalSensor.h"
> +

Include *after* nsDeviceMotion.h, please.

@@ +55,2 @@
>  using mozilla::TimeStamp;
>  using mozilla::TimeDuration;

These two should go now.

@@ +123,1 @@
>  }

Revert

@@ +267,5 @@
>      if (domdoc) {
>        nsCOMPtr<nsIDOMEventTarget> target = do_QueryInterface(windowListeners[i]);
>        if (type == nsIDeviceMotionData::TYPE_ACCELERATION || 
>          type == nsIDeviceMotionData::TYPE_LINEAR_ACCELERATION || 
> +	type == nsIDeviceMotionData::TYPE_GYROSCOPE )

You're introducing a tab here.

::: widget/android/nsAppShell.cpp
@@ +333,5 @@
>  
>      case AndroidGeckoEvent::SENSOR_ACCURACY:
> +        // todo:
> +        //        if (curEvent->Flags() == 0)
> +        //            gDeviceMotionSystem->NeedsCalibration();

Don't forget.

@@ +342,3 @@
>          mPendingSensorEvents = false;
> +        InfallibleTArray<float> values;
> +        mozilla::hal::SensorType type = (mozilla::hal::SensorType) curEvent->Flags();

'mozilla::hal::SensorType(curEvent->Flags())'; this is C++

::: xpcom/system/nsIDeviceMotion.idl
@@ +42,5 @@
>  interface nsIDeviceMotionData : nsISupports
>  {
> +  // Keep in sync with hal/HalSensor.h
> +  const unsigned long TYPE_ORIENTATION = 0;
> +  const unsigned long TYPE_ACCELERATION = 1;

Add a MOZ_STATIC_ASSERT that they match somewhere
Comment 8 Doug Turner (:dougt) 2012-03-15 07:47:17 PDT
> Don't forget.

See 4th patch.

> Add a MOZ_STATIC_ASSERT that they match somewhere

Huh?
Comment 9 Josh Matthews [:jdm] 2012-03-18 18:54:37 PDT
Comment on attachment 605453 [details] [diff] [review]
core and android

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

Address Ms2ger's concerns and ship it!

::: widget/android/AndroidBridge.cpp
@@ -306,5 @@
> -{
> -    ALOG_BRIDGE("AndroidBridge::EnableDeviceMotion");
> -
> -    // bug 734855 - we probably can make this finer grain based on
> -    // the DOM APIs that are being invoked.

This comment didn't make it to nsDeviceMotion::Startup. Should it?
Comment 10 Josh Matthews [:jdm] 2012-03-18 18:56:01 PDT
Comment on attachment 605456 [details] [diff] [review]
Cocoa

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

::: hal/Makefile.in
@@ +105,5 @@
> +  FallbackHal.cpp \
> +  $(NULL)
> +CMMSRCS += \
> +  CocoaSensor.mm \
> +  smslib.mm \

These two mm files are missing.
Comment 11 Josh Matthews [:jdm] 2012-03-18 19:12:47 PDT
Is there simply no replacement for the linux/QT implementations that are being removed?
Comment 12 Josh Matthews [:jdm] 2012-03-18 19:19:08 PDT
Comment on attachment 606099 [details] [diff] [review]
support compassneedscalibration event via hal

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

::: mobile/android/base/GeckoApp.java
@@ +2606,5 @@
>      public GeckoLayerClient getLayerClient() { return mLayerClient; }
>      public LayerController getLayerController() { return mLayerController; }
>  
>      // accelerometer
>      public void onAccuracyChanged(Sensor sensor, int accuracy)

Can this be removed now?
Comment 13 Doug Turner (:dougt) 2012-03-19 13:22:52 PDT
> Is there simply no replacement for the linux/QT implementations that are being removed?

I filed follow up bugs.  The implementations that were there are not returning the right units.

> Can this be removed now?

Yes!

> This comment didn't make it to nsDeviceMotion::Startup. Should it?

Yes!
Comment 14 Doug Turner (:dougt) 2012-03-19 13:27:28 PDT
Created attachment 607279 [details] [diff] [review]
rollup v.2

rollup of the patches you saw w/ fixes and .mm files.
Comment 15 Doug Turner (:dougt) 2012-03-19 13:28:59 PDT
Created attachment 607280 [details] [diff] [review]
gonk build bustage and cleanup/bug fixes

gonk didn't build with the first patch.  This fixes the problem and includes some required bug fixes (like us not writing to random memory).
Comment 16 Andreas Gal :gal 2012-03-19 16:59:06 PDT
When content isn't listening we should have much longer delays (1000ms?) so we can pick up screen rotation. Also, when screen is off, we should stop polling altogether. We currently poll while the device sleeps.
Comment 17 Michael Wu [:mwu] 2012-03-19 17:15:15 PDT
*** Bug 734874 has been marked as a duplicate of this bug. ***
Comment 18 Josh Matthews [:jdm] 2012-03-19 21:06:17 PDT
Comment on attachment 607279 [details] [diff] [review]
rollup v.2

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

I'd like to see a patch that fixes the nsCOMPtr issue (just the change itself for review, please).

::: dom/system/nsDeviceMotion.cpp
@@ +55,2 @@
>  using mozilla::TimeStamp;
>  using mozilla::TimeDuration;

The more specific ones here can be removed.

@@ +118,5 @@
>  {
>    NS_ENSURE_ARG_POINTER(aZ);
>    *aZ = mZ;
>    return NS_OK;
> +

Kill it.

::: hal/cocoa/CocoaSensor.mm
@@ +13,5 @@
> +
> +namespace mozilla {
> +namespace hal_impl {
> +
> +static nsCOMPtr<nsITimer> sUpdateTimer;

Static nsCOMPtrs should be avoided at all costs.
Comment 19 Doug Turner (:dougt) 2012-03-19 21:23:54 PDT
> When content isn't listening we should have much longer delays (1000ms?) so we can pick up screen rotation. Also, when screen is off, we should stop polling altogether. We currently poll while the device sleeps.

Andreas, that is already done when the window is deactivated.  See http://mxr.mozilla.org/mozilla-central/source/dom/base/nsGlobalWindow.cpp#10069
Comment 20 Josh Matthews [:jdm] 2012-03-19 21:38:57 PDT
Comment on attachment 607280 [details] [diff] [review]
gonk build bustage and cleanup/bug fixes

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

::: hal/gonk/GonkSensor.cpp
@@ +108,5 @@
> +
> +  NS_IMETHOD Run()
> +  {
> +
> +

Destroy all^H^H^Hsome blank lines.

@@ +109,5 @@
> +  NS_IMETHOD Run()
> +  {
> +
> +
> +  printf_stderr(" NotifySensorChange type = %d - %f, %f, %f, %d\n", 

Indentation here is weird.

@@ +188,5 @@
>  
>      void Switch() {
>       SensorDevice& device = SensorDevice::getInstance();
>       device.activate((void*)threadId, sensor.handle, activate);
> +     device.setDelay((void*)threadId, sensor.handle, 100000000 /*100ms*/);

Make this a named value instead of a magic number.
Comment 21 Doug Turner (:dougt) 2012-03-19 21:40:45 PDT
Created attachment 607445 [details] [diff] [review]
remove static nsCOMPtr from cocoa hal sensor implementation
Comment 22 Doug Turner (:dougt) 2012-03-19 21:51:10 PDT
Comment on attachment 607445 [details] [diff] [review]
remove static nsCOMPtr from cocoa hal sensor implementation

wtf. sorry.
Comment 23 Doug Turner (:dougt) 2012-03-19 21:52:05 PDT
Created attachment 607449 [details] [diff] [review]
remove static nsCOMPtr from cocoa hal sensor implementation
Comment 24 Matt Brubeck (:mbrubeck) 2012-03-20 17:03:55 PDT
Sorry, I backed this out because XUL Fennec was broken by one of the bugs this landed with.  This can probably re-land if it doesn't depend on bug 734854.
https://hg.mozilla.org/integration/mozilla-inbound/rev/4c0b0a3c272f
Comment 25 Matt Brubeck (:mbrubeck) 2012-03-20 19:24:50 PDT
This also failed to build in all Mac builds:

https://tbpl.mozilla.org/php/getParsedLog.php?id=10226368&tree=Mozilla-Inbound#error0
ld: duplicate symbol smsGetCalibration(sms_calibration*)      in ../../hal/smslib.o and ../../dom/system/cocoa/smslib.o

None of the builds were clobbers; I don't know whether clobbering would fix the build error or not.
Comment 26 Matt Brubeck (:mbrubeck) 2012-03-20 22:37:51 PDT
A Mac clobber build eventually succeeded, so it looks like all you probably need to do is set a clobber on all Mac platforms before re-landing this.
Comment 27 Matt Brubeck (:mbrubeck) 2012-03-21 10:25:33 PDT
This broke the Mac build again; I clobbered the builders so hopefully it will work now.  Inbound sheriffs, please clobber all Mac builds on m-c when this is merged.

Until bug 717372 is fixed, it would be good to put something in the commit message when you push a change that you know requires a clobber.
Comment 29 Victor Porof [:vporof][:vp] 2012-03-26 18:17:08 PDT
This broke my build on OS X Lion (using clang). I clobbered, distcleaned, even got myself a fresh clone. I tried with both:

Apple clang version 3.1 (tags/Apple/clang-318.0.58) (based on LLVM 3.1svn)
Target: x86_64-apple-darwin11.3.0

and

clang version 2.9 (tags/RELEASE_29/final)
Target: x86_64-apple-darwin11

Error is:

ld: duplicate symbol smsStartup(objc_object*, objc_selector*) in ../../hal/smslib.o and ../../dom/system/cocoa/smslib.o for architecture x86_64
clang: error: linker command failed with exit code 1 (use -v to see invocation)
Comment 30 Josh Matthews [:jdm] 2012-03-26 18:24:23 PDT
Are you completely removing the objdir you're building? I don't understand how you can be seeing this error if you clobbered.
Comment 31 Victor Porof [:vporof][:vp] 2012-03-27 00:49:42 PDT
Removing the objdir worked. This is totally weird, I would have expected clobbering to work as well. Sorry for the spam.

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