713792 - stop QueryInterface()ing to nsIAccessibleImage internally

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[Trevor Saunders (:tbsaunde)]

1. allow downcasting from nsAccessible to nsHTMLImageAccessible
1.1 add eImageAccessible to  AccessibleTypes enum in accessible/src/base/nsAccessible.h (keep the elements in alphabetical order)
1.2 add prototype for AsImage() returning nsHTMLImageAccessible to nsAccessible.h in order near AsDoc() / AsRoot() etc.
1.3 implement AsImage() in accessible/src/html/nsHTMLImageAccessible.h as an inline method (see AsHypertext() in accessible/src/html/nsHypertextAccessible.h for an example).

2. replace QueryInterface() to nsIAccessibleImage with AsImage()

should be a good bug for someone familiar with C/C++ but fairly new to accessible/

Comment 1

[Santiago Gimeno]

Attachment: Replace QueryInterface()-ing to nsIAccessibleImage with AsImage

It compiles. I don't know if it's correct though.

In file nsMaiInterfaceImage.cpp I tried to do:

nsCOMPtr<nsHTMLImageAccessible> image = accWrap->AsImage();

but this threw a compilation error:

In file included from /home/sgimeno/mozilla/src/accessible/src/atk/nsAccessibleWrap.h:44,
                 from /home/sgimeno/mozilla/src/accessible/src/atk/nsMaiInterfaceImage.cpp:40:
../../../dist/include/nsCOMPtr.h: In constructor ‘nsCOMPtr<T>::nsCOMPtr(T*) [with T = nsHTMLImageAccessible]’:
/home/sgimeno/mozilla/src/accessible/src/atk/nsMaiInterfaceImage.cpp:65:   instantiated from here
../../../dist/include/nsCOMPtr.h:561: error: ‘nsISupports’ is an ambiguous base of ‘nsHTMLImageAccessible’

Comment 2

[Trevor Saunders (:tbsaunde)]

Comment on attachment 584787 [details] [diff] [review]
Replace QueryInterface()-ing to nsIAccessibleImage with AsImage

This is pretty good all told, but I'd like to see another version.

>+    nsHTMLImageAccessible* image = accWrap->AsImage();
>     if (!image)
>       return;

this is fine, but a nicer method might be if (!accWrap || !accWrap->IsImageAccessible())
  return <what ever>;

nsHTMLImageAccessible* image = accWrap->AsImage();
image->DoSomething();

>+    nsHTMLImageAccessible* image = accWrap->AsImage();
>     if (!image)
>       return;

same

> class nsAccessible;
> class nsHyperTextAccessible;
> class nsHTMLLIAccessible;
>+class nsHTMLImageAccessible;

this list is only somewhat ordered, but try to not add more things out of order.

>   inline bool IsHTMLListItem() const { return mFlags & eHTMLListItemAccessible; }
>   nsHTMLLIAccessible* AsHTMLListItem();
> 
>+  inline bool IsImage() const { return mFlags && eImageAccessible; }
>+  nsHTMLImageAccessible* AsImage();

keep these in order too if you can

btw does the compiler let us call it const since it doesn't change the object itself.  I would think it doesn't matter much since it is inline.

>+inline nsHTMLImageAccessible*
>+nsAccessible::AsImage()
>+{
>+  return mFlags & eImageAccessible ?

I'd use IsImageAccessible() instead of the raw compare.

You missed the QueryInterface() in atk/nsAccessibleWrap.cpp, sorry if I didn't mention it in the description.

general comment, we don't generally fix unrelated white space issues like you do, but I can't really complain.

Comment 3

[Santiago Gimeno]

Attachment: Address Trevor Saunders comments

Comment 4

[Trevor Saunders (:tbsaunde)]

Comment on attachment 584948 [details] [diff] [review]
Address Trevor Saunders comments



just a couple more things, most of which are really minor

>     //nsIAccessibleImage

The comment is pretty obvious I'd prefer you just remove it, but you can update it if you really want

>+    if (IsImageAccessible()) {
>         interfacesBits |= 1 << MAI_INTERFACE_IMAGE;
>     }

get rid of braces while your her

>+    
spaces on blank line

>+    nsHTMLImageAccessible* image = accWrap->AsImage();
> 
>     PRUint32 geckoCoordType = 

blank line not needed

>+    

white space at end of line

>+    nsHTMLImageAccessible* image = accWrap->AsImage();
>     image->GetImageSize(aAccWidth, aAccHeight);

get rid of the local variable

>   inline bool IsHTMLListItem() const { return mFlags & eHTMLListItemAccessible; }
>   nsHTMLLIAccessible* AsHTMLListItem();
>+  
>+  inline bool IsImageAccessible() const { return mFlags && eImageAccessible; }

one, still out of order.

Also you want mFlags &  eImageAccessible, not &&

>     eHTMLListItemAccessible = 1 << 9,
>-    eListControlAccessible = 1 << 10,
>-    eMenuButtonAccessible = 1 << 11,
>-    eMenuPopupAccessible = 1 << 12,
>-    eRootAccessible = 1 << 13,
>-    eTextLeafAccessible = 1 << 14
>+    eImageAccessible = 1 << 10,

out of order

> #endif
>-

I'd leave that alone for now

Comment 5

[Santiago Gimeno]

Attachment: Address more comments

Hi,

Thanks for the review! I'm not sure if the lines in nsAccesible.h are now in order, if not could you explain the issue more thoroughly?

Cheers,

Santi

Comment 6

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

They're in alphabetical order, so you'll want eImageAccessible after eHTMLListItemAccessible.

Comment 7

[Santiago Gimeno]

(In reply to Ms2ger from comment #6)
> They're in alphabetical order, so you'll want eImageAccessible after
> eHTMLListItemAccessible.

Yeah, that's what I did in the first and second patches, but he commented it was still out of order. So I thought he was "really" thinking on HTMLImageAccessible.

Comment 8

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Oh, hmm. Trevor?

Comment 9

[Trevor Saunders (:tbsaunde)]

(In reply to Santiago Gimeno from comment #7)
> (In reply to Ms2ger from comment #6)
> > They're in alphabetical order, so you'll want eImageAccessible after
> > eHTMLListItemAccessible.
> 
> Yeah, that's what I did in the first and second patches, but he commented it
> was still out of order. So I thought he was "really" thinking on
> HTMLImageAccessible.

yeah, I was wrong, you had it right, my bad sorry.

Comment 10

[Trevor Saunders (:tbsaunde)]

Comment on attachment 585306 [details] [diff] [review]
Address more comments

r=me with the bit in comment 9 back the way you had it initially

Comment 11

[Santiago Gimeno]

Attachment: Change the order as it was in previous_patches

I hope this is what you wanted.

BTW, what does "r=me" mean?

Comment 12

[Trevor Saunders (:tbsaunde)]

https://hg.mozilla.org/projects/accessibility/rev/91c2ca8c6029

Comment 13

[Trevor Saunders (:tbsaunde)]

https://hg.mozilla.org/mozilla-central/rev/91c2ca8c6029

Comment 14

[alexander :surkov (:asurkov)]

It appears nsHTMLImageAccessible should be renamed to ImageAccessible and moved to base directory since the class is used for HTML and XUL both. Please file a bug for that.