Skip to content

Commit

Permalink
Bug 1767876 - part 3: Get rid of nsITransactionListener interface r…
Browse files Browse the repository at this point in the history 
…=m_kato

Now, nobody (including comm-central) uses `nsITransactionListener` interface.

This patch also optimizes the notifying methods of `HTMLEditor` from
`TransactionManager` to make them safer and simpler.  The `HTMLEditor`'s methods
can drop some arguments, but I'd like to keep them to make new listeners in the
future handle them easier and safer, but the `ComposerCommandsUpdater` does not
require them.  This is the reason why the difference of the methods' arguments.

Depends on D145664

Differential Revision: https://phabricator.services.mozilla.com/D145667
  • Loading branch information
@masayuki-nakano
masayuki-nakano committed May 9, 2022
1 parent 05f5948 commit 9fd1e2d
Show file tree
Hide file tree
Showing 10 changed files with 56 additions and 238 deletions.
9 changes: 3 additions & 6 deletions editor/composer/ComposerCommandsUpdater.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -53,8 +53,7 @@ NS_IMPL_CYCLE_COLLECTION(ComposerCommandsUpdater, mUpdateTimer, mDOMWindow,
#endif

void ComposerCommandsUpdater::DidDoTransaction(
TransactionManager& aTransactionManager, nsITransaction* aTransaction,
nsresult aDoTransactionResult) {
TransactionManager& aTransactionManager) {
// only need to update if the status of the Undo menu item changes.
if (aTransactionManager.NumberOfUndoItems() == 1) {
if (mFirstDoOfFirstUndo) {
Expand All @@ -65,17 +64,15 @@ void ComposerCommandsUpdater::DidDoTransaction(
}

void ComposerCommandsUpdater::DidUndoTransaction(
TransactionManager& aTransactionManager, nsITransaction* aTransaction,
nsresult aUndoTransactionResult) {
TransactionManager& aTransactionManager) {
if (!aTransactionManager.NumberOfUndoItems()) {
mFirstDoOfFirstUndo = true; // reset the state for the next do
}
UpdateCommandGroup(CommandGroup::Undo);
}

void ComposerCommandsUpdater::DidRedoTransaction(
TransactionManager& aTransactionManager, nsITransaction* aTransaction,
nsresult aRedoTransactionResult) {
TransactionManager& aTransactionManager) {
UpdateCommandGroup(CommandGroup::Undo);
}

Expand Down
12 changes: 5 additions & 7 deletions editor/composer/ComposerCommandsUpdater.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -85,17 +85,15 @@ class ComposerCommandsUpdater final : public nsITimerCallback, public nsINamed {

/**
* The following methods are called when aTransactionManager did
* `DoTransaction`, `UndoTransaction` or `RedoTransaction` of aTransaction.
* `DoTransaction`, `UndoTransaction` or `RedoTransaction` of a transaction
* instance.
*/
MOZ_CAN_RUN_SCRIPT void DidDoTransaction(
TransactionManager& aTransactionManager, nsITransaction* aTransaction,
nsresult aDoTransactionResult);
TransactionManager& aTransactionManager);
MOZ_CAN_RUN_SCRIPT void DidUndoTransaction(
TransactionManager& aTransactionManager, nsITransaction* aTransaction,
nsresult aUndoTransactionResult);
TransactionManager& aTransactionManager);
MOZ_CAN_RUN_SCRIPT void DidRedoTransaction(
TransactionManager& aTransactionManager, nsITransaction* aTransaction,
nsresult aRedoTransactionResult);
TransactionManager& aTransactionManager);

protected:
virtual ~ComposerCommandsUpdater();
Expand Down
15 changes: 6 additions & 9 deletions editor/libeditor/HTMLEditor.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4425,32 +4425,29 @@ class HTMLEditor final : public EditorBase,
}

MOZ_CAN_RUN_SCRIPT void DidDoTransaction(
TransactionManager& aTransactionManager, nsITransaction* aTransaction,
TransactionManager& aTransactionManager, nsITransaction& aTransaction,
nsresult aDoTransactionResult) {
if (mComposerCommandsUpdater) {
RefPtr<ComposerCommandsUpdater> updater(mComposerCommandsUpdater);
updater->DidDoTransaction(aTransactionManager, aTransaction,
aDoTransactionResult);
updater->DidDoTransaction(aTransactionManager);
}
}

MOZ_CAN_RUN_SCRIPT void DidUndoTransaction(
TransactionManager& aTransactionManager, nsITransaction* aTransaction,
TransactionManager& aTransactionManager, nsITransaction& aTransaction,
nsresult aUndoTransactionResult) {
if (mComposerCommandsUpdater) {
RefPtr<ComposerCommandsUpdater> updater(mComposerCommandsUpdater);
updater->DidUndoTransaction(aTransactionManager, aTransaction,
aUndoTransactionResult);
updater->DidUndoTransaction(aTransactionManager);
}
}

MOZ_CAN_RUN_SCRIPT void DidRedoTransaction(
TransactionManager& aTransactionManager, nsITransaction* aTransaction,
TransactionManager& aTransactionManager, nsITransaction& aTransaction,
nsresult aRedoTransactionResult) {
if (mComposerCommandsUpdater) {
RefPtr<ComposerCommandsUpdater> updater(mComposerCommandsUpdater);
updater->DidRedoTransaction(aTransactionManager, aTransaction,
aRedoTransactionResult);
updater->DidRedoTransaction(aTransactionManager);
}
}

Expand Down
24 changes: 8 additions & 16 deletions editor/txmgr/TransactionItem.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -143,16 +143,12 @@ nsresult TransactionItem::UndoChildren(
mRedoStack->Push(transactionItem.forget());
}

nsresult rv2 = aTransactionManager->DidUndoNotify(transaction, rv);
NS_WARNING_ASSERTION(NS_SUCCEEDED(rv2),
"TransactionManager::DidUndoNotify() failed");
if (NS_SUCCEEDED(rv)) {
rv = rv2;
if (transaction) {
aTransactionManager->DidUndoNotify(*transaction, rv);
}
}
// XXX NS_OK if there is no Undo items or all methods work fine, otherwise,
// the result of the last item's UndoTransaction() or
// DidUndoNotify() if UndoTransaction() succeeded.
// NS_OK if there is no Undo items or all methods work fine, otherwise,
// the result of the last item's UndoTransaction().
return rv;
}

Expand Down Expand Up @@ -206,16 +202,12 @@ nsresult TransactionItem::RedoChildren(
}

// XXX Shouldn't this DidRedoNotify()? (bug 1311626)
nsresult rv2 = aTransactionManager->DidUndoNotify(transaction, rv);
NS_WARNING_ASSERTION(NS_SUCCEEDED(rv2),
"TransactionManager::DidUndoNotify() failed");
if (NS_SUCCEEDED(rv)) {
rv = rv2;
if (transaction) {
aTransactionManager->DidUndoNotify(*transaction, rv);
}
}
// XXX NS_OK if there is no Redo items or all methods work fine, otherwise,
// the result of the last item's RedoTransaction() or
// DidUndoNotify() if UndoTransaction() succeeded.
// NS_OK if there is no Redo items or all methods work fine, otherwise,
// the result of the last item's RedoTransaction().
return rv;
}

Expand Down
120 changes: 28 additions & 92 deletions editor/txmgr/TransactionManager.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,6 @@
#include "nsISupportsBase.h"
#include "nsISupportsUtils.h"
#include "nsITransaction.h"
#include "nsITransactionListener.h"
#include "nsIWeakReference.h"
#include "TransactionItem.h"

Expand All @@ -32,7 +31,6 @@ NS_IMPL_CYCLE_COLLECTION_CLASS(TransactionManager)

NS_IMPL_CYCLE_COLLECTION_UNLINK_BEGIN(TransactionManager)
NS_IMPL_CYCLE_COLLECTION_UNLINK(mHTMLEditor)
NS_IMPL_CYCLE_COLLECTION_UNLINK(mListeners)
tmp->mDoStack.DoUnlink();
tmp->mUndoStack.DoUnlink();
tmp->mRedoStack.DoUnlink();
Expand All @@ -41,7 +39,6 @@ NS_IMPL_CYCLE_COLLECTION_UNLINK_END

NS_IMPL_CYCLE_COLLECTION_TRAVERSE_BEGIN(TransactionManager)
NS_IMPL_CYCLE_COLLECTION_TRAVERSE(mHTMLEditor)
NS_IMPL_CYCLE_COLLECTION_TRAVERSE(mListeners)
tmp->mDoStack.DoTraverse(cb);
tmp->mUndoStack.DoTraverse(cb);
tmp->mRedoStack.DoTraverse(cb);
Expand All @@ -67,35 +64,31 @@ void TransactionManager::Detach(const HTMLEditor& aHTMLEditor) {
}
}

NS_IMETHODIMP TransactionManager::DoTransaction(nsITransaction* aTransaction) {
MOZ_CAN_RUN_SCRIPT_BOUNDARY NS_IMETHODIMP
TransactionManager::DoTransaction(nsITransaction* aTransaction) {
if (NS_WARN_IF(!aTransaction)) {
return NS_ERROR_INVALID_ARG;
}
OwningNonNull<nsITransaction> transaction = *aTransaction;

nsresult rv = BeginTransaction(aTransaction, nullptr);
nsresult rv = BeginTransaction(transaction, nullptr);
if (NS_FAILED(rv)) {
NS_WARNING("TransactionManager::BeginTransaction() failed");
DebugOnly<nsresult> rvIgnored = DidDoNotify(aTransaction, rv);
NS_WARNING_ASSERTION(
NS_SUCCEEDED(rvIgnored),
"TransactionManager::DidDoNotify() failed, but ignored");
DidDoNotify(transaction, rv);
return rv;
}

rv = EndTransaction(false);
NS_WARNING_ASSERTION(NS_SUCCEEDED(rv),
"TransactionManager::EndTransaction() failed");

nsresult rv2 = DidDoNotify(aTransaction, rv);
NS_WARNING_ASSERTION(NS_SUCCEEDED(rv2),
"TransactionManager::DidDoNotify() failed");

// The result of EndTransaction() or DidDoNotify() if EndTransaction()
// succeeded.
return NS_SUCCEEDED(rv) ? rv2 : rv;
DidDoNotify(transaction, rv);
return rv;
}

NS_IMETHODIMP TransactionManager::UndoTransaction() { return Undo(); }
MOZ_CAN_RUN_SCRIPT NS_IMETHODIMP TransactionManager::UndoTransaction() {
return Undo();
}

nsresult TransactionManager::Undo() {
// It's possible to be called Undo() again while the transaction manager is
Expand Down Expand Up @@ -123,16 +116,16 @@ nsresult TransactionManager::Undo() {
mRedoStack.Push(transactionItem.forget());
}

nsresult rv2 = DidUndoNotify(transaction, rv);
NS_WARNING_ASSERTION(NS_SUCCEEDED(rv2),
"TransactionManager::DidUndoNotify() failed");

// The result of UndoTransaction() or DidUndoNotify() if UndoTransaction()
// succeeded.
return NS_SUCCEEDED(rv) ? rv2 : rv;
if (transaction) {
DidUndoNotify(*transaction, rv);
}
return rv;
}

NS_IMETHODIMP TransactionManager::RedoTransaction() { return Redo(); }
MOZ_CAN_RUN_SCRIPT_BOUNDARY NS_IMETHODIMP
TransactionManager::RedoTransaction() {
return Redo();
}

nsresult TransactionManager::Redo() {
// It's possible to be called Redo() again while the transaction manager is
Expand Down Expand Up @@ -160,13 +153,10 @@ nsresult TransactionManager::Redo() {
mUndoStack.Push(transactionItem.forget());
}

nsresult rv2 = DidRedoNotify(transaction, rv);
NS_WARNING_ASSERTION(NS_SUCCEEDED(rv2),
"TransactionManager::DidRedoNotify() failed");

// The result of RedoTransaction() or DidRedoNotify() if RedoTransaction()
// succeeded.
return NS_SUCCEEDED(rv) ? rv2 : rv;
if (transaction) {
DidRedoNotify(*transaction, rv);
}
return rv;
}

NS_IMETHODIMP TransactionManager::Clear() {
Expand Down Expand Up @@ -376,24 +366,6 @@ nsresult TransactionManager::RemoveTopUndo() {
return NS_OK;
}

NS_IMETHODIMP TransactionManager::AddListener(
nsITransactionListener* aListener) {
if (NS_WARN_IF(!aListener)) {
return NS_ERROR_INVALID_ARG;
}
return NS_WARN_IF(!AddTransactionListener(*aListener)) ? NS_ERROR_FAILURE
: NS_OK;
}

NS_IMETHODIMP TransactionManager::RemoveListener(
nsITransactionListener* aListener) {
if (NS_WARN_IF(!aListener)) {
return NS_ERROR_INVALID_ARG;
}
return NS_WARN_IF(!RemoveTransactionListener(*aListener)) ? NS_ERROR_FAILURE
: NS_OK;
}

NS_IMETHODIMP TransactionManager::ClearUndoStack() {
if (NS_WARN_IF(!mDoStack.IsEmpty())) {
return NS_ERROR_FAILURE;
Expand All @@ -410,64 +382,28 @@ NS_IMETHODIMP TransactionManager::ClearRedoStack() {
return NS_OK;
}

nsresult TransactionManager::DidDoNotify(nsITransaction* aTransaction,
nsresult aDoResult) {
void TransactionManager::DidDoNotify(nsITransaction& aTransaction,
nsresult aDoResult) {
if (mHTMLEditor) {
RefPtr<HTMLEditor> htmlEditor(mHTMLEditor);
htmlEditor->DidDoTransaction(*this, aTransaction, aDoResult);
}
nsCOMArray<nsITransactionListener> listeners(mListeners);
for (nsITransactionListener* listener : listeners) {
if (NS_WARN_IF(!listener)) {
return NS_ERROR_FAILURE;
}
nsresult rv = listener->DidDo(this, aTransaction, aDoResult);
if (NS_FAILED(rv)) {
NS_WARNING("nsITransactionListener::DidDo() failed");
return rv;
}
}
return NS_OK;
}

nsresult TransactionManager::DidUndoNotify(nsITransaction* aTransaction,
nsresult aUndoResult) {
void TransactionManager::DidUndoNotify(nsITransaction& aTransaction,
nsresult aUndoResult) {
if (mHTMLEditor) {
RefPtr<HTMLEditor> htmlEditor(mHTMLEditor);
htmlEditor->DidUndoTransaction(*this, aTransaction, aUndoResult);
}
nsCOMArray<nsITransactionListener> listeners(mListeners);
for (nsITransactionListener* listener : listeners) {
if (NS_WARN_IF(!listener)) {
return NS_ERROR_FAILURE;
}
nsresult rv = listener->DidUndo(this, aTransaction, aUndoResult);
if (NS_FAILED(rv)) {
NS_WARNING("nsITransactionListener::DidUndo() failed");
return rv;
}
}
return NS_OK;
}

nsresult TransactionManager::DidRedoNotify(nsITransaction* aTransaction,
nsresult aRedoResult) {
void TransactionManager::DidRedoNotify(nsITransaction& aTransaction,
nsresult aRedoResult) {
if (mHTMLEditor) {
RefPtr<HTMLEditor> htmlEditor(mHTMLEditor);
htmlEditor->DidRedoTransaction(*this, aTransaction, aRedoResult);
}
nsCOMArray<nsITransactionListener> listeners(mListeners);
for (nsITransactionListener* listener : listeners) {
if (NS_WARN_IF(!listener)) {
return NS_ERROR_FAILURE;
}
nsresult rv = listener->DidRedo(this, aTransaction, aRedoResult);
if (NS_FAILED(rv)) {
NS_WARNING("nsITransactionListener::DidRedo() failed");
return rv;
}
}
return NS_OK;
}

nsresult TransactionManager::BeginTransaction(nsITransaction* aTransaction,
Expand Down
24 changes: 6 additions & 18 deletions editor/txmgr/TransactionManager.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,6 @@
#include "nscore.h"

class nsITransaction;
class nsITransactionListener;

namespace mozilla {

Expand Down Expand Up @@ -55,25 +54,15 @@ class TransactionManager final : public nsITransactionManager,
return true;
}

bool AddTransactionListener(nsITransactionListener& aListener) {
// XXX Shouldn't we check if aListener has already been in mListeners?
return mListeners.AppendObject(&aListener);
}
bool RemoveTransactionListener(nsITransactionListener& aListener) {
return mListeners.RemoveObject(&aListener);
}
void Attach(HTMLEditor& aHTMLEditor);
void Detach(const HTMLEditor& aHTMLEditor);

// FYI: We don't need to treat the following methods as `MOZ_CAN_RUN_SCRIPT`
// for now because only ComposerCommandUpdater is the listener and it
// does not do something dangerous synchronously.
MOZ_CAN_RUN_SCRIPT_BOUNDARY nsresult DidDoNotify(nsITransaction* aTransaction,
nsresult aDoResult);
MOZ_CAN_RUN_SCRIPT_BOUNDARY nsresult
DidUndoNotify(nsITransaction* aTransaction, nsresult aUndoResult);
MOZ_CAN_RUN_SCRIPT_BOUNDARY nsresult
DidRedoNotify(nsITransaction* aTransaction, nsresult aRedoResult);
MOZ_CAN_RUN_SCRIPT void DidDoNotify(nsITransaction& aTransaction,
nsresult aDoResult);
MOZ_CAN_RUN_SCRIPT void DidUndoNotify(nsITransaction& aTransaction,
nsresult aUndoResult);
MOZ_CAN_RUN_SCRIPT void DidRedoNotify(nsITransaction& aTransaction,
nsresult aRedoResult);

/**
* Exposing non-virtual methods of nsITransactionManager methods.
Expand All @@ -92,7 +81,6 @@ class TransactionManager final : public nsITransactionManager,
TransactionStack mDoStack;
TransactionStack mUndoStack;
TransactionStack mRedoStack;
nsCOMArray<nsITransactionListener> mListeners;
RefPtr<HTMLEditor> mHTMLEditor;
};

Expand Down
1 change: 0 additions & 1 deletion editor/txmgr/moz.build
View file
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,6 @@ TEST_DIRS += ["tests"]

XPIDL_SOURCES += [
"nsITransaction.idl",
"nsITransactionListener.idl",
"nsITransactionManager.idl",
]

Expand Down
Loading

0 comments on commit 9fd1e2d

Please sign in to comment.