379585 - implement IAccessibleTable

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[alexander :surkov (:asurkov)]

Comment 1

[alexander :surkov (:asurkov)]

Attachment: patch

Comment 2

[Aaron Leventhal]

Would it be easy later if we wanted to be able to implement nsIAccessibleTable for an nsAccessible where a web author used ARIA markup to define a table or spreadsheet?

Comment 3

[alexander :surkov (:asurkov)]

(In reply to comment #2)
> Would it be easy later if we wanted to be able to implement nsIAccessibleTable
> for an nsAccessible where a web author used ARIA markup to define a table or
> spreadsheet?
> 

It shoudn't be a problem while we keep IAccessibleTable implementation separetely. I guess nsAccessibleWrap should be inherited from CAccessibleTable.

Comment 4

[Aaron Leventhal]

Okay, we can deal with that later then. I'm not sure if I even want to tackle that in Firefox 3. As long as it's fairly easy to change later.

Comment 5

[Aaron Leventhal]

+ #define GET_NSIACCESSIBLETABLE
I've read that in C++ it's preferred to use static inline instead of #define. It's more readable. You can define a static already_AddRefs() inline method and it won't take up any more size, but it's easier to understand -- right now it's not clear that a variable called tableAcc comes out of it, without going back and checking the definitom.

Out params should be initialized at the top of each method, before any early returns. For example, set *aAccessible = nsnull so that even on an error return the pointer will be set to something reasonable.

I see return rv a few times, even though we're expecting COM error returns. I suppose nsresults actually use the same values as HRESULTS, so it's probably okay. Did you intend that?

Comment 6

[alexander :surkov (:asurkov)]

Attachment: patch2

Comment 7

[alexander :surkov (:asurkov)]

(In reply to comment #5)
> + #define GET_NSIACCESSIBLETABLE
> I've read that in C++ it's preferred to use static inline instead of #define.
> It's more readable. You can define a static already_AddRefs() inline method and
> it won't take up any more size, but it's easier to understand -- right now it's
> not clear that a variable called tableAcc comes out of it, without going back
> and checking the definitom.

I'm agree. But it would be something like:

inline already_AddRefs<nsIAccessibleTable> getBlaBla(const CAccessibleTable& aObj)
{
  nsIAccessibleTable* table;
  NS_ASSERTION(table, "Oops");
  NS_IF_ADDREF(table, do_QueryInterface(aObj));
}

nsCOMPtr<nsIAccessibleTable> table(getBlaBla(this));
if (!table)
  return E_FAIL;

It doesn't save code. Though probably I missed something.

> Out params should be initialized at the top of each method, before any early
> returns. For example, set *aAccessible = nsnull so that even on an error return
> the pointer will be set to something reasonable.

Fixed.

> I see return rv a few times, even though we're expecting COM error returns. I
> suppose nsresults actually use the same values as HRESULTS, so it's probably
> okay. Did you intend that?
> 

It's error because nsresult values can't be converted to HRESULT automatically of course. Thank you for the catch.

Comment 8

[Aaron Leventhal]

Actually if you use inline and not static inline then you don't have to pass |this|. It's not so much about saving code, but about readability. It's the same amount of compiled code either way, with an inline vs. a #define.

Comment 9

[alexander :surkov (:asurkov)]

(In reply to comment #8)
> Actually if you use inline and not static inline then you don't have to pass
> |this|. It's not so much about saving code, but about readability. It's the
> same amount of compiled code either way, with an inline vs. a #define.
> 

Actually, it looks we'll save NS_ASSERTION only with inline method :). Do we need inline function then? Possibly I can use neither macros nor inline method.

Comment 10

[Aaron Leventhal]

> Actually, it looks we'll save NS_ASSERTION only with inline method :)
What do you mean?

Comment 11

[alexander :surkov (:asurkov)]

(In reply to comment #10)
> > Actually, it looks we'll save NS_ASSERTION only with inline method :)
> What do you mean?
> 

Code with macros:
MACROS__

Code with inline method:
nsCOMPtr<nsIAccessibleTable> table(getBlaBla());
if (!table)
  return E_FAIL;

Code without inline method:
nsCOMPtr<nsIAccessibleTable> table(do_QueryInterface(this));
NS_ASSERTION(table, "blabla");
if (!table)
  return E_FAIL;

The difference is NS_ASSERTION

Comment 12

[Aaron Leventhal]

You can't have an assertion in an inline method?

Comment 13

[alexander :surkov (:asurkov)]

(In reply to comment #12)
> You can't have an assertion in an inline method?
> 

I think I can. I wonder is it a worth to do because we do not save much code size by assertion moving into inline method?

Comment 14

[Aaron Leventhal]

The assertion only affects code size in debug builds, so it's fine.

Comment 15

[alexander :surkov (:asurkov)]

(In reply to comment #14)
> The assertion only affects code size in debug builds, so it's fine.
> 

Sorry, I didn't have in view that. I mean inline method doesn't win in code lines.

Comment 16

[Aaron Leventhal]

Right, it's not fewer lines of code, but easier to understand and the same amount of compiled machine code.

Comment 17

[alexander :surkov (:asurkov)]

Attachment: patch3

Comment 18

[alexander :surkov (:asurkov)]

Attachment: patch 4

for checkin

Comment 19

[alexander :surkov (:asurkov)]

checked in