Bug 1911264 - Simplify runtime-type access for message nsITransaction undo/redo objects. r=mkmelin

This patch removes all the support for the various generic PropertyBag interfaces.
It adds a new interface, nsIMsgTxn, which extends nsIMsgTxn to add msgWindow and txnType read-only attributes.
Methods in nsMessenger that previously QIed to nsIPropertyBag2 now just QI to nsIMsgTxn instead to access the dedicated txnType attr.

Differential Revision: https://phabricator.services.mozilla.com/D218399

--HG--
extra : amend_source : bd62cadadb35599190f310ed619a585d4ae3c3c5

mailnews/base/public/moz.build

@@ -52,6 +52,7 @@ XPIDL_SOURCES += [
     "nsIMsgStatusFeedback.idl",
     "nsIMsgTagService.idl",
     "nsIMsgThread.idl",
+    "nsIMsgTxn.idl",
     "nsIMsgUserFeedbackListener.idl",
     "nsIMsgWindow.idl",
     "nsISpamSettings.idl",

mailnews/base/public/nsIMsgTxn.idl

@@ -0,0 +1,24 @@
+/* -*- Mode: IDL; tab-width: 4; indent-tabs-mode: nil; c-basic-offset: 4 -*- */
+/* This Source Code Form is subject to the terms of the Mozilla Public
+ * License, v. 2.0. If a copy of the MPL was not distributed with this
+ * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
+
+#include "nsISupports.idl"
+#include "nsITransaction.idl"
+
+interface nsIMsgWindow;
+
+/**
+ * Base class for transactions that can be exposed via the undo/redo UI.
+ * Just adds a msgWindow and type field to the base nsITransaction.
+ */
+[scriptable, uuid(159abb88-5066-11ef-9439-c78c6b666b57)]
+interface nsIMsgTxn : nsITransaction
+{
+    readonly attribute nsIMsgWindow msgWindow;
+
+    /**
+     * One of the types defined in nsIMessenger (eCopyMsg etc).
+     */
+    readonly attribute unsigned long txnType;
+};

mailnews/base/src/nsMessenger.cpp

@@ -1169,17 +1169,17 @@ nsMessenger::MsgHdrFromURI(const nsACString& aUri, nsIMsgDBHdr** aMsgHdr) {
 
 NS_IMETHODIMP nsMessenger::GetUndoTransactionType(uint32_t* txnType) {
   NS_ENSURE_TRUE(txnType && mTxnMgr, NS_ERROR_NULL_POINTER);
-
-  nsresult rv;
   *txnType = nsMessenger::eUnknown;
   nsCOMPtr<nsITransaction> txn;
-  rv = mTxnMgr->PeekUndoStack(getter_AddRefs(txn));
-  if (NS_SUCCEEDED(rv) && txn) {
-    nsCOMPtr<nsIPropertyBag2> propertyBag = do_QueryInterface(txn, &rv);
-    NS_ENSURE_SUCCESS(rv, rv);
-    return propertyBag->GetPropertyAsUint32(u"type"_ns, txnType);
+  nsresult rv = mTxnMgr->PeekUndoStack(getter_AddRefs(txn));
+  NS_ENSURE_SUCCESS(rv, rv);
+  if (!txn) {
+    return NS_OK;  // Nothing to undo.
   }
-  return rv;
+  // Manager holds nsITransactions, but txnType is added by nsIMsgTxn.
+  nsCOMPtr<nsIMsgTxn> msgTxn = do_QueryInterface(txn, &rv);
+  NS_ENSURE_SUCCESS(rv, rv);
+  return msgTxn->GetTxnType(txnType);
 }
 
 NS_IMETHODIMP nsMessenger::CanUndo(bool* bValue) {
@@ -1196,16 +1196,17 @@ NS_IMETHODIMP nsMessenger::CanUndo(bool* bValue) {
 NS_IMETHODIMP nsMessenger::GetRedoTransactionType(uint32_t* txnType) {
   NS_ENSURE_TRUE(txnType && mTxnMgr, NS_ERROR_NULL_POINTER);
 
-  nsresult rv;
   *txnType = nsMessenger::eUnknown;
   nsCOMPtr<nsITransaction> txn;
-  rv = mTxnMgr->PeekRedoStack(getter_AddRefs(txn));
-  if (NS_SUCCEEDED(rv) && txn) {
-    nsCOMPtr<nsIPropertyBag2> propertyBag = do_QueryInterface(txn, &rv);
-    NS_ENSURE_SUCCESS(rv, rv);
-    return propertyBag->GetPropertyAsUint32(u"type"_ns, txnType);
+  nsresult rv = mTxnMgr->PeekRedoStack(getter_AddRefs(txn));
+  NS_ENSURE_SUCCESS(rv, rv);
+  if (!txn) {
+    return NS_OK;  // Nothing to redo.
   }
-  return rv;
+  // Manager holds nsITransactions, but txnType is added by nsIMsgTxn.
+  nsCOMPtr<nsIMsgTxn> msgTxn = do_QueryInterface(txn, &rv);
+  NS_ENSURE_SUCCESS(rv, rv);
+  return msgTxn->GetTxnType(txnType);
 }
 
 NS_IMETHODIMP nsMessenger::CanRedo(bool* bValue) {

mailnews/base/src/nsMsgDBFolder.cpp

@@ -64,7 +64,7 @@
 #include "mozilla/Utf8.h"
 #include "nsIPromptService.h"
 #include "nsEmbedCID.h"
-#include "nsIPropertyBag2.h"
+#include "nsIWritablePropertyBag2.h"
 
 #define oneHour 3600000000U
 

mailnews/base/src/nsMsgReadStateTxn.cpp

@@ -18,7 +18,7 @@ nsresult nsMsgReadStateTxn::Init(nsIMsgFolder* aParentFolder, uint32_t aNumKeys,
   mParentFolder = aParentFolder;
   mMarkedMessages.AppendElements(aMsgKeyArray, aNumKeys);
 
-  return nsMsgTxn::Init();
+  return NS_OK;
 }
 
 NS_IMETHODIMP

mailnews/base/src/nsMsgTxn.cpp

@@ -4,192 +4,24 @@
  * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
 
 #include "nsMsgTxn.h"
+#include "nsIMessenger.h"  // For nsIMessenger::eUnknown et al.
 #include "nsIMsgHdr.h"
 #include "nsIMsgDatabase.h"
-#include "nsCOMArray.h"
-#include "nsArrayEnumerator.h"
-#include "nsVariant.h"
-#include "nsIProperty.h"
-#include "nsMsgMessageFlags.h"
 #include "nsIMsgFolder.h"
+#include "nsMsgMessageFlags.h"
 
-NS_IMPL_ADDREF(nsMsgTxn)
-NS_IMPL_RELEASE(nsMsgTxn)
-NS_INTERFACE_MAP_BEGIN(nsMsgTxn)
-  NS_INTERFACE_MAP_ENTRY(nsIWritablePropertyBag)
-  NS_INTERFACE_MAP_ENTRY_AMBIGUOUS(nsIPropertyBag, nsIWritablePropertyBag)
-  NS_INTERFACE_MAP_ENTRY_AMBIGUOUS(nsISupports, nsIWritablePropertyBag)
-  NS_INTERFACE_MAP_ENTRY(nsITransaction)
-  NS_INTERFACE_MAP_ENTRY(nsIPropertyBag2)
-  NS_INTERFACE_MAP_ENTRY(nsIWritablePropertyBag2)
-NS_INTERFACE_MAP_END
+NS_IMPL_ISUPPORTS(nsMsgTxn, nsIMsgTxn, nsITransaction)
 
-nsMsgTxn::nsMsgTxn() { m_txnType = 0; }
+nsMsgTxn::nsMsgTxn() : m_txnType(nsIMessenger::eUnknown) {}
 
 nsMsgTxn::~nsMsgTxn() {}
 
-nsresult nsMsgTxn::Init() { return NS_OK; }
-
-NS_IMETHODIMP nsMsgTxn::HasKey(const nsAString& name, bool* aResult) {
-  *aResult = mPropertyHash.Get(name, nullptr);
-  return NS_OK;
-}
-
-NS_IMETHODIMP nsMsgTxn::Get(const nsAString& name, nsIVariant** _retval) {
-  mPropertyHash.Get(name, _retval);
-  return NS_OK;
-}
-
-NS_IMETHODIMP nsMsgTxn::GetProperty(const nsAString& name,
-                                    nsIVariant** _retval) {
-  return mPropertyHash.Get(name, _retval) ? NS_OK : NS_ERROR_FAILURE;
-}
-
-NS_IMETHODIMP nsMsgTxn::SetProperty(const nsAString& name, nsIVariant* value) {
-  NS_ENSURE_ARG_POINTER(value);
-  mPropertyHash.InsertOrUpdate(name, value);
-  return NS_OK;
-}
-
-NS_IMETHODIMP nsMsgTxn::DeleteProperty(const nsAString& name) {
-  if (!mPropertyHash.Get(name, nullptr)) return NS_ERROR_FAILURE;
-
-  mPropertyHash.Remove(name);
-  return mPropertyHash.Get(name, nullptr) ? NS_ERROR_FAILURE : NS_OK;
-}
-
-//
-// nsMailSimpleProperty class and impl; used for GetEnumerator
-// This is same as nsSimpleProperty but for external API use.
-//
-
-class nsMailSimpleProperty final : public nsIProperty {
- public:
-  nsMailSimpleProperty(const nsAString& aName, nsIVariant* aValue)
-      : mName(aName), mValue(aValue) {}
-
-  NS_DECL_ISUPPORTS
-  NS_DECL_NSIPROPERTY
- protected:
-  ~nsMailSimpleProperty() {}
-
-  nsString mName;
-  nsCOMPtr<nsIVariant> mValue;
-};
-
-NS_IMPL_ISUPPORTS(nsMailSimpleProperty, nsIProperty)
-
-NS_IMETHODIMP nsMailSimpleProperty::GetName(nsAString& aName) {
-  aName.Assign(mName);
-  return NS_OK;
-}
-
-NS_IMETHODIMP nsMailSimpleProperty::GetValue(nsIVariant** aValue) {
-  NS_IF_ADDREF(*aValue = mValue);
-  return NS_OK;
-}
-
-// end nsMailSimpleProperty
-
-NS_IMETHODIMP nsMsgTxn::GetEnumerator(nsISimpleEnumerator** _retval) {
-  nsCOMArray<nsIProperty> propertyArray;
-  for (auto iter = mPropertyHash.Iter(); !iter.Done(); iter.Next()) {
-    nsMailSimpleProperty* sprop =
-        new nsMailSimpleProperty(iter.Key(), iter.Data());
-    propertyArray.AppendObject(sprop);
-  }
-  return NS_NewArrayEnumerator(_retval, propertyArray, NS_GET_IID(nsIProperty));
-}
-
-#define IMPL_GETSETPROPERTY_AS(Name, Type)                              \
-  NS_IMETHODIMP                                                         \
-  nsMsgTxn::GetPropertyAs##Name(const nsAString& prop, Type* _retval) { \
-    nsIVariant* v = mPropertyHash.GetWeak(prop);                        \
-    if (!v) return NS_ERROR_NOT_AVAILABLE;                              \
-    return v->GetAs##Name(_retval);                                     \
-  }                                                                     \
-                                                                        \
-  NS_IMETHODIMP                                                         \
-  nsMsgTxn::SetPropertyAs##Name(const nsAString& prop, Type value) {    \
-    nsCOMPtr<nsIWritableVariant> var = new nsVariant();                 \
-    var->SetAs##Name(value);                                            \
-    return SetProperty(prop, var);                                      \
-  }
-
-IMPL_GETSETPROPERTY_AS(Int32, int32_t)
-IMPL_GETSETPROPERTY_AS(Uint32, uint32_t)
-IMPL_GETSETPROPERTY_AS(Int64, int64_t)
-IMPL_GETSETPROPERTY_AS(Uint64, uint64_t)
-IMPL_GETSETPROPERTY_AS(Double, double)
-IMPL_GETSETPROPERTY_AS(Bool, bool)
-
-NS_IMETHODIMP nsMsgTxn::GetPropertyAsAString(const nsAString& prop,
-                                             nsAString& _retval) {
-  nsIVariant* v = mPropertyHash.GetWeak(prop);
-  if (!v) return NS_ERROR_NOT_AVAILABLE;
-  return v->GetAsAString(_retval);
-}
-
-NS_IMETHODIMP nsMsgTxn::GetPropertyAsACString(const nsAString& prop,
-                                              nsACString& _retval) {
-  nsIVariant* v = mPropertyHash.GetWeak(prop);
-  if (!v) return NS_ERROR_NOT_AVAILABLE;
-  return v->GetAsACString(_retval);
-}
-
-NS_IMETHODIMP nsMsgTxn::GetPropertyAsAUTF8String(const nsAString& prop,
-                                                 nsACString& _retval) {
-  nsIVariant* v = mPropertyHash.GetWeak(prop);
-  if (!v) return NS_ERROR_NOT_AVAILABLE;
-  return v->GetAsAUTF8String(_retval);
-}
-
-NS_IMETHODIMP nsMsgTxn::GetPropertyAsInterface(const nsAString& prop,
-                                               const nsIID& aIID,
-                                               void** _retval) {
-  nsIVariant* v = mPropertyHash.GetWeak(prop);
-  if (!v) return NS_ERROR_NOT_AVAILABLE;
-  nsCOMPtr<nsISupports> val;
-  nsresult rv = v->GetAsISupports(getter_AddRefs(val));
-  if (NS_FAILED(rv)) return rv;
-  if (!val) {
-    // We have a value, but it's null
-    *_retval = nullptr;
-    return NS_OK;
-  }
-  return val->QueryInterface(aIID, _retval);
-}
-
-NS_IMETHODIMP nsMsgTxn::SetPropertyAsAString(const nsAString& prop,
-                                             const nsAString& value) {
-  nsCOMPtr<nsIWritableVariant> var = new nsVariant();
-  var->SetAsAString(value);
-  return SetProperty(prop, var);
-}
-
-NS_IMETHODIMP nsMsgTxn::SetPropertyAsACString(const nsAString& prop,
-                                              const nsACString& value) {
-  nsCOMPtr<nsIWritableVariant> var = new nsVariant();
-  var->SetAsACString(value);
-  return SetProperty(prop, var);
-}
-
-NS_IMETHODIMP nsMsgTxn::SetPropertyAsAUTF8String(const nsAString& prop,
-                                                 const nsACString& value) {
-  nsCOMPtr<nsIWritableVariant> var = new nsVariant();
-  var->SetAsAUTF8String(value);
-  return SetProperty(prop, var);
-}
-
-NS_IMETHODIMP nsMsgTxn::SetPropertyAsInterface(const nsAString& prop,
-                                               nsISupports* value) {
-  nsCOMPtr<nsIWritableVariant> var = new nsVariant();
-  var->SetAsISupports(value);
-  return SetProperty(prop, var);
-}
-
 /////////////////////// Transaction Stuff //////////////////
-NS_IMETHODIMP nsMsgTxn::DoTransaction(void) { return NS_OK; }
+NS_IMETHODIMP nsMsgTxn::DoTransaction() { return NS_OK; }
+
+NS_IMETHODIMP nsMsgTxn::UndoTransaction() { return NS_ERROR_NOT_IMPLEMENTED; }
+
+NS_IMETHODIMP nsMsgTxn::RedoTransaction() { return NS_ERROR_NOT_IMPLEMENTED; }
 
 NS_IMETHODIMP nsMsgTxn::GetIsTransient(bool* aIsTransient) {
   if (nullptr != aIsTransient)
@@ -204,19 +36,31 @@ NS_IMETHODIMP nsMsgTxn::Merge(nsITransaction* aTransaction, bool* aDidMerge) {
   return NS_OK;
 }
 
-nsresult nsMsgTxn::GetMsgWindow(nsIMsgWindow** msgWindow) {
+NS_IMETHODIMP nsMsgTxn::GetMsgWindow(nsIMsgWindow** msgWindow) {
   if (!msgWindow || !m_msgWindow) return NS_ERROR_NULL_POINTER;
   NS_ADDREF(*msgWindow = m_msgWindow);
   return NS_OK;
 }
 
+NS_IMETHODIMP nsMsgTxn::GetTxnType(uint32_t* txnType) {
+  MOZ_ASSERT(txnType);
+  *txnType = m_txnType;
+  return NS_OK;
+}
+
 nsresult nsMsgTxn::SetMsgWindow(nsIMsgWindow* msgWindow) {
   m_msgWindow = msgWindow;
   return NS_OK;
 }
 
 nsresult nsMsgTxn::SetTransactionType(uint32_t txnType) {
-  return SetPropertyAsUint32(u"type"_ns, txnType);
+  MOZ_ASSERT(m_txnType == nsIMessenger::eUnknown);  // Initialisation only.
+  m_txnType = txnType;
+  return NS_OK;
+}
+
+NS_IMETHODIMP nsMsgTxn::GetAsEditTransactionBase(EditTransactionBase**) {
+  return NS_ERROR_NOT_IMPLEMENTED;
 }
 
 /*none of the callers pass null aFolder,

mailnews/base/src/nsMsgTxn.h

@@ -6,15 +6,11 @@
 #ifndef nsMsgTxn_h__
 #define nsMsgTxn_h__
 
-#include "nsITransaction.h"
+#include "nsIMsgTxn.h"
 #include "msgCore.h"
 #include "nsCOMPtr.h"
 #include "nsIMsgWindow.h"
-#include "nsInterfaceHashtable.h"
 #include "MailNewsTypes2.h"
-#include "nsIVariant.h"
-#include "nsIWritablePropertyBag.h"
-#include "nsIWritablePropertyBag2.h"
 
 #include "mozilla/EditTransactionBase.h"
 
@@ -27,48 +23,35 @@ using mozilla::EditTransactionBase;
     }                                                \
   }
 /**
- * base class for all message undo/redo transactions.
+ * Base class to support undo/redo for moving/copying/deleting/marking
+ * messages.
+ * Just a thin layer on top of nsITransaction which adds fields for runtime
+ * type and for msgWindow. The UI needs the transaction type to describe the
+ * undo/redo action in the GUI (eg "Undo deletion"), but there's no provision
+ * for this in the base nsITransaction interface.
  */
-
-class nsMsgTxn : public nsITransaction,
-                 public nsIWritablePropertyBag,
-                 public nsIWritablePropertyBag2 {
+class nsMsgTxn : public nsIMsgTxn {
  public:
   nsMsgTxn();
 
-  nsresult Init();
-
-  NS_IMETHOD DoTransaction(void) override;
-
-  NS_IMETHOD UndoTransaction(void) override = 0;
-
-  NS_IMETHOD RedoTransaction(void) override = 0;
-
-  NS_IMETHOD GetIsTransient(bool* aIsTransient) override;
-
-  NS_IMETHOD Merge(nsITransaction* aTransaction, bool* aDidMerge) override;
-
-  nsresult GetMsgWindow(nsIMsgWindow** msgWindow);
+  // These should only be called once, to set up the object initially.
   nsresult SetMsgWindow(nsIMsgWindow* msgWindow);
   nsresult SetTransactionType(uint32_t txnType);
 
-  NS_DECL_THREADSAFE_ISUPPORTS
-  NS_DECL_NSIPROPERTYBAG
-  NS_DECL_NSIPROPERTYBAG2
-  NS_DECL_NSIWRITABLEPROPERTYBAG
-  NS_DECL_NSIWRITABLEPROPERTYBAG2
-
-  NS_IMETHOD GetAsEditTransactionBase(EditTransactionBase**) final {
-    return NS_ERROR_NOT_IMPLEMENTED;
-  }
+  NS_DECL_ISUPPORTS
+  NS_DECL_NSITRANSACTION
+  NS_DECL_NSIMSGTXN
 
  protected:
   virtual ~nsMsgTxn();
 
-  // a hash table of string -> nsIVariant
-  nsInterfaceHashtable<nsStringHashKey, nsIVariant> mPropertyHash;
   nsCOMPtr<nsIMsgWindow> m_msgWindow;
   uint32_t m_txnType;
+
+  // Helper function which returns true if the specified message has the
+  // nsMsgMessageFlags::IMAPDeleted flag set.
+  // NOTE: This doesn't rely on nsMsgTxn state, and should probably be moved
+  // out to somewhere more sensible.
   nsresult CheckForToggleDelete(nsIMsgFolder* aFolder, const nsMsgKey& aMsgKey,
                                 bool* aResult);
 };

mailnews/imap/src/nsImapMailFolder.cpp

@@ -78,6 +78,7 @@
 #include "nsReadableUtils.h"
 #include "UrlListener.h"
 #include "nsIObserverService.h"
+#include "nsIPropertyBag2.h"
 
 #define NS_PARSEMAILMSGSTATE_CID                   \
   { /* 2B79AC51-1459-11d3-8097-006008128C4E */     \

mailnews/imap/src/nsImapUndoTxn.cpp

@@ -66,7 +66,7 @@ nsresult nsImapMoveCopyMsgTxn::Init(nsIMsgFolder* srcFolder,
       m_srcMessageIds.AppendElement(messageId);
     }
   }
-  return nsMsgTxn::Init();
+  return NS_OK;
 }
 
 nsImapMoveCopyMsgTxn::~nsImapMoveCopyMsgTxn() {}

mailnews/local/src/nsLocalMailFolder.cpp

@@ -50,6 +50,7 @@
 #include "mozilla/Components.h"
 #include "mozilla/UniquePtr.h"
 #include "StoreIndexer.h"
+#include "nsIPropertyBag2.h"
 
 #include <algorithm>
 #include <functional>

mailnews/local/src/nsLocalUndoTxn.cpp

@@ -40,7 +40,7 @@ nsresult nsLocalMoveCopyMsgTxn::Init(nsIMsgFolder* srcFolder,
   rv = srcFolder->GetURI(protocolType);
   protocolType.SetLength(protocolType.FindChar(':'));
   if (protocolType.LowerCaseEqualsLiteral("imap")) m_srcIsImap4 = true;
-  return nsMsgTxn::Init();
+  return NS_OK;
 }
 nsresult nsLocalMoveCopyMsgTxn::GetSrcIsImap(bool* isImap) {
   *isImap = m_srcIsImap4;