1540029 - Merge TextEditRules and HTMLEditRules with TextEditor and HTMLEditor

Status: RESOLVED FIXED

(Core :: DOM: Editor, task, P3)

Description

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

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.

Comment 1

[Makoto Kato [:m_kato]]

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

Comment 2

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(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.

Comment 3

[Makoto Kato [:m_kato]]

(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.

Comment 4

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

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.

Comment 5

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Bug 1540029 - part 1: Move old edit action listener methods of `HTMLEditRules` to `EditorBase::TopLevelEditSubActionData`

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

Comment 6

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Bug 1540029 - part 2: Get rid of `TextEditRules::DocumentIsEmpty()` and `HTMLEditRules::DocumentIsEmpty()`

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.

Comment 7

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Bug 1540029 - part 3: Move `HTMLEditRules::DocumentModified()` to `HTMLEditor`

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.

Comment 8

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Bug 1540029 - part 4: Merge `BeforeEdit()` and `OnStartToHandleTopLevelEditSubAction()`, and `AfterEdit()` and `OnEndHandlingTopLevelEditSubAction()`

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.

Comment 9

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Bug 1540029 - part 5: Replace `HTMLEditRules::GetListState()` with new stack class

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

Comment 10

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Bug 1540029 - part 6: Replace `HTMLEditRules::GetListItemState()` with new stack only class

Comment 11

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Bug 1540029 - part 7: Replace `HTMLEditRules::GetAlignment()` with new stack only class

Comment 12

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Bug 1540029 - part 8: Replace `HTMLEditRules::GetParagraphState()` with new stack only class

Comment 13

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Bug 1540029 - part 9: Move `TextEditRules::Init()` and `HTMLEditRules::Init()` to `TextEditor` and `HTMLEditor`

Comment 14

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Bug 1540029 - part 10: Get rid of `TextEditRules` and `HTMLEditRules`

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.

Comment 15

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Bug 1540029 - part 11: Get rid of `AutoEditInitRulesTrigger`

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.

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/862e39a846fe
part 1: Move old edit action listener methods of `HTMLEditRules` to `EditorBase::TopLevelEditSubActionData` r=m_kato

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/c9c5ddab9b18
part 2: Get rid of `TextEditRules::DocumentIsEmpty()` and `HTMLEditRules::DocumentIsEmpty()` r=m_kato

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/46c0a2f0acb4
part 3: Move `HTMLEditRules::DocumentModified()` to `HTMLEditor` r=m_kato

Comment 19

[Raul Gurzau (:RaulG)]

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

Comment 20

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/396a5f6301a3
part 4: Merge `BeforeEdit()` and `OnStartToHandleTopLevelEditSubAction()`, and `AfterEdit()` and `OnEndHandlingTopLevelEditSubAction()` r=m_kato
https://hg.mozilla.org/integration/autoland/rev/7442cbe672da
part 5: Replace `HTMLEditRules::GetListState()` with new stack class r=m_kato

Comment 21

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/f1c646b0f812
part 6: Replace `HTMLEditRules::GetListItemState()` with new stack only class r=m_kato
https://hg.mozilla.org/integration/autoland/rev/e71921b729c6
part 7: Replace `HTMLEditRules::GetAlignment()` with new stack only class r=m_kato

Comment 22

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/3211676536b0
part 8: Replace `HTMLEditRules::GetParagraphState()` with new stack only class r=m_kato
https://hg.mozilla.org/integration/autoland/rev/56e0e203c4b9
part 9: Move `TextEditRules::Init()` and `HTMLEditRules::Init()` to `TextEditor` and `HTMLEditor` r=m_kato

Comment 23

[Alexandru Michis [:malexandru]]

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

Comment 24

[Noemi Erli[:noemi_erli]]

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

Comment 25

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/72e2b71294b4
part 10: Get rid of `TextEditRules` and `HTMLEditRules` r=m_kato
https://hg.mozilla.org/integration/autoland/rev/620fa59408e8
part 11: Get rid of `AutoEditInitRulesTrigger` r=m_kato

Comment 26

[Cosmin Sabou [:CosminS]]

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