859193 - Cleanup nsRange

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[David Zbarsky (:dzbarsky)]

Attachment: Patch

Comment 1

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Comment on attachment 734455 [details] [diff] [review]
Patch

Review of attachment 734455 [details] [diff] [review]:
-----------------------------------------------------------------

This is from a quick glance. One question, though. There are a bunch of places where you're checking GetStartContainer and friends for failure, but those can only fail if !mIsPositioned. I'm thinking it might be better to check that up-front, and—certainly in private functions—assert that, and use GetStartParent instead. What do you think?

::: content/base/src/nsRange.cpp
@@ +1406,5 @@
>    ~RangeSubtreeIterator()
>    {
>    }
>  
> +  nsresult Init(nsRange *aRange);

* to the left

@@ +1504,1 @@
>  RangeSubtreeIterator::GetCurrentNode()

Return a raw pointer

@@ +1510,2 @@
>    } else if (mIterState == eUseEnd && mEnd)
> +    node = mEnd;

{}

@@ +1654,5 @@
>    // First get both end points and their common ancestor.
>  
> +  ErrorResult res;
> +  nsCOMPtr<nsINode> commonAncestor = aRange->GetCommonAncestorContainer(res);
> +  if (res.Failed()) return res.ErrorCode();

NS_ENSURE

@@ +2138,5 @@
>    {
> +    ErrorResult rv;
> +    nsCOMPtr<nsINode> clone = parent->CloneNode(false, rv);
> +
> +    if (rv.Failed()) return rv.ErrorCode();

{}

@@ +2143,1 @@
>      if (!clone)         return NS_ERROR_FAILURE;

{}

@@ +2143,4 @@
>      if (!clone)         return NS_ERROR_FAILURE;
>  
>      if (! firstParent)
>        firstParent = lastParent = clone;

{}

@@ +2189,5 @@
>    // Create a new document fragment in the context of this document,
>    // which might be null
>  
>  
>    nsCOMPtr<nsIDocument> doc(do_QueryInterface(document));

Clean this up too? It can just be

nsCOMPtr<nsIDocument> doc = mStartParent->OwnerDoc();
MOZ_ASSERT(doc);

@@ +2233,5 @@
>    // correctly places this new subtree into the doc fragment.
>  
>    while (!iter.IsDone())
>    {
> +    nsCOMPtr<nsINode> iNode(iter.GetCurrentNode());

I'd call this 'node', actually. Check how much work that is?

@@ +2318,5 @@
>  
>      // Place the cloned subtree into the cloned doc frag tree!
>  
>      if (closestAncestor)
>      {

Move the braces onto the right lines here, please

@@ +2350,5 @@
>        return nullptr;
>      }
>  
>      // Get node and nextNode's common parent.
> +    commonAncestor = do_QueryInterface(nsContentUtils::GetCommonAncestor(iNode, nextNode));

I don't think you need to QI here

@@ +2364,1 @@
>      {

{ on the previous line

@@ +2364,3 @@
>      {
> +      iNode = iNode->GetParentNode();
> +      if (!iNode) {

Given that we know that commonAncestor is an ancestor of iNode, we can drop this if and make the loop condition just (iNode != commonAncestor), right?

@@ +2434,3 @@
>  
>    nsCOMPtr<nsIDOMText> startTextNode(do_QueryInterface(tStartContainer));
> +  nsCOMPtr<nsINodeList> tChildList;

You can remove this nodelist entirely; use GetChildAt and GetChildCount below.

Comment 2

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

But in general, this patch does make me happy :)

Comment 3

[Boris Zbarsky [:bzbarsky]]

I would really appreciate someone like smaug giving this a once-over too.  This code has a bunch of random non-obvious dependencies.  :(

Comment 4

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 734455 [details] [diff] [review]
Patch

else if (mIterState == eUseIterator && mIter) {
->
} else if (mIterState == eUseIterator && mIter) {

if (expr) {
  stmt;
}
in new code even if the old code uses some odd coding style occasionally

I don't think we want to change usage of mStart/EventParent in ::CutContents.
Remember, we do have still mutation events, so better to keep the old
behavior when possible.
Because of that, r-

Comment 5

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Comment on attachment 734455 [details] [diff] [review]
Patch

Perfectly happy to leave this to smaug :)

Comment 6

[David Zbarsky (:dzbarsky)]

Attachment: Patch

Let me know if you would prefer an interdiff.

Comment 7

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 737753 [details] [diff] [review]
Patch

>@@ -1893,17 +1859,17 @@ nsRange::CutContents(dom::DocumentFragme
>               nsAutoString cutValue;
>               rv = charData->SubstringData(startOffset, endOffset - startOffset,
>                                            cutValue);
>               NS_ENSURE_SUCCESS(rv, rv);
>               nsCOMPtr<nsIDOMNode> clone;
>               rv = charData->CloneNode(false, 1, getter_AddRefs(clone));
>               NS_ENSURE_SUCCESS(rv, rv);
>               clone->SetNodeValue(cutValue);
>-              nodeToResult = clone;
>+              nodeToResult = do_QueryInterface(clone);

Any reason why you don't make CloneNode to use non xpidl version?


>-nsRange::CloneParentsBetween(nsIDOMNode *aAncestor,
>-                             nsIDOMNode *aNode,
>-                             nsIDOMNode **aClosestAncestor,
>-                             nsIDOMNode **aFarthestAncestor)
>+nsRange::CloneParentsBetween(nsINode *aAncestor,
>+                             nsINode *aNode,
>+                             nsINode **aClosestAncestor,
>+                             nsINode **aFarthestAncestor)
could you move * to go with the type

>+already_AddRefed<DocumentFragment>
> nsRange::CloneContents(ErrorResult& aRv)
> {
>-  nsCOMPtr<nsIDOMNode> commonAncestor;
>-  aRv = GetCommonAncestorContainer(getter_AddRefs(commonAncestor));
>+  nsCOMPtr<nsINode> commonAncestor = GetCommonAncestorContainer(aRv);
>   MOZ_ASSERT(!aRv.Failed(), "GetCommonAncestorContainer() shouldn't fail!");
> 
>-  nsCOMPtr<nsIDOMDocument> document =
>-    do_QueryInterface(mStartParent->OwnerDoc());
>-  NS_ASSERTION(document, "CloneContents needs a document to continue.");
>-  if (!document) {
>+  nsCOMPtr<nsIDocument> doc = mStartParent->OwnerDoc();
>+  NS_ASSERTION(doc, "CloneContents needs a document to continue.");
>+  if (!doc) {
>     aRv.Throw(NS_ERROR_FAILURE);
>     return nullptr;
>   }
just remove the null check for OwnerDoc(). OwnerDoc() returns always non-null.

>   while (!iter.IsDone())
>   {
>-    nsCOMPtr<nsIDOMNode> node(iter.GetCurrentNode());
>-    nsCOMPtr<nsINode> iNode = do_QueryInterface(node);
>-    bool deepClone = !iNode->IsElement() ||
>-                       (!(iNode == mEndParent && mEndOffset == 0) &&
>-                        !(iNode == mStartParent &&
>+    nsINode* node = iter.GetCurrentNode();
I'd prefer keeping this nsCOMPtr<nsINode>

>+  static nsresult CloneParentsBetween(nsINode *aAncestor,
>+                                      nsINode *aNode,
>+                                      nsINode **aClosestAncestor,
>+                                      nsINode **aFarthestAncestor);
* with type

Comment 8

[David Zbarsky (:dzbarsky)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/da72319354d8

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/da72319354d8