Last Comment Bug 779091 - Fix incorrect nsresult usage in accessible/
: Fix incorrect nsresult usage in accessible/
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Disability Access APIs (show other bugs)
: Trunk
: All All
: -- enhancement (vote)
: mozilla17
Assigned To: :Aryeh Gregor (away until August 15)
:
Mentors:
Depends on: 779520
Blocks: cleana11y nsresult-enum
  Show dependency treegraph
 
Reported: 2012-07-31 04:59 PDT by :Aryeh Gregor (away until August 15)
Modified: 2012-08-02 19:10 PDT (History)
2 users (show)
ayg: in‑testsuite-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch -- Make Accessible::Init() infallible (12.44 KB, patch)
2012-07-31 05:12 PDT, :Aryeh Gregor (away until August 15)
surkov.alexander: review+
Details | Diff | Review

Description :Aryeh Gregor (away until August 15) 2012-07-31 04:59:15 PDT
Split off from bug 777292, which was getting too crowded.
Comment 1 :Aryeh Gregor (away until August 15) 2012-07-31 05:12:02 PDT
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.

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) 2012-07-31 19:29:51 PDT
(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 2012-07-31 20:20:31 PDT
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 2012-07-31 20:22:53 PDT
(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) 2012-07-31 20:39:40 PDT
(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 (away until August 15) 2012-08-01 02:22:29 PDT
(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 2012-08-01 05:11:17 PDT
would you mind please to file follow up to merge Init() and constructor?
Comment 8 :Aryeh Gregor (away until August 15) 2012-08-02 02:32:04 PDT
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] 2012-08-02 19:10:21 PDT
https://hg.mozilla.org/mozilla-central/rev/865a6f2d3a83

Note You need to log in before you can comment on or make changes to this bug.