564411 - nsIEditorIMESupport methods/attributes should be moved to nsIEditor (or nsIPlaintextEditor)

Status: RESOLVED FIXED

(Core :: DOM: Editor, defect)

Description

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away: 6/25-6/28)]

I think that nsIEditorIMESupport should be removed because we always support IME on all editors and scriptable methods are not many.

Comment 1

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away: 6/25-6/28)]

Basically, nsIPlaintextEditor is better interface because IME is an UI for text editing. So, if there is non text editable editor inherits nsEditor, IME shouldn't be usable. However, nsIEditor is more convenient for editor users (especially script) because it's first interface they can get from DOM element or DOM window.

I think that the methods should be in nsIEditor and the implementations should be moved to nsPlaintextEditor.

Comment 2

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away: 6/25-6/28)]

Looks like that some addons use nsIEditorIMESupport. I guess that they copied some lines from our toolkit for checking if there is composition.
https://mxr.mozilla.org/addons/search?string=nsIEditorIMESupport

So, we cannot remove nsIEditorIMESupport. But for reducing redundant QI, we should still move the members of nsIEditorIMESupport. I was thinking that the moved interface should be nsIPlaintextEditor. However, if we do so, we cannot reduce the number of QI. Additionally, it's not realistic scenario that somebody implements new editor only with nsIEditor.

Therefore, I think that we can move the members of nsIEditorIMESupport to nsIEditor. Additionally, I guess that we can merge nsIEditor and nsIPlaintextEditor in the future (but not scope of this bug).

Comment 3

[Makoto Kato [:m_kato]]

Humm, except to our addons, this interface is 1 addon only...

Comment 4

[Makoto Kato [:m_kato]]

Also, c-c uses it into editor code.  so we should move all members to nsIEditor at first, then remove this interface per comment #1 and comment #2.

Comment 5

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away: 6/25-6/28)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=367ededef946

Comment 6

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away: 6/25-6/28)]

Attachment: Bug 564411 Move all methods/attributes of nsIEditorIMESupport to nsIEditor

Doing QI from nsIEditor to nsIEditorIMESupport doesn't make sense because editor should always support all methods and attributes of nsIEditorIMESupport (it does NOT mean that all nsIEditor implementation need to support IME).

This patch moves all of them to nsIEditor for avoiding redundant QIs.

Review commit: https://reviewboard.mozilla.org/r/99688/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/99688/

Comment 7

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8820143 [details]
Bug 564411 Move all methods/attributes of nsIEditorIMESupport to nsIEditor

https://reviewboard.mozilla.org/r/99688/#review100122

::: editor/nsIEditorIMESupport.idl:9
(Diff revision 1)
> -
> -native IMEState(mozilla::widget::IMEState);
>  
>  [scriptable, uuid(0ba7f490-afb8-46dd-87fc-bc6137fbc899)]
> -
>  interface nsIEditorIMESupport : nsISupports

If we want to ensure addons and thunderbird etc which might use this interface keep working, perhaps nsIEditorIMESupport should inherit nsIEditor, not nsISupports

Comment 8

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away: 6/25-6/28)]

(In reply to Olli Pettay [:smaug] from comment #7)
> Comment on attachment 8820143 [details]
> Bug 564411 Move all methods/attributes of nsIEditorIMESupport to nsIEditor
> 
> https://reviewboard.mozilla.org/r/99688/#review100122
> 
> ::: editor/nsIEditorIMESupport.idl:9
> (Diff revision 1)
> > -
> > -native IMEState(mozilla::widget::IMEState);
> >  
> >  [scriptable, uuid(0ba7f490-afb8-46dd-87fc-bc6137fbc899)]
> > -
> >  interface nsIEditorIMESupport : nsISupports
> 
> If we want to ensure addons and thunderbird etc which might use this
> interface keep working, perhaps nsIEditorIMESupport should inherit
> nsIEditor, not nsISupports

Oh, right. I forgot to change it, thanks!

Comment 9

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away: 6/25-6/28)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=7b329f425b3d

Comment 10

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away: 6/25-6/28)]

Comment on attachment 8820143 [details]
Bug 564411 Move all methods/attributes of nsIEditorIMESupport to nsIEditor

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/99688/diff/1-2/

Comment 11

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away: 6/25-6/28)]

Comment on attachment 8820143 [details]
Bug 564411 Move all methods/attributes of nsIEditorIMESupport to nsIEditor

https://reviewboard.mozilla.org/r/99688/#review100122

> If we want to ensure addons and thunderbird etc which might use this interface keep working, perhaps nsIEditorIMESupport should inherit nsIEditor, not nsISupports

Okay, I made nsIEditorIMEsupport inherit nsIEditor and EditorBase not inherit nsIEditor directly.

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/20dc8f3fa6ed
Move all methods/attributes of nsIEditorIMESupport to nsIEditor r=smaug

Comment 13

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/20dc8f3fa6ed