Last Comment Bug 734325 - implement device motion - compassneedscalibration support
: implement device motion - compassneedscalibration support
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:
Blocks:
  Show dependency treegraph
 
Reported: 2012-03-08 22:14 PST by Doug Turner (:dougt)
Modified: 2012-03-14 10:09 PDT (History)
3 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
v.1 (14.13 KB, patch)
2012-03-08 22:14 PST, Doug Turner (:dougt)
josh: review+
Details | Diff | Review

Description Doug Turner (:dougt) 2012-03-08 22:14:09 PST
Created attachment 604310 [details] [diff] [review]
v.1

implement compassneedscalibration support on android
Comment 1 Josh Matthews [:jdm] 2012-03-08 22:41:58 PST
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.
Comment 2 Doug Turner (:dougt) 2012-03-08 22:54:23 PST
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!.
Comment 4 Matt Brubeck (:mbrubeck) 2012-03-14 10:09:26 PDT
https://hg.mozilla.org/mozilla-central/rev/781e874d0ff5

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