1447924 - Optimize undo/redo methods of editor

Status: RESOLVED FIXED

(Core :: DOM: Editor, enhancement)

Description

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

undo/redo related methods are still XPCOM methods and also they use XPCOM methods too.

Comment 1

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

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

Comment 2

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

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

Comment 3

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

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

Comment 4

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

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

Comment 5

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

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

Comment 6

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

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

Comment 7

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

Attachment: Bug 1447924 - part 1: Rename nsTransactionManager to mozilla::TransactionManager

We should rename nsTransactionManager to mozilla::TransactionManager before
adding new non-virtual methods.

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

Comment 8

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

Attachment: Bug 1447924 - part 2: Rename nsTransactionItem to mozilla::TransactionItem

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

Comment 9

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

Attachment: Bug 1447924 - part 3: Rename nsTransactionStack to mozilla::TransactionStack

Then, all classes in editor/txmgr is now in mozilla namespace and all
headers which are included by other directory are now exposed.  So,
we can remote local includes from other directories now.

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

Comment 10

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

Attachment: Bug 1447924 - part 4: Optimize NumbeOfUndoItems(), NumbeOfRedoItems(), CanUndo() and CanRedo() of EditorBase

Now, both TransactionManager.h and TransactionStack.h are exposed.  So,
TransactionManager::GetNumberOfUndoItems() and
TransactionManager::GetNumberOfRedoItems() can be rewritten with non-virtual
inline methods because they just return mUndoStack.GetSize() and
mRedoStack.GetSize().  Then, we can implement EditorBase::NumbeOfUndoItems(),
EditorBase::NumberOfRedoItems(), EditorBase::CanUndo() and
EditorBase::CanRedo() as inline methods.

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

Comment 11

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

Attachment: Bug 1447924 - part 5: Merge TextEditor::Undo()/Redo() with EditorBase::Undo()/Redo()

EditorBase::Undo() and EditorBase::Redo() implement only undo/redo function.
TextEditor::Undo() and TextEditor::Redo() call them with calling some
notification methods.  However, this causes redundant AutoRules instance
creation and doesn't make sense to separate them under current design.

Therefore this patch merges them into TextEditor.  Unfortunately, they are
XPCOM methods but we cannot implement proper overloads of non-virtual since
they are already minimized design.  Fortunately, reducing only one virtual
call per undo/redo isn't so effective.  So, let's keep simpler design.

Additionally, this patch makes them stop committing composition because
Chrome does not commit composition even if "undo"/"redo" is requested with
execCommand() during composition and it doesn't make sense to try to
undo/redo after committing composition since first undoable transaction
becomes the committing composition and committing composition causes
removing all transactions from redo transaction queue.

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

Comment 12

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

Attachment: Bug 1447924 - part 6: Implement EnableUndoRedo(), DisableUndoRedo() and ClearUndoRedo() in EditorBase and TransactionManager

nsIEditor::EnableUndo() and nsITransactionManager::Clear(),
nsITransactionManager::SetMaxTransactionCount() are called a lot but they are
virtual and some of or all of them are called once.  There should be each
non-virtual method to do what each root caller wants.  Therefore, this patch
adds EditorBase::EnableUndoRedo(), EditorBase::DisableUndoRedo(),
EditorBase::ClearUndoRedo(), TransactionManager::EnableUndoRedo(),
TransactionManager::DisableUndoRedo() and TransactionManager::ClearUndoRedo().

Note that this patch makes TransactionManager won't clear mUndoStack nor
mRedoStack if mDoStack is not empty.  This is checked only by
TransactionManager::SetMaxTransactionCount() but according to the comment,
TransactionManager::Clear(), TransactionManager::UndoStack() and
TransactionManager::RedoStack() should check it too.

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

Comment 13

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

Attachment: Bug 1447924 - part 7: Implement AddTransactionListener() and RemoveTransactionListener() in EditorBase and TransactionManager

nsITransactionListener::AddListener() and
nsITransactionListener::RemoveListener() are of course virtual methods.
Additionally, they are safe to call without grabbing the TransactionManager's
instance with local variable.  Therefore, if EditorBase has methods to
add/remove transaction listener to/from its transaction manager, we don't
need EditorBase::GetTransactionManager() anymore.

So, this patch adds AddTransactionListener() and RemoveTransactionListener() to
EditorBase and TransactionManager, and remove
EditorBase::GetTransactionManager() (not nsIEditor's one).

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

Comment 14

[Makoto Kato [:m_kato]]

Comment on attachment 8961962 [details]
Bug 1447924 - part 1: Rename nsTransactionManager to mozilla::TransactionManager

https://reviewboard.mozilla.org/r/230550/#review236874

Comment 15

[Makoto Kato [:m_kato]]

Comment on attachment 8961963 [details]
Bug 1447924 - part 2: Rename nsTransactionItem to mozilla::TransactionItem

https://reviewboard.mozilla.org/r/230552/#review236876

::: editor/txmgr/TransactionManager.cpp:632
(Diff revision 1)
>                                       nsISupports* aData)
>  {
>    // XXX: POSSIBLE OPTIMIZATION
>    //      We could use a factory that pre-allocates/recycles transaction items.
> -  RefPtr<nsTransactionItem> tx = new nsTransactionItem(aTransaction);
> -  if (!tx) {
> +  RefPtr<TransactionItem> transactionItem = new TransactionItem(aTransaction);
> +  if (!transactionItem) {

Since Gecko uses infallible allocator, new operator doens't return nullptr.  So, please remove NS_ERROR_OUT_OF_MEMORY condition.

::: editor/txmgr/nsTransactionStack.cpp:105
(Diff revision 1)
>  }
>  
>  void
>  nsTransactionStack::DoTraverse(nsCycleCollectionTraversalCallback &cb)
>  {
>    int32_t size = GetSize();

If possible, could you change to the following?
```
size_t size = GetSize();
```
then, could you chage type of i to size_t?

Comment 16

[Makoto Kato [:m_kato]]

Comment on attachment 8961964 [details]
Bug 1447924 - part 3: Rename nsTransactionStack to mozilla::TransactionStack

https://reviewboard.mozilla.org/r/230554/#review236884

Comment 17

[Makoto Kato [:m_kato]]

Comment on attachment 8961965 [details]
Bug 1447924 - part 4: Optimize NumbeOfUndoItems(), NumbeOfRedoItems(), CanUndo() and CanRedo() of EditorBase

https://reviewboard.mozilla.org/r/230556/#review236886

::: editor/libeditor/EditorBase.cpp:831
(Diff revision 1)
>  }
>  
>  NS_IMETHODIMP
>  EditorBase::GetNumberOfUndoItems(int32_t* aNumItems)
>  {
> -  *aNumItems = NumberOfUndoItems();
> +  *aNumItems = static_cast<int32_t>(NumberOfUndoItems());

Althoguh CheckedInt32 may be better instad of this change, this method will seem to be removed, so it is OK.

Comment 18

[Makoto Kato [:m_kato]]

Comment on attachment 8961966 [details]
Bug 1447924 - part 5: Merge TextEditor::Undo()/Redo() with EditorBase::Undo()/Redo()

https://reviewboard.mozilla.org/r/230558/#review236888

Comment 19

[Makoto Kato [:m_kato]]

Comment on attachment 8961967 [details]
Bug 1447924 - part 6: Implement EnableUndoRedo(), DisableUndoRedo() and ClearUndoRedo() in EditorBase and TransactionManager

https://reviewboard.mozilla.org/r/230560/#review236892

Comment 20

[Makoto Kato [:m_kato]]

Comment on attachment 8961968 [details]
Bug 1447924 - part 7: Implement AddTransactionListener() and RemoveTransactionListener() in EditorBase and TransactionManager

https://reviewboard.mozilla.org/r/230562/#review236898

Comment 21

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

Comment on attachment 8961962 [details]
Bug 1447924 - part 1: Rename nsTransactionManager to mozilla::TransactionManager

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

Comment 22

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

Comment on attachment 8961963 [details]
Bug 1447924 - part 2: Rename nsTransactionItem to mozilla::TransactionItem

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

Comment 23

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

Comment on attachment 8961964 [details]
Bug 1447924 - part 3: Rename nsTransactionStack to mozilla::TransactionStack

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

Comment 24

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

Comment on attachment 8961965 [details]
Bug 1447924 - part 4: Optimize NumbeOfUndoItems(), NumbeOfRedoItems(), CanUndo() and CanRedo() of EditorBase

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

Comment 25

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

Comment on attachment 8961966 [details]
Bug 1447924 - part 5: Merge TextEditor::Undo()/Redo() with EditorBase::Undo()/Redo()

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

Comment 26

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

Comment on attachment 8961967 [details]
Bug 1447924 - part 6: Implement EnableUndoRedo(), DisableUndoRedo() and ClearUndoRedo() in EditorBase and TransactionManager

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

Comment 27

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

Comment on attachment 8961968 [details]
Bug 1447924 - part 7: Implement AddTransactionListener() and RemoveTransactionListener() in EditorBase and TransactionManager

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

Comment 28

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/08e82e58140d
part 1: Rename nsTransactionManager to mozilla::TransactionManager r=m_kato
https://hg.mozilla.org/integration/autoland/rev/c630f585884a
part 2: Rename nsTransactionItem to mozilla::TransactionItem r=m_kato
https://hg.mozilla.org/integration/autoland/rev/362c6382d265
part 3: Rename nsTransactionStack to mozilla::TransactionStack r=m_kato
https://hg.mozilla.org/integration/autoland/rev/08b900a07115
part 4: Optimize NumbeOfUndoItems(), NumbeOfRedoItems(), CanUndo() and CanRedo() of EditorBase r=m_kato
https://hg.mozilla.org/integration/autoland/rev/869a1445816b
part 5: Merge TextEditor::Undo()/Redo() with EditorBase::Undo()/Redo() r=m_kato
https://hg.mozilla.org/integration/autoland/rev/f44840389420
part 6: Implement EnableUndoRedo(), DisableUndoRedo() and ClearUndoRedo() in EditorBase and TransactionManager r=m_kato
https://hg.mozilla.org/integration/autoland/rev/d3a025d0c732
part 7: Implement AddTransactionListener() and RemoveTransactionListener() in EditorBase and TransactionManager r=m_kato

Comment 29

[Bogdan Tara[:bogdan_tara | bogdant]]

https://hg.mozilla.org/mozilla-central/rev/08e82e58140d
https://hg.mozilla.org/mozilla-central/rev/c630f585884a
https://hg.mozilla.org/mozilla-central/rev/362c6382d265
https://hg.mozilla.org/mozilla-central/rev/08b900a07115
https://hg.mozilla.org/mozilla-central/rev/869a1445816b
https://hg.mozilla.org/mozilla-central/rev/f44840389420
https://hg.mozilla.org/mozilla-central/rev/d3a025d0c732