TextComposition needs to get identifier of native IME context for managing composition per it

RESOLVED FIXED in mozilla19

Status

()

Core
Widget
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: masayuki, Assigned: masayuki)

Tracking

({inputmethod})

Trunk
mozilla19
inputmethod
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite ?

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(7 attachments)

TextComposition should manage composition per IME native context instead of per widget.

Windows, Mac and GTK have native IME context we can use the pointer.

Qt and Android have only one IME context per our process.

I'm not sure about b2g, but it should be the latter case.

I'm planning to add a void* member to InputContext. GetInputContext() implementation should set it if the platform supports multiple IME context. Otherwise, set it nullptr.
Created attachment 672607 [details] [diff] [review]
part.1 Add mNativeIMEContext to InputContext and TextComposition should use it instead of nsIWidget

One composition must be available per native IME context which can be shared by multiple native widgets. Currently, TextComposition and nsIMEStateManager manage it per widget. So, if we will allow to move composition across native widgets, it becomes problem.

We can use opaque pointer as identifier of native IME context. This patch adds mNativeIMEContext to the InputContext struct. It can be gotten with nsIWidget::GetInputContext(). See following patches for the widget side implementation.
Attachment #672607 - Flags: superreview?(roc)
Attachment #672607 - Flags: review?(bugs)
Created attachment 672608 [details] [diff] [review]
part.2 Set InputContext::mNativeIMEContext on Windows
Created attachment 672609 [details] [diff] [review]
part.3 Set InputContext::mNativeIMEContext on Linux
Created attachment 672610 [details] [diff] [review]
part.4 Set InputContext::mNativeIMEContext on Mac
Created attachment 672611 [details] [diff] [review]
part.5 Set InputContext::mNativeIMEContext on Android
Created attachment 672612 [details] [diff] [review]
part.6 Set InputContext::mNativeIMEContext on OS/2
Created attachment 672613 [details] [diff] [review]
part.7 Set InputContext::mNativeIMEContext on B2G
Attachment #672607 - Flags: superreview?(roc) → superreview+
Comment on attachment 672607 [details] [diff] [review]
part.1 Add mNativeIMEContext to InputContext and TextComposition should use it instead of nsIWidget


> TextComposition::TextComposition(nsPresContext* aPresContext,
>                                  nsINode* aNode,
>                                  nsGUIEvent* aEvent) :
>   mPresContext(aPresContext), mNode(aNode),
>-  // temporarily, we should assume that one native IME context is per native
>-  // widget.
>-  mNativeContext(aEvent->widget),
>+  mNativeContext(aEvent->widget->GetInputContext().mNativeIMEContext),
Hmm, if GetInputContext() never returns null, could we name it 
InputContext().
Could be done in a followup bug.
Attachment #672607 - Flags: review?(bugs) → review+
(In reply to Olli Pettay [:smaug] from comment #8)
> Comment on attachment 672607 [details] [diff] [review]
> part.1 Add mNativeIMEContext to InputContext and TextComposition should use
> it instead of nsIWidget
> 
> 
> > TextComposition::TextComposition(nsPresContext* aPresContext,
> >                                  nsINode* aNode,
> >                                  nsGUIEvent* aEvent) :
> >   mPresContext(aPresContext), mNode(aNode),
> >-  // temporarily, we should assume that one native IME context is per native
> >-  // widget.
> >-  mNativeContext(aEvent->widget),
> >+  mNativeContext(aEvent->widget->GetInputContext().mNativeIMEContext),
> Hmm, if GetInputContext() never returns null, could we name it 
> InputContext().
> Could be done in a followup bug.

Hmm, then, the pair should be SetInputContext() and InputContext()?
Comment on attachment 672608 [details] [diff] [review]
part.2 Set InputContext::mNativeIMEContext on Windows

When IME is disabled, we disassociate the IMC from window. When IME is enabled, we associate the default IMC from the window. So, we cannot get the context at GetInputContext() if IME is disabled.

This patch stores the default IMC at creating the window. And if SetInputContext() cannot get IMC for the window, it's not replacing the stored IMC. Otherwise, using the latest IMC. Then, GetInputContext() always returns the stored IMC.
Attachment #672608 - Flags: review?(jmathies)
Attachment #672609 - Flags: review?(karlt)
Comment on attachment 672610 [details] [diff] [review]
part.4 Set InputContext::mNativeIMEContext on Mac

Fortunately, we can use NSView's inputContext for getting the input manager's pointer (available on 10.6 and later!). All of the values are same in each top level window.
Attachment #672610 - Flags: review?(smichaud)
Comment on attachment 672611 [details] [diff] [review]
part.5 Set InputContext::mNativeIMEContext on Android

I think that there is only one IME context in a process on Android. I.e., there are never two or more composition strings at a time. Then, we should set nullptr.

Otherwise, we need to set the pointer of IME context for the widget.
Attachment #672611 - Flags: review?(nchen)
Attachment #672612 - Flags: review?(daveryeo)
Comment on attachment 672613 [details] [diff] [review]
part.7 Set InputContext::mNativeIMEContext on B2G

I guess that b2g only supports only one composition in a process, isn't it? If so, we should set nullptr to the mNativeIMEContext. Otherwise, we need to set native IME context pointer (e.g., if one composition is available per widget, we should set the pointer of the widget).
Attachment #672613 - Flags: review?(jones.chris.g)
Comment on attachment 672613 [details] [diff] [review]
part.7 Set InputContext::mNativeIMEContext on B2G

Nit: s/b2g/gonk/.  B2G is a product, like fennec.  Gonk is one of the operating systems that b2g runs on.

I'm not really sure what this patch is doing, but gonk doesn't have any native IMEs and gecko only creates one widget there, so this seems fine.
Attachment #672613 - Flags: review?(jones.chris.g) → review+
(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #9)
> Hmm, then, the pair should be SetInputContext() and InputContext()?
That would be ok. Especially in content/ and dom/ code
methods which start with Get* (and are not from .idl interfaces) can return null, but
without Get prefix it is guaranteed to be non-null.
(In reply to Olli Pettay [:smaug] from comment #15)
> (In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #9)
> > Hmm, then, the pair should be SetInputContext() and InputContext()?
> That would be ok. Especially in content/ and dom/ code
> methods which start with Get* (and are not from .idl interfaces) can return
> null, but
> without Get prefix it is guaranteed to be non-null.

Even if it returns reference? (not pointer)
(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #16)
> (In reply to Olli Pettay [:smaug] from comment #15)
> > (In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #9)
> > > Hmm, then, the pair should be SetInputContext() and InputContext()?
> > That would be ok. Especially in content/ and dom/ code
> > methods which start with Get* (and are not from .idl interfaces) can return
> > null, but
> > without Get prefix it is guaranteed to be non-null.
> 
> Even if it returns reference? (not pointer)

Er, it's not reference. It returns a copy of the struct.
Attachment #672611 - Flags: review?(nchen) → review+

Updated

6 years ago
Attachment #672608 - Flags: review?(jmathies) → review+
Attachment #672609 - Flags: review?(karlt) → review+

Updated

6 years ago
Attachment #672612 - Flags: review?(daveryeo) → review+
Attachment #672610 - Flags: review?(smichaud) → review+
> Flags: in-testsuite?

It's impossible to test this now because our editor commits composition forcibly when it's losing focus. This fix will be necessary when we stop committing composition forcibly at that time.
Comment on attachment 672607 [details] [diff] [review]
part.1 Add mNativeIMEContext to InputContext and TextComposition should use it instead of nsIWidget

Review of attachment 672607 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/ipc/PBrowser.ipdl
@@ +156,5 @@
>       */
>      sync EndIMEComposition(bool cancel) returns (nsString composition);
>  
> +    sync GetInputContext() returns (int32_t IMEEnabled, int32_t IMEOpen,
> +                                    int64_t NativeIMEContext);

I guess we can't use intptr_t here?
Oh, right. I didn't know it. I'll file a followup bug with a patch.
Depends on: 806300
filed bug 806300 for s/int64_t/intptr_t
You need to log in before you can comment on or make changes to this bug.