Last Comment Bug 713792 - stop QueryInterface()ing to nsIAccessibleImage internally
: stop QueryInterface()ing to nsIAccessibleImage internally
Status: RESOLVED FIXED
[good first bug][mentor=trev.saunders...
:
Product: Core
Classification: Components
Component: Disability Access APIs (show other bugs)
: 12 Branch
: All All
: -- normal (vote)
: mozilla12
Assigned To: Santiago Gimeno
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2011-12-27 18:55 PST by Trevor Saunders (:tbsaunde)
Modified: 2012-01-30 05:24 PST (History)
5 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Replace QueryInterface()-ing to nsIAccessibleImage with AsImage (7.30 KB, patch)
2011-12-29 10:15 PST, Santiago Gimeno
no flags Details | Diff | Review
Address Trevor Saunders comments (6.24 KB, patch)
2011-12-30 05:33 PST, Santiago Gimeno
no flags Details | Diff | Review
Address more comments (6.33 KB, patch)
2012-01-02 10:31 PST, Santiago Gimeno
tbsaunde+mozbugs: review+
Details | Diff | Review
Change the order as it was in previous_patches (6.31 KB, patch)
2012-01-05 05:16 PST, Santiago Gimeno
tbsaunde+mozbugs: review+
Details | Diff | Review

Description Trevor Saunders (:tbsaunde) 2011-12-27 18:55:01 PST
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 2011-12-29 10:15:33 PST
Created attachment 584787 [details] [diff] [review]
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) 2011-12-29 15:38:48 PST
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 2011-12-30 05:33:25 PST
Created attachment 584948 [details] [diff] [review]
Address Trevor Saunders comments
Comment 4 Trevor Saunders (:tbsaunde) 2011-12-30 08:09:29 PST
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 2012-01-02 10:31:54 PST
Created attachment 585306 [details] [diff] [review]
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 2012-01-02 10:34:13 PST
They're in alphabetical order, so you'll want eImageAccessible after eHTMLListItemAccessible.
Comment 7 Santiago Gimeno 2012-01-04 02:36:23 PST
(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 2012-01-04 03:06:24 PST
Oh, hmm. Trevor?
Comment 9 Trevor Saunders (:tbsaunde) 2012-01-04 12:42:53 PST
(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) 2012-01-04 12:44:15 PST
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 2012-01-05 05:16:38 PST
Created attachment 586044 [details] [diff] [review]
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) 2012-01-06 14:32:43 PST
https://hg.mozilla.org/projects/accessibility/rev/91c2ca8c6029
Comment 13 Trevor Saunders (:tbsaunde) 2012-01-10 18:28:51 PST
https://hg.mozilla.org/mozilla-central/rev/91c2ca8c6029
Comment 14 alexander :surkov 2012-01-19 08:51:14 PST
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.

Note You need to log in before you can comment on or make changes to this bug.