Closed Bug 1447924 Opened 6 years ago Closed 6 years ago

Optimize undo/redo methods of editor

Tracking

()

Status:

RESOLVED FIXED

Milestone:

mozilla61

Tracking Flags:

Tracking

Status

firefox61

---

fixed

People

(Reporter: masayuki, Assigned: masayuki)

References

(Blocks 1 open bug)

Details

Attachments

(7 files)

Bug 1447924 - part 1: Rename nsTransactionManager to mozilla::TransactionManager 6 years ago Masayuki Nakano [:masayuki] (he/him)(JST, +0900) 59 bytes, text/x-review-board-request	m_kato : review+	Details
Bug 1447924 - part 2: Rename nsTransactionItem to mozilla::TransactionItem 6 years ago Masayuki Nakano [:masayuki] (he/him)(JST, +0900) 59 bytes, text/x-review-board-request	m_kato : review+	Details
Bug 1447924 - part 3: Rename nsTransactionStack to mozilla::TransactionStack 6 years ago Masayuki Nakano [:masayuki] (he/him)(JST, +0900) 59 bytes, text/x-review-board-request	m_kato : review+	Details
Bug 1447924 - part 4: Optimize NumbeOfUndoItems(), NumbeOfRedoItems(), CanUndo() and CanRedo() of EditorBase 6 years ago Masayuki Nakano [:masayuki] (he/him)(JST, +0900) 59 bytes, text/x-review-board-request	m_kato : review+	Details
Bug 1447924 - part 5: Merge TextEditor::Undo()/Redo() with EditorBase::Undo()/Redo() 6 years ago Masayuki Nakano [:masayuki] (he/him)(JST, +0900) 59 bytes, text/x-review-board-request	m_kato : review+	Details
Bug 1447924 - part 6: Implement EnableUndoRedo(), DisableUndoRedo() and ClearUndoRedo() in EditorBase and TransactionManager 6 years ago Masayuki Nakano [:masayuki] (he/him)(JST, +0900) 59 bytes, text/x-review-board-request	m_kato : review+	Details
Bug 1447924 - part 7: Implement AddTransactionListener() and RemoveTransactionListener() in EditorBase and TransactionManager 6 years ago Masayuki Nakano [:masayuki] (he/him)(JST, +0900) 59 bytes, text/x-review-board-request	m_kato : review+	Details

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

Assignee

Description

•

6 years ago

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

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

Assignee

Comment 1

•

6 years ago

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

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

Assignee

Comment 2

•

6 years ago

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

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

Assignee

Comment 3

•

6 years ago

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

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

Assignee

Comment 4

•

6 years ago

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

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

Assignee

Comment 5

•

6 years ago

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

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

Assignee

Comment 6

•

6 years ago

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

Comment hidden (mozreview-request)

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 hidden (mozreview-request)

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

Comment hidden (mozreview-request)

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 hidden (mozreview-request)

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 hidden (mozreview-request)

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 hidden (mozreview-request)

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 hidden (mozreview-request)

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/

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

Assignee

Updated

•

6 years ago

Blocks: 1448780

Makoto Kato [:m_kato]

Comment 14

•

6 years ago

mozreview-review

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

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

Attachment #8961962 - Flags: review?(m_kato) → review+

Makoto Kato [:m_kato]

Comment 15

•

6 years ago

mozreview-review

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?

Attachment #8961963 - Flags: review?(m_kato) → review+

Makoto Kato [:m_kato]

Comment 16

•

6 years ago

mozreview-review

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

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

Attachment #8961964 - Flags: review?(m_kato) → review+

Makoto Kato [:m_kato]

Comment 17

•

6 years ago

mozreview-review

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.

Attachment #8961965 - Flags: review?(m_kato) → review+

Makoto Kato [:m_kato]

Comment 18

•

6 years ago

mozreview-review

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

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

Attachment #8961966 - Flags: review?(m_kato) → review+

Makoto Kato [:m_kato]

Comment 19

•

6 years ago

mozreview-review

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

Attachment #8961967 - Flags: review?(m_kato) → review+

Makoto Kato [:m_kato]

Comment 20

•

6 years ago

mozreview-review

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

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

Attachment #8961968 - Flags: review?(m_kato) → review+

Comment hidden (mozreview-request)

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 hidden (mozreview-request)

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 hidden (mozreview-request)

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 hidden (mozreview-request)

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 hidden (mozreview-request)

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 hidden (mozreview-request)

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 hidden (mozreview-request)

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/

Pulsebot

Comment 28

•

6 years ago

Pushed by masayuki@d-toybox.com:
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

Bogdan Tara[:bogdan_tara | bogdant]

Comment 29

•

6 years ago

bugherder

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

Status: ASSIGNED → RESOLVED

Closed: 6 years ago

status-firefox61: affected → fixed

Resolution: --- → FIXED

Target Milestone: --- → mozilla61

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

Assignee

Updated

•

3 years ago

Blocks: 1720061

You need to log in before you can comment on or make changes to this bug.