760143 - Get rid of useless nsresult in editor/

Status: RESOLVED FIXED

(Core :: DOM: Editor, enhancement)

Description

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

After discussion in bug 757371 comment 8 ff., I poked around at nsresult usage in editor that could be easily removed.  There's not as much as I'd hoped, but still quite a bit.

One major category of nontrivial nsresult usage is stuff that QI's to nsINode or nsIContent and does NS_ENSURE_TRUE; that will go away as more functions are ported to use those types directly.  Likewise things use nsIDOMNode methods like GetChildNodes() that return nsresult, where the corresponding nsIContent etc. methods always succeed.

The difficult thing is when errors are being returned because the function actually can't do anything sensible.  A selection that has no ranges or that has an endpoint in a weird place (like a Document) is a very common scenario that needs to be handled.  I don't think returning nsresult is the right thing to do there, but I'm not sure what is.

I'm about to attach a patch, which removes nsresult from functions in the following cases:

* A surprising number of methods returned NS_OK unconditionally but still returned nsresult.
* Some did null pointer checks, which I changed to MOZ_ASSERT.  We seem to be okay with doing that, right?  (I'm not actually sure why.)
* nsWSRunObject::GetRuns() does a lot of NS_ENSURE_TRUE after "new WSFragment()".  I was told that that might be infallible already, and if it's not it should be soon.  Anyway, MOZ_ASSERT is certainly fine here.
* nsWSRunObject::GetChar(Before|After) did NS_ENSURE_TRUE on an array element after doing explicit bounds-checking that should make it impossible for it to be null (unless the array can actually have null pointers inserted into it?).  Changed to MOZ_ASSERT.

Sadly, this doesn't make a real dent in nsresult use.  I was hoping that once I removed nsresult from enough easy methods, their callers could stop using it too, but too make things trace back to things like QI to nsIContent that can't be removed quite this trivially.

Comment 1

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

Attachment: Patch v1

https://tbpl.mozilla.org/?tree=Try&rev=c938f6d5a7aa

Comment 2

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

Comment on attachment 628758 [details] [diff] [review]
Patch v1

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

\o/

::: editor/libeditor/base/nsEditor.cpp
@@ +4373,3 @@
>  nsEditor::GetIMEBufferLength(PRInt32* length)
>  {
>    *length = mIMEBufferLength;

Return the PRInt32 directly?

::: editor/libeditor/html/nsWSRunObject.cpp
@@ +1857,5 @@
>        }
>      }
>      run = run->mRight;
>    }
> +  return;

Drop this

::: editor/libeditor/html/nsWSRunObject.h
@@ +262,5 @@
>                           AreaRestriction aAR = eAnywhere);
> +    void     GetCharAfter(nsIDOMNode *aNode, PRInt32 aOffset, WSPoint *outPoint);
> +    void     GetCharBefore(nsIDOMNode *aNode, PRInt32 aOffset, WSPoint *outPoint);
> +    void     GetCharAfter(WSPoint &aPoint, WSPoint *outPoint);
> +    void     GetCharBefore(WSPoint &aPoint, WSPoint *outPoint);

I wonder if all those can return WSPoint

Comment 3

[(no longer active)]

Comment on attachment 628758 [details] [diff] [review]
Patch v1

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

This looks very good, thanks for doing this!  I'd like to take a look at another version of this which addresses our comments, before r+-ing it.

::: editor/libeditor/base/nsEditor.cpp
@@ +4373,3 @@
>  nsEditor::GetIMEBufferLength(PRInt32* length)
>  {
>    *length = mIMEBufferLength;

Yeah, it's best to do that.

::: editor/libeditor/html/nsHTMLEditor.cpp
@@ +5342,5 @@
>  nsHTMLEditor::GetReturnInParagraphCreatesNewParagraph(bool *aCreatesNewParagraph)
>  {
>    *aCreatesNewParagraph = mCRInParagraphCreatesParagraph;
>    return NS_OK;
>  }

You can get rid of this variant if you change the other call site for it.

::: editor/libeditor/html/nsWSRunObject.cpp
@@ +972,5 @@
>    }
>    
>    // otherwise a little trickier.  shucks.
>    mStartRun = new WSFragment();
> +  MOZ_ASSERT(mStartRun);

Please remove this pattern here and below.  operator new is infallible.

::: editor/libeditor/html/nsWSRunObject.h
@@ +273,3 @@
>      PRUnichar GetCharAt(nsIContent *aTextNode, PRInt32 aOffset);
> +    void     GetWSPointAfter(nsIDOMNode *aNode, PRInt32 aOffset, WSPoint *outPoint);
> +    void     GetWSPointBefore(nsIDOMNode *aNode, PRInt32 aOffset, WSPoint *outPoint);

What Ms2ger said above, but for the rest of these as well.

Comment 4

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

So it turns out that a) I left a stray "nsresult res;" somewhere and it caused almost all platforms to fail building (sigh), and b) my use of MOZ_ASSERT uncovered a bug in nsHTMLEditRules::StandardBreakImpl, which in some cases passes a null node to nsWSRunObject::NextVisibleNode, which will need further thought.

(In reply to Ehsan Akhgari [:ehsan] from comment #3)
> ::: editor/libeditor/html/nsHTMLEditor.cpp
> @@ +5342,5 @@
> >  nsHTMLEditor::GetReturnInParagraphCreatesNewParagraph(bool *aCreatesNewParagraph)
> >  {
> >    *aCreatesNewParagraph = mCRInParagraphCreatesParagraph;
> >    return NS_OK;
> >  }
> 
> You can get rid of this variant if you change the other call site for it.

This implements the getter for nsIHTMLEditor::returnInParagraphCreatesNewParagraph, doesn't it?

Comment 5

[(no longer active)]

(In reply to Aryeh Gregor from comment #4)
> So it turns out that a) I left a stray "nsresult res;" somewhere and it
> caused almost all platforms to fail building (sigh), and b) my use of
> MOZ_ASSERT uncovered a bug in nsHTMLEditRules::StandardBreakImpl, which in
> some cases passes a null node to nsWSRunObject::NextVisibleNode, which will
> need further thought.

Feel free to split this into multiple bugs if that makes it easier for you.

> (In reply to Ehsan Akhgari [:ehsan] from comment #3)
> > ::: editor/libeditor/html/nsHTMLEditor.cpp
> > @@ +5342,5 @@
> > >  nsHTMLEditor::GetReturnInParagraphCreatesNewParagraph(bool *aCreatesNewParagraph)
> > >  {
> > >    *aCreatesNewParagraph = mCRInParagraphCreatesParagraph;
> > >    return NS_OK;
> > >  }
> > 
> > You can get rid of this variant if you change the other call site for it.
> 
> This implements the getter for
> nsIHTMLEditor::returnInParagraphCreatesNewParagraph, doesn't it?

Ah, right, freaking xpconnect... :(

Comment 6

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

(In reply to :Ms2ger from comment #2)
> ::: editor/libeditor/html/nsWSRunObject.h
> @@ +262,5 @@
> >                           AreaRestriction aAR = eAnywhere);
> > +    void     GetCharAfter(nsIDOMNode *aNode, PRInt32 aOffset, WSPoint *outPoint);
> > +    void     GetCharBefore(nsIDOMNode *aNode, PRInt32 aOffset, WSPoint *outPoint);
> > +    void     GetCharAfter(WSPoint &aPoint, WSPoint *outPoint);
> > +    void     GetCharBefore(WSPoint &aPoint, WSPoint *outPoint);
> 
> I wonder if all those can return WSPoint

What's the proper incantation to allocate the WSPoint that I want to return?  Everything else in the file seems to just allocate directly on the stack, which clearly I can't do if I want to return it.  My C++ is not very good -- I mostly just cargo-cult things like nsCOMPtr/nsRefPtr/etc., but I realize those won't work here.

Comment 7

[(no longer active)]

(In reply to Aryeh Gregor from comment #6)
> (In reply to :Ms2ger from comment #2)
> > ::: editor/libeditor/html/nsWSRunObject.h
> > @@ +262,5 @@
> > >                           AreaRestriction aAR = eAnywhere);
> > > +    void     GetCharAfter(nsIDOMNode *aNode, PRInt32 aOffset, WSPoint *outPoint);
> > > +    void     GetCharBefore(nsIDOMNode *aNode, PRInt32 aOffset, WSPoint *outPoint);
> > > +    void     GetCharAfter(WSPoint &aPoint, WSPoint *outPoint);
> > > +    void     GetCharBefore(WSPoint &aPoint, WSPoint *outPoint);
> > 
> > I wonder if all those can return WSPoint
> 
> What's the proper incantation to allocate the WSPoint that I want to return?
> Everything else in the file seems to just allocate directly on the stack,
> which clearly I can't do if I want to return it.  My C++ is not very good --
> I mostly just cargo-cult things like nsCOMPtr/nsRefPtr/etc., but I realize
> those won't work here.

First add a copy ctor to WSPoint like this:

WSPoint(const WSPoint& rhs) :
  mTextNode(rhs.mTextNode),
  mOffset(rhs.mOffset),
  mChar(rhs.mChar)
{}

Then you should be able to do:

WSPoint foobar() {
  WSPoint pt;
  // fill in pt
  return pt;
}

And the caller would do:

  WSPoint result = foobar();

Comment 8

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

Attachment: Patch v2

I didn't make anything return WSPoint in this patch version -- when I tried, I got

/mnt/extra/checkouts/mozilla-central/editor/libeditor/html/nsWSRunObject.cpp:1559:1: error: ‘WSPoint’
 does not name a type
/mnt/extra/checkouts/mozilla-central/editor/libeditor/html/nsWSRunObject.cpp:1578:1: error: ‘WSPoint’
 does not name a type
/mnt/extra/checkouts/mozilla-central/editor/libeditor/html/nsWSRunObject.cpp:1597:1: error: ‘WSPoint’
 does not name a type
/mnt/extra/checkouts/mozilla-central/editor/libeditor/html/nsWSRunObject.cpp:1634:1: error: ‘WSPoint’ does not name a type
/mnt/extra/checkouts/mozilla-central/editor/libeditor/html/nsWSRunObject.cpp:1859:1: error: ‘WSPoint’ does not name a type
/mnt/extra/checkouts/mozilla-central/editor/libeditor/html/nsWSRunObject.cpp:1913:1: error: ‘WSPoint’ does not name a type

The errors are the return types for the functions in nsWSRunObject.cpp, like

  WSPoint <--
  nsWSRunObject::GetCharAfter(WSPoint &aPoint)
  {

If you tell me how to fix that, I can retry.  The code sure looks a lot neater!

I fixed the StandardBreakImpl issue by adding NS_ENSURE_TRUE so it would bail out if the relevant variable is null.  This might cause it to return an error in an additional code path.  (GetNodeLocation returns NS_OK if the node passed in has no parent -- why do we bother with error codes if they aren't returned on errors?)

Try: https://tbpl.mozilla.org/?tree=Try&rev=75532d2f4e42

Comment 9

[(no longer active)]

Comment on attachment 629162 [details] [diff] [review]
Patch v2

(In reply to Aryeh Gregor from comment #8)
> Created attachment 629162 [details] [diff] [review]
> Patch v2
> 
> I didn't make anything return WSPoint in this patch version -- when I tried,
> I got
> 
> /mnt/extra/checkouts/mozilla-central/editor/libeditor/html/nsWSRunObject.cpp:
> 1559:1: error: ‘WSPoint’
>  does not name a type
> /mnt/extra/checkouts/mozilla-central/editor/libeditor/html/nsWSRunObject.cpp:
> 1578:1: error: ‘WSPoint’
>  does not name a type
> /mnt/extra/checkouts/mozilla-central/editor/libeditor/html/nsWSRunObject.cpp:
> 1597:1: error: ‘WSPoint’
>  does not name a type
> /mnt/extra/checkouts/mozilla-central/editor/libeditor/html/nsWSRunObject.cpp:
> 1634:1: error: ‘WSPoint’ does not name a type
> /mnt/extra/checkouts/mozilla-central/editor/libeditor/html/nsWSRunObject.cpp:
> 1859:1: error: ‘WSPoint’ does not name a type
> /mnt/extra/checkouts/mozilla-central/editor/libeditor/html/nsWSRunObject.cpp:
> 1913:1: error: ‘WSPoint’ does not name a type
> 
> The errors are the return types for the functions in nsWSRunObject.cpp, like
> 
>   WSPoint <--
>   nsWSRunObject::GetCharAfter(WSPoint &aPoint)
>   {
> 
> If you tell me how to fix that, I can retry.  The code sure looks a lot
> neater!

WSPoint is a nested class in nsWSRunObject, so you should say:

nsWSRunObject::WSPoint
nsWSRunObject::GetCharAfter(nsWSRunObject::WSPoint &aPoint) ...

(yeah, I know, C++... ;-)

Clearing the request since you volunteered to handle the WSPoint stuff as well.

> I fixed the StandardBreakImpl issue by adding NS_ENSURE_TRUE so it would
> bail out if the relevant variable is null.  This might cause it to return an
> error in an additional code path.  (GetNodeLocation returns NS_OK if the
> node passed in has no parent -- why do we bother with error codes if they
> aren't returned on errors?)

Beware of assuming any sanity when dealing with editor code.  There usually is none!

Comment 10

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

Attachment: Patch v3, changes return values to WSPoint

https://tbpl.mozilla.org/?tree=Try&rev=b5aa66eb4a40

It seems I didn't need to define a copy constructor at all -- the default one suffices.

(In reply to Ehsan Akhgari [:ehsan] from comment #9)
> WSPoint is a nested class in nsWSRunObject, so you should say:
> 
> nsWSRunObject::WSPoint
> nsWSRunObject::GetCharAfter(nsWSRunObject::WSPoint &aPoint) ...
> 
> (yeah, I know, C++... ;-)

This works:

  nsWSRunObject::WSPoint
  nsWSRunObject::GetCharAfter(WSPoint &aPoint)
  {

So the parameter types are scoped to the class, but the return type is not.  C++ is fun!

> Beware of assuming any sanity when dealing with editor code.  There usually
> is none!

Yeah, I know.  :/

Comment 11

[(no longer active)]

Comment on attachment 629588 [details] [diff] [review]
Patch v3, changes return values to WSPoint

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

::: editor/libeditor/html/nsWSRunObject.cpp
@@ +1083,4 @@
>  nsWSRunObject::MakeSingleWSRun(PRInt16 aType)
>  {
>    mStartRun = new WSFragment();
> +  MOZ_ASSERT(mStartRun);

Please remove this assertion.  It's not needed.

@@ +1809,1 @@
>        }

Can you please move these return statements to here?

@@ +1829,1 @@
>        }

Same here.

@@ +1842,1 @@
>        }

And here.

Comment 12

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/577a88fc97b6

Comment 13

[Geoff Lankow (:darktrojan)]

https://hg.mozilla.org/mozilla-central/rev/577a88fc97b6