Note: There are a few cases of duplicates in user autocompletion which are being worked on.

move device motion from dom/system to hal

RESOLVED FIXED in mozilla14

Status

()

Core
Geolocation
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: dougt, Assigned: dougt)

Tracking

unspecified
mozilla14
x86
Mac OS X
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 6 obsolete attachments)

(Assignee)

Description

5 years ago
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.
(Assignee)

Comment 1

5 years ago
Created attachment 605453 [details] [diff] [review]
core and android
Assignee: nobody → doug.turner
Attachment #605453 - Flags: review?(josh)
(Assignee)

Comment 2

5 years ago
Created attachment 605456 [details] [diff] [review]
Cocoa
Attachment #605456 - Flags: review?(josh)
(Assignee)

Comment 3

5 years ago
Created attachment 605457 [details] [diff] [review]
others
Attachment #605457 - Flags: review?(josh)
(Assignee)

Comment 4

5 years ago
Created attachment 605458 [details] [diff] [review]
others
Attachment #605457 - Attachment is obsolete: true
Attachment #605458 - Flags: review?(josh)
Attachment #605457 - Flags: review?(josh)
(Assignee)

Updated

5 years ago
Blocks: 735337
(Assignee)

Updated

5 years ago
Blocks: 735345
(Assignee)

Updated

5 years ago
Blocks: 735346
(Assignee)

Comment 5

5 years ago
Created attachment 606099 [details] [diff] [review]
support compassneedscalibration event via hal
Attachment #606099 - Flags: review?(josh)
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 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
(Assignee)

Comment 8

5 years ago
> Don't forget.

See 4th patch.

> Add a MOZ_STATIC_ASSERT that they match somewhere

Huh?

Comment 9

5 years ago
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?
Attachment #605453 - Flags: review?(josh) → review+
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.
Attachment #605456 - Flags: review?(josh)
Is there simply no replacement for the linux/QT implementations that are being removed?
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?
Attachment #606099 - Flags: review?(josh) → review+
(Assignee)

Comment 13

5 years ago
> 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!
(Assignee)

Comment 14

5 years ago
Created attachment 607279 [details] [diff] [review]
rollup v.2

rollup of the patches you saw w/ fixes and .mm files.
Attachment #605453 - Attachment is obsolete: true
Attachment #605456 - Attachment is obsolete: true
Attachment #605458 - Attachment is obsolete: true
Attachment #606099 - Attachment is obsolete: true
Attachment #605458 - Flags: review?(josh)
(Assignee)

Updated

5 years ago
Attachment #607279 - Flags: review?(josh)
(Assignee)

Comment 15

5 years ago
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).
Attachment #607280 - Flags: review?(josh)

Comment 16

5 years ago
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.

Updated

5 years ago
Duplicate of this bug: 734874
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.
Attachment #607279 - Flags: review?(josh)
(Assignee)

Comment 19

5 years ago
> 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 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.
Attachment #607280 - Flags: review?(josh) → review+
(Assignee)

Comment 21

5 years ago
Created attachment 607445 [details] [diff] [review]
remove static nsCOMPtr from cocoa hal sensor implementation
(Assignee)

Comment 22

5 years ago
Comment on attachment 607445 [details] [diff] [review]
remove static nsCOMPtr from cocoa hal sensor implementation

wtf. sorry.
Attachment #607445 - Attachment is obsolete: true
(Assignee)

Comment 23

5 years ago
Created attachment 607449 [details] [diff] [review]
remove static nsCOMPtr from cocoa hal sensor implementation

Updated

5 years ago
Attachment #607449 - Flags: review+

Updated

5 years ago
Attachment #607279 - Flags: review+
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
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.
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.
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.
Whiteboard: [inbound - clobber OSX builds after merging]

Updated

5 years ago
Depends on: 738102
https://hg.mozilla.org/mozilla-central/rev/96bcc7997727
https://hg.mozilla.org/mozilla-central/rev/3aa1b84ef807
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Whiteboard: [inbound - clobber OSX builds after merging]
Target Milestone: --- → mozilla14
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)
Are you completely removing the objdir you're building? I don't understand how you can be seeing this error if you clobbered.
Removing the objdir worked. This is totally weird, I would have expected clobbering to work as well. Sorry for the spam.
You need to log in before you can comment on or make changes to this bug.