461921 - remove nsPIAccessible

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Comment 1

[alexander :surkov (:asurkov)]

Attachment: wip

Comment 2

[alexander :surkov (:asurkov)]

Attachment: patch

Comment 3

[Marco Zehe (:MarcoZ)]

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

Nits:
>+  NS_ASSERTION(mDocument, "No document duiring initialization!");

>+  NS_ASSERTION(parentDoc, "No parent document duiring initialization!");

>+  NS_ASSERTION(content, "No content duiring accessible tree building!");

"duiring" should be "during".

Also a question, shouldn't accessible/public/nsPIAccessible.idl be removed with this patch if it is being obsoleted?

Comment 4

[Marco Zehe (:MarcoZ)]

Oops forgot to add: r*me for the functional part, didn't find any regressions while testing with JAWS and NVDA.

Comment 5

[alexander :surkov (:asurkov)]

Attachment: patch2

with Marco's comments

Comment 6

[David Bolter [:davidb] (NeedInfo me for attention)]

Can someone remind me why nsPIAccessible was created in the first place?

Comment 7

[alexander :surkov (:asurkov)]

(In reply to comment #6)
> Can someone remind me why nsPIAccessible was created in the first place?

Originally we were interface oriented, so we created not scriptable interfaces to work with objects internally. Since we started to use objects directly (via nsRefPtr) then we don't need these interfaces any more.

Comment 8

[alexander :surkov (:asurkov)]

(In reply to comment #7)
> (In reply to comment #6)
> > Can someone remind me why nsPIAccessible was created in the first place?
> 
> Originally we were interface oriented, so we created not scriptable interfaces
> to work with objects internally. Since we started to use objects directly (via
> nsRefPtr) then we don't need these interfaces any more.

Also you could refer to bug 358557 comment #1.

Comment 9

[David Bolter [:davidb] (NeedInfo me for attention)]

Comment on attachment 383215 [details] [diff] [review]
patch2

A nit:

>   /**
>+   * Query nsAccessible from the given nsIAccessible.
>+   */
>+  static already_AddRefed<nsAccessible>
>+    QueryAccessible(nsIAccessNode *aAccessNode);
>+

Still reviewing...

Comment 10

[David Bolter [:davidb] (NeedInfo me for attention)]

I accidentally deleted my nit:

Should the comment say "from the given nsIAccessNode"?

Comment 11

[alexander :surkov (:asurkov)]

(In reply to comment #10)
> Should the comment say "from the given nsIAccessNode"?

should :)

Comment 12

[David Bolter [:davidb] (NeedInfo me for attention)]

Comment on attachment 383215 [details] [diff] [review]
patch2

r=me, but...

Please double check that you update any interace or class ids if necessary.

You did a lot of cleanup here. Looks good -- I hope I didn't miss anything (it is 1:20am for me). When you compile do you get any warnings about hiding non-virtual base class methods? I don't think you will but worth it to double check.

(Scott Meyers, Effective C++, Item 37: Never redefine an inherited nonvirtual function)

>-    child = static_cast<nsAccessible*>(next.get());
>+    child = nsAccUtils::QueryAccessible(next);

I wonder if this hits performance.

>-  NS_IMETHOD AppendTextTo(nsAString& aText, PRUint32 aStartOffset, PRUint32 aLength);
>+  virtual void SetParent(nsIAccessible *aParent);
>+  virtual nsresult AppendTextTo(nsAString& aText, PRUint32 aStartOffset,
>+                                PRUint32 aLength);

I'm curious why you replace "NS_IMETHOD" with "virtual nsresult"?  I think it is the same thing right?  (You do this in a few places)

I prefer "virtual nsresult" actually.

Comment 13

[alexander :surkov (:asurkov)]

(In reply to comment #12)
> (From update of attachment 383215 [details] [diff] [review])
> r=me, but...
> 
> Please double check that you update any interace or class ids if necessary.

thanks, I will.

> You did a lot of cleanup here. Looks good -- I hope I didn't miss anything (it
> is 1:20am for me). When you compile do you get any warnings about hiding
> non-virtual base class methods? I don't think you will but worth it to double
> check.
> 
> (Scott Meyers, Effective C++, Item 37: Never redefine an inherited nonvirtual
> function)

no, I didn't.

> 
> >-    child = static_cast<nsAccessible*>(next.get());
> >+    child = nsAccUtils::QueryAccessible(next);
> 
> I wonder if this hits performance.

we shouldn't, it's more right way I think.

> I'm curious why you replace "NS_IMETHOD" with "virtual nsresult"?  I think it
> is the same thing right?  (You do this in a few places)
> 
> I prefer "virtual nsresult" actually.

NS_IMETHOD is used for interface methods implementation. Where we haven't interface methods then I prefer to use C++ syntax.

Comment 14

[alexander :surkov (:asurkov)]

checked in with David's comments on 1.9.2 - http://hg.mozilla.org/mozilla-central/rev/30b481fd82f5

Comment 15

[alexander :surkov (:asurkov)]

(In reply to comment #12)

> You did a lot of cleanup here. Looks good -- I hope I didn't miss anything (it
> is 1:20am for me). When you compile do you get any warnings about hiding
> non-virtual base class methods? I don't think you will but worth it to double
> check.

It isn't related with this bug but. We have a lot of warnings for GetChildAtPoint like 

./../base/nsAccessible.h:126: warning: ‘virtual nsresult nsAccessible::GetChildAtPoint(PRInt32, PRInt32, nsIAccessible**)’ was hidden
./../base/nsBaseWidgetAccessible.h:73: warning:   by ‘virtual nsresult nsLeafAccessible::GetChildAtPoint(PRInt32, PRInt32, PRBool, nsIAccessible**)’

Why is this warning?