1463327 - Reorganize scope of methods of EditorBase, TextEditor and HTMLEditor

Status: RESOLVED FIXED

(Core :: DOM: Editor, enhancement)

Description

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

Currently, some public methods but not inherited from nsI*Editor may be referred from:

- classes handling comment
- *EditRules class
- *Transaction class
- WSRunObject class

Actual external call is only the first case. And some of those classes are already friend of the editor classes.

At implementing InputEvent.inputType, editor classes need to store top level input type when public methods called from external.

Therefore, for making editor classes safer, public methods which are called only by internal classes of editor should be private or protected but they should be explicitly documented which can be called by internal classes.

Comment 1

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

Merged the change of bug 1463330.

Comment 2

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

https://treeherder.mozilla.org/#/jobs?repo=try&revision=1bf707b094a7fef3942e49ffaa93c78b3a5bf6d9

Comment 3

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

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

Comment 4

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

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

Comment 5

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

Attachment: Bug 1463327 - part 1: Change scope of some methods of EditorBase which won't be called by non-helper classes of editing to protected

EditorBase (and other editor classes) have 2 type of public methods.  One is
true-public methods.  I.e., they should be able to be called by anybody.
E.g., command handlers, event listeners, or JS via nsIEditor interface.
The other is semi-public methods.  They are not called by the above examples
but called by other classes which are helper classes to handle edit actions.
E.g., TextEditRules, HTMLEditRules, HTMLEditUtils, CSSEditUtils and Transaction
classes.

When we will implement InputEvent.inputType, we need to create new stack
class and create its instance at every true-public methods to manage current
inputType (like TextEditRules::AutoSafeEditorData).  Therefore, it should not
happen that new code starts to call semi-public methods without the new
stack class instance.

For preventing this issue, we should make EditorBase have only the true-public
methods as public.  The other public methods should be protected and their
users should be friend classes.  Then, we can protect such method from external
classes.

Note that this patch just moves the methods without any changes in EditorBase.h
(except removes GetName() since there is no body of this method and removes
IterDirection since it's unused).

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

Comment 6

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

Attachment: Bug 1463327 - part 2: Change scope of some methods of TextEditor which won't be called by non-helper classes of editing to protected

TextEditor has 2 type of public methods.  One is true-public methods.  I.e.,
they should be able to be called by anybody.  E.g., command handlers, event
listeners, or JS via nsIEditor interface.  The other is semi-public methods.
 They are not called by the above examples but called by other classes which
are helper classes to handle edit actions.  E.g., TextEditRules, HTMLEditRules,
HTMLEditUtils, CSSEditUtils and Transaction classes.

When we will implement InputEvent.inputType, we need to create new stack
class and create its instance at every true-public methods to manage current
inputType (like TextEditRules::AutoSafeEditorData).  Therefore, it should not
happen that new code starts to call semi-public methods without the new
stack class instance.

For preventing this issue, we should make TextEditor have only the true-public
methods as public.  The other public methods should be protected and their
users should be friend classes.  Then, we can protect such method from external
classes.

Note that this patch just moves the methods without any changes in TextEditor.h.

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

Comment 7

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

Attachment: Bug 1463327 - part 3: Change scope of some methods of HTMLEditor which won't be called by non-helper classes of editing to protected

HTMLEditor has 2 type of public methods.  One is rue-public methods.  I.e.,
they should be able to be called by anybody.  E.g., command handlers, event
listeners, or JS via nsIEditor interface.  The other is semi-public methods.
They are not called by the above examples but called by other classes which
are helper classes to handle edit actions.  E.g., TextEditRules, HTMLEditRules,
HTMLEditUtils, CSSEditUtils and Transaction classes.

When we will implement InputEvent.inputType, we need to create new stack
class and create its instance at every true-public methods to manage current
inputType (like TextEditRules::AutoSafeEditorData).  Therefore, it should not
happen that new code starts to call semi-public methods without the new
stack class instance.

For preventing this issue, we should make HTMLEditor have only the true-public
methods as public.  The other public methods should be protected and their
users should be friend classes.  Then, we can protect such method from external
classes.

Note that this patch just moves the methods without any changes in HTMLEditor.h
(except removes BlockTransformationType since it's unused and replaces
ResizingRequestID with new enum class ResizeAt since normal enum isn't hit by
searchfox.org).

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

Comment 8

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

Attachment: Bug 1463327 - part 4: Add comments to explain which kind of methods should be public methods

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

Comment 9

[Makoto Kato [:m_kato]]

Comment on attachment 8979925 [details]
Bug 1463327 - part 1: Change scope of some methods of EditorBase which won't be called by non-helper classes of editing to protected

https://reviewboard.mozilla.org/r/246116/#review252422

::: editor/libeditor/HTMLStyleEditor.cpp:1054
(Diff revision 1)
>          break;
>        }
>  
>        // just ignore any non-editable nodes
>        if (content->GetAsText() && (!IsEditable(content) ||
> -                                   IsEmptyTextNode(this, content))) {
> +                                   IsEmptyTextNode(*content))) {

Since GetAsText is true, this can replace IsEmptyTextNode with IsEmptyNode.

Comment 10

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

Comment on attachment 8979925 [details]
Bug 1463327 - part 1: Change scope of some methods of EditorBase which won't be called by non-helper classes of editing to protected

https://reviewboard.mozilla.org/r/246116/#review252422

> Since GetAsText is true, this can replace IsEmptyTextNode with IsEmptyNode.

Hmm, yes, but IsEmptyNode() requires out param. Perhaps, we should sort out around IsEmptyNode() first (bug 1359404).

Comment 11

[Makoto Kato [:m_kato]]

Comment on attachment 8979926 [details]
Bug 1463327 - part 2: Change scope of some methods of TextEditor which won't be called by non-helper classes of editing to protected

https://reviewboard.mozilla.org/r/246118/#review252460

Comment 12

[Makoto Kato [:m_kato]]

Comment on attachment 8979927 [details]
Bug 1463327 - part 3: Change scope of some methods of HTMLEditor which won't be called by non-helper classes of editing to protected

https://reviewboard.mozilla.org/r/246120/#review252424

::: editor/libeditor/HTMLEditor.h:116
(Diff revision 1)
> +
> +  bool GetReturnInParagraphCreatesNewParagraph();
> +  Element* GetSelectionContainer();
> +
> +  // nsISelectionListener overrides
> +  NS_DECL_NSISELECTIONLISTENER

Why doesn't NS_DECL_NSISELECTIONLISTENER move to previous HTMLEditor() define?

::: editor/libeditor/HTMLEditor.h:120
(Diff revision 1)
> +  // nsISelectionListener overrides
> +  NS_DECL_NSISELECTIONLISTENER
>  
> -  // Miscellaneous
> +  // nsICSSLoaderObserver
> +  NS_IMETHOD StyleSheetLoaded(StyleSheet* aSheet,
> +                              bool aWasAlternate, nsresult aStatus) override;

Why doesn't nsICSSLoaderObserver's overload move to previous HTMLEditor() define?

Comment 13

[Makoto Kato [:m_kato]]

Comment on attachment 8979928 [details]
Bug 1463327 - part 4: Add comments to explain which kind of methods should be public methods

https://reviewboard.mozilla.org/r/246122/#review252466

Comment 14

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

Comment on attachment 8979925 [details]
Bug 1463327 - part 1: Change scope of some methods of EditorBase which won't be called by non-helper classes of editing to protected

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

Comment 15

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

Comment on attachment 8979926 [details]
Bug 1463327 - part 2: Change scope of some methods of TextEditor which won't be called by non-helper classes of editing to protected

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

Comment 16

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

Comment on attachment 8979927 [details]
Bug 1463327 - part 3: Change scope of some methods of HTMLEditor which won't be called by non-helper classes of editing to protected

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

Comment 17

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

Comment on attachment 8979928 [details]
Bug 1463327 - part 4: Add comments to explain which kind of methods should be public methods

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

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/5c9278b7b027
part 1: Change scope of some methods of EditorBase which won't be called by non-helper classes of editing to protected r=m_kato
https://hg.mozilla.org/integration/autoland/rev/f0cb4203b12d
part 2: Change scope of some methods of TextEditor which won't be called by non-helper classes of editing to protected r=m_kato
https://hg.mozilla.org/integration/autoland/rev/5d7484ed3b99
part 3: Change scope of some methods of HTMLEditor which won't be called by non-helper classes of editing to protected r=m_kato
https://hg.mozilla.org/integration/autoland/rev/ac00527001f9
part 4: Add comments to explain which kind of methods should be public methods r=m_kato

Comment 19

[Cristian Brindusan [:cbrindusan]]

https://hg.mozilla.org/mozilla-central/rev/5c9278b7b027
https://hg.mozilla.org/mozilla-central/rev/f0cb4203b12d
https://hg.mozilla.org/mozilla-central/rev/5d7484ed3b99
https://hg.mozilla.org/mozilla-central/rev/ac00527001f9