Closed Bug 801989 Opened 7 years ago Closed 7 years ago

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

Categories

(Core :: Widget, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla19

People

(Reporter: masayuki, Assigned: masayuki)

References

Details

(Keywords: inputmethod)

Attachments

(7 files)

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.
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)
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+
Attachment #672608 - Flags: review?(jmathies) → review+
Attachment #672609 - Flags: review?(karlt) → review+
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.
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.