Closed Bug 582644 Opened 9 years ago Closed 9 years ago

Support IME Text Events in e10s fennec

Categories

(Firefox for Android Graveyard :: General, defect)

ARM
Android
defect
Not set

Tracking

(blocking2.0 -, fennec2.0a1+)

VERIFIED FIXED
Tracking Status
blocking2.0 --- -
fennec 2.0a1+ ---

People

(Reporter: jbos, Assigned: jchen)

References

Details

Attachments

(7 files, 12 obsolete files)

6.80 KB, patch
mfinkle
: review-
Details | Diff | Splinter Review
49.23 KB, patch
masayuki
: review-
Details | Diff | Splinter Review
23.49 KB, patch
dougt
: review+
Details | Diff | Splinter Review
8.06 KB, patch
jchen
: review+
Details | Diff | Splinter Review
12.91 KB, patch
jchen
: review+
Details | Diff | Splinter Review
766 bytes, patch
blassey
: review+
Details | Diff | Splinter Review
3.96 KB, patch
blassey
: review+
Details | Diff | Splinter Review
User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.1.9) Gecko/20100317 SUSE/3.5.9-0.1.1 Firefox/3.5.9
Build Identifier: 

As a user i want to use Input Method Extender Plugins to enter text. This is not working in e10s. 

Reproducible: Always
Attached patch Fennec PartSplinter Review
Fix for Fennec
Attachment #460899 - Flags: review?(mark.finkle)
Attached patch Xulrunner PartSplinter Review
ulrunner Part
Attachment #460900 - Flags: review?(doug.turner)
OS: Linux → All
Hardware: x86 → All
Attachment #460899 - Flags: review?(masayuki)
Attachment #460899 - Flags: review?(mark.finkle)
Attachment #460899 - Flags: review?(Olli.Pettay)
Attachment #460900 - Flags: review?(masayuki)
Attachment #460900 - Flags: review?(doug.turner)
Attachment #460900 - Flags: review?(Olli.Pettay)
Comment on attachment 460899 [details] [diff] [review]
Fennec Part

Someone who knows Fennec code should review this.
Attachment #460899 - Flags: review?(Olli.Pettay)
Comment on attachment 460899 [details] [diff] [review]
Fennec Part

In an effort to _not_ create too many Modules and CustomHandlxtModule code into KeyModule and the ContentCustomTextSender code into ContentCustomKeySender?

I'd like to reduce the code we have even further, but for now, let's just merge that code together.

Remove the debug dumps too.
Attachment #460899 - Flags: review?(masayuki) → review-
Comment on attachment 460900 [details] [diff] [review]
Xulrunner Part

First, I don't understand the goal of this bug. Do you really need the nsIPrivateText* to be scriptable? And sounds they will not be "private".

>+  void sendCrossProcessTextEvent(in AString aCompositionString,
>+                                 in long aFirstClauseLength,
>+                                 in unsigned long aFirstClauseAttr,
>+                                 in long aSecondClauseLength,
>+                                 in unsigned long aSecondClauseAttr,
>+                                 in long aThirdClauseLength,
>+                                 in unsigned long aThirdClauseAttr,
>+                                 in long aCaretStart,
>+                                 in long aCaretLength);
>+

This is bad. Why is this only 3 clauses? If you watched the nsIDOMWindowUtils' method, it's wrong. The method is only implemented for automated tests. If you want to implement this method, you must create a new interface which can hold all composition clauses. (Then, nsIDOMWindowUtils should use it!)

>diff --git a/dom/interfaces/events/nsIDOMTextEvent.idl b/dom/interfaces/events/nsIDOMTextEvent.idl
>new file mode 100644
>--- /dev/null
>+++ b/dom/interfaces/events/nsIDOMTextEvent.idl

>+interface nsIDOMTextEvent : nsIDOMUIEvent
>+{
>+  readonly attribute DOMString data;
>+  void initTextEvent(in DOMString typeArg,
>+          in boolean canBubbleArg,
>+          in boolean cancelableArg,
>+          in nsIDOMAbstractView viewArg,
>+          in DOMString dataArg);
>+};

Hmm, looks bad... see: http://www.w3.org/TR/DOM-Level-3-Events/#events-textevents

>+bool TabChild::RecvsendTextEvent(
>+        const nsString& aCompositionString,
>+        const PRInt32& aFirstClauseLength,
>+        const PRUint32& aFirstClauseAttr,
>+        const PRInt32& aSecondClauseLength,
>+        const PRUint32& aSecondClauseAttr,
>+        const PRInt32& aThirdClauseLength,
>+        const PRUint32& aThirdClauseAttr,
>+        const PRInt32& aCaretStart,
>+        const PRInt32& aCaretLength)
>+{
>+  nsCOMPtr<nsPIDOMWindow> window = do_GetInterface(mWebNav);
>+  nsCOMPtr<nsIDOMWindowUtils> utils = do_GetInterface(window);
>+  NS_ENSURE_TRUE(utils, true);
>+  PRBool ignored = PR_FALSE;
>+  utils->SendTextEvent(
>+          aCompositionString,
>+          aFirstClauseLength,
>+          aFirstClauseAttr,
>+          aSecondClauseLength,
>+          aSecondClauseAttr,
>+          aThirdClauseLength,
>+          aThirdClauseAttr,
>+          aCaretStart,
>+          aCaretLength);
>+  return true;

Don't use nsIDOMWindowUtils::SendTextEvent() except automated tests.
Attachment #460900 - Flags: review?(masayuki) → review-
(In reply to comment #5)
> Don't use nsIDOMWindowUtils::SendTextEvent() except automated tests.
Why?
Where is it said that the method is only for automated tests?
(In reply to comment #7)
> (In reply to comment #5)
> > Don't use nsIDOMWindowUtils::SendTextEvent() except automated tests.
> Why?
> Where is it said that the method is only for automated tests?

Hmm, I didn't document that in nsIDOMWindowUtils... The 3 clauses are not enough for Japanese IME daily use. So, the method isn't usable for real event handling.
Comment on attachment 460900 [details] [diff] [review]
Xulrunner Part

Clearing the review request, because masayuki gave already r-
Attachment #460900 - Flags: review?(Olli.Pettay)
Status: UNCONFIRMED → NEW
Ever confirmed: true
Masayuki, can we get a quick outline of what you would be looking for in a patch for this? 

As to your initial question the goal isn't to make these events (nsCompositionEvent, nsQueryContentEvent and nsSelectionEvent) scriptable but to be able to pass them between the child and parent processes.
Basically we have to intercept the events in the chrome process, pass them to the focused content process, and pass the results back to the chrome process. 

We can intercept the events inside their event handlers or in the event dispatch code.

Shouldn't really be difficult to pass them to the other process, but we do have different types of events and we have to write wrappers for each type.

Once on the other side we can pass the event off to the puppet widget or directly to event dispatch code.
(In reply to comment #10)
> As to your initial question the goal isn't to make these events
> (nsCompositionEvent, nsQueryContentEvent and nsSelectionEvent) scriptable but
> to be able to pass them between the child and parent processes.

So, if we can pass the events in nsGUIEvents.h, we don't need to make scriptable them. I wanted the fact about this. If we need to create a scriptable text event, it should be accessible to all clauses and all styles of each clauses. So, it wiil need a lot of methods or a method which returns a native pointer of nsTextEvent.

(In reply to comment #11)
> Shouldn't really be difficult to pass them to the other process, but we do have
> different types of events and we have to write wrappers for each type.

Sounds it doesn't make sense for me. Cannot create only on wrapper interface for all event structures and it just provides a method which returns native pointer of the ns*Event?

So, I don't understand that why the interfaces should be scriptable on the patch.

> Once on the other side we can pass the event off to the puppet widget or
> directly to event dispatch code.

Sorry, I don't understand this paragraph. What does this mean?
(In reply to comment #12)
> (In reply to comment #10)
> 
> So, if we can pass the events in nsGUIEvents.h, we don't need to make
> scriptable them. I wanted the fact about this. If we need to create a
> scriptable text event, it should be accessible to all clauses and all styles of
> each clauses. So, it wiil need a lot of methods or a method which returns a
> native pointer of nsTextEvent.
> 
Correct. We don't have to make them scriptable.

> (In reply to comment #11)
> 
> Sounds it doesn't make sense for me. Cannot create only on wrapper interface
> for all event structures and it just provides a method which returns native
> pointer of the ns*Event?
> 
> So, I don't understand that why the interfaces should be scriptable on the
> patch.
> 
The patch was one idea of how to fix this. Now we are talking about another way.

And yes, we can create one generic wrapper, but we still need code to handle things like the nsTextRange array inside nsTextEvent.

> > Once on the other side we can pass the event off to the puppet widget or
> > directly to event dispatch code.
> 
> Sorry, I don't understand this paragraph. What does this mean?
>
Sorry, what I meant was our current event dispatch code can stay the same, as long as we find out how to pass the events between chrome and content.
(In reply to comment #13)
> (In reply to comment #12)
> > (In reply to comment #10)
> > (In reply to comment #11)
> > Sounds it doesn't make sense for me. Cannot create only on wrapper interface
> > for all event structures and it just provides a method which returns native
> > pointer of the ns*Event?
> > 
> > So, I don't understand that why the interfaces should be scriptable on the
> > patch.
> > 
> The patch was one idea of how to fix this. Now we are talking about another
> way.

Okay. I see.

> And yes, we can create one generic wrapper, but we still need code to handle
> things like the nsTextRange array inside nsTextEvent.

Where is needed? If the place is C++ code, you can use nsIPrivateDOMEvent::GetInternalNSEvent(). Then, you can access nsTextEvent, nsTextRange array and its style.

> > > Once on the other side we can pass the event off to the puppet widget or
> > > directly to event dispatch code.
> > 
> > Sorry, I don't understand this paragraph. What does this mean?
> >
> Sorry, what I meant was our current event dispatch code can stay the same, as
> long as we find out how to pass the events between chrome and content.

Sure.
flagging for fennec 2.0 to get on triage plate
blocking2.0: --- → ?
tracking-fennec: --- → ?
Attached patch IME event remoting (WIP) v0.1 (obsolete) — Splinter Review
WIP: largely boilerplate code
tracking-fennec: ? → 2.0a1+
Attached patch IME event remoting (WIP) v0.2 (obsolete) — Splinter Review
WIP: Still trying to figure out how to get the currently focused PBrowserParent in C++. Last resort would be requesting it from JavaScript somehow.
Attachment #464139 - Attachment is obsolete: true
Assignee: nobody → jchen
smaug: Right now the problem is we have to forward IME events from chrome process to the active content process. The IME events are not scriptable, and I'm having a hard time finding a way in C++ to get the PBrowserParent corresponding to the active content process. I don't have a way of getting the remote document/window/FrameLoader, unless through JS. Do you have any suggestions?
CC'ing Chris since he might be able to answer comment 18
Sorry, I'm afraid I don't know how to do this either :(.  You'll want to get the PBrowserParent from the "currently active" nsFrameLoader.  I know of several routes to the nsFrameLoader from various DOM things (thanks bz!), but I don't know how to get any of the "currently active" ones of those things.
does nsIWindowWatcher::GetActiveWindow() send you down the right path? Specifically I'm thinking:

    nsresult rv;
    nsCOMPtr<nsIWindowWatcher> wwatch =
        (do_GetService(NS_WINDOWWATCHER_CONTRACTID, &rv));
    if (wwatch) {
        nsCOMPtr<nsIDOMWindow> window;
        wwatch->GetActiveWindow(getter_AddRefs(window));
        if (window) {
            nsCOMPtr<nsIDOMDocument> domDocument;
            window->GetDocument(getter_AddRefs(domDocument));
            if (domDocument) {
                nsCOMPtr<nsIDOMElement> element;
                domDocument->GetDocumentElement(getter_AddRefs(element));
                if (element) {
                    nsCOMPtr<nsIFrameLoaderOwner> loaderOwner = do_QueryInterface(element);

                    __android_log_print(ANDROID_LOG_INFO, "GeckoWindow" ,
                                        "frame loader owner: %p", loaderOwner.get());
                }
            }
        }
    }

That gives me a valid frame loader, question being if its the "right" one.
See also bug 583976, which is when the content process doesn't have a real widget any more.
Changes to events along with e10s serialization support.
Attachment #464633 - Attachment is obsolete: true
Changes to content code. nsQueryContentEvent and nsSelectionEvent are forwarded correctly, but nsCompositionEvent and nsTextEvent are not.
Changes to Android code.
Correct patch. Sorry for the spam
Attachment #465117 - Attachment is obsolete: true
Events are correctly forwarded now. Has some problems with focus management.
Attachment #465116 - Attachment is obsolete: true
Attachment #465118 - Attachment is obsolete: true
Attachment #465115 - Attachment is obsolete: true
Attachment #465433 - Flags: review?(doug.turner)
Attachment #465134 - Attachment is obsolete: true
Attachment #465135 - Attachment is obsolete: true
Attachment #465457 - Flags: review?(blassey.bugs)
Attachment #465456 - Flags: review?(Olli.Pettay)
Comment on attachment 465457 [details] [diff] [review]
(3/3) IME event remoting (android) v0.5

>+#ifdef ANDROID
>+  if (!event.mSucceeded) {
>+    AndroidBridge::Bridge()->ReturnIMEQueryResult(nsnull, 0, 0, 0);
>     return true;
>+  }
>+
>+  switch (event.message) {
>+  case NS_QUERY_TEXT_CONTENT:
>+    AndroidBridge::Bridge()->ReturnIMEQueryResult(
>+        event.mReply.mString.get(), event.mReply.mString.Length(), 0, 0);
>+    return true;
>+  case NS_QUERY_SELECTED_TEXT:
>+    AndroidBridge::Bridge()->ReturnIMEQueryResult(
>+        event.mReply.mString.get(),
>+        event.mReply.mString.Length(),
>+        event.GetSelectionStart(),
>+        event.GetSelectionEnd() - event.GetSelectionStart());
>+    return true;
>+  }
>+#endif
>+  return false;
> }

My understanding is you're not supposed to return false from a stub function, which this essentially is for all platforms other than android.



>+#ifdef MOZ_IPC
>+    // It's possible that we are in chrome process
>+    //  but sBridge is not initialized yet
>+    else if (XRE_GetProcessType() == GeckoProcessType_Content)
>         mozilla::dom::ContentChild::GetSingleton()->SendNotifyIME(aType, aState);
>+#endif

you can use XRE_GetProcessType() to find out for sure
Attachment #465457 - Flags: review?(blassey.bugs) → review+
(In reply to comment #32)
> Comment on attachment 465457 [details] [diff] [review]
> (3/3) IME event remoting (android) v0.5
> 
> >+#ifdef ANDROID
> >+  if (!event.mSucceeded) {
> >+    AndroidBridge::Bridge()->ReturnIMEQueryResult(nsnull, 0, 0, 0);
> >     return true;
> >+  }
> >+
> >+  switch (event.message) {
> >+  case NS_QUERY_TEXT_CONTENT:
> >+    AndroidBridge::Bridge()->ReturnIMEQueryResult(
> >+        event.mReply.mString.get(), event.mReply.mString.Length(), 0, 0);
> >+    return true;
> >+  case NS_QUERY_SELECTED_TEXT:
> >+    AndroidBridge::Bridge()->ReturnIMEQueryResult(
> >+        event.mReply.mString.get(),
> >+        event.mReply.mString.Length(),
> >+        event.GetSelectionStart(),
> >+        event.GetSelectionEnd() - event.GetSelectionStart());
> >+    return true;
> >+  }
> >+#endif
> >+  return false;
> > }
> 
> My understanding is you're not supposed to return false from a stub function,
> which this essentially is for all platforms other than android.
> 

It's OK if this case is the equivalent of NS_NOTREACHED() (which I hope it is).
Attachment #465456 - Flags: review?(masayuki)
Comment on attachment 465456 [details] [diff] [review]
(2/3) IME event remoting (content) v0.5

If this works, great, but I'm not at all sure it works on all
platforms.
This makes nsQueryContentEvent handling asynchronous, and we need to make
sure it is ok everywhere.
If that isn't ok, we need other approach where content process sends
nsQueryContentEvent related data to chrome process whenever it has some.

So r- until either it is ensured that this approach works on all
platforms, or other approach is implemented.
(I really wish this approach works everywhere. Rather simple and clean way
to handle IME.)
Attachment #465456 - Flags: review?(Olli.Pettay) → review-
My understanding is that the chrome process can never block on the the content process. So is pretty much the way it needs to be and we need to figure out how to make this approach work on other platforms.

Chris, maybe you can chime in here.
(In reply to comment #35)
> So is pretty much the way it needs to be and we need to figure out how
> to make this approach work on other platforms.
The other approach is that content process sends relevant data even before
chrome process needs the data.
We need to figure out which one is possible or easier to implement.
FWIW I don't think we should r- this patch if it's android only, and targeted at a1 only.  If fennec is going to be unusable on a lot of android devices without it (is that true?), then I think we should take the patch as a stopgap, with plans to revisit the design after a1 (like the shmem canvas patches).  We're not going to block b1 on IME for other platforms, BTW.

But yeah, for 2.0 final, we'll need to do whatever is necessary to make IME work without blocking chrome on content.  I don't understand all the problems yet but I'd be glad to help down the road.
Right, a large portion of keyboards won't work without this, and this is the best/quickest way I've found to get it working for a1. I can add #ifdef ANDROID if needed.

Asynchronous querying is not going to work for other platforms IF remote content is enabled. For non-remote content the code falls back to synchronous query, with no change from current behavior. I didn't see any regressions from the try server results.

And I agree too that we need to reevaluate event handling after a1. I see better ways to do this once puppet widgets and layers land.
(In reply to comment #37)
> FWIW I don't think we should r- this patch if it's android only, and targeted
> at a1 only. 
Well, at least the code needs to be then ifdef ANDROIDed.
I could accept that as a short term approach.
Currently the code would run (although perhaps not really do anything, I hope)
even in desktop Firefox.
Comment on attachment 465433 [details] [diff] [review]
(1/3) IME event remoting (widget) v0.4

> 
>+    QueryContentResult(nsQueryContentEvent event);
>+


Is this really needed?

> 
>+    CompositionEvent(nsCompositionEvent event);
>+
>+    TextEvent(nsTextEvent event);
>+
>+    QueryContentEvent(nsQueryContentEvent event);
>+
>+    SelectionEvent(nsSelectionEvent event);
>+

These are only going to be used on Android for now -- can these files be #ifdef preprocessed?

In all of your Recv* methods, please do not return the value of the DispatchWidgetEvent.  Just return true.


>+  if (event.rangeArray)
>+    delete [] event.rangeArray; // delete copy of text range array
>+  return succeeded;

I really hate this.  Anything cleaner?  Maybe a ParamTraits function to hide this...


> mozilla::ipc::PDocumentRendererChild*
> TabChild::AllocPDocumentRenderer(
>         const PRInt32& x,
>         const PRInt32& y,
>         const PRInt32& w,
>         const PRInt32& h,
>         const nsString& bgcolor,
>         const PRUint32& flags,

while you are here, please re-indent.

> bool
>+TabParent::RecvQueryContentResult(const nsQueryContentEvent& event)
>+{
>+    return true;
>+}

like I mentioned above, maybe we can drop this?


>+#ifdef MOZ_IPC
>+  nsEvent()
>+  {
>+  }
>+#endif // MOZ_IPC

why?  and why not public?


>+ * Contributor(s):
>+ *

Add your name to the blame.


Getting close!

also need to check to see if there is a better way of going from a nsPIDOMWindow to the best nsIWidget.
Attachment #465433 - Flags: review?(doug.turner) → review-
(In reply to comment #34)
> Comment on attachment 465456 [details] [diff] [review]
> (2/3) IME event remoting (content) v0.5
> 
> If this works, great, but I'm not at all sure it works on all
> platforms.
> This makes nsQueryContentEvent handling asynchronous, and we need to make
> sure it is ok everywhere.
> If that isn't ok, we need other approach where content process sends
> nsQueryContentEvent related data to chrome process whenever it has some.
> 
> So r- until either it is ensured that this approach works on all
> platforms, or other approach is implemented.
> (I really wish this approach works everywhere. Rather simple and clean way
> to handle IME.)

The all IME events, query content events and key events must be synchronous. Non-Linux platform's native IME evnets need result. If we process them, asynchronous, we cannot return the correct result to the system.
(In reply to comment #40)
> Comment on attachment 465433 [details] [diff] [review]
> (1/3) IME event remoting (widget) v0.4
> 
> >+    QueryContentResult(nsQueryContentEvent event);
> >+
> 
> Is this really needed?
> 
Yes, chrome calls QueryContentEvent(...) to query something from content, but since QueryContentEvent can't be synchronous, content has to return the query result to chrome some other way. RecvQueryContentResult is actually implemented in the android patch.

> These are only going to be used on Android for now -- can these files be #ifdef
> preprocessed?
> 
I don't think ipdl should be preprocessed. See https://bugzilla.mozilla.org/show_bug.cgi?id=581535#c8

> In all of your Recv* methods, please do not return the value of the
> DispatchWidgetEvent.  Just return true.
> 
OK

> >+  if (event.rangeArray)
> >+    delete [] event.rangeArray; // delete copy of text range array
> >+  return succeeded;
> 
> I really hate this.  Anything cleaner?  Maybe a ParamTraits function to hide
> this...
> 
OK. Added allocation code to nsTextEvent itself.

> >+#ifdef MOZ_IPC
> >+  nsEvent()
> >+  {
> >+  }
> >+#endif // MOZ_IPC
> 
> why?  and why not public?
> 
Non-IPC code should use the other constructor with arguments. This constructor is for ipdlc-generated code which requires a default constructor.

> also need to check to see if there is a better way of going from a
> nsPIDOMWindow to the best nsIWidget.
>
The code is adapted from nsDOMWindowUtils::GetWidget. I could also just add the method to nsDOMWindowUtils.

I tried going directly to nsIBaseWindow and using nsIBaseWindow::GetMainWidget but it gave me a wrong widget.
Attachment #465433 - Attachment is obsolete: true
Attachment #465971 - Flags: review?(doug.turner)
smaug, masayuki: The code will fall back to current behavior on current platforms other than Android. In any case I added #ifdef ANDROID's to not include useless code for other platforms.
Attachment #465456 - Attachment is obsolete: true
Attachment #465973 - Flags: review?(Olli.Pettay)
Attachment #465456 - Flags: review?(masayuki)
Addressing blassey's comments (returning true from RecvQueryContentResult)
Attachment #465457 - Attachment is obsolete: true
Attachment #465974 - Flags: review+
Comment on attachment 465973 [details] [diff] [review]
(2/3) IME event remoting (content) v0.6

>+  static nsresult GetActiveFrameLoader(nsFrameLoader **aFrameLoader);
Make this return already_AddRefed<nsFrameLoader>

But ok.
This is ugly, temporary solution for Android only.
Attachment #465973 - Flags: review?(Olli.Pettay) → review+
Attachment #465971 - Flags: review?(doug.turner) → review+
Attachment #465973 - Attachment is obsolete: true
Attachment #466431 - Flags: review+
Backed out because of mochitest-3 orange:

http://hg.mozilla.org/mozilla-central/rev/7f27cd7d8e59
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Fixes mochitest orange
Attachment #466504 - Flags: review?(doug.turner)
Instead of using nsTextEvent destructor, reverts to using separate function to free nsTextRange array.
Attachment #466554 - Flags: review?(doug.turner)
Attachment #466504 - Flags: review?(doug.turner) → review+
Attachment #466554 - Flags: review?(doug.turner) → review+
Blassey tells me this is a android fix only.   Should revisit this when nokia wants to implement for meego
OS: All → Android
Hardware: All → ARM
Verified against japanese, chinese, and korean input apps.   Mozilla/5.0 (Android; Linux armv7l; rv:2.0b5pre) Gecko/20100826 Namoroka/4.0b5pre Fennec/2.0a1pre
Status: RESOLVED → VERIFIED
blocking2.0: ? → -
You need to log in before you can comment on or make changes to this bug.