Closed Bug 1447924 Opened 2 years ago Closed 2 years ago

Optimize undo/redo methods of editor

Categories

(Core :: Editor, enhancement)

enhancement
Not set

Tracking

()

RESOLVED FIXED
mozilla61
Tracking Status
firefox61 --- fixed

People

(Reporter: masayuki, Assigned: masayuki)

References

(Blocks 1 open bug)

Details

Attachments

(7 files)

59 bytes, text/x-review-board-request
m_kato
: review+
Details
59 bytes, text/x-review-board-request
m_kato
: review+
Details
59 bytes, text/x-review-board-request
m_kato
: review+
Details
59 bytes, text/x-review-board-request
m_kato
: review+
Details
59 bytes, text/x-review-board-request
m_kato
: review+
Details
59 bytes, text/x-review-board-request
m_kato
: review+
Details
59 bytes, text/x-review-board-request
m_kato
: review+
Details
undo/redo related methods are still XPCOM methods and also they use XPCOM methods too.
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+
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+
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+
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+
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+
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+
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+
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
You need to log in before you can comment on or make changes to this bug.