I've not considered anything though.

Currently, editor is implemented with 5 big classes. 3 of them are manages most handling of editor and they represent "editor" from the point of view of outside. The classes are:

EditorBase -> TextEditor -> HTMLEditor.

On the other hand, the other 2 classes handles some edit operations, but not all of them. They are:

TextEditRules -> HTMLEditRules

When TextEditor or HTMLEditor instance is initialized, TextEditRules is created and stored by the former, HTMLEditRules is created and stored by the latter. And then, TextEditRules and HTMLEditRules stores reference of its owner editor instance.

And then, WillDoAction() of TextEditRules and HTMLEditRules is called when one of the editor classes handles something, then, some actions are handled in it. Some other actions are handled by editor classes by themselves after that, and finally, DidDoAction() of them is called and some other actions are handled at this time.

While TextEditRules and HTMLEditRules are dosing something, they create a stack class instance of AudoSafeEditorData and it guarantees that the life time of the owner editor.

However, now this does not look safe from our source code analysis. I.e., we need to access them with MOZ_KnownLive() like here. This is really ugly and makes the code harder to read.

Additionally, TextEditRules and HTMLEditRules may call editor methods finally like above. So, I have no idea why we need to keep this design. In my understanding, it was designed for making TextEditor and HTMLEditor be possible to have different behavior mode. However, although HTMLEditor has both HTML editing mode and plain text editing mode but the difference is handled in each method. Furthermore, it does not make sense to implement such different editing mode with another EditRules class because editor module is not cheap to maintain around security issues so that shared code is better for this place.

So, currently, I'm thinking that we should move each editing action in TextEditRules and HTMLEditRules to the caller of WillDoAction() or create new editor method and make editor call it instead. Then, finally, we should remove those classes.

If you guys have some ideas about this issue, let me know.

If HTMLEditor isn't inheritance class TextEditor, EditorBase is necessary. Password support (in TextEditor) is unnecessary for HTMLEditor.

(In reply to Makoto Kato [:m_kato] from comment #1)

If HTMLEditor isn't inheritance class TextEditor, EditorBase is necessary. Password support (in TextEditor) is unnecessary for HTMLEditor.

Right. But can we make HTMLEditor inherit EditorBase directly? (Just question, I've not investigated it.) If we do it, nsIPlaintextEditor interface needs to be implemented by EditorBase at least.

(In reply to Masayuki Nakano [:masayuki] (JST, +0900)(Sick, maybe slow response) from comment #2)

(In reply to Makoto Kato [:m_kato] from comment #1)

If HTMLEditor isn't inheritance class TextEditor, EditorBase is necessary. Password support (in TextEditor) is unnecessary for HTMLEditor.

Right. But can we make HTMLEditor inherit EditorBase directly? (Just question, I've not investigated it.)

Yes, as object orient design. But a lot of codes such as spellchecker already use TextEditor directly and plain text only member variable is a few in TextEditor class. So this isn't strong opinion to keep this class desing.

Before merging this, we should move a lot of node operations in nsIEditor to nsIHTMLEditor. It is unnecessary for TextEditor.

If we do it, nsIPlaintextEditor interface needs to be implemented by EditorBase at least.

nsIPlaintextEditor is another decision. nsIPlaintextEditor's methods can move to nsIEditor and nsIEditorMailSupport. Of course, I can remove nsIEditorMailSupport if we merge TextEditor with EditorBase.

Anyway, I think that we should merge them into editor classes then:

• EditRules does not need to manage editor, selection, etc by themselves.
• a lot of methods which are accessed from EditRules can be private/protected completely. (currently they are accessed as friend classes)

But perhaps, we should keep the files for making developers can access each line history easier. So, in a while, TextEditRules.cpp and HTMLEditRules.cpp may implement some methods of EditorBase, TextEditor or HTMLEditor.

The main purpose of them is modifying
TopLevelEditSubActionData::mChangedRange. Therefore, they should be in the
struct.

TextEditRules::DocumentIsEmpty() is a wrapper of TextEditor::IsEmpty() so
that we can get rid of it simply.

HTMLEditRules::DocumentIsEmpty() needs to change. It's oddly checks only
EditorBase::mPaddingBRElementForEmptyEditor is nullptr or not. However,
the editor may be completely empty. And the result may be different from
TextEditor::IsEmpty() which is not overridden by HTMLEditor. For partially
solving this issue, this patch makes HTMLEditor overrides IsEmpty() and
optimizes TextEditor::IsEmpty().

With this change, the caller of HTMLEditRules::DocumentIsEmpty() may behave
differently in the only caller, HTMLEditor::SelectEntireDocument(). And
unfortunately, its root called from SelectAllCommand::DoCommand(). However,
it does just collapse Selection into the root element (<body> or
Document.documentElement) if it returns true. Therefore, this change
must be safe since anyway SelectionRefPtr()->SelectAllChildren() with
the root element is exactly same as SelectionRefPtr()->Collapse() with
the empty root element if it's truly empty.

And also this patch make each AutoEditSubActionNotifier creator check
the result of OnStartToHandleTopLevelEditSubAction() at least for
NS_ERROR_EDITOR_DESTROYED.

We need to take care of its destructor's result later, though.

And also this patch make each AutoEditSubActionNotifier creator check
the result of OnStartToHandleTopLevelEditSubAction() at least for
NS_ERROR_EDITOR_DESTROYED.

We need to take care of its destructor's result later, though.

It takes a lot of bool out arguments. Therefore, we should make it a
stack only class and caller should retrieve only necessary information.

Now, we can get rid of TextEditRules and HTMLEditRules completely.
And also this patch renames their cpp files to TextEditSubActionHandler
and HTMLEditSubActionHandler.

TextEditor::Init() and HTMLEditor::Init() are still complicated due to
AutoEditInitRulesTrigger. The following patch will remove it.

Only for solving the order of AutoEditInitRulesTrigger destruction when
HTMLEditor::Init() calls TextEditor::Init(), TextEditor has unnecessary
counter and the initialization code is made harder to understand.

Therefore, this patch makes TextEditor::Init() and HTMLEditor::Init()
directly call InitEditorContentAndSelection() at appropriate time.
Additionally, HTMLEditor::Init() can call EditorBase::Init() instead of
TextEditor::Init() since TextEditor::Init() does nothing for HTMLEditor
actually.

https://hg.mozilla.org/mozilla-central/rev/862e39a846fe

https://hg.mozilla.org/mozilla-central/rev/c9c5ddab9b18
https://hg.mozilla.org/mozilla-central/rev/46c0a2f0acb4
https://hg.mozilla.org/mozilla-central/rev/396a5f6301a3
https://hg.mozilla.org/mozilla-central/rev/7442cbe672da
https://hg.mozilla.org/mozilla-central/rev/f1c646b0f812
https://hg.mozilla.org/mozilla-central/rev/e71921b729c6

https://hg.mozilla.org/mozilla-central/rev/3211676536b0
https://hg.mozilla.org/mozilla-central/rev/56e0e203c4b9

https://hg.mozilla.org/mozilla-central/rev/72e2b71294b4
https://hg.mozilla.org/mozilla-central/rev/620fa59408e8