741697 - dexpcomify CAccessibleComponent

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[Trevor Saunders (:tbsaunde)]

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

[alexander :surkov (:asurkov)]

yes

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

Comment 2

[Jignesh Kakadiya [:jhk]]

Attachment: Patch

Comment 3

[alexander :surkov (:asurkov)]

Jignesh, you should do hg rename instead

Comment 4

[Jignesh Kakadiya [:jhk]]

Attachment: Patch_2

Comment 5

[alexander :surkov (:asurkov)]

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

Comment 6

[Jignesh Kakadiya [:jhk]]

Attachment: Patch_3

Fixed above comments.

Comment 7

[Trevor Saunders (:tbsaunde)]

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_

Comment 8

[Jignesh Kakadiya [:jhk]]

Attachment: Patch_4

Fixed all above comments. we need nsCoreUtils.h for nsIFrame hence I kept it.

Comment 9

[Jignesh Kakadiya [:jhk]]

Attachment: Patch_5

Please review this one.

Comment 10

[Trevor Saunders (:tbsaunde)]

(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?

Comment 11

[Trevor Saunders (:tbsaunde)]

Comment on attachment 614318 [details] [diff] [review]
Patch_5

> #include "nsIDOMNSRGBAColor.h"

why didn't you get rid of this include?

Comment 12

[Jignesh Kakadiya [:jhk]]

Attachment: Patch_6

Good to go?

Comment 13

[Jignesh Kakadiya [:jhk]]

Attachment: Patch_7

Header position fixed.

Comment 14

[alexander :surkov (:asurkov)]

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

[alexander :surkov (:asurkov)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/ae42e4d0ec0b

Comment 16

[Marco Bonardo [:mak]]

https://hg.mozilla.org/mozilla-central/rev/ae42e4d0ec0b