779091 - Fix incorrect nsresult usage in accessible/

Status: RESOLVED FIXED

(Core :: Disability Access APIs, enhancement)

Description

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

Split off from bug 777292, which was getting too crowded.

Comment 1

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

Attachment: Patch -- Make Accessible::Init() infallible

There are essentially no ways that Init() can fail in any subclass I can see, so it may as well just return void.  The only ways that any subclass' Init() can fail, other than if its parent's implementation fails, are:

* DocAccessible::Init fails if "new NotificationController(this, mPresShell)" returns null, but operator new is infallible in Gecko, so this can never happen.
* XULTreeGridCellAccessible::Init fails if mTreeView is null.  In this patch, I make it an NS_ASSERTION instead.  Is that okay, or will it break things?  It seems like a shame to make Init() fallible just for the sake of this one possibility.

Making things infallible tends to make code cleaner, of course.  In this case, it could be made cleaner still by merging Init() into the constructor.

However, if you don't like my approach here, I can just change nsAccessNode.cpp to not incorrectly call NS_FAILED on the result of Init, as you requested in bug 777292 comment 87.  I didn't do that in my original patch because I didn't want to change the meaning of the code.  I'm fine with anything you like as long as it doesn't involve assigning a bool to nsresult.  :)

Comment 2

[Trevor Saunders (:tbsaunde)]

(In reply to :Aryeh Gregor from comment #1)
> Created attachment 647498 [details] [diff] [review]
> Patch -- Make Accessible::Init() infallible
> 
> There are essentially no ways that Init() can fail in any subclass I can
> see, so it may as well just return void.  The only ways that any subclass'
> Init() can fail, other than if its parent's implementation fails, are:
> 
> * DocAccessible::Init fails if "new NotificationController(this,
> mPresShell)" returns null, but operator new is infallible in Gecko, so this
> can never happen.
> * XULTreeGridCellAccessible::Init fails if mTreeView is null.  In this
> patch, I make it an NS_ASSERTION instead.  Is that okay, or will it break
> things?  It seems like a shame to make Init() fallible just for the sake of
> this one possibility.

I hope it can't happen, but off hand I think it might just  be possible.

> Making things infallible tends to make code cleaner, of course.  In this
> case, it could be made cleaner still by merging Init() into the constructor.

So, I haven't read the patch yet.  Given that you've looked I suspect its proably true we don't really need a falable Init().

However I'm sort of tempted to be conservative here and leave Init() falable and over time move things to the constructor, I'm not sure off hand what the reasons for the current archetecture are.  On the other hand it seems a shame to not make things infalable when you've already checked it can be done.

Comment 3

[alexander :surkov (:asurkov)]

Comment on attachment 647498 [details] [diff] [review]
Patch -- Make Accessible::Init() infallible

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

nice, thank you

::: accessible/src/xul/XULTreeGridAccessible.cpp
@@ +784,5 @@
> +  LeafAccessible::Init();
> +
> +  NS_ASSERTION(mTreeView, "mTreeView is null");
> +  if (!mTreeView)
> +	return;

it shouldn't ever happen, you can keep an assertion (just in case if somebody writes wrong code) but you don't need this return. In either way indentation is wrong.

Comment 4

[alexander :surkov (:asurkov)]

(In reply to Trevor Saunders (:tbsaunde) from comment #2)

> > * XULTreeGridCellAccessible::Init fails if mTreeView is null.  In this
> > patch, I make it an NS_ASSERTION instead.  Is that okay, or will it break
> > things?  It seems like a shame to make Init() fallible just for the sake of
> > this one possibility.
> 
> I hope it can't happen, but off hand I think it might just  be possible.

we shouldn't create (initialize) a cell when there's no cells. Any code path?

> > Making things infallible tends to make code cleaner, of course.  In this
> > case, it could be made cleaner still by merging Init() into the constructor.

it makes sense, the idea of Init() was in error return value.

Comment 5

[Trevor Saunders (:tbsaunde)]

(In reply to alexander :surkov from comment #4)
> (In reply to Trevor Saunders (:tbsaunde) from comment #2)
> 
> > > * XULTreeGridCellAccessible::Init fails if mTreeView is null.  In this
> > > patch, I make it an NS_ASSERTION instead.  Is that okay, or will it break
> > > things?  It seems like a shame to make Init() fallible just for the sake of
> > > this one possibility.
> > 
> > I hope it can't happen, but off hand I think it might just  be possible.
> 
> we shouldn't create (initialize) a cell when there's no cells. Any code path?

I think I was thinking about creating the tree object, not a cell.  So ignore me (atleast until I learn to read ;)

Comment 6

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

(In reply to alexander :surkov from comment #3)
> it shouldn't ever happen, you can keep an assertion (just in case if
> somebody writes wrong code) but you don't need this return.

Probably no need for the assertion then, because with no return it will crash a few lines later.  But I'll leave it anyway.

> In either way indentation is wrong.

Bah, recently vim started trying to use tabs for indent in some files . . .

I forgot to push to try, so I'll do that now, just in case mTreeView is ever null on some path hit by tests: https://tbpl.mozilla.org/?tree=Try&rev=a792a0bb9898

Comment 7

[alexander :surkov (:asurkov)]

would you mind please to file follow up to merge Init() and constructor?

Comment 8

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/865a6f2d3a83

(In reply to alexander :surkov from comment #7)
> would you mind please to file follow up to merge Init() and constructor?

Bug 779520.

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/865a6f2d3a83