750025 - clean up nsAccessNodeWrap::MakeAccessNode()

Status: NEW

(Core :: Disability Access APIs, defect)

Description

[Trevor Saunders (:tbsaunde)]

rm a QI to nsIContent, and do a little cleanup while there.

Comment 1

[Trevor Saunders (:tbsaunde)]

Attachment: patch

Comment 2

[alexander :surkov (:asurkov)]

Comment on attachment 619365 [details] [diff] [review]
patch

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

::: accessible/src/msaa/nsAccessNodeWrap.cpp
@@ +426,4 @@
>  
>      newNode->Init();
>      iNode = static_cast<ISimpleDOMNode*>(newNode);
>      iNode->AddRef();

you need to fix indentation since you move this block out of 'else if' construction

aNode->AsElement() should make us crash in case of inaccessbile document or text node since they aren't inherited from Element class.

Comment 3

[Trevor Saunders (:tbsaunde)]

> ::: accessible/src/msaa/nsAccessNodeWrap.cpp
> @@ +426,4 @@
> >  
> >      newNode->Init();
> >      iNode = static_cast<ISimpleDOMNode*>(newNode);
> >      iNode->AddRef();
> 
> you need to fix indentation since you move this block out of 'else if'
> construction
> 
> aNode->AsElement() should make us crash in case of inaccessbile document or
> text node since they aren't inherited from Element class.

yeah, I guess the assertion we had there before wasn't really correct.  However we still shouldn't crash because ISimpleDOMNode methods should handle case of defunct content and I think most do.

Comment 4

[alexander :surkov (:asurkov)]

(In reply to Trevor Saunders (:tbsaunde) from comment #3)
> yeah, I guess the assertion we had there before wasn't really correct.

what's wrong with it?

Comment 5

[Trevor Saunders (:tbsaunde)]

(In reply to alexander :surkov from comment #4)
> (In reply to Trevor Saunders (:tbsaunde) from comment #3)
> > yeah, I guess the assertion we had there before wasn't really correct.
> 
> what's wrong with it?

I thought in some cases we intentionally don't create accessible documents for dom docs? for example temporary ones or svg?

Comment 6

[alexander :surkov (:asurkov)]

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

> I thought in some cases we intentionally don't create accessible documents
> for dom docs? for example temporary ones or svg?

yes, we don't create them in some cases but it doesn't prevent an AT to access to the content by ISimpleDOMNode.

Comment 7

[Trevor Saunders (:tbsaunde)]

(In reply to alexander :surkov from comment #6)
> (In reply to Trevor Saunders (:tbsaunde) from comment #5)
> 
> > I thought in some cases we intentionally don't create accessible documents
> > for dom docs? for example temporary ones or svg?
> 
> yes, we don't create them in some cases but it doesn't prevent an AT to
> access to the content by ISimpleDOMNode.

yes, and that would mean that the assert wasn't quiet right wouldn't it since the assert would fire and we'd not give t an ISimpleDOMNode?  I'd think technical in this case of inaccessible document we should return object that is a ISimpleDOMDOcument as well as a ISimpleDOMNode, but that might take work and no at seems to care.

Comment 8

[Trevor Saunders (:tbsaunde)]

Attachment: patch v2

Comment 9

[alexander :surkov (:asurkov)]

Comment on attachment 619877 [details] [diff] [review]
patch v2

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

::: accessible/src/msaa/nsAccessNodeWrap.cpp
@@ +424,3 @@
>  
> +  if (!aNode->IsElement())
> +    return nsnull;

you ignore text nodes

Comment 10

[alexander :surkov (:asurkov)]

Comment on attachment 619877 [details] [diff] [review]
patch v2

canceling request until comments are addressed

Comment 11

[alexander :surkov (:asurkov)]

It will be fixed by bug 767756.