Closed Bug 1343075 Opened 3 years ago Closed 3 years ago

Use GeckoEditableSupport in content processes

Categories

(Firefox for Android :: Keyboards and IME, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 55
Tracking Status
firefox55 --- fixed

People

(Reporter: jchen, Assigned: jchen)

References

Details

(Keywords: inputmethod)

Attachments

(11 files, 1 obsolete file)

4.39 KB, patch
masayuki
: review+
Details | Diff | Splinter Review
7.82 KB, patch
rbarker
: review+
Details | Diff | Splinter Review
8.77 KB, patch
rbarker
: review+
snorp
: review+
Details | Diff | Splinter Review
40.36 KB, patch
esawin
: review+
Details | Diff | Splinter Review
8.94 KB, patch
esawin
: review+
Details | Diff | Splinter Review
14.79 KB, patch
esawin
: review+
Details | Diff | Splinter Review
6.33 KB, patch
esawin
: review+
Details | Diff | Splinter Review
6.41 KB, patch
jchen
: review+
Details | Diff | Splinter Review
4.32 KB, patch
masayuki
: review+
Details | Diff | Splinter Review
19.12 KB, patch
masayuki
: review+
Details | Diff | Splinter Review
119.34 KB, patch
jchen
: review+
Details | Diff | Splinter Review
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.
Attachment #8841771 - Flags: review?(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.
Attachment #8841772 - Flags: review?(masayuki)
Add AIDL definition and implementation for an interface for the main
process that child processes can access.
Attachment #8841773 - Flags: review?(rbarker)
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.
Attachment #8841774 - Flags: review?(snorp)
Attachment #8841774 - Flags: review?(rbarker)
(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.
Flags: needinfo?(nchen)
(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.
Flags: needinfo?(nchen)
Attachment #8841773 - Flags: review?(rbarker) → review+
Attachment #8841774 - Flags: review?(rbarker) → review+
(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.
Attachment #8841774 - Flags: review?(snorp) → review+
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.
Attachment #8841771 - Flags: review?(masayuki) → review-
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?
Flags: needinfo?(nchen)
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.
Attachment #8842591 - Flags: review?(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.
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.
Flags: needinfo?(nchen)
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.
Attachment #8842686 - Flags: review?(masayuki)
Attachment #8841771 - Attachment is obsolete: true
This patch implements GetIMEUpdatePreference for all
TextEventDispatcherListener implementations, by moving previous
implementations of nsIWidget::GetIMEUpdatePreference.
Attachment #8842687 - Flags: review?(masayuki)
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.
Attachment #8842687 - Flags: review?(masayuki) → review+
Attachment #8842686 - Flags: review?(masayuki) → review+
(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.
Attachment #8841772 - Flags: review?(masayuki) → review+
Attachment #8842590 - Flags: review?(esawin)
Attachment #8842592 - Flags: review?(esawin)
Attachment #8842593 - Flags: review?(esawin)
Attachment #8842590 - Flags: review?(esawin) → review+
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?
Attachment #8842591 - Flags: review?(esawin) → review+
Attachment #8842592 - Flags: review?(esawin) → review+
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.
Attachment #8842593 - Flags: review?(esawin) → review+
(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
Attachment #8844735 - Flags: review+
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
https://hg.mozilla.org/mozilla-central/rev/f3b22a31ea36
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 55
You need to log in before you can comment on or make changes to this bug.