581535 - Remote android bridge

Status: RESOLVED FIXED

(Core Graveyard :: Widget: Android, defect)

Description

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

Attachment: WIP (very rough)

Some of the functions provided by the android bridge are needed in the content process

Comment 1

[(inactive) Jim Chen [:jchen] [:darchons]]

Thanks for your work Brad!

The following need to be forwarded for IME to work,
AndroidGeckoEvent from chrome to the content that has focus.
AndroidBridge::ReturnIMEQueryResult from content to chrome.
AndroidBridge::NotifyIME from content to chrome

NotifyIME replaces ShowIME in the rewrite (Bug 576065)

Comment 2

[Mark Finkle (:mfinkle) (use needinfo?)]

Does this IPC interface need to be Android specific or is it more of a general Widget interface?

Comment 3

[(inactive) Jim Chen [:jchen] [:darchons]]

(In reply to comment #2)
> Does this IPC interface need to be Android specific or is it more of a general
> Widget interface?

It's Android-specific and a temporary solution for alpha1. After a1 this will be backed out and IME will be handled by fake/puppet widgets.

Comment 4

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

So as blassey found out, IPDL doesn't support conditional compilation, and I don't want it to.  This code can still be android-only extreme pain: just stub out the message handlers for non-ANDROID builds.  We do this already in plugin code [1].  It's not gorgeous, but you could just make a stub header for non-ANDROID.

[1] http://hg.mozilla.org/mozilla-central/file/1b14b3b3095d/dom/plugins/PluginInstanceChild.cpp#l669

Comment 5

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

(In reply to comment #4)
> This code can still be android-only extreme pain: just stub

*without* extreme pain, sorry.  But maybe that's debatable ;).

Comment 6

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Also, are these process-level interfaces?  Like, do you have one context for making these calls per chrome process?  If so, I'd be fine with these messages being stuck on PContent.  I'm not sure how we should handle these in the case of multiple content processes, but we can figure that out later.

Comment 7

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

Attachment: patch to remote NotifyIME

this, along with the patch on bug 576065 makes the keyboard work for content on android e10s builds

Comment 8

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Drive-by comment: I realize that "doesn't support conditional compilation, and I don't want it to" may have been ambiguous: to be clear, I meant "I'm against using cpp".  IPDL exists to (hopefully) make reasoning about multiple thread contexts easier; it's hard.  Allowing ~cpp (in general) adds cognitive overhead that makes reasoning about concurrency combinatorially worse.  So I'd really really prefer to keep IPDL specs like IDL specs, where (save for |%{C++|) the spec is the superset of platform capabilities, and where we don't yet support a message/method, we leave an NYI stub.  I'm not trying to pick on this patch, I'm just way over-generalizing and being very pessimistic.  (Also FWIW, comment 4 referred to an interface for painting plugins into shmem, which we do eventually want to support on other platforms, we just don't yet.)

More specific to this implemention: |NotifyIME()| corresponds to some process-scoped interface with the same life cycle as PContent, right?  That is, they don't exist independently, say by some IME context that we can have multiple instances of?  If so, then I'd recommend just
  protocol PContent {
...
    NotifyIME(...);
...
  };
It's fine that that's totally android-specific.  We can generify after 2.0a1 in bug 582057, maybe.  For non-android, IPDL already generates stub impls you can use; look at the bottom of $objdir/ipc/ipdl/_ipdlheaders/mozilla/dom/PContentParent.h and *Child.h.  If you want to keep the C++ impl of the android stuff separate from the other Content*.cpp code (which sounds good), you could make Content*_android.cpp or something that implement the pure-virtual Recv* and *alloc* methods.

So I guess mainly, I'd rather not bring in ~cpp and all its associated issues for one or a handful of messages that we already know we'll (most likely) want to make more generic.  I think in this case, C++ stubs would be less new code and goop to boot.

Comment 9

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

Attachment: patch following cjones's suggestions

Comment 10

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Comment on attachment 461655 [details] [diff] [review]
patch following cjones's suggestions

Thanks, looks good.

>diff --git a/dom/ipc/PContent.ipdl b/dom/ipc/PContent.ipdl
>--- a/dom/ipc/PContent.ipdl
>+++ b/dom/ipc/PContent.ipdl
>@@ -93,6 +93,7 @@ parent:
> 
>     // PermissionsManager messages
>     sync TestPermission(URI uri, nsCString type, PRBool exact) returns (PRUint32 retValue);
>+    NotifyIME(int aType, int aArg1, int aArg2, int aArg3);

Comment plz?

>diff --git a/widget/src/android/AndroidBridge.cpp b/widget/src/android/AndroidBridge.cpp

Didn't look at anything from here on down.

Comment 11

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

Attachment: patch v2

updated this patch to the latest patches on bug 576065. Last patch got cjones's review for the ipc parts, requesting mwu's for the android widget parts now.

Comment 12

[Michael Wu [:mwu]]

Comment on attachment 462640 [details] [diff] [review]
patch v2

>@@ -186,22 +187,30 @@ AndroidBridge::EnsureJNIThread()
> void
> AndroidBridge::NotifyIME(int aType, int aState)
> {
>-    mJNIEnv->CallStaticVoidMethod(mGeckoAppShellClass,
>-                                  jNotifyIME, aType, aState);
>+    if(sBridge)
Space after the if

>+        JNI()->CallStaticVoidMethod(sBridge->mGeckoAppShellClass, 
>+                                    sBridge->jNotifyIME,  aType, aState);
>+    else
>+        mozilla::dom::ContentChild::GetSingleton()->SendNotifyIME(aType, aState);
> }
> 
> void
> AndroidBridge::NotifyIMEChange(const PRUnichar *aText, PRUint32 aTextLen,
>                                int aStart, int aEnd, int aNewEnd)
> {
>-    jvalue args[4];
>-    AutoLocalJNIFrame jniFrame(1);
>-    args[0].l = mJNIEnv->NewString(aText, aTextLen);
>-    args[1].i = aStart;
>-    args[2].i = aEnd;
>-    args[3].i = aNewEnd;
>-    mJNIEnv->CallStaticVoidMethodA(mGeckoAppShellClass,
>-                                   jNotifyIMEChange, args);
>+    if(sBridge) {
>+        jvalue args[4];
>+        AutoLocalJNIFrame jniFrame(1);
>+        args[0].l = JNI()->NewString(aText, aTextLen);
>+        args[1].i = aStart;
>+        args[2].i = aEnd;
>+        args[3].i = aNewEnd;
>+        JNI()->CallStaticVoidMethodA(sBridge->mGeckoAppShellClass,
>+                                     sBridge->jNotifyIMEChange, args);
>+    } else {
>+        mozilla::dom::ContentChild::GetSingleton()->
>+            SendNotifyIMEChange(nsAutoString(aText), aTextLen, aStart, aEnd, aNewEnd);
>+    }
How about something like this at the top of the function?
if (!sBridge) {
    mozilla::dom::ContentChild::GetSingleton()->
        SendNotifyIMEChange(nsAutoString(aText), aTextLen, aStart, aEnd, aNewEnd);
    return;
}

>@@ -1552,8 +1546,8 @@ nsWindow::OnIMEFocusChange(PRBool aFocus
>     ALOGIME("IME: OnIMEFocusChange: f=%d", aFocus);
>     
>     if (AndroidBridge::Bridge())
>-        AndroidBridge::Bridge()->NotifyIME(
>-            AndroidBridge::NOTIFY_IME_FOCUSCHANGE, int(aFocus));
>+        AndroidBridge::NotifyIME(AndroidBridge::NOTIFY_IME_FOCUSCHANGE, 
>+                                 int(aFocus));
No need to check for the bridge.

Comment 13

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

Attachment: patch for landing

Comment 14

[Matt Brubeck (:mbrubeck)]

http://hg.mozilla.org/mozilla-central/rev/2d93b0b69925

Comment 16

[Ioana Chiorean]

the bad behavior is reproducible on Android:

Build Identifier: Mozilla/5.0 (Android; Linux armv7l; en-US; rv:2.0b7pre)
Gecko/20090919 Firefox/4.0b7pre Fennec/2.0b1pre 

Device Motorola Droid 2.2 

http://www-archive.mozilla.org/quality/browser/front-end/testcases/wallet/login.html
www.bugzilla.mozilla.org

the SW keyboard doesn't pop up when trying to enter user or password

Comment 17

[Mark Finkle (:mfinkle) (use needinfo?)]

(In reply to comment #16)
 
> the SW keyboard doesn't pop up when trying to enter user or password

Sounds like bug 591047