605341 - remote orientation api for android

Status: RESOLVED FIXED

(Core Graveyard :: Widget: Android, defect)

Description

[Brad Lassey [:blassey] (use needinfo?)]

AndroidBridge isn't available in the content process

Comment 1

[Brad Lassey [:blassey] (use needinfo?)]

Attachment: patch

Comment 2

[Doug Turner (:dougt)]

Comment on attachment 484245 [details] [diff] [review]
patch


>+#ifdef ANDROID 
>+#include "../system/android/nsAccelerometerSystem.h"
>+extern nsAccelerometerSystem *gAccel;
>+#endif

how about static nsAccelerometerSystem::AccelerationChanged() instead of exposing a pointer.  This allows you to do the null check (or even allocate inside of nsAccelerometerSystem.

>@@ -56,10 +57,14 @@ void nsAccelerometerSystem::Startup()
> {
>     if (AndroidBridge::Bridge())
>         AndroidBridge::Bridge()->EnableAccelerometer(true);
>+    else
>+        mozilla::dom::ContentChild::GetSingleton()->SendEnableAccelerometer(true);
> }

Hide this in the bridge?



> 
> void nsAccelerometerSystem::Shutdown()
> {
>     if (AndroidBridge::Bridge())
>         AndroidBridge::Bridge()->EnableAccelerometer(false);
>+    else
>+        mozilla::dom::ContentChild::GetSingleton()->SendEnableAccelerometer(false);
> }

same.  can we hide this in the bridge?

>diff --git a/widget/src/android/nsAppShell.cpp b/widget/src/android/nsAppShell.cpp
>--- a/widget/src/android/nsAppShell.cpp
>+++ b/widget/src/android/nsAppShell.cpp
>@@ -36,6 +36,7 @@
>  *
>  * ***** END LICENSE BLOCK ***** */
> 
>+#include "mozilla/dom/ContentParent.h"
> #include "nsAppShell.h"
> #include "nsWindow.h"
> #include "nsThreadUtils.h"
>@@ -222,7 +223,10 @@ nsAppShell::ProcessNextNativeEvent(PRBoo
>         break;
> 
>     case AndroidGeckoEvent::SENSOR_EVENT:
>-        gAccel->AccelerationChanged(-curEvent->X(), curEvent->Y(), curEvent->Z());
>+        if (gAccel)
>+            gAccel->AccelerationChanged(-curEvent->X(), curEvent->Y(), curEvent->Z());
>+        mozilla::dom::ContentParent::GetSingleton()->
>+            SendAccelerationChanged(-curEvent->X(), curEvent->Y(), curEvent->Z());
>         break;

Just do the SendAccelerationChanged call inside of nsAccelerometerSystem

Comment 3

[Brad Lassey [:blassey] (use needinfo?)]

Attachment: patch

Comment 4

[Doug Turner (:dougt)]

Comment on attachment 484586 [details] [diff] [review]
patch

Lets try this.  I think you should do this similar to what we did for geo.  Add an AccelerationUpdate() message (instead of AccelerationChanged) to be consistent.


Instead of:
+    async AddAccelerometerListener();
+    async RemoveAccelerometerListener();

Just add:

AccelerometerStart();
AccelerometerStop();

Similar to Geo.  Extra points to just pass a flag (like on/off) and convert the geo ones to use a flag.

Then make the content parent just be a listener, like you are doing -- but you don't have to have specific calls in the pidl (just start and stop)

You can follow what I did in ContentParent and ContentChild classes to give you a better idea of what I am asking for.

Comment 5

[Brad Lassey [:blassey] (use needinfo?)]

Attachment: patch

Comment 6

[Doug Turner (:dougt)]

Comment on attachment 485137 [details] [diff] [review]
patch



>+    nsCOMPtr<nsIAccelerometer> iac = 
>+        do_GetService(NS_ACCELEROMETER_CONTRACTID);
>+    nsCOMPtr<nsAccelerometer> ac = do_QueryInterface(iac);
>+    if (ac)
>+        ac->AccelerationChanged(x, y, z);

I am surprised this works at all.

I'd rather you add a new interface.


> class nsAcceleration : public nsIAcceleration
> {
>@@ -156,7 +158,11 @@ nsAccelerometer::TimeoutHandler(nsITimer
>   
>   // what about listeners that don't clean up properly?  they will leak
>   if (self->mListeners.Count() == 0 && self->mWindowListeners.Count() == 0) {
>-    self->Shutdown();
>+    if (XRE_GetProcessType() == GeckoProcessType_Default)
>+      self->Shutdown();
>+    else
>+      mozilla::dom::ContentChild::GetSingleton()->
>+        SendRemoveAccelerometerListener();


would it make sense to just put the |SendRemoveAccelerometerListener| in Shutdown()?


>     mNewListener = PR_TRUE;
>-    Startup();
>+    
>+    if (XRE_GetProcessType() == GeckoProcessType_Default)
>+      Startup();
>+    else
>+      mozilla::dom::ContentChild::GetSingleton()->
>+        SendAddAccelerometerListener();


Same idea here -- move SendAddAccelerometerListener into Startup()

Comment 7

[Brad Lassey [:blassey] (use needinfo?)]

(In reply to comment #6)
> Comment on attachment 485137 [details] [diff] [review]
> patch
> 
> 
> 
> >+    nsCOMPtr<nsIAccelerometer> iac = 
> >+        do_GetService(NS_ACCELEROMETER_CONTRACTID);
> >+    nsCOMPtr<nsAccelerometer> ac = do_QueryInterface(iac);
> >+    if (ac)
> >+        ac->AccelerationChanged(x, y, z);
> 
> I am surprised this works at all.
> 
> I'd rather you add a new interface.
> 
> 
> > class nsAcceleration : public nsIAcceleration
> > {
> >@@ -156,7 +158,11 @@ nsAccelerometer::TimeoutHandler(nsITimer
> >   
> >   // what about listeners that don't clean up properly?  they will leak
> >   if (self->mListeners.Count() == 0 && self->mWindowListeners.Count() == 0) {
> >-    self->Shutdown();
> >+    if (XRE_GetProcessType() == GeckoProcessType_Default)
> >+      self->Shutdown();
> >+    else
> >+      mozilla::dom::ContentChild::GetSingleton()->
> >+        SendRemoveAccelerometerListener();
> 
> 
> would it make sense to just put the |SendRemoveAccelerometerListener| in
> Shutdown()?
> 
> 
> >     mNewListener = PR_TRUE;
> >-    Startup();
> >+    
> >+    if (XRE_GetProcessType() == GeckoProcessType_Default)
> >+      Startup();
> >+    else
> >+      mozilla::dom::ContentChild::GetSingleton()->
> >+        SendAddAccelerometerListener();
> 
> 
> Same idea here -- move SendAddAccelerometerListener into Startup()

Startup() and Shutdown() are implemented in 7 different places. Wouldn't it make sense to centralize this? Or do you want this to only be remoted for android?

Comment 8

[Brad Lassey [:blassey] (use needinfo?)]

Attachment: patch

Comment 9

[Brad Lassey [:blassey] (use needinfo?)]

Attachment: patch

Comment 10

[Brad Lassey [:blassey] (use needinfo?)]

pushed http://hg.mozilla.org/mozilla-central/rev/22121066081d