Closed Bug 581535 Opened 10 years ago Closed 10 years ago

Remote android bridge

Categories

(Core :: Widget: Android, defect)

x86_64
Linux
defect
Not set

Tracking

()

RESOLVED FIXED
Tracking Status
fennec 2.0a1+ ---

People

(Reporter: blassey, Assigned: blassey)

References

Details

Attachments

(1 file, 4 obsolete files)

Attached patch WIP (very rough) (obsolete) — Splinter Review
Some of the functions provided by the android bridge are needed in the content process
Blocks: 580269
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)
Does this IPC interface need to be Android specific or is it more of a general Widget interface?
(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.
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
(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 ;).
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.
Attached patch patch to remote NotifyIME (obsolete) — Splinter Review
this, along with the patch on bug 576065 makes the keyboard work for content on android e10s builds
Assignee: nobody → blassey.bugs
Attachment #459900 - Attachment is obsolete: true
Attachment #461484 - Flags: review?(doug.turner)
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.
Attachment #461484 - Attachment is obsolete: true
Attachment #461655 - Flags: review?(jones.chris.g)
Attachment #461484 - Flags: review?(doug.turner)
Attachment #461655 - Flags: review?(jchen)
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.
Attachment #461655 - Flags: review?(jones.chris.g) → review+
Attached patch patch v2 (obsolete) — Splinter Review
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.
Attachment #461655 - Attachment is obsolete: true
Attachment #462640 - Flags: review?(mwu)
Attachment #461655 - Flags: review?(jchen)
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.
Attachment #462640 - Flags: review?(mwu) → review+
Attachment #462640 - Attachment is obsolete: true
Attachment #463280 - Flags: review+
tracking-fennec: --- → 2.0a1+
http://hg.mozilla.org/mozilla-central/rev/2d93b0b69925
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Duplicate of this bug: 580269
Keywords: checkin-needed
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
(In reply to comment #16)
 
> the SW keyboard doesn't pop up when trying to enter user or password

Sounds like bug 591047
Duplicate of this bug: 580269
You need to log in before you can comment on or make changes to this bug.