If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

Remove some COM usage in selection code

RESOLVED FIXED

Status

()

Core
Selection
RESOLVED FIXED
9 years ago
8 years ago

People

(Reporter: bz, Assigned: bz)

Tracking

Trunk
x86
Mac OS X
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments)

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.
Created attachment 370775 [details] [diff] [review]
Proposed fix
Attachment #370775 - Flags: superreview?(Olli.Pettay)
Attachment #370775 - Flags: review?(Olli.Pettay)
Created attachment 370777 [details]
Output of hg out -p; might be easier to review

smaug, I could also post the 7 individial changeset diffs if you prefer.
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.

Updated

9 years ago
Attachment #370775 - Flags: superreview?(Olli.Pettay)
Attachment #370775 - Flags: superreview+
Attachment #370775 - Flags: review?(Olli.Pettay)
Attachment #370775 - Flags: review+

Comment 4

9 years ago
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
> 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.
> Nit, extra space after =

Fixed.

> I'd prefer ?: expression

OK.
Created attachment 371262 [details] [diff] [review]
Updated to comments
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
Status: NEW → RESOLVED
Last Resolved: 9 years ago
Flags: in-testsuite-
Resolution: --- → FIXED
And http://hg.mozilla.org/mozilla-central/rev/6db145f8b14d to fix Windows build bustage.
This cut about 5KB off Z.
Depends on: 488417

Updated

8 years ago
Depends on: 520070
You need to log in before you can comment on or make changes to this bug.