dexpcomify CAccessibleComponent

RESOLVED FIXED in mozilla14

Status

()

Core
Disability Access APIs
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: tbsaunde, Assigned: jhk)

Tracking

unspecified
mozilla14
All
Windows 7
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 6 obsolete attachments)

(Reporter)

Description

5 years ago
1. include nsAccessibleWrap.h instead of nsAccessible.h
2. change nsRefPtr<nsAccessible> acc = do_QueryObject(this) to static_cast<nsAccessibleWrap*>(this);
3. get rid of the use of the xpcom GetParent() on an accessible replacing it with Parent()
4. rename the class to ia2AccessibleComponent

surkov sound good?

Comment 1

5 years ago
yes

5. remove NS_IMETHOD QueryInterface(const nsIID& uuid, void** result) = 0;
(Assignee)

Comment 2

5 years ago
Created attachment 613717 [details] [diff] [review]
Patch
Attachment #613717 - Flags: review?(trev.saunders)

Comment 3

5 years ago
Jignesh, you should do hg rename instead
(Assignee)

Comment 4

5 years ago
Created attachment 613901 [details] [diff] [review]
Patch_2
Attachment #613717 - Attachment is obsolete: true
Attachment #613717 - Flags: review?(trev.saunders)
Attachment #613901 - Flags: review?(trev.saunders)

Comment 5

5 years ago
Comment on attachment 613901 [details] [diff] [review]
Patch_2

Review of attachment 613901 [details] [diff] [review]:
-----------------------------------------------------------------

::: accessible/src/msaa/Makefile.in
@@ +64,5 @@
>    nsApplicationAccessibleWrap.cpp \
>    nsWinUtils.cpp \
>    ia2AccessibleAction.cpp \
>    CAccessibleImage.cpp \
> +  ia2AccessibleComponent.cpp \

put it after is2AccessibleAction please

@@ +93,5 @@
>    nsHTMLTableAccessibleWrap.h \
>    nsApplicationAccessibleWrap.h \
>    ia2AccessibleAction.h \
>    CAccessibleImage.h \
> +  ia2AccessibleComponent.h \

same

::: accessible/src/msaa/CAccessibleComponent.cpp
@@ -37,5 @@
>   * the terms of any one of the MPL, the GPL or the LGPL.
>   *
>   * ***** END LICENSE BLOCK ***** */
>  
> -#include "CAccessibleComponent.h"

it makes sense to keep it even it comes from nsAccessibleWrap.h

@@ +70,5 @@
>  
>  // IAccessibleComponent
>  
>  STDMETHODIMP
> +ia2AccessibleComponent::get_locationInParent(long *aX, long *aY)

type* name please
Attachment #613901 - Flags: feedback+
(Assignee)

Comment 6

5 years ago
Created attachment 613948 [details] [diff] [review]
Patch_3

Fixed above comments.
Attachment #613901 - Attachment is obsolete: true
Attachment #613901 - Flags: review?(trev.saunders)
Attachment #613948 - Flags: review?(trev.saunders)
(Reporter)

Comment 7

5 years ago
Comment on attachment 613948 [details] [diff] [review]
Patch_3

> 
>-#include "nsAccessible.h"
>+#include "nsAccessibleWrap.h"
> #include "nsCoreUtils.h"
> #include "nsWinUtils.h"

I'm pretty sure these headers aren't needed any more, it'd be nice if you could remove them if that's actually the case.

> #include "States.h"
> 
> #include "nsString.h"
> 
> #include "nsIDOMCSSPrimitiveValue.h"
> #include "nsIDOMNSRGBAColor.h"

pretty sure you don't need these last three either.

>+  nsAccessibleWrap* acc = static_cast<nsAccessibleWrap*>(this);
>+  if (acc->IsDefunct())
>     return E_FAIL;

that should be CO_E_OBJECTNOTCONNECTED now.

>+  nsAccessibleWrap* acc = static_cast<nsAccessibleWrap*>(this);
>   if (acc->IsDefunct())
>     return E_FAIL;

same, though since you didn't change this maybe you just need to rebase.

>-#ifndef _ACCESSIBLE_COMPONENt_H
>+#ifndef _ACCESSIBLE_COMPONENT_H
> #define _ACCESSIBLE_COMPONENT_H

not sure what you change here, but it would be nice if you rename the guard to something like IA2_ACCESSIBLE_COMPONENT_H_
Attachment #613948 - Flags: review?(trev.saunders)
(Assignee)

Comment 8

5 years ago
Created attachment 614317 [details] [diff] [review]
Patch_4

Fixed all above comments. we need nsCoreUtils.h for nsIFrame hence I kept it.
Attachment #613948 - Attachment is obsolete: true
Attachment #614317 - Flags: review?(trev.saunders)
(Assignee)

Comment 9

5 years ago
Created attachment 614318 [details] [diff] [review]
Patch_5

Please review this one.
Attachment #614317 - Attachment is obsolete: true
Attachment #614317 - Flags: review?(trev.saunders)
(Assignee)

Updated

5 years ago
Attachment #614318 - Flags: review?(trev.saunders)
(Reporter)

Comment 10

5 years ago
(In reply to Jignesh Kakadiya [:jhk] from comment #8)
> Created attachment 614317 [details] [diff] [review]
> Patch_4
> 
> Fixed all above comments. we need nsCoreUtils.h for nsIFrame hence I kept it.

why not include nsIFrame.h directly then?
(Reporter)

Comment 11

5 years ago
Comment on attachment 614318 [details] [diff] [review]
Patch_5

> #include "nsIDOMNSRGBAColor.h"

why didn't you get rid of this include?
Attachment #614318 - Flags: review?(trev.saunders) → review+
(Assignee)

Comment 12

5 years ago
Created attachment 614326 [details] [diff] [review]
Patch_6

Good to go?
(Assignee)

Comment 13

5 years ago
Created attachment 614332 [details] [diff] [review]
Patch_7

Header position fixed.
Attachment #614326 - Attachment is obsolete: true

Updated

5 years ago
Attachment #614318 - Attachment is obsolete: true

Comment 14

5 years ago
Comment on attachment 614332 [details] [diff] [review]
Patch_7

Review of attachment 614332 [details] [diff] [review]:
-----------------------------------------------------------------

I'll fix nits before landing

::: accessible/src/msaa/CAccessibleComponent.h
@@ +38,5 @@
>   *
>   * ***** END LICENSE BLOCK ***** */
>  
> +#ifndef  IA2_ACCESSIBLE_COMPONENT_H_
> +#define  IA2_ACCESSIBLE_COMPONENT_H_

extra space after ifndef and define

@@ +43,4 @@
>  
>  #include "AccessibleComponent.h"
>  
> +class ia2AccessibleComponent: public IAccessibleComponent

space before :

Comment 15

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/ae42e4d0ec0b
Target Milestone: --- → mozilla14
https://hg.mozilla.org/mozilla-central/rev/ae42e4d0ec0b
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.