Bug 181137 - part 2: Make nsContentIterator class is a base class of other concrete classes r=smaug
☠☠ backed out by 5562d6967f3d ☠ ☠
authorMasayuki Nakano <masayuki@d-toybox.com>
Thu, 10 Jan 2019 08:44:24 +0000
changeset 453213 654fdbad67db2de8a0eca4b0a2d6326d743cc127
parent 453212 90a1ff49b6b1463090cb19cbc54a85f152f2488f
child 453214 b6fc7a332db7a7a3e3fe89f311b3c44e49145ad6
push id35349
push userbtara@mozilla.com
push dateThu, 10 Jan 2019 17:19:27 +0000
treeherdermozilla-central@a51746f37520 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewerssmaug
bugs181137
milestone66.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 181137 - part 2: Make nsContentIterator class is a base class of other concrete classes r=smaug Currently, ContentIterator is created with a bool flag to decide whether the instance lists up post-order or pre-order. However, this is not clear. For example: nsCOMPtr<nsIContentIterator> preOrderIter = new ContentIterator(false); This is not clear whether this does right thing or not. This patch makes any users can create PostContentIterator for post-order iterator, and creates PreContentIterator for pre-order iterator. So, now, each creator needs to writhe above as: nsCOMPtr<nsIContentIterator> preOrderIter = new PreContentIterator(); or nsCOMPtr<nsIContentIterator> postOrderIter = new PostContentIterator(); Additionally, with this change, if each user starts to use concrete classes directly, compiler can stop using virtual calls because of all concrete classes are now marked as "final". Differential Revision: https://phabricator.services.mozilla.com/D15918
dom/base/ContentIterator.cpp
dom/base/ContentIterator.h
--- a/dom/base/ContentIterator.cpp
+++ b/dom/base/ContentIterator.cpp
@@ -10,22 +10,22 @@
 #include "mozilla/RangeBoundary.h"
 
 #include "nsContentUtils.h"
 #include "nsElementTable.h"
 #include "nsIContent.h"
 #include "nsRange.h"
 
 already_AddRefed<nsIContentIterator> NS_NewContentIterator() {
-  nsCOMPtr<nsIContentIterator> iter = new mozilla::ContentIterator(false);
+  nsCOMPtr<nsIContentIterator> iter = new mozilla::PostContentIterator();
   return iter.forget();
 }
 
 already_AddRefed<nsIContentIterator> NS_NewPreContentIterator() {
-  nsCOMPtr<nsIContentIterator> iter = new mozilla::ContentIterator(true);
+  nsCOMPtr<nsIContentIterator> iter = new mozilla::PreContentIterator();
   return iter.forget();
 }
 
 already_AddRefed<nsIContentIterator> NS_NewContentSubtreeIterator() {
   nsCOMPtr<nsIContentIterator> iter = new mozilla::ContentSubtreeIterator();
   return iter.forget();
 }
 
@@ -78,43 +78,44 @@ static bool NodeIsInTraversalRange(nsINo
   }
 
   // Pre mode: start <= node < end.
   RawRangeBoundary beforeNode(parent, aNode->GetPreviousSibling());
   return nsContentUtils::ComparePoints(aStart, beforeNode) <= 0 &&
          nsContentUtils::ComparePoints(aEnd, beforeNode) > 0;
 }
 
-NS_IMPL_CYCLE_COLLECTING_ADDREF(ContentIterator)
-NS_IMPL_CYCLE_COLLECTING_RELEASE_WITH_LAST_RELEASE(ContentIterator,
+NS_IMPL_CYCLE_COLLECTING_ADDREF(ContentIteratorBase)
+NS_IMPL_CYCLE_COLLECTING_RELEASE_WITH_LAST_RELEASE(ContentIteratorBase,
                                                    LastRelease())
 
-NS_INTERFACE_MAP_BEGIN(ContentIterator)
+NS_INTERFACE_MAP_BEGIN(ContentIteratorBase)
   NS_INTERFACE_MAP_ENTRY(nsIContentIterator)
   NS_INTERFACE_MAP_ENTRY_AMBIGUOUS(nsISupports, nsIContentIterator)
-  NS_INTERFACE_MAP_ENTRIES_CYCLE_COLLECTION(ContentIterator)
+  NS_INTERFACE_MAP_ENTRIES_CYCLE_COLLECTION(ContentIteratorBase)
 NS_INTERFACE_MAP_END
 
-NS_IMPL_CYCLE_COLLECTION(ContentIterator, mCurNode, mFirst, mLast,
+NS_IMPL_CYCLE_COLLECTION(ContentIteratorBase, mCurNode, mFirst, mLast,
                          mCommonParent)
 
-void ContentIterator::LastRelease() {
+void ContentIteratorBase::LastRelease() {
   mCurNode = nullptr;
   mFirst = nullptr;
   mLast = nullptr;
   mCommonParent = nullptr;
 }
 
-ContentIterator::ContentIterator(bool aPre) : mIsDone(false), mPre(aPre) {}
+ContentIteratorBase::ContentIteratorBase(bool aPre)
+    : mIsDone(false), mPre(aPre) {}
 
 /******************************************************
  * Init routines
  ******************************************************/
 
-nsresult ContentIterator::Init(nsINode* aRoot) {
+nsresult ContentIteratorBase::Init(nsINode* aRoot) {
   if (NS_WARN_IF(!aRoot)) {
     return NS_ERROR_NULL_POINTER;
   }
 
   mIsDone = false;
 
   if (mPre) {
     mFirst = aRoot;
@@ -126,57 +127,59 @@ nsresult ContentIterator::Init(nsINode* 
     mLast = aRoot;
   }
 
   mCommonParent = aRoot;
   mCurNode = mFirst;
   return NS_OK;
 }
 
-nsresult ContentIterator::Init(nsRange* aRange) {
+nsresult ContentIteratorBase::Init(nsRange* aRange) {
   mIsDone = false;
 
   if (NS_WARN_IF(!aRange)) {
     return NS_ERROR_INVALID_ARG;
   }
 
   if (NS_WARN_IF(!aRange->IsPositioned())) {
     return NS_ERROR_INVALID_ARG;
   }
 
   return InitInternal(aRange->StartRef().AsRaw(), aRange->EndRef().AsRaw());
 }
 
-nsresult ContentIterator::Init(nsINode* aStartContainer, uint32_t aStartOffset,
-                               nsINode* aEndContainer, uint32_t aEndOffset) {
+nsresult ContentIteratorBase::Init(nsINode* aStartContainer,
+                                   uint32_t aStartOffset,
+                                   nsINode* aEndContainer,
+                                   uint32_t aEndOffset) {
   mIsDone = false;
 
   if (NS_WARN_IF(!nsRange::IsValidPoints(aStartContainer, aStartOffset,
                                          aEndContainer, aEndOffset))) {
     return NS_ERROR_INVALID_ARG;
   }
 
   return InitInternal(RawRangeBoundary(aStartContainer, aStartOffset),
                       RawRangeBoundary(aEndContainer, aEndOffset));
 }
 
-nsresult ContentIterator::Init(const RawRangeBoundary& aStart,
-                               const RawRangeBoundary& aEnd) {
+nsresult ContentIteratorBase::Init(const RawRangeBoundary& aStart,
+                                   const RawRangeBoundary& aEnd) {
   mIsDone = false;
 
   if (NS_WARN_IF(!nsRange::IsValidPoints(aStart.Container(), aStart.Offset(),
                                          aEnd.Container(), aEnd.Offset()))) {
     return NS_ERROR_INVALID_ARG;
   }
 
   return InitInternal(aStart, aEnd);
 }
 
-nsresult ContentIterator::InitInternal(const RawRangeBoundary& aStart,
-                                       const RawRangeBoundary& aEnd) {
+nsresult ContentIteratorBase::InitInternal(const RawRangeBoundary& aStart,
+                                           const RawRangeBoundary& aEnd) {
   // get common content parent
   mCommonParent =
       nsContentUtils::GetCommonAncestor(aStart.Container(), aEnd.Container());
   if (NS_WARN_IF(!mCommonParent)) {
     return NS_ERROR_FAILURE;
   }
 
   bool startIsData = aStart.Container()->IsCharacterData();
@@ -348,71 +351,71 @@ nsresult ContentIterator::InitInternal(c
   }
 
   mCurNode = mFirst;
   mIsDone = !mCurNode;
 
   return NS_OK;
 }
 
-void ContentIterator::MakeEmpty() {
+void ContentIteratorBase::MakeEmpty() {
   mCurNode = nullptr;
   mFirst = nullptr;
   mLast = nullptr;
   mCommonParent = nullptr;
   mIsDone = true;
 }
 
-nsINode* ContentIterator::GetDeepFirstChild(nsINode* aRoot) {
+nsINode* ContentIteratorBase::GetDeepFirstChild(nsINode* aRoot) {
   if (NS_WARN_IF(!aRoot) || !aRoot->HasChildren()) {
     return aRoot;
   }
 
   return GetDeepFirstChild(aRoot->GetFirstChild());
 }
 
-nsIContent* ContentIterator::GetDeepFirstChild(nsIContent* aRoot) {
+nsIContent* ContentIteratorBase::GetDeepFirstChild(nsIContent* aRoot) {
   if (NS_WARN_IF(!aRoot)) {
     return nullptr;
   }
 
   nsIContent* node = aRoot;
   nsIContent* child = node->GetFirstChild();
 
   while (child) {
     node = child;
     child = node->GetFirstChild();
   }
 
   return node;
 }
 
-nsINode* ContentIterator::GetDeepLastChild(nsINode* aRoot) {
+nsINode* ContentIteratorBase::GetDeepLastChild(nsINode* aRoot) {
   if (NS_WARN_IF(!aRoot) || !aRoot->HasChildren()) {
     return aRoot;
   }
 
   return GetDeepLastChild(aRoot->GetLastChild());
 }
 
-nsIContent* ContentIterator::GetDeepLastChild(nsIContent* aRoot) {
+nsIContent* ContentIteratorBase::GetDeepLastChild(nsIContent* aRoot) {
   if (NS_WARN_IF(!aRoot)) {
     return nullptr;
   }
 
   nsIContent* node = aRoot;
   while (node->HasChildren()) {
     nsIContent* child = node->GetLastChild();
     node = child;
   }
   return node;
 }
 
 // Get the next sibling, or parent's next sibling, or grandpa's next sibling...
-nsIContent* ContentIterator::GetNextSibling(nsINode* aNode) {
+nsIContent* ContentIteratorBase::GetNextSibling(nsINode* aNode) {
   if (NS_WARN_IF(!aNode)) {
     return nullptr;
   }
 
   if (aNode->GetNextSibling()) {
     return aNode->GetNextSibling();
   }
 
@@ -427,17 +430,17 @@ nsIContent* ContentIterator::GetNextSibl
   if (parent->GetLastChild() && parent->GetLastChild() != aNode) {
     return parent->GetFirstChild();
   }
 
   return GetNextSibling(parent);
 }
 
 // Get the prev sibling, or parent's prev sibling, or grandpa's prev sibling...
-nsIContent* ContentIterator::GetPrevSibling(nsINode* aNode) {
+nsIContent* ContentIteratorBase::GetPrevSibling(nsINode* aNode) {
   if (NS_WARN_IF(!aNode)) {
     return nullptr;
   }
 
   if (aNode->GetPreviousSibling()) {
     return aNode->GetPreviousSibling();
   }
 
@@ -451,17 +454,17 @@ nsIContent* ContentIterator::GetPrevSibl
   // the last child of our parent.
   if (parent->GetFirstChild() && parent->GetFirstChild() != aNode) {
     return parent->GetLastChild();
   }
 
   return GetPrevSibling(parent);
 }
 
-nsINode* ContentIterator::NextNode(nsINode* aNode) {
+nsINode* ContentIteratorBase::NextNode(nsINode* aNode) {
   nsINode* node = aNode;
 
   // if we are a Pre-order iterator, use pre-order
   if (mPre) {
     // if it has children then next node is first child
     if (node->HasChildren()) {
       nsIContent* firstChild = node->GetFirstChild();
       MOZ_ASSERT(firstChild);
@@ -485,17 +488,17 @@ nsINode* ContentIterator::NextNode(nsINo
   if (sibling) {
     // next node is sibling's "deep left" child
     return GetDeepFirstChild(sibling);
   }
 
   return parent;
 }
 
-nsINode* ContentIterator::PrevNode(nsINode* aNode) {
+nsINode* ContentIteratorBase::PrevNode(nsINode* aNode) {
   nsINode* node = aNode;
 
   // if we are a Pre-order iterator, use pre-order
   if (mPre) {
     nsINode* parent = node->GetParentNode();
     if (NS_WARN_IF(!parent)) {
       MOZ_ASSERT(parent, "The node is the root node but not the first node");
       mIsDone = true;
@@ -515,73 +518,73 @@ nsINode* ContentIterator::PrevNode(nsINo
     return node->GetLastChild();
   }
 
   // else prev sibling is previous
   return GetPrevSibling(node);
 }
 
 /******************************************************
- * ContentIterator routines
+ * ContentIteratorBase routines
  ******************************************************/
 
-void ContentIterator::First() {
+void ContentIteratorBase::First() {
   if (mFirst) {
     mozilla::DebugOnly<nsresult> rv = PositionAt(mFirst);
     NS_ASSERTION(NS_SUCCEEDED(rv), "Failed to position iterator!");
   }
 
   mIsDone = mFirst == nullptr;
 }
 
-void ContentIterator::Last() {
+void ContentIteratorBase::Last() {
   // Note that mLast can be nullptr if MakeEmpty() is called in Init() since
   // at that time, Init() returns NS_OK.
   if (!mLast) {
     MOZ_ASSERT(mIsDone);
     return;
   }
 
   mozilla::DebugOnly<nsresult> rv = PositionAt(mLast);
   NS_ASSERTION(NS_SUCCEEDED(rv), "Failed to position iterator!");
 
   mIsDone = mLast == nullptr;
 }
 
-void ContentIterator::Next() {
+void ContentIteratorBase::Next() {
   if (mIsDone || NS_WARN_IF(!mCurNode)) {
     return;
   }
 
   if (mCurNode == mLast) {
     mIsDone = true;
     return;
   }
 
   mCurNode = NextNode(mCurNode);
 }
 
-void ContentIterator::Prev() {
+void ContentIteratorBase::Prev() {
   if (NS_WARN_IF(mIsDone) || NS_WARN_IF(!mCurNode)) {
     return;
   }
 
   if (mCurNode == mFirst) {
     mIsDone = true;
     return;
   }
 
   mCurNode = PrevNode(mCurNode);
 }
 
-bool ContentIterator::IsDone() { return mIsDone; }
+bool ContentIteratorBase::IsDone() { return mIsDone; }
 
 // Keeping arrays of indexes for the stack of nodes makes PositionAt
 // interesting...
-nsresult ContentIterator::PositionAt(nsINode* aCurNode) {
+nsresult ContentIteratorBase::PositionAt(nsINode* aCurNode) {
   if (NS_WARN_IF(!aCurNode)) {
     return NS_ERROR_NULL_POINTER;
   }
 
   // take an early out if this doesn't actually change the position
   if (mCurNode == aCurNode) {
     mIsDone = false;
     return NS_OK;
@@ -630,42 +633,54 @@ nsresult ContentIterator::PositionAt(nsI
     mIsDone = true;
     return NS_ERROR_FAILURE;
   }
 
   mIsDone = false;
   return NS_OK;
 }
 
-nsINode* ContentIterator::GetCurrentNode() {
+nsINode* ContentIteratorBase::GetCurrentNode() {
   if (mIsDone) {
     return nullptr;
   }
 
   NS_ASSERTION(mCurNode, "Null current node in an iterator that's not done!");
 
   return mCurNode;
 }
 
 /******************************************************
+ * PostContentIterator
+ ******************************************************/
+
+NS_IMPL_ISUPPORTS_CYCLE_COLLECTION_INHERITED_0(PostContentIterator,
+                                               ContentIteratorBase)
+NS_IMPL_CYCLE_COLLECTION_INHERITED(PostContentIterator, ContentIteratorBase)
+
+/******************************************************
+ * PreContentIterator
+ ******************************************************/
+
+NS_IMPL_ISUPPORTS_CYCLE_COLLECTION_INHERITED_0(PreContentIterator,
+                                               ContentIteratorBase)
+NS_IMPL_CYCLE_COLLECTION_INHERITED(PreContentIterator, ContentIteratorBase)
+
+/******************************************************
  * ContentSubtreeIterator
  ******************************************************/
 
-NS_IMPL_ADDREF_INHERITED(ContentSubtreeIterator, ContentIterator)
-NS_IMPL_RELEASE_INHERITED(ContentSubtreeIterator, ContentIterator)
-
-NS_INTERFACE_MAP_BEGIN_CYCLE_COLLECTION(ContentSubtreeIterator)
-NS_INTERFACE_MAP_END_INHERITING(ContentIterator)
-
-NS_IMPL_CYCLE_COLLECTION_INHERITED(ContentSubtreeIterator, ContentIterator,
+NS_IMPL_ISUPPORTS_CYCLE_COLLECTION_INHERITED_0(ContentSubtreeIterator,
+                                               ContentIteratorBase)
+NS_IMPL_CYCLE_COLLECTION_INHERITED(ContentSubtreeIterator, ContentIteratorBase,
                                    mRange)
 
 void ContentSubtreeIterator::LastRelease() {
   mRange = nullptr;
-  ContentIterator::LastRelease();
+  ContentIteratorBase::LastRelease();
 }
 
 /******************************************************
  * Init routines
  ******************************************************/
 
 nsresult ContentSubtreeIterator::Init(nsINode* aRoot) {
   return NS_ERROR_NOT_IMPLEMENTED;
--- a/dom/base/ContentIterator.h
+++ b/dom/base/ContentIterator.h
@@ -13,24 +13,29 @@
 #include "nsIContent.h"
 #include "nsIContentIterator.h"
 #include "nsRange.h"
 #include "nsTArray.h"
 
 namespace mozilla {
 
 /**
- * A simple iterator class for traversing the content in "close tag" order.
+ * ContentIteratorBase is a base class of PostContentIterator,
+ * PreContentIterator and ContentSubtreeIterator.  Making each concrete
+ * classes "final", compiler can avoid virtual calls if they are treated
+ * by the users directly.
  */
-class ContentIterator : public nsIContentIterator {
+class ContentIteratorBase : public nsIContentIterator {
  public:
+  ContentIteratorBase() = delete;
+  ContentIteratorBase(const ContentIteratorBase&) = delete;
+  ContentIteratorBase& operator=(const ContentIteratorBase&) = delete;
+
   NS_DECL_CYCLE_COLLECTING_ISUPPORTS
-  NS_DECL_CYCLE_COLLECTION_CLASS(ContentIterator)
-
-  explicit ContentIterator(bool aPre);
+  NS_DECL_CYCLE_COLLECTION_CLASS(ContentIteratorBase)
 
   virtual nsresult Init(nsINode* aRoot) override;
   virtual nsresult Init(nsRange* aRange) override;
   virtual nsresult Init(nsINode* aStartContainer, uint32_t aStartOffset,
                         nsINode* aEndContainer, uint32_t aEndOffset) override;
   virtual nsresult Init(const RawRangeBoundary& aStart,
                         const RawRangeBoundary& aEnd) override;
 
@@ -41,17 +46,18 @@ class ContentIterator : public nsIConten
 
   virtual nsINode* GetCurrentNode() override;
 
   virtual bool IsDone() override;
 
   virtual nsresult PositionAt(nsINode* aCurNode) override;
 
  protected:
-  virtual ~ContentIterator() = default;
+  explicit ContentIteratorBase(bool aPre);
+  virtual ~ContentIteratorBase() = default;
 
   /**
    * Callers must guarantee that:
    * - Neither aStartContainer nor aEndContainer is nullptr.
    * - aStartOffset and aEndOffset are valid for its container.
    * - The start point and the end point are in document order.
    */
   nsresult InitInternal(const RawRangeBoundary& aStart,
@@ -79,32 +85,64 @@ class ContentIterator : public nsIConten
 
   nsCOMPtr<nsINode> mCurNode;
   nsCOMPtr<nsINode> mFirst;
   nsCOMPtr<nsINode> mLast;
   nsCOMPtr<nsINode> mCommonParent;
 
   bool mIsDone;
   bool mPre;
+};
 
- private:
-  ContentIterator(const ContentIterator&) = delete;
-  ContentIterator& operator=(const ContentIterator&) = delete;
+/**
+ * A simple iterator class for traversing the content in "close tag" order.
+ */
+class PostContentIterator final : public ContentIteratorBase {
+ public:
+  PostContentIterator() : ContentIteratorBase(false) {}
+  PostContentIterator(const PostContentIterator&) = delete;
+  PostContentIterator& operator=(const PostContentIterator&) = delete;
+
+  NS_DECL_ISUPPORTS_INHERITED
+  NS_DECL_CYCLE_COLLECTION_CLASS_INHERITED(PostContentIterator,
+                                           ContentIteratorBase)
+
+ protected:
+  virtual ~PostContentIterator() = default;
+};
+
+/**
+ * A simple iterator class for traversing the content in "start tag" order.
+ */
+class PreContentIterator final : public ContentIteratorBase {
+ public:
+  PreContentIterator() : ContentIteratorBase(true) {}
+  PreContentIterator(const PreContentIterator&) = delete;
+  PreContentIterator& operator=(const PreContentIterator&) = delete;
+
+  NS_DECL_ISUPPORTS_INHERITED
+  NS_DECL_CYCLE_COLLECTION_CLASS_INHERITED(PreContentIterator,
+                                           ContentIteratorBase)
+
+ protected:
+  virtual ~PreContentIterator() = default;
 };
 
 /**
  *  A simple iterator class for traversing the content in "top subtree" order.
  */
-class ContentSubtreeIterator final : public ContentIterator {
+class ContentSubtreeIterator final : public ContentIteratorBase {
  public:
-  ContentSubtreeIterator() : ContentIterator(false) {}
+  ContentSubtreeIterator() : ContentIteratorBase(true) {}
+  ContentSubtreeIterator(const ContentSubtreeIterator&) = delete;
+  ContentSubtreeIterator& operator=(const ContentSubtreeIterator&) = delete;
 
   NS_DECL_ISUPPORTS_INHERITED
   NS_DECL_CYCLE_COLLECTION_CLASS_INHERITED(ContentSubtreeIterator,
-                                           ContentIterator)
+                                           ContentIteratorBase)
 
   virtual nsresult Init(nsINode* aRoot) override;
   virtual nsresult Init(nsRange* aRange) override;
   virtual nsresult Init(nsINode* aStartContainer, uint32_t aStartOffset,
                         nsINode* aEndContainer, uint32_t aEndOffset) override;
   virtual nsresult Init(const RawRangeBoundary& aStart,
                         const RawRangeBoundary& aEnd) override;
 
@@ -127,20 +165,16 @@ class ContentSubtreeIterator final : pub
 
   // Returns the highest inclusive ancestor of aNode that's in the range
   // (possibly aNode itself).  Returns null if aNode is null, or is not itself
   // in the range.  A node is in the range if (node, 0) comes strictly after
   // the range endpoint, and (node, node.length) comes strictly before it, so
   // the range's start and end nodes will never be considered "in" it.
   nsIContent* GetTopAncestorInRange(nsINode* aNode);
 
-  // no copy's or assigns  FIX ME
-  ContentSubtreeIterator(const ContentSubtreeIterator&) = delete;
-  ContentSubtreeIterator& operator=(const ContentSubtreeIterator&) = delete;
-
   virtual void LastRelease() override;
 
   RefPtr<nsRange> mRange;
 
   AutoTArray<nsIContent*, 8> mEndNodes;
 };
 
 }  // namespace mozilla