Closed
Bug 713792
Opened 13 years ago
Closed 12 years ago
stop QueryInterface()ing to nsIAccessibleImage internally
Categories
(Core :: Disability Access APIs, defect)
Tracking
()
RESOLVED
FIXED
mozilla12
People
(Reporter: tbsaunde, Assigned: santiago.gimeno)
Details
(Whiteboard: [good first bug][mentor=trev.saunders@gmail.com][lang=c++])
Attachments
(1 file, 3 obsolete files)
6.31 KB,
patch
|
tbsaunde
:
review+
|
Details | Diff | Splinter Review |
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/
Reporter | ||
Updated•13 years ago
|
Whiteboard: [good first bug][mentor=trev.saunders@gmail.com][lang=c++]
Assignee | ||
Comment 1•13 years ago
|
||
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’
Attachment #584787 -
Flags: review?(trev.saunders)
Updated•13 years ago
|
Attachment #584787 -
Attachment is patch: true
Reporter | ||
Comment 2•13 years ago
|
||
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.
Attachment #584787 -
Flags: review?(trev.saunders)
Assignee | ||
Comment 3•13 years ago
|
||
Attachment #584787 -
Attachment is obsolete: true
Attachment #584948 -
Flags: review?(trev.saunders)
Reporter | ||
Comment 4•13 years ago
|
||
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
Attachment #584948 -
Flags: review?(trev.saunders)
Assignee | ||
Comment 5•13 years ago
|
||
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
Attachment #584948 -
Attachment is obsolete: true
Attachment #585306 -
Flags: review?(trev.saunders)
Comment 6•13 years ago
|
||
They're in alphabetical order, so you'll want eImageAccessible after eHTMLListItemAccessible.
Assignee | ||
Comment 7•13 years ago
|
||
(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•13 years ago
|
||
Oh, hmm. Trevor?
Reporter | ||
Comment 9•13 years ago
|
||
(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.
Reporter | ||
Comment 10•13 years ago
|
||
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
Attachment #585306 -
Flags: review?(trev.saunders) → review+
Assignee | ||
Comment 11•13 years ago
|
||
I hope this is what you wanted. BTW, what does "r=me" mean?
Attachment #585306 -
Attachment is obsolete: true
Attachment #586044 -
Flags: review?(trev.saunders)
Reporter | ||
Updated•13 years ago
|
Attachment #586044 -
Flags: review?(trev.saunders) → review+
Reporter | ||
Comment 12•13 years ago
|
||
https://hg.mozilla.org/projects/accessibility/rev/91c2ca8c6029
Reporter | ||
Comment 13•12 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/91c2ca8c6029
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Updated•12 years ago
|
Target Milestone: --- → mozilla12
Comment 14•12 years ago
|
||
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.
Updated•12 years ago
|
Assignee: nobody → santiago.gimeno
You need to log in
before you can comment on or make changes to this bug.
Description
•