In content processes, attach GeckoEditableSupport to PuppetWidget so that the content process communicates directly with Java IME code over AIDL, rather than going through the widget content cache.

Add a GetIMEUpdatePreference method to TextEventDispatcherListener to optionally control which IME notifications are received by NotifyIME. This patch also makes nsBaseWidget forward its GetIMEUpdatePreference call to the current TextEventDispatcher/TextEventDispatcherListener. This is useful when a TextEventDispatcherListener needs to receive different types of IME notifications than the platform's native widget.

In PuppetWidget, add getter and setter for the widget's native TextEventDispatcherListener. This allows overriding of PuppetWidget's default IME handling. For example, on Android, the PuppetWidget's native TextEventDispatcherListener will communicate directly with Java IME code in the main process.

Add AIDL definition and implementation for an interface for the main process that child processes can access.

Add a JNIEnv* parameter to XRE_SetAndroidChildFds, which is used to set the Gecko thread JNIEnv for child processes. XRE_SetAndroidChildFds is the only Android-specific entry point for child processes, so I think it's the most logical place to initialize JNI.

(In reply to Jim Chen [:jchen] [:darchons] from comment #3) > Created attachment 8841773 [details] [diff] [review] > 3. Add AIDL interface for main process (v1) > > Add AIDL definition and implementation for an interface for the main > process that child processes can access. Are you planning to do something with this ProcessManager AIDL in the future? I'm not sure I understand what this current patch is adding.

(In reply to Jim Chen [:jchen] [:darchons] from comment #4) > Created attachment 8841774 [details] [diff] [review] > 4. Set Gecko thread JNIEnv for child process (v1) > > Add a JNIEnv* parameter to XRE_SetAndroidChildFds, which is used to set > the Gecko thread JNIEnv for child processes. XRE_SetAndroidChildFds is > the only Android-specific entry point for child processes, so I think > it's the most logical place to initialize JNI. I wonder if rather than drilling more hole to pass the JNIEnv into XRE, is there a reason we aren't just calling mozilla::jni::SetGeckoThreadEnv in Java_org_mozilla_gecko_mozglue_GeckoLoader_nativeRun for both parent process and child process? Then there is a single call site for both process types and makes the code a little simpler.

(In reply to Randall Barker [:rbarker] from comment #5) > (In reply to Jim Chen [:jchen] [:darchons] from comment #3) > > Created attachment 8841773 [details] [diff] [review] > > 3. Add AIDL interface for main process (v1) > > > > Add AIDL definition and implementation for an interface for the main > > process that child processes can access. > > Are you planning to do something with this ProcessManager AIDL in the > future? I'm not sure I understand what this current patch is adding. Yeah I have a future patch that adds a method to it. Right now it's just a skeleton. (In reply to Randall Barker [:rbarker] from comment #6) > (In reply to Jim Chen [:jchen] [:darchons] from comment #4) > > Created attachment 8841774 [details] [diff] [review] > > 4. Set Gecko thread JNIEnv for child process (v1) > > > > Add a JNIEnv* parameter to XRE_SetAndroidChildFds, which is used to set > > the Gecko thread JNIEnv for child processes. XRE_SetAndroidChildFds is > > the only Android-specific entry point for child processes, so I think > > it's the most logical place to initialize JNI. > > I wonder if rather than drilling more hole to pass the JNIEnv into XRE, is > there a reason we aren't just calling mozilla::jni::SetGeckoThreadEnv in > Java_org_mozilla_gecko_mozglue_GeckoLoader_nativeRun for both parent process > and child process? Then there is a single call site for both process types > and makes the code a little simpler. I guess we would have to export mozilla::jni::SetGeckoThreadEnv somehow. We likely have to wrap it in another function since we tend to export functions as C-style functions, so I'm not sure if that's any simpler than this.

(In reply to Jim Chen [:jchen] [:darchons] from comment #7) > (In reply to Randall Barker [:rbarker] from comment #5) > > (In reply to Jim Chen [:jchen] [:darchons] from comment #3) > > I wonder if rather than drilling more hole to pass the JNIEnv into XRE, is > > there a reason we aren't just calling mozilla::jni::SetGeckoThreadEnv in > > Java_org_mozilla_gecko_mozglue_GeckoLoader_nativeRun for both parent process > > and child process? Then there is a single call site for both process types > > and makes the code a little simpler. > > I guess we would have to export mozilla::jni::SetGeckoThreadEnv somehow. We > likely have to wrap it in another function since we tend to export functions > as C-style functions, so I'm not sure if that's any simpler than this. Right, I remember now thanks.

Comment on attachment 8841771 [details] [diff] [review] 1. Add TextEventDispatcherListener::GetIMEUpdatePreference (v1) I'd like you to brush up this patch before giving r+. >+nsIMEUpdatePreference >+TextEventDispatcher::GetIMEUpdatePreference() Although, it's odd "event dispatcher" to know IME update preference, it's okay. >diff --git a/widget/TextEventDispatcherListener.h b/widget/TextEventDispatcherListener.h > #ifndef mozilla_textinputdispatcherlistener_h_ > #define mozilla_textinputdispatcherlistener_h_ > >+#include "nsIWidget.h" I don't like this. You should use forward declaration for nsIMEUpdatePreference. > /** >+ * Returns preference for which IME notification are received by NotifyIME(). >+ */ >+ NS_IMETHOD_(nsIMEUpdatePreference) GetIMEUpdatePreference() >+ { return nsIMEUpdatePreference(); } Then, you should move this implementation into TextEventDispatcher.cpp. >+nsIMEUpdatePreference >+nsBaseWidget::GetIMEUpdatePreference() >+{ >+ if (!mTextEventDispatcher) { >+ return nsIMEUpdatePreference(); >+ } >+ return mTextEventDispatcher->GetIMEUpdatePreference(); >+} I think that you should move all nsIWidget::GetIMEUpdatePreference() overrides to TextEventDispatcherListener::GetIMEUpdatePreference() because it's difficult to understand which is used by a call of nsIWidget::GetIMEUpdatePreference(). Additionally, we can make nsIWidget::GetIMEUpdatePreference() a non-virtual method because there is GetNativeTextEventDispatcherListener(). So, you should rename this method to nsIWidget::GetIMEUpdatePreference() (and move it to implementations of nsIWidget methods). >diff --git a/widget/nsBaseWidget.h b/widget/nsBaseWidget.h > bool ComputeShouldAccelerate(); > virtual bool WidgetTypeSupportsAcceleration() { return true; } >- virtual nsIMEUpdatePreference GetIMEUpdatePreference() override { return nsIMEUpdatePreference(); } >+ virtual nsIMEUpdatePreference GetIMEUpdatePreference() override; Then, you can just remove this.

Comment on attachment 8841772 [details] [diff] [review] 2. Allow setting a PuppetWidget's native TextEventDispatcherListener (v1) Hmm, I don't understand this patch because nobody (including the following patches) calls PuppetWidget::SetNativeTextEventDispatcherListener(). If it's an overriding method of nsIWidget, I can guess the usage in the future, though. BTW, I wonder, we could remove NotifyIMEInternal() because it has remained only for Android widget but it's now unnecessary because of your work. Cannot make PuppetWidget as a sub class of TextEventDispatcherListener or create "PuppetTextEventDispatcherListener" and call its NotifyIME() instead?

Support remote GeckoEditableChild instances that are created in the content processes and connect to the parent process GeckoEditableParent through binders. Support having multiple GeckoEditableChild instances in GeckoEditable by keeping track of which child is currently focused, and only allow calls to/from the focused child by using access tokens.

Add IProcessManager.getEditableParent, which a content process can call to get the GeckoEditableParent instance that corresponds to a given content process tab, from the main process.

Support creating and running GeckoEditableSupport attached to a PuppetWidget in content processes. Because we don't know PuppetWidget's lifetime as well as nsWindow's, when attached to PuppetWidget, we need to attach/detach our native object on focus/blur, respectively.

Listen to the "tab-child-created" notification and attach our content process GeckoEditableSupport to the new PuppetWidget.

(In reply to Masayuki Nakano [:masayuki] from comment #10) > Comment on attachment 8841772 [details] [diff] [review] > 2. Allow setting a PuppetWidget's native TextEventDispatcherListener (v1) > > Hmm, I don't understand this patch because nobody (including the following > patches) calls PuppetWidget::SetNativeTextEventDispatcherListener(). If it's > an overriding method of nsIWidget, I can guess the usage in the future, > though. I just posted part 8, which uses SetNativeTextEventDispatcherListener. We want to use the Android-specific TextEventDispatcherListener for PuppetWidget.

This patch changes nsBaseWidget::GetIMEUpdatePreference to call GetIMEUpdatePreference on the native TextEventDispatcherListener, so TextEventDispatcher is no longer involved. I kept nsIWidget::GetIMEUpdatePreference as virtual because PuppetWidget needs to override GetIMEUpdatePreference and does not have a native TextEventDispatcherListener implementation.

This patch implements GetIMEUpdatePreference for all TextEventDispatcherListener implementations, by moving previous implementations of nsIWidget::GetIMEUpdatePreference.

Comment on attachment 8842687 [details] [diff] [review] 1b. Implement GetIMEUpdatePreference for all TextEventDispatcherListener (v1) >+NS_IMETHODIMP_(nsIMEUpdatePreference) >+TextInputProcessor::GetIMEUpdatePreference() >+{ >+ // TextInputProcessor::NotifyIME does not require extra change notifications. >+ return nsIMEUpdatePreference(); >+} Ah, nice. This is better than current implementation.

(In reply to Jim Chen [:jchen] [:darchons] from comment #17) > I kept nsIWidget::GetIMEUpdatePreference as virtual because PuppetWidget > needs > to override GetIMEUpdatePreference and does not have a native > TextEventDispatcherListener implementation. Thank you. Although, if PuppetWidget always has GeckoEditableSupport, PuppetWidget could be able to inherit TextEventDispatcherListener only when it's not built on Android. However, it's also not so simple design for the other developers. Okay, let's keep it.

Comment on attachment 8842591 [details] [diff] [review] 6. Add method to get GeckoEditableParent instance (v1) Review of attachment 8842591 [details] [diff] [review]: ----------------------------------------------------------------- ::: widget/android/nsWindow.h @@ +314,5 @@ > // Call this function when the users activity is the direct cause of an > // event (like a keypress or mouse click). > void UserActivity(); > > + mozilla::java::GeckoEditable::Ref& GetEditableParent() { return mEditable; } Can we avoid returning a reference here?

Comment on attachment 8842593 [details] [diff] [review] 8. Connect GeckoEditableSupport on PuppetWidget creation (v1) Review of attachment 8842593 [details] [diff] [review]: ----------------------------------------------------------------- ::: widget/android/nsAppShell.cpp @@ +601,5 @@ > + // Associate the PuppetWidget of the newly-created TabChild with a > + // GeckoEditableChild instance. > + MOZ_ASSERT(!XRE_IsParentProcess()); > + > + dom::ContentChild* cc = dom::ContentChild::GetSingleton(); Const and could use a more descriptive name, same for tc.

(In reply to Eugen Sawin [:esawin] from comment #21) > Comment on attachment 8842591 [details] [diff] [review] > 6. Add method to get GeckoEditableParent instance (v1) > > Review of attachment 8842591 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: widget/android/nsWindow.h > @@ +314,5 @@ > > // Call this function when the users activity is the direct cause of an > > // event (like a keypress or mouse click). > > void UserActivity(); > > > > + mozilla::java::GeckoEditable::Ref& GetEditableParent() { return mEditable; } > > Can we avoid returning a reference here? I think we want a reference to avoid extra JNI calls (the alternative is to return java::GeckoEditable::LocalRef).

Bug 1343075 - 1a. Add TextEventDispatcherListener::GetIMEUpdatePreference; r=masayuki Add a GetIMEUpdatePreference method to TextEventDispatcherListener to optionally control which IME notifications are received by NotifyIME. This patch also makes nsBaseWidget forward its GetIMEUpdatePreference call to the widget's native TextEventDispatcherListener. Bug 1343075 - 1b. Implement GetIMEUpdatePreference for all TextEventDispatcherListener; r=masayuki This patch implements GetIMEUpdatePreference for all TextEventDispatcherListener implementations, by moving previous implementations of nsIWidget::GetIMEUpdatePreference. Bug 1343075 - 2. Allow setting a PuppetWidget's native TextEventDispatcherListener; r=masayuki In PuppetWidget, add getter and setter for the widget's native TextEventDispatcherListener. This allows overriding of PuppetWidget's default IME handling. For example, on Android, the PuppetWidget's native TextEventDispatcherListener will communicate directly with Java IME code in the main process. Bug 1343075 - 3. Add AIDL interface for main process; r=rbarker Add AIDL definition and implementation for an interface for the main process that child processes can access. Bug 1343075 - 4. Set Gecko thread JNIEnv for child process; r=snorp Add a JNIEnv* parameter to XRE_SetAndroidChildFds, which is used to set the Gecko thread JNIEnv for child processes. XRE_SetAndroidChildFds is the only Android-specific entry point for child processes, so I think it's the most logical place to initialize JNI. Bug 1343075 - 5. Support multiple remote GeckoEditableChild; r=esawin Support remote GeckoEditableChild instances that are created in the content processes and connect to the parent process GeckoEditableParent through binders. Support having multiple GeckoEditableChild instances in GeckoEditable by keeping track of which child is currently focused, and only allow calls to/from the focused child by using access tokens. Bug 1343075 - 6. Add method to get GeckoEditableParent instance; r=esawin Add IProcessManager.getEditableParent, which a content process can call to get the GeckoEditableParent instance that corresponds to a given content process tab, from the main process. Bug 1343075 - 7. Support GeckoEditableSupport in content processes; r=esawin Support creating and running GeckoEditableSupport attached to a PuppetWidget in content processes. Because we don't know PuppetWidget's lifetime as well as nsWindow's, when attached to PuppetWidget, we need to attach/detach our native object on focus/blur, respectively. Bug 1343075 - 8. Connect GeckoEditableSupport on PuppetWidget creation; r=esawin Listen to the "tab-child-created" notification and attach our content process GeckoEditableSupport to the new PuppetWidget. Bug 1343075 - 9. Update auto-generated bindings; r=me

Pushed by nchen@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/f3b22a31ea36 Use GeckoEditableSupport from PuppetWidget; r=masayuki r=rbarker r=snorp r=esawin