486547 - Remove some COM usage in selection code

Status: RESOLVED FIXED

(Core :: DOM: Selection, defect)

Description

[Boris Zbarsky [:bzbarsky]]

nsTypedSelection uses nsIDOMRange and nsIDOMNode all over.  I'd like to switch it over to nsIRange and nsINode as possible, to avoid the addref/release and QI costs, not to mention the virtual function costs, etc.

Comment 1

[Boris Zbarsky [:bzbarsky]]

Attachment: Proposed fix

Comment 2

[Boris Zbarsky [:bzbarsky]]

Attachment: Output of hg out -p; might be easier to review

smaug, I could also post the 7 individial changeset diffs if you prefer.

Comment 3

[Boris Zbarsky [:bzbarsky]]

Note: in patch 4, the AddRange(nsIDOMRange*) impl is wrong: it should pass in |range|, not |aRange| to the AddRange() call it makes.  Fixed locally.

Comment 4

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

Comment on attachment 370775 [details] [diff] [review]
Proposed fix

>   virtual nsresult Init(nsIDOMRange* aRange) = 0;
>+  virtual nsresult Init(nsIRange* aRange) = 0;
Not sure if I like adding new Init, but perhaps it is easier for the callers
to keep the nIDOMRange* version and not just replace that with nsIRange*.
So, this change is ok to me.

>+nsresult nsRange::CloneRange(nsIRange** aReturn) const
>+{
>+  nsRange* clone;
>+  nsresult rv = DoCloneRange(&clone);
>+  if (NS_SUCCEEDED(rv)) {
>+    *aReturn = clone;
>+  }
>+  return rv;
>+}
Could DoCloneRange return nsIRange as its out param?
Then this could be
nsresult nsRange::CloneRange(nsIRange** aReturn) { return DoCloneRange(aReturn); }

>+  limiter = aFrameSel->GetAncestorLimiter();
>+  if (limiter)
>+  {
>+    return nsContentUtils::ContentIsDescendantOf(aNode, limiter);
>+  }
>+
>   return PR_TRUE;
> }
Maybe
  return limiter ?
    nsContentUtils::ContentIsDescendantOf(aNode, limiter) : PR_TRUE;

>+  nsTypedSelection* sel = mDomSelections[index];
Actually, might be worth to check if sel should be nsRefPtr.
Depends on what ScrollIntoView may do.

>-      nsCOMPtr<nsIDOMRange> newRange;
>-      NS_NewRange(getter_AddRefs(newRange));
>-
>-      newRange->SetStart(domNode,aContentOffset);
>-      newRange->SetEnd(domNode,aContentOffset);
>+      nsCOMPtr<nsIRange> newRange = new nsRange();
>+
Could you add OOM check here.

>+GetFirstSelectedContent(nsIRange* aRange)
>+{
>+  if (!aRange) {
>+    return nsnull;
>+  }
>+
>+  NS_PRECONDITION(aRange->GetStartParent(), "Must have start paren!");
Nit, s/paren/parent/

>-          range->GetStartOffset(&offset);
>+          PRInt32 offset =  range->StartOffset();
Nit, extra space after =

>+  if (tableNode1 == tableNode2)
>+  {
>+    // Want false for the test if both are null
>+    return tableNode1;
>+  }
>+  return nsnull;
>+}
I'd prefer ?: expression

>+  // XXXbz why bother with the eELEMENT check above?
>+  if (!startNode->IsNodeOfType(nsINode::eHTML)) {
>+    return NS_OK;
>+  }
Could you remove the eELEMENT check.

>   // Get an iterator
>   nsSelectionIterator iter(mDomSelections[index]);
>   res = iter.First();
>   if (NS_FAILED(res))
>     return res;
> 
>-  nsCOMPtr<nsIDOMRange> range;
>   while (iter.IsDone())
>   {
>-    res = iter.CurrentItem(static_cast<nsIDOMRange**>(getter_AddRefs(range)));
>-    if (NS_FAILED(res))
>-      return res;
>-    res = range->DeleteContents();
>+    res = iter.CurrentItem()->DeleteContents();
Does nsSelectionIterator keep the range alive while DeleteContents is called?
Or is it ok to call DeleteContents without owning reference?

>-  PRUint16 endNodeType = nsIDOMNode::ELEMENT_NODE;
>-  endNode->GetNodeType(&endNodeType);
>-  if (endNodeType == nsIDOMNode::TEXT_NODE) {
>+  if (endNode->IsNodeOfType(nsINode::eDATA_NODE)) {
Why eDATA_NODE and not eTEXT?

With comments addressed, r+sr

Comment 5

[Boris Zbarsky [:bzbarsky]]

> perhaps it is easier for the callers to keep the nIDOMRange* version and not
> just replace that with nsIRange*

Yes, and easier for some of the implementors of this interface too.  Longer term we should switch it all over to nsIRange, I think.

> Could DoCloneRange return nsIRange as its out param?

Yes.  I wrote that code before changing the inheritance.  Good catch.

>  return limiter ?
>    nsContentUtils::ContentIsDescendantOf(aNode, limiter) : PR_TRUE;

I made this:

  return !limiter || nsContentUtils::ContentIsDescendantOf(aNode, limiter);

> Depends on what ScrollIntoView may do.

We're not worse off than we were before... in any case, I made this nsRefPtr just to be safe.

> Could you add OOM check here.

Done.

> Nit, s/paren/parent/

Fixed.

> Could you remove the eELEMENT check.

Done.

> Does nsSelectionIterator keep the range alive while DeleteContents is called?

No, and no.  Good catch.  I've made this use a strong ref.

> Why eDATA_NODE and not eTEXT?

I suppose I can do the latter.  All this code assumes it's not going to be dealing with non-text data nodes in any case.

Comment 6

[Boris Zbarsky [:bzbarsky]]

> Nit, extra space after =

Fixed.

> I'd prefer ?: expression

OK.

Comment 7

[Boris Zbarsky [:bzbarsky]]

Attachment: Updated to comments

Comment 8

[Boris Zbarsky [:bzbarsky]]

Pushed:
http://hg.mozilla.org/mozilla-central/rev/00051494c0f8
http://hg.mozilla.org/mozilla-central/rev/315fe9967fd3
http://hg.mozilla.org/mozilla-central/rev/4031f4e5a4fa
http://hg.mozilla.org/mozilla-central/rev/5506e9239176
http://hg.mozilla.org/mozilla-central/rev/a10ff1269f64
http://hg.mozilla.org/mozilla-central/rev/0741badbaac9
http://hg.mozilla.org/mozilla-central/rev/eb2161c1fc55

Comment 9

[Boris Zbarsky [:bzbarsky]]

And http://hg.mozilla.org/mozilla-central/rev/6db145f8b14d to fix Windows build bustage.

Comment 10

[Boris Zbarsky [:bzbarsky]]

This cut about 5KB off Z.