Closed Bug 932789 Opened 6 years ago Closed 6 years ago

isolate nsIAccessibleSelectable implementation

Categories

(Core :: Disability Access APIs, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla28

People

(Reporter: surkov, Assigned: surkov)

References

Details

(Whiteboard: [qa-])

Attachments

(1 file)

Attached patch patchSplinter Review
1) move impl into separate class
2) clean up nsIAccessibleSelectable interface (rename methods, use unsigned int bug 682770, fix name collision with nsIAccessibleText::selectionCount)
Attachment #824646 - Flags: review?(trev.saunders)
Comment on attachment 824646 [details] [diff] [review]
patch

>diff --git a/accessible/public/nsIAccessibleSelectable.idl b/accessible/public/nsIAccessibleSelectable.idl
>--- a/accessible/public/nsIAccessibleSelectable.idl
>+++ b/accessible/public/nsIAccessibleSelectable.idl
>@@ -1,72 +1,62 @@
> /* -*- Mode: C++; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2 -*-
> /* This Source Code Form is subject to the terms of the Mozilla Public
>  * License, v. 2.0. If a copy of the MPL was not distributed with this
>  * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
>-#include "nsISupports.idl"
>+
> #include "nsIAccessible.idl"
> #include "nsIArray.idl"

why? afaik the only one you actually need to include is nsISupports.idl the others can be forward declared.

btw index stuff in this interface is still nasty because of difference between index into selected things and generic index.

>+xpcAccessibleSelectable::GetSelectedItems(nsIArray** aSelectedItems)
>+{
>+  NS_ENSURE_ARG_POINTER(aSelectedItems);
>+  *aSelectedItems = nullptr;
>+
>+  Accessible* acc = static_cast<Accessible*>(this);
>+  if (acc->IsDefunct() || !acc->IsSelect())

why not just assert its a select? if its not you shouldn't be able to qi to this in the first place right?

>+++ b/accessible/src/xpcom/xpcAccessibleSelectable.h
>@@ -0,0 +1,33 @@
>+public:
>+  xpcAccessibleSelectable() { }

why not just let the compiler define that for you (or better make it private and Accessible a friend so Accessible is the only thing that can inherit from it.

>+  NS_DECL_NSIACCESSIBLESELECTABLE

it might be worth inling that so you can mark all the methods as final (if you do that make sure to make them over ride as well.
Attachment #824646 - Flags: review?(trev.saunders) → review+
(In reply to Trevor Saunders (:tbsaunde) from comment #1)

> why? afaik the only one you actually need to include is nsISupports.idl the
> others can be forward declared.

ok

> btw index stuff in this interface is still nasty because of difference
> between index into selected things and generic index.

interface is not ideal, I think it just copies some AT part

> >+  Accessible* acc = static_cast<Accessible*>(this);
> >+  if (acc->IsDefunct() || !acc->IsSelect())
> 
> why not just assert its a select? if its not you shouldn't be able to qi to
> this in the first place right?

agree

> >+++ b/accessible/src/xpcom/xpcAccessibleSelectable.h
> >@@ -0,0 +1,33 @@
> >+public:
> >+  xpcAccessibleSelectable() { }
> 
> why not just let the compiler define that for you (or better make it private
> and Accessible a friend so Accessible is the only thing that can inherit
> from it

ok

> 
> >+  NS_DECL_NSIACCESSIBLESELECTABLE
> 
> it might be worth inling that so you can mark all the methods as final (if
> you do that make sure to make them over ride as well.

ok
https://hg.mozilla.org/mozilla-central/rev/0803bb0d11d0
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla28
Whiteboard: [qa-]
You need to log in before you can comment on or make changes to this bug.