564412 - Remove non-scriptable internal methods from nsIEditorIMESupport

Status: RESOLVED FIXED

(Core :: DOM: Editor, defect)

Description

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

nsIEditorIMESupports::beginComposition(), nsIEditorIMESupports::setCompositionString() and 
nsIEditorIMESupports::endComposition() are non-scriptable methods and they are only referred from nsEditorEventListener. nsEditorEventListener knows the editor is nsEditor or its sub class. So, there is no reason they should be in the interface.

Comment 1

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

The difference between nsEditor and nsPlaintextEditor is strange. Some text editing code is implemented in nsEditor but most code is in nsPlaintextEditor. I'm guessing that it's better the IME related code is implemented in nsPlaintextEditor.

Comment 2

[(no longer active)]

(In reply to comment #0)
> nsIEditorIMESupports::beginComposition(),
> nsIEditorIMESupports::setCompositionString() and 
> nsIEditorIMESupports::endComposition() are non-scriptable methods and they are
> only referred from nsEditorEventListener. nsEditorEventListener knows the
> editor is nsEditor or its sub class. So, there is no reason they should be in
> the interface.

Those methods are marked as noscript, so I don't see why this is an issue.  In fact, is this interface _ever_ meant to be accessed by script?  If not, we should not mark it as scriptable in the first place!

(In reply to comment #1)
> The difference between nsEditor and nsPlaintextEditor is strange. Some text
> editing code is implemented in nsEditor but most code is in nsPlaintextEditor.
> I'm guessing that it's better the IME related code is implemented in
> nsPlaintextEditor.

Well, if I were to design the editor hierarchy today, I'd go for:

class nsEditor;
class nsPlaintextEditor : nsEditor;
class nsHTMLEditor : nsEditor;

But I guess that ship has sailed for some time!  :-)

In fact, I'd prefer the IME code to live in nsEditor, because it's not plaintext specific (right?).

Comment 3

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

(In reply to comment #2)
> (In reply to comment #0)
> > nsIEditorIMESupports::beginComposition(),
> > nsIEditorIMESupports::setCompositionString() and 
> > nsIEditorIMESupports::endComposition() are non-scriptable methods and they are
> > only referred from nsEditorEventListener. nsEditorEventListener knows the
> > editor is nsEditor or its sub class. So, there is no reason they should be in
> > the interface.
> 
> Those methods are marked as noscript, so I don't see why this is an issue.  In
> fact, is this interface _ever_ meant to be accessed by script?  If not, we
> should not mark it as scriptable in the first place!

My goal is that I remove nsIEditorIMESupport interface. However, the other three methods are needed by other modules. So, I want to remove the *editor* internal methods first, and I want to move the remains to nsIEditor. I think that, for editor users, there is no reason that we should separate to multiple interfaces except nsIHTMLEditor.

> (In reply to comment #1)
> > The difference between nsEditor and nsPlaintextEditor is strange. Some text
> > editing code is implemented in nsEditor but most code is in nsPlaintextEditor.
> > I'm guessing that it's better the IME related code is implemented in
> > nsPlaintextEditor.
> 
> Well, if I were to design the editor hierarchy today, I'd go for:
> 
> class nsEditor;
> class nsPlaintextEditor : nsEditor;
> class nsHTMLEditor : nsEditor;

But nsHTMLEditor can edit plaintext too (in each node). So, I think that the right hierarchy is:

class nsEditor; // includes nsPlaintextEditor
class nsHTMLEditor: nsEditor;

However, I think nsPlaintextEditor is too big to combine with nsEditor actually. Therefore, I think that only all nsIPlaintextEditor members should be combined to nsIEditor. Then, the users don't need the redundant QIs.

> In fact, I'd prefer the IME code to live in nsEditor, because it's not
> plaintext specific (right?).

If your idea is right, I agree, the code should be in nsEditor. However, IME is a middle ware for international text input. So, I feel that it's strange IME isn't in plaintext editor on my idea.

Comment 4

[(no longer active)]

(In reply to comment #3)
> (In reply to comment #2)
> > (In reply to comment #0)
> > > nsIEditorIMESupports::beginComposition(),
> > > nsIEditorIMESupports::setCompositionString() and 
> > > nsIEditorIMESupports::endComposition() are non-scriptable methods and they are
> > > only referred from nsEditorEventListener. nsEditorEventListener knows the
> > > editor is nsEditor or its sub class. So, there is no reason they should be in
> > > the interface.
> > 
> > Those methods are marked as noscript, so I don't see why this is an issue.  In
> > fact, is this interface _ever_ meant to be accessed by script?  If not, we
> > should not mark it as scriptable in the first place!
> 
> My goal is that I remove nsIEditorIMESupport interface. However, the other
> three methods are needed by other modules. So, I want to remove the *editor*
> internal methods first, and I want to move the remains to nsIEditor. I think
> that, for editor users, there is no reason that we should separate to multiple
> interfaces except nsIHTMLEditor.

Let's remove the internal methods, but I think we should not merge the remaining methods in nsIEditor.  That interface is already bloated, and I don't want to make it any worse than it already is.

> > (In reply to comment #1)
> > > The difference between nsEditor and nsPlaintextEditor is strange. Some text
> > > editing code is implemented in nsEditor but most code is in nsPlaintextEditor.
> > > I'm guessing that it's better the IME related code is implemented in
> > > nsPlaintextEditor.
> > 
> > Well, if I were to design the editor hierarchy today, I'd go for:
> > 
> > class nsEditor;
> > class nsPlaintextEditor : nsEditor;
> > class nsHTMLEditor : nsEditor;
> 
> But nsHTMLEditor can edit plaintext too (in each node).

That's not entirely true.  Our HTML editor can edit plain text, but its behavior is different to that of the plaintext editor.

> So, I think that the
> right hierarchy is:
> 
> class nsEditor; // includes nsPlaintextEditor
> class nsHTMLEditor: nsEditor;

That could be the way to go if HTML editor was really a conceptual subset of the plaintext editor.  But we don't have that luxury unfortunately.  :-(

> However, I think nsPlaintextEditor is too big to combine with nsEditor
> actually. Therefore, I think that only all nsIPlaintextEditor members should be
> combined to nsIEditor. Then, the users don't need the redundant QIs.

Like I said above, the correct solution IMO is to have multiple tightly scoped interfaces instead.  Callers should not need too many QIs if those interfaces are well designed.  But these are all long-term goals.  We have quite a few more useful things to do with our pet (which is more like a beast!).

> > In fact, I'd prefer the IME code to live in nsEditor, because it's not
> > plaintext specific (right?).
> 
> If your idea is right, I agree, the code should be in nsEditor. However, IME is
> a middle ware for international text input. So, I feel that it's strange IME
> isn't in plaintext editor on my idea.

Hmm, I couldn't correctly parse what you said.  Do you mean that IME input can only happen in plaintext editors?  For example, can I use IME if I'm typing an email inside a contenteditable field?

Comment 5

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

(In reply to comment #4)
> (In reply to comment #3)
> > (In reply to comment #2)
> > > (In reply to comment #0)
> > > > nsIEditorIMESupports::beginComposition(),
> > > > nsIEditorIMESupports::setCompositionString() and 
> > > > nsIEditorIMESupports::endComposition() are non-scriptable methods and they are
> > > > only referred from nsEditorEventListener. nsEditorEventListener knows the
> > > > editor is nsEditor or its sub class. So, there is no reason they should be in
> > > > the interface.
> > > 
> > > Those methods are marked as noscript, so I don't see why this is an issue.  In
> > > fact, is this interface _ever_ meant to be accessed by script?  If not, we
> > > should not mark it as scriptable in the first place!
> > 
> > My goal is that I remove nsIEditorIMESupport interface. However, the other
> > three methods are needed by other modules. So, I want to remove the *editor*
> > internal methods first, and I want to move the remains to nsIEditor. I think
> > that, for editor users, there is no reason that we should separate to multiple
> > interfaces except nsIHTMLEditor.
> 
> Let's remove the internal methods, but I think we should not merge the
> remaining methods in nsIEditor.  That interface is already bloated, and I don't
> want to make it any worse than it already is.

Yeah, nsIEditor is already bloated, cannot we reorganize the interface? E.g., nsIEditorInternal should be for some methods such as Init(), postCreate(), preDestroy(), transactionManager, doTransaction(), beginTransaction(), endTransaction(), beginPlaceHolderTransaction(), endPlaceHolderTransaction(), shouldTxnSetSelection(), setShouldTxnSetSelection(), beginningOfDocument(), endOfDocument(), doDrag(), insertFromDrop(), setAttribute(), getAttributeValue(), removeAttribute(), cloneAttribute(), cloneAttributes(), createNode(), insertNode(), splitNode(), joinNodes(), deleteNode(), markNodeDirty(), dumpContentTree(), debugDumpContent(), debugUnitTests() and isModifiableNode().

I think that nsIEditor should have only the methods which are useful for the users (like chrome and addons).

> > However, I think nsPlaintextEditor is too big to combine with nsEditor
> > actually. Therefore, I think that only all nsIPlaintextEditor members should be
> > combined to nsIEditor. Then, the users don't need the redundant QIs.
> 
> Like I said above, the correct solution IMO is to have multiple tightly scoped
> interfaces instead.  Callers should not need too many QIs if those interfaces
> are well designed.  But these are all long-term goals.  We have quite a few
> more useful things to do with our pet (which is more like a beast!).

I agree, but I think that nsIEditorIMESupport should be merged to nsIEditor (if you want to reorganize the nsIEditor, I should do it after that) because it could be one step of the reorganization.

> > > In fact, I'd prefer the IME code to live in nsEditor, because it's not
> > > plaintext specific (right?).
> > 
> > If your idea is right, I agree, the code should be in nsEditor. However, IME is
> > a middle ware for international text input. So, I feel that it's strange IME
> > isn't in plaintext editor on my idea.
> 
> Hmm, I couldn't correctly parse what you said.  Do you mean that IME input can
> only happen in plaintext editors?  For example, can I use IME if I'm typing an
> email inside a contenteditable field?

Sorry for my strange English. I meant you should think that IME related methods are a part of key event handling. In other words, the IME APIs shouldn't be in different interface.

Comment 6

[(no longer active)]

(In reply to comment #5)
> Yeah, nsIEditor is already bloated, cannot we reorganize the interface? E.g.,
> nsIEditorInternal should be for some methods such as Init(), postCreate(),
> preDestroy(), transactionManager, doTransaction(), beginTransaction(),
> endTransaction(), beginPlaceHolderTransaction(), endPlaceHolderTransaction(),
> shouldTxnSetSelection(), setShouldTxnSetSelection(), beginningOfDocument(),
> endOfDocument(), doDrag(), insertFromDrop(), setAttribute(),
> getAttributeValue(), removeAttribute(), cloneAttribute(), cloneAttributes(),
> createNode(), insertNode(), splitNode(), joinNodes(), deleteNode(),
> markNodeDirty(), dumpContentTree(), debugDumpContent(), debugUnitTests() and
> isModifiableNode().
> 
> I think that nsIEditor should have only the methods which are useful for the
> users (like chrome and addons).

Of course we can.  I think ideally addons should not need to use nsIEditor* methods, but it makes sense to clean them up anyways.  I just don't think that this is high priority enough for me to work on right now.

> > > However, I think nsPlaintextEditor is too big to combine with nsEditor
> > > actually. Therefore, I think that only all nsIPlaintextEditor members should be
> > > combined to nsIEditor. Then, the users don't need the redundant QIs.
> > 
> > Like I said above, the correct solution IMO is to have multiple tightly scoped
> > interfaces instead.  Callers should not need too many QIs if those interfaces
> > are well designed.  But these are all long-term goals.  We have quite a few
> > more useful things to do with our pet (which is more like a beast!).
> 
> I agree, but I think that nsIEditorIMESupport should be merged to nsIEditor (if
> you want to reorganize the nsIEditor, I should do it after that) because it
> could be one step of the reorganization.

Well, what's the benefit of merging it if we're going to split it off later?  :-)

> > > > In fact, I'd prefer the IME code to live in nsEditor, because it's not
> > > > plaintext specific (right?).
> > > 
> > > If your idea is right, I agree, the code should be in nsEditor. However, IME is
> > > a middle ware for international text input. So, I feel that it's strange IME
> > > isn't in plaintext editor on my idea.
> > 
> > Hmm, I couldn't correctly parse what you said.  Do you mean that IME input can
> > only happen in plaintext editors?  For example, can I use IME if I'm typing an
> > email inside a contenteditable field?
> 
> Sorry for my strange English. I meant you should think that IME related methods
> are a part of key event handling. In other words, the IME APIs shouldn't be in
> different interface.

So it would make sense to assume that any nsIEditorIMESupport method applies both to plaintext and HTML editors, right?

Comment 7

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

(In reply to comment #6)
> (In reply to comment #5)
> > Yeah, nsIEditor is already bloated, cannot we reorganize the interface? E.g.,
> > nsIEditorInternal should be for some methods such as Init(), postCreate(),
> > preDestroy(), transactionManager, doTransaction(), beginTransaction(),
> > endTransaction(), beginPlaceHolderTransaction(), endPlaceHolderTransaction(),
> > shouldTxnSetSelection(), setShouldTxnSetSelection(), beginningOfDocument(),
> > endOfDocument(), doDrag(), insertFromDrop(), setAttribute(),
> > getAttributeValue(), removeAttribute(), cloneAttribute(), cloneAttributes(),
> > createNode(), insertNode(), splitNode(), joinNodes(), deleteNode(),
> > markNodeDirty(), dumpContentTree(), debugDumpContent(), debugUnitTests() and
> > isModifiableNode().
> > 
> > I think that nsIEditor should have only the methods which are useful for the
> > users (like chrome and addons).
> 
> Of course we can.  I think ideally addons should not need to use nsIEditor*
> methods, but it makes sense to clean them up anyways.  I just don't think that
> this is high priority enough for me to work on right now.

Yeah, I agree. But I want to do it when I have enough time.

> > > > However, I think nsPlaintextEditor is too big to combine with nsEditor
> > > > actually. Therefore, I think that only all nsIPlaintextEditor members should be
> > > > combined to nsIEditor. Then, the users don't need the redundant QIs.
> > > 
> > > Like I said above, the correct solution IMO is to have multiple tightly scoped
> > > interfaces instead.  Callers should not need too many QIs if those interfaces
> > > are well designed.  But these are all long-term goals.  We have quite a few
> > > more useful things to do with our pet (which is more like a beast!).
> > 
> > I agree, but I think that nsIEditorIMESupport should be merged to nsIEditor (if
> > you want to reorganize the nsIEditor, I should do it after that) because it
> > could be one step of the reorganization.
> 
> Well, what's the benefit of merging it if we're going to split it off later? 
> :-)

I have no idea of the direct benefit except reducing QIs. However, the current interfaces are very complex and have unnecessary methods for the users. So, I believe that the cleaning up could help the addon developers and some new comers in the future.

> > > > > In fact, I'd prefer the IME code to live in nsEditor, because it's not
> > > > > plaintext specific (right?).
> > > > 
> > > > If your idea is right, I agree, the code should be in nsEditor. However, IME is
> > > > a middle ware for international text input. So, I feel that it's strange IME
> > > > isn't in plaintext editor on my idea.
> > > 
> > > Hmm, I couldn't correctly parse what you said.  Do you mean that IME input can
> > > only happen in plaintext editors?  For example, can I use IME if I'm typing an
> > > email inside a contenteditable field?
> > 
> > Sorry for my strange English. I meant you should think that IME related methods
> > are a part of key event handling. In other words, the IME APIs shouldn't be in
> > different interface.
> 
> So it would make sense to assume that any nsIEditorIMESupport method applies
> both to plaintext and HTML editors, right?

Yes.

Comment 8

[(no longer active)]

OK, so let's remove those internal methods from nsIEditorIMESupport for the scope of this bug, and get more bugs on nsIEditor refactoring.  If you would like to submit patches for those bugs, I'll gladly owe you a beer!  :-)

Comment 9

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

Attachment: Patch v1.0

(In reply to comment #8)
> OK, so let's remove those internal methods from nsIEditorIMESupport for the
> scope of this bug, and get more bugs on nsIEditor refactoring.

O.K.

> If you would
> like to submit patches for those bugs, I'll gladly owe you a beer!  :-)

:-)

Comment 10

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

http://hg.mozilla.org/mozilla-central/rev/7a7f39366fb2