Closed Bug 461921 Opened 11 years ago Closed 11 years ago

remove nsPIAccessible

Categories

(Core :: Disability Access APIs, defect)

defect
Not set

Tracking

()

RESOLVED FIXED

People

(Reporter: surkov, Assigned: surkov)

References

Details

Attachments

(1 file, 2 obsolete files)

No description provided.
Blocks: 496783
Attached patch wip (obsolete) — Splinter Review
Assignee: nobody → surkov.alexander
Status: NEW → ASSIGNED
Attached patch patch (obsolete) — Splinter Review
Attachment #382023 - Attachment is obsolete: true
Attachment #383085 - Flags: review?(marco.zehe)
Attachment #383085 - Flags: review?(bolterbugz)
Attachment #383085 - Flags: review?(marco.zehe) → review+
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?
Oops forgot to add: r*me for the functional part, didn't find any regressions while testing with JAWS and NVDA.
Attached patch patch2Splinter Review
with Marco's comments
Attachment #383085 - Attachment is obsolete: true
Attachment #383215 - Flags: review?(bolterbugz)
Attachment #383085 - Flags: review?(bolterbugz)
Can someone remind me why nsPIAccessible was created in the first place?
(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.
(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 on attachment 383215 [details] [diff] [review]
patch2

A nit:

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

Still reviewing...
I accidentally deleted my nit:

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

should :)
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.
Attachment #383215 - Flags: review?(bolterbugz) → review+
(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.
checked in with David's comments on 1.9.2 - http://hg.mozilla.org/mozilla-central/rev/30b481fd82f5
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
(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?
You need to log in before you can comment on or make changes to this bug.