Closed Bug 734325 Opened 12 years ago Closed 12 years ago

implement device motion - compassneedscalibration support

Categories

(Core :: DOM: Geolocation, defect)

x86
macOS
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla14

People

(Reporter: dougt, Assigned: dougt)

Details

Attachments

(1 file)

Attached patch v.1Splinter Review
implement compassneedscalibration support on android
Attachment #604310 - Flags: review?(josh)
Comment on attachment 604310 [details] [diff] [review]
v.1

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

Looks fine to me.

::: dom/system/nsDeviceMotion.cpp
@@ +49,5 @@
>  #include "nsIServiceManager.h"
>  #include "nsIPrefService.h"
>  #include "nsDOMDeviceMotionEvent.h"
>  
> +#include <android/log.h>

#ifdef?

@@ +256,5 @@
>  
> +NS_IMETHODIMP
> +nsDeviceMotion::NeedsCalibration()
> +{
> +  __android_log_print(ANDROID_LOG_ERROR, "Gecko", "### FireNeedsCalibration - NeedsCalibration ");

I expect this needs some kind of ifdef goop for non-Android?

@@ +261,5 @@
> +
> +  if (!mEnabled)
> +    return NS_ERROR_NOT_INITIALIZED;
> +
> +  for (PRUint32 i = mListeners.Count(); i > 0 ; ) {

Any particular reason these loops need to go backwards?

@@ +270,5 @@
> +  for (PRUint32 i = mWindowListeners.Length(); i > 0 ; ) {
> +    --i;
> +
> +    // check to see if this window is in the background.  if
> +    // it is, don't send any device motion to it.

I think it's worth moving this common check into a helper.

@@ +294,5 @@
> +nsDeviceMotion::FireNeedsCalibration(nsIDOMDocument *domdoc,
> +				    nsIDOMEventTarget *target)
> +{
> +
> +  __android_log_print(ANDROID_LOG_ERROR, "Gecko", "### FireNeedsCalibration ");

#ifdef, I presume.

@@ +297,5 @@
> +
> +  __android_log_print(ANDROID_LOG_ERROR, "Gecko", "### FireNeedsCalibration ");
> +
> +  nsCOMPtr<nsIDOMEvent> event;
> +  bool defaultActionEnabled = true;

Move this down by where it's used.
Attachment #604310 - Flags: review?(josh) → review+
I removed the logging code.  It really isn't needed.

> I think it's worth moving this common check into a helper.

yes!  I am fixing this up in another patch.  You'll get the review request soon, i hope!.
Assignee: nobody → doug.turner
https://hg.mozilla.org/mozilla-central/rev/781e874d0ff5
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla14
You need to log in before you can comment on or make changes to this bug.