Last Comment Bug 418025 - IA2 - case by case analysis of exception return values
: IA2 - case by case analysis of exception return values
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Disability Access APIs (show other bugs)
: unspecified
: x86 Windows XP
: -- normal (vote)
: ---
Assigned To: alexander :surkov
:
: alexander :surkov
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2008-02-16 18:49 PST by Pete Brunet
Modified: 2008-03-29 20:25 PDT (History)
2 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
wip (16.75 KB, patch)
2008-03-11 04:13 PDT, alexander :surkov
no flags Details | Diff | Splinter Review
patch (94.77 KB, patch)
2008-03-16 02:49 PDT, alexander :surkov
no flags Details | Diff | Splinter Review
patch2 (93.32 KB, patch)
2008-03-19 23:20 PDT, alexander :surkov
aaronlev: review+
mbeltzner: approval1.9+
Details | Diff | Splinter Review

Description Pete Brunet 2008-02-16 18:49:08 PST
User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.1.12) Gecko/20080201 Firefox/2.0.0.12
Build Identifier: 


	
I went through all the IA2 methods looking at what the exceptions should be so I can add this info to the IA2 docs.  Please let me know if you see any errors.

IAccessible2
HRESULT  nRelations ([out, retval] long *nRelations)
 no exception
HRESULT  relation ([in] long relationIndex,[out, retval] IAccessibleRelation **relation)
 E_INVALIDARG if bad [in] passed, [out] value is NULL
HRESULT  relations ([in] long maxRelations,[out, size_is(maxRelations), length_is(*nRelations)] IAccessibleRelation **relation,[out, retval] long *nRelations)
  S_FALSE returned if there are no relations, [out] values are NULL and 0 respectively
HRESULT  role ([out, retval] long *role)
  no exception
HRESULT  scrollTo ([in] enum IA2ScrollType scrollType)
 E_INVALIDARG if bad [in] passed
HRESULT  scrollToPoint ([in] enum IA2CoordinateType coordinateType,[in] long x,[in] long y)
 E_INVALIDARG if bad [in] passed
HRESULT  groupPosition ([out] long *groupLevel,[out] long *similarItemsInGroup,[out, retval] long *positionInGroup)
  0 returned for any [out] that is NA, S_OK returned if at least one value is valid, S_FALSE is no values are valid
HRESULT  states ([out, retval] AccessibleStates *states)
 no exception
HRESULT  extendedRole ([out, retval] BSTR *extendedRole)
  S_FALSE returned if there is nothing to return, [out] value is NULL
HRESULT  localizedExtendedRole ([out, retval] BSTR *localizedExtendedRole)
  S_FALSE returned if there is nothing to return, [out] value is NULL
HRESULT  nExtendedStates ([out, retval] long *nExtendedStates)
 no exception
HRESULT  extendedStates ([in] long maxExtendedStates,[out, size_is(, maxExtendedStates), length_is(,*nExtendedStates)] BSTR **extendedStates,[out, retval] long *nExtendedStates)
  S_FALSE returned if there are no relations, [out] values are NULL and 0 respectively
HRESULT  localizedExtendedStates ([in] long maxLocalizedExtendedStates,[out, size_is(, maxLocalizedExtendedStates), length_is(,*nLocalizedExtendedStates)] BSTR **localizedExtendedStates,[out, retval] long *nLocalizedExtendedStates)
  S_FALSE returned if there are no relations, [out] values are NULL and 0 respectively
HRESULT  uniqueID ([out, retval] long *uniqueID)
  no exception
HRESULT  windowHandle ([out, retval] HWND *windowHandle)
 no exception
HRESULT  indexInParent ([out, retval] long *indexInParent)
  S_FALSE returned if no parent, [out] value is -1
HRESULT  locale ([out, retval] IA2Locale *locale)
 no exception
HRESULT  attributes ([out, retval] BSTR *attributes)
  S_FALSE returned if there is nothing to return, [out] value is NULL

IAAction
HRESULT  nActions ([out, retval] long *nActions)
  no exception
HRESULT  doAction ([in] long actionIndex)
  E_INVALIDARG if bad [in] passed
HRESULT  description ([in] long actionIndex,[out, retval] BSTR *description)
  E_INVALIDARG if bad [in] passed, [out] value is NULL
 S_FALSE returned if there is nothing to return, [out] value is NULL
HRESULT  keyBinding ([in] long actionIndex,[in] long nMaxBinding,[out, size_is(, nMaxBinding), length_is(,*nBinding)] BSTR **keyBinding,[out, retval] long *nBinding)
  E_INVALIDARG if bad [in] passed, [out] values are NULL and 0 respectively
  S_FALSE returned if there are no relations, [out] values are NULL and 0 respectively
HRESULT  name ([in] long actionIndex,[out, retval] BSTR *name)
  E_INVALIDARG if bad [in] passed, [out] value is NULL
 S_FALSE returned if there is nothing to return, [out] value is NULL
HRESULT  localizedName ([in] long actionIndex,[out, retval] BSTR *localizedName)
  E_INVALIDARG if bad [in] passed, [out] value is NULL
 S_FALSE returned if there is nothing to return, [out] value is NULL

IAApplication
HRESULT  appName ([out, retval] BSTR *name)
 S_FALSE returned if there is nothing to return, [out] value is NULL
HRESULT  appVersion ([out, retval] BSTR *version)
 S_FALSE returned if there is nothing to return, [out] value is NULL
HRESULT  toolkitName ([out, retval] BSTR *name)
 S_FALSE returned if there is nothing to return, [out] value is NULL
HRESULT  toolkitVersion ([out, retval] BSTR *version)
 S_FALSE returned if there is nothing to return, [out] value is NULL

IAComponent
HRESULT  locationInParent ([out] long *x,[out, retval] long *y)
 no exception
HRESULT  foreground ([out, retval] IA2Color *foreground)
 no exception
HRESULT  background ([out, retval] IA2Color *background)
 no exception

IAHypertext
HRESULT  nHyperlinks ([out, retval] long *hyperlinkCount)
 no exception
HRESULT  hyperlink ([in] long index,[out, retval] IAccessibleHyperlink **hyperlink)
  E_INVALIDARG if bad [in] passed, [out] value is NULL
HRESULT  hyperlinkIndex ([in] long charIndex,[out, retval] long *hyperlinkIndex)
  E_INVALIDARG if bad [in] passed, [out] value is 0
 S_FALSE returned if there is nothing to return, [out] value is 0

IAHyperlink
HRESULT  anchor ([in] long index,[out, retval] VARIANT *anchor)
  E_INVALIDARG if bad [in] passed, [out] value is NULL
 S_FALSE returned if there is nothing to return, [out] value is NULL
HRESULT  anchorTarget ([in] long index,[out, retval] VARIANT *anchorTarget)
  E_INVALIDARG if bad [in] passed, [out] value is NULL
 S_FALSE returned if there is nothing to return, [out] value is NULL
HRESULT  startIndex ([out, retval] long *index)
 no exception
HRESULT  endIndex ([out, retval] long *index)
 no exception
HRESULT  valid ([out, retval] boolean *valid)
 S_FALSE returned if there is nothing to return, [out] value is false

IAImage
HRESULT  description ([out, retval] BSTR *description)
 S_FALSE returned if there is nothing to return, [out] value is NULL
HRESULT  imagePosition ([in] enum IA2CoordinateType coordinateType,[out] long *x,[out, retval] long *y)
 no exception
HRESULT  imageSize ([out] long *height,[out, retval] long *width)
 no exception

IARelation
HRESULT  relationType ([out, retval] BSTR *relationType)
 no exception
HRESULT  localizedRelationType ([out, retval] BSTR *localizedRelationType)
 no exception
HRESULT  nTargets ([out, retval] long *nTargets)
 no exception
HRESULT  target ([in] long targetIndex,[out, retval] IUnknown **target)
  E_INVALIDARG if bad [in] passed, [out] value is NULL
 S_FALSE returned if there is nothing to return, [out] value is NULL
HRESULT  targets ([in] long maxTargets,[out, size_is(maxTargets), length_is(*nTargets)] IUnknown **target,[out, retval] long *nTargets)
 S_FALSE returned if there is nothing to return, [out] values are NULL and 0 respectively

IATable
HRESULT  accessibleAt ([in] long row,[in] long column,[out, retval] IUnknown **accessible)
  E_INVALIDARG if bad [in] passed, [out] value is NULL
HRESULT  caption ([out, retval] IUnknown **accessible)
 S_FALSE returned if there is nothing to return, [out] value is NULL
HRESULT  childIndex ([in] long rowIndex,[in] long columnIndex,[out, retval] long *childIndex)
  E_INVALIDARG if bad [in] passed, [out] value is 0
HRESULT  columnDescription ([in] long column,[out, retval] BSTR *description)
  E_INVALIDARG if bad [in] passed, [out] value is NULL
 S_FALSE returned if there is nothing to return, [out] value is NULL
HRESULT  columnExtentAt ([in] long row,[in] long column,[out, retval] long *nColumnsSpanned)
  E_INVALIDARG if bad [in] passed, [out] value is 0
HRESULT  columnHeader ([out] IAccessibleTable **accessibleTable,[out, retval] long *startingRowIndex)
  S_FALSE returned if there is no header, [out] values are NULL and 0 respectively
HRESULT  columnIndex ([in] long childIndex,[out, retval] long *columnIndex)
  E_INVALIDARG if bad [in] passed, [out] value is 0
HRESULT  nColumns ([out, retval] long *columnCount)
 no exception
HRESULT  nRows ([out, retval] long *rowCount)
 no exception
HRESULT  nSelectedChildren ([out, retval] long *childCount)
 no exception
HRESULT  nSelectedColumns ([out, retval] long *columnCount)
 no exception
HRESULT  nSelectedRows ([out, retval] long *rowCount)
 no exception
HRESULT  rowDescription ([in] long row,[out, retval] BSTR *description)
  E_INVALIDARG if bad [in] passed, [out] value is NULL
 S_FALSE returned if there is nothing to return, [out] value is NULL
HRESULT  rowExtentAt ([in] long row,[in] long column,[out, retval] long *nRowsSpanned)
  E_INVALIDARG if bad [in] passed, [out] value is 0
HRESULT  rowHeader ([out] IAccessibleTable **accessibleTable,[out, retval] long *startingColumnIndex)
  S_FALSE returned if there is no header, [out] values are NULL and 0 respectively
HRESULT  rowIndex ([in] long childIndex,[out, retval] long *rowIndex)
  E_INVALIDARG if bad [in] passed, [out] value is 0
HRESULT  selectedChildren ([in] long maxChildren,[out, size_is(, maxChildren), length_is(,*nChildren)] long **children,[out, retval] long *nChildren)
  S_FALSE returned if there are none, [out] values are NULL and 0 respectively
HRESULT  selectedColumns ([in] long maxColumns,[out, size_is(, maxColumns), length_is(,*nColumns)] long **columns,[out, retval] long *nColumns)
  S_FALSE returned if there are none, [out] values are NULL and 0 respectively
HRESULT  selectedRows ([in] long maxRows,[out, size_is(, maxRows), length_is(,*nRows)] long **rows,[out, retval] long *nRows)
  S_FALSE returned if there are none, [out] values are NULL and 0 respectively
HRESULT  summary ([out, retval] IUnknown **accessible)
 S_FALSE returned if there is nothing to return, [out] value is NULL
HRESULT  isColumnSelected ([in] long column,[out, retval] boolean *isSelected)
  E_INVALIDARG if bad [in] passed, [out] value is false
HRESULT  isRowSelected ([in] long row,[out, retval] boolean *isSelected)
  E_INVALIDARG if bad [in] passed, [out] value is false
HRESULT  isSelected ([in] long row,[in] long column,[out, retval] boolean *isSelected)
  E_INVALIDARG if bad [in] passed, [out] value is false
HRESULT  selectRow ([in] long row)
  E_INVALIDARG if bad [in] passed
HRESULT  selectColumn ([in] long column)
  E_INVALIDARG if bad [in] passed
HRESULT  unselectRow ([in] long row)
  E_INVALIDARG if bad [in] passed
HRESULT  unselectColumn ([in] long column)
  E_INVALIDARG if bad [in] passed
HRESULT  rowColumnExtentsAtIndex ([in] long index,[out] long *row,[out] long *column,[out] long *rowExtents,[out] long *columnExtents,[out, retval] boolean *isSelected)
  E_INVALIDARG if bad [in] passed, [out] values are 0s and false respectively
HRESULT  modelChange ([out, retval] IA2TableModelChange *modelChange)
 S_FALSE returned if there is nothing to return, [out] value is NULL

IAText
HRESULT  addSelection ([in] long startOffset,[in] long endOffset)
  E_INVALIDARG if bad [in] passed
HRESULT  attributes ([in] long offset,[out] long *startOffset,[out] long *endOffset,[out, retval] BSTR *textAttributes)
  E_INVALIDARG if bad [in] passed, [out] values are 0s and NULL respectively
 S_FALSE returned if there is nothing to return, [out] values are 0s and NULL respectively
HRESULT  caretOffset ([out, retval] long *offset)
  no exception
HRESULT  characterExtents ([in] long offset,[in] enum IA2CoordinateType coordType,[out] long *x,[out] long *y,[out] long *width,[out, retval] long *height)
  E_INVALIDARG if bad [in] passed, [out] values are 0s
HRESULT  nSelections ([out, retval] long *nSelections)
  no exception
HRESULT  offsetAtPoint ([in] long x,[in] long y,[in] enum IA2CoordinateType coordType,[out, retval] long *offset)
  E_INVALIDARG if bad [in] passed, [out] values is 0
HRESULT  selection ([in] long selectionIndex,[out] long *startOffset,[out, retval] long *endOffset)
  E_INVALIDARG if bad [in] passed, [out] values are 0s
 S_FALSE returned if there is nothing to return, [out] values are 0s
HRESULT  text ([in] long startOffset,[in] long endOffset,[out, retval] BSTR *text)
  E_INVALIDARG if bad [in] passed, [out] values is NULL
HRESULT  textBeforeOffset ([in] long offset,[in] enum IA2TextBoundaryType boundaryType,[out] long *startOffset,[out] long *endOffset,[out, retval] BSTR *text)
  E_INVALIDARG if bad [in] passed, [out] values are 0s and NULL respectively
 S_FALSE returned if there is nothing to return, [out] values are 0s and NULL respectively
HRESULT  textAfterOffset ([in] long offset,[in] enum IA2TextBoundaryType boundaryType,[out] long *startOffset,[out] long *endOffset,[out, retval] BSTR *text)
  E_INVALIDARG if bad [in] passed, [out] values are 0s and NULL respectively
 S_FALSE returned if there is nothing to return, [out] values are 0s and NULL respectively
HRESULT  textAtOffset ([in] long offset,[in] enum IA2TextBoundaryType boundaryType,[out] long *startOffset,[out] long *endOffset,[out, retval] BSTR *text)
  E_INVALIDARG if bad [in] passed, [out] values are 0s and NULL respectively
 S_FALSE returned if there is nothing to return, [out] values are 0s and NULL respectively
HRESULT  removeSelection ([in] long selectionIndex)
  E_INVALIDARG if bad [in] passed
HRESULT  setCaretOffset ([in] long offset)
  E_INVALIDARG if bad [in] passed
HRESULT  setSelection ([in] long selectionIndex,[in] long startOffset,[in] long endOffset)
  E_INVALIDARG if bad [in] passed
HRESULT  nCharacters ([out, retval] long *nCharacters)
  no exception
HRESULT  scrollSubstringTo ([in] long startIndex,[in] long endIndex,[in] enum IA2ScrollType scrollType)
  E_INVALIDARG if bad [in] passed
HRESULT  scrollSubstringToPoint ([in] long startIndex,[in] long endIndex,[in] enum IA2CoordinateType coordinateType,[in] long x,[in] long y)
  E_INVALIDARG if bad [in] passed
HRESULT  newText ([out, retval] IA2TextSegment *newText)
 S_FALSE returned if there is nothing to return, [out] value is NULL
HRESULT  oldText ([out, retval] IA2TextSegment *oldText)
 S_FALSE returned if there is nothing to return, [out] value is NULL

IAEditableText
HRESULT  copyText ([in] long startOffset,[in] long endOffset)
  E_INVALIDARG if bad [in] passed
HRESULT  deleteText ([in] long startOffset,[in] long endOffset)
  E_INVALIDARG if bad [in] passed
HRESULT  insertText ([in] long offset,[in] BSTR *text)
  E_INVALIDARG if bad [in] passed
HRESULT  cutText ([in] long startOffset,[in] long endOffset)
  E_INVALIDARG if bad [in] passed
HRESULT  pasteText ([in] long offset)
  E_INVALIDARG if bad [in] passed
HRESULT  replaceText ([in] long startOffset,[in] long endOffset,[in] BSTR *text)
  E_INVALIDARG if bad [in] passed
HRESULT  setAttributes ([in] long startOffset,[in] long endOffset,[in] BSTR *attributes)
  E_INVALIDARG if bad [in] passed

IAValue
HRESULT  currentValue ([out, retval] VARIANT *currentValue)
 S_FALSE returned if there is nothing to return, [out] value is NULL
HRESULT  setCurrentValue ([in] VARIANT value)
 no exception
HRESULT  maximumValue ([out, retval] VARIANT *maximumValue)
 no exception
HRESULT  minimumValue ([out, retval] VARIANT *minimumValue)
 no exception

Reproducible: Always

Steps to Reproduce:
1.
2.
3.
Comment 1 alexander :surkov 2008-03-11 04:13:31 PDT
Created attachment 308593 [details] [diff] [review]
wip
Comment 2 alexander :surkov 2008-03-16 02:49:59 PDT
Created attachment 309738 [details] [diff] [review]
patch
Comment 3 Aaron Leventhal 2008-03-16 06:43:56 PDT
There can be other nsresults that are successful than NS_OK. So I think GetHResult() should return S_OK for any NS_SUCCEEDED(rv).
Comment 4 alexander :surkov 2008-03-16 18:08:59 PDT
You're right I'll fix it.
Comment 5 Ginn Chen 2008-03-16 23:25:40 PDT
Comment on attachment 309738 [details] [diff] [review]
patch

too much msaa/IA2 code in this patch, I think I'm not capable to review it.
Comment 6 alexander :surkov 2008-03-16 23:31:51 PDT
Comment on attachment 309738 [details] [diff] [review]
patch

rerequesting
Comment 7 Aaron Leventhal 2008-03-17 11:40:20 PDT
Comment on attachment 309738 [details] [diff] [review]
patch

Since success values aren't always NS_OK, you don't have to make changes like this:
-  return rv;
+  return NS_OK;
Comment 8 Aaron Leventhal 2008-03-17 11:59:52 PDT
Comment on attachment 309738 [details] [diff] [review]
patch

*aAccessible is no longer cleared to nsnull if aIndex ! = 0

-  if (aIndex != 0) {
-    *aAccessible = nsnull;
-    return NS_ERROR_FAILURE;
-  }
+  NS_ENSURE_ARG(aIndex == 0);
+  NS_ENSURE_ARG_POINTER(aAccessible);
+  *aAccessible = nsnull;
+

This seems like a good change but a small comment would help, since it looks odd:
  if (rv == NS_ERROR_ILLEGAL_VALUE)
    return NS_ERROR_INVALID_ARG;

Please remove the whitespace only changes. I see some in hsHTMLImageAccessible.cpp

Use GetHRESULT() here?
-  nsresult rv =  winAccessNode->QueryNativeInterface(IID_IUnknown,
-                                                     &instancePtr);
+  rv =  winAccessNode->QueryNativeInterface(IID_IUnknown, &instancePtr);
   if (NS_FAILED(rv))
     return E_FAIL;
Comment 9 Aaron Leventhal 2008-03-17 12:31:13 PDT
Comment on attachment 309738 [details] [diff] [review]
patch

Extra whitespace change at the end of nsAccessibleRelation::GetTargets()

In CAccessibleTable:
-  nsresult rv = winAccessNode->QueryNativeInterface(IID_IAccessible2,
-                                                    &instancePtr);
+  rv = winAccessNode->QueryNativeInterface(IID_IAccessible2, &instancePtr);
   if (NS_FAILED(rv))
     return E_FAIL;
Use GetHResult()?
IN general it looks like we may have missed a few opportunities to use GetHResult(). 

I see the code is removed from CAccessibleText::get_attributes()
I think we might support one text attribute already called static=true for list bullets. Are you sure it's good to remove that code?

nsAccessibleWrap::get_relation
I think if !*aRelation we should return S_FALSE.

	

nsAccessibleWrap::get_attributes()
If attributes.Length() == 0 we should return S_FALSE, right? I don't think the returned array will usually be null in that case.

Are there any other S_FALSE opportunities that we missed? I did not check carefully.

Did you also look at return values for IAccessible* methods?

Fix all those nits and r+=aaronlev
Comment 10 Aaron Leventhal 2008-03-17 15:06:17 PDT
Also be careful of return NULL with S_FALSE. We should return "" with S_FALSE.
Comment 11 Aaron Leventhal 2008-03-17 15:07:16 PDT
Or S_OK with "".

I'm not sure what the spec says about empty strings, but in any case it breaks JAWS support to return NULL for getTitle().
Comment 12 Aaron Leventhal 2008-03-17 15:20:11 PDT
Pete Brunet says if we return "" return S_OK
If we return NULL then return S_FALSE

But we need to check around. I'll send email.
Comment 13 alexander :surkov 2008-03-17 20:51:12 PDT
(In reply to comment #7)
> (From update of attachment 309738 [details] [diff] [review])
> Since success values aren't always NS_OK, you don't have to make changes like
> this:
> -  return rv;
> +  return NS_OK;
> 

Not sure I follow you. What else we might want to return?

> Use GetHRESULT() here?
> -  nsresult rv =  winAccessNode->QueryNativeInterface(IID_IUnknown,
> -                                                     &instancePtr);
> +  rv =  winAccessNode->QueryNativeInterface(IID_IUnknown, &instancePtr);
>    if (NS_FAILED(rv))
>      return E_FAIL;

No, I guess. QueryInterface may return no interface but it's not suitable for us. In general it won't fail in any case but if it will then let's return common error.

(In reply to comment #9)
> I see the code is removed from CAccessibleText::get_attributes()
> I think we might support one text attribute already called static=true for list
> bullets. Are you sure it's good to remove that code?

Ok, I missed that point. Should I file bug to move static attribute to text attributes only?
 
> nsAccessibleWrap::get_relation
> I think if !*aRelation we should return S_FALSE.

How it's possible? GetRelation(aRelationIndex, ...) returns invalid arg if there is no relation at the given index.

> 
> 
> nsAccessibleWrap::get_attributes()
> If attributes.Length() == 0 we should return S_FALSE, right? I don't think the
> returned array will usually be null in that case.

In the end of method I check if our created attribute string is empty then I return S_FALSE.

> Are there any other S_FALSE opportunities that we missed? I did not check
> carefully.

I hope no. I tried to catch all cases.

> Did you also look at return values for IAccessible* methods?

yes I moved by the list above which includes all IA2 interfaces.
Comment 14 Aaron Leventhal 2008-03-18 07:17:37 PDT
> -  return rv;
> +  return NS_OK;
> 
Just return rv as we do now.

For the static text attribute bug, we can just take care of that in the other text attribute bug. Or file a separate bug if you want.

Everything else is okay.

Just please create a try server build for Marco to test first. After the last issue with get_title() and JAWS I want to be extra careful now.
Comment 15 Aaron Leventhal 2008-03-19 09:01:55 PDT
Comment on attachment 309738 [details] [diff] [review]
patch

Actually let's just get this in after the beta. 

I want a final patch so Marco can test how screen readers react to it thoroughly.
Comment 16 alexander :surkov 2008-03-19 22:37:31 PDT
(In reply to comment #14)
> > -  return rv;
> > +  return NS_OK;
> > 

there NS_ENSURE_SUCCESS() above
Comment 17 alexander :surkov 2008-03-19 23:20:56 PDT
Created attachment 310706 [details] [diff] [review]
patch2
Comment 19 Marco Zehe (:MarcoZ) 2008-03-23 13:43:09 PDT
This try server build gives me this error:

The procedure entry point "EventRegister" was not found in the DLL "ADVAPI32.dll".
Comment 20 Aaron Leventhal 2008-03-23 15:02:33 PDT
(In reply to comment #16)
> (In reply to comment #14)
> > > -  return rv;
> > > +  return NS_OK;
> > > 
> 
> there NS_ENSURE_SUCCESS() above

Super reviewers have told me to allow the successful rv's to go through unchanged, because technically success does not have to be NS_OK. So I guess the change is not necessary, although I don't really care :)
Comment 21 Aaron Leventhal 2008-03-23 15:03:49 PDT
Comment on attachment 310706 [details] [diff] [review]
patch2

r+=aaronlev after MarcoZ gives his blessing from his test builds.
Comment 22 Marco Zehe (:MarcoZ) 2008-03-23 15:05:52 PDT
Comment on attachment 310706 [details] [diff] [review]
patch2

After building with it myself, I ran with it for a while and didn't notice any bad behaviour. Also, AccPro seemed OK with it.
Comment 23 Aaron Leventhal 2008-03-23 20:36:00 PDT
Returning NULL for strings with S_FALSE may be causing crashes in older screen readers that don't get that quite right.
Comment 24 alexander :surkov 2008-03-23 21:26:04 PDT
(In reply to comment #19)
> This try server build gives me this error:
> 
> The procedure entry point "EventRegister" was not found in the DLL
> "ADVAPI32.dll".
> 

it was discussed on irc: this error is rised because the build has been built on Vista but you try on XP. Is it your case?
Comment 25 Aaron Leventhal 2008-03-23 21:58:12 PDT
When he built his own there was no problem.
Comment 26 Marco Zehe (:MarcoZ) 2008-03-23 23:08:54 PDT
(In reply to comment #24)
> (In reply to comment #19)
> > This try server build gives me this error:
> > 
> > The procedure entry point "EventRegister" was not found in the DLL
> > "ADVAPI32.dll".
> it was discussed on irc: this error is rised because the build has been built
> on Vista but you try on XP. Is it your case?

Yes, I am on XP.
Comment 27 Mike Beltzner [:beltzner, not reading bugmail] 2008-03-29 13:51:47 PDT
Comment on attachment 310706 [details] [diff] [review]
patch2

a=beltzner
Comment 28 alexander :surkov 2008-03-29 20:25:39 PDT
/cvsroot/mozilla/accessible/src/base/nsAccessNode.cpp,v  <--  nsAccessNode.cpp
new revision: 1.78; previous revision: 1.77
done
Checking in accessible/src/base/nsAccessible.cpp;
/cvsroot/mozilla/accessible/src/base/nsAccessible.cpp,v  <--  nsAccessible.cpp
new revision: 1.369; previous revision: 1.368
done
Checking in accessible/src/base/nsAccessibleRelation.cpp;
/cvsroot/mozilla/accessible/src/base/nsAccessibleRelation.cpp,v  <--  nsAccessibleRelation.cpp
new revision: 1.3; previous revision: 1.2
done
Checking in accessible/src/base/nsApplicationAccessible.cpp;
/cvsroot/mozilla/accessible/src/base/nsApplicationAccessible.cpp,v  <--  nsApplicationAccessible.cpp
new revision: 1.6; previous revision: 1.5
done
Checking in accessible/src/base/nsApplicationAccessible.h;
/cvsroot/mozilla/accessible/src/base/nsApplicationAccessible.h,v  <--  nsApplicationAccessible.h
new revision: 1.4; previous revision: 1.3
done
Checking in accessible/src/html/nsHTMLImageAccessible.cpp;
/cvsroot/mozilla/accessible/src/html/nsHTMLImageAccessible.cpp,v  <--  nsHTMLImageAccessible.cpp
new revision: 1.48; previous revision: 1.47
done
Checking in accessible/src/html/nsHyperTextAccessible.cpp;
/cvsroot/mozilla/accessible/src/html/nsHyperTextAccessible.cpp,v  <--  nsHyperTextAccessible.cpp
new revision: 1.116; previous revision: 1.115
done
Checking in accessible/src/msaa/CAccessibleAction.cpp;
/cvsroot/mozilla/accessible/src/msaa/CAccessibleAction.cpp,v  <--  CAccessibleAction.cpp
new revision: 1.9; previous revision: 1.8
done
Checking in accessible/src/msaa/CAccessibleComponent.cpp;
/cvsroot/mozilla/accessible/src/msaa/CAccessibleComponent.cpp,v  <--  CAccessibleComponent.cpp
new revision: 1.6; previous revision: 1.5
done
Checking in accessible/src/msaa/CAccessibleEditableText.cpp;
/cvsroot/mozilla/accessible/src/msaa/CAccessibleEditableText.cpp,v  <--  CAccessibleEditableText.cpp
new revision: 1.5; previous revision: 1.4
done
Checking in accessible/src/msaa/CAccessibleHyperlink.cpp;
/cvsroot/mozilla/accessible/src/msaa/CAccessibleHyperlink.cpp,v  <--  CAccessibleHyperlink.cpp
new revision: 1.8; previous revision: 1.7
done
Checking in accessible/src/msaa/CAccessibleHypertext.cpp;
/cvsroot/mozilla/accessible/src/msaa/CAccessibleHypertext.cpp,v  <--  CAccessibleHypertext.cpp
new revision: 1.6; previous revision: 1.5
done
Checking in accessible/src/msaa/CAccessibleImage.cpp;
/cvsroot/mozilla/accessible/src/msaa/CAccessibleImage.cpp,v  <--  CAccessibleImage.cpp
new revision: 1.7; previous revision: 1.6
done
Checking in accessible/src/msaa/CAccessibleTable.cpp;
/cvsroot/mozilla/accessible/src/msaa/CAccessibleTable.cpp,v  <--  CAccessibleTable.cpp
new revision: 1.11; previous revision: 1.10
done
Checking in accessible/src/msaa/CAccessibleText.cpp;
/cvsroot/mozilla/accessible/src/msaa/CAccessibleText.cpp,v  <--  CAccessibleText.cpp
new revision: 1.11; previous revision: 1.10
done
Checking in accessible/src/msaa/CAccessibleValue.cpp;
/cvsroot/mozilla/accessible/src/msaa/CAccessibleValue.cpp,v  <--  CAccessibleValue.cpp
new revision: 1.4; previous revision: 1.3
done
Checking in accessible/src/msaa/nsAccessNodeWrap.cpp;
/cvsroot/mozilla/accessible/src/msaa/nsAccessNodeWrap.cpp,v  <--  nsAccessNodeWrap.cpp
new revision: 1.44; previous revision: 1.43
done
Checking in accessible/src/msaa/nsAccessNodeWrap.h;
/cvsroot/mozilla/accessible/src/msaa/nsAccessNodeWrap.h,v  <--  nsAccessNodeWrap.h
new revision: 1.13; previous revision: 1.12
done
Checking in accessible/src/msaa/nsAccessibleRelationWrap.cpp;
/cvsroot/mozilla/accessible/src/msaa/nsAccessibleRelationWrap.cpp,v  <--  nsAccessibleRelationWrap.cpp
new revision: 1.6; previous revision: 1.5
done
Checking in accessible/src/msaa/nsAccessibleWrap.cpp;
/cvsroot/mozilla/accessible/src/msaa/nsAccessibleWrap.cpp,v  <--  nsAccessibleWrap.cpp
new revision: 1.116; previous revision: 1.115
done
Checking in accessible/src/msaa/nsApplicationAccessibleWrap.cpp;
/cvsroot/mozilla/accessible/src/msaa/nsApplicationAccessibleWrap.cpp,v  <--  nsApplicationAccessibleWrap.cpp
new revision: 1.9; previous revision: 1.8
done

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