932789 - isolate nsIAccessibleSelectable implementation

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[alexander :surkov (:asurkov)]

Attachment: patch

1) move impl into separate class
2) clean up nsIAccessibleSelectable interface (rename methods, use unsigned int bug 682770, fix name collision with nsIAccessibleText::selectionCount)

Comment 1

[Trevor Saunders (:tbsaunde)]

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.

Comment 2

[alexander :surkov (:asurkov)]

(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

Comment 3

[alexander :surkov (:asurkov)]

http://hg.mozilla.org/integration/mozilla-inbound/rev/0803bb0d11d0

Comment 4

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/0803bb0d11d0