Last Comment Bug 741697 - dexpcomify CAccessibleComponent
: dexpcomify CAccessibleComponent
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Disability Access APIs (show other bugs)
: unspecified
: All Windows 7
: -- normal (vote)
: mozilla14
Assigned To: Jignesh Kakadiya [:jhk]
:
: alexander :surkov
Mentors:
Depends on:
Blocks: dexpcoma11y
  Show dependency treegraph
 
Reported: 2012-04-02 23:01 PDT by Trevor Saunders (:tbsaunde)
Modified: 2012-04-14 06:42 PDT (History)
3 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch (19.50 KB, patch)
2012-04-10 12:29 PDT, Jignesh Kakadiya [:jhk]
no flags Details | Diff | Splinter Review
Patch_2 (8.92 KB, patch)
2012-04-11 01:24 PDT, Jignesh Kakadiya [:jhk]
surkov.alexander: feedback+
Details | Diff | Splinter Review
Patch_3 (9.46 KB, patch)
2012-04-11 04:46 PDT, Jignesh Kakadiya [:jhk]
no flags Details | Diff | Splinter Review
Patch_4 (10.23 KB, patch)
2012-04-12 02:55 PDT, Jignesh Kakadiya [:jhk]
no flags Details | Diff | Splinter Review
Patch_5 (9.70 KB, patch)
2012-04-12 03:00 PDT, Jignesh Kakadiya [:jhk]
tbsaunde+mozbugs: review+
Details | Diff | Splinter Review
Patch_6 (9.72 KB, patch)
2012-04-12 03:49 PDT, Jignesh Kakadiya [:jhk]
no flags Details | Diff | Splinter Review
Patch_7 (9.72 KB, patch)
2012-04-12 04:33 PDT, Jignesh Kakadiya [:jhk]
no flags Details | Diff | Splinter Review

Description Trevor Saunders (:tbsaunde) 2012-04-02 23:01:13 PDT
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 2012-04-02 23:03:54 PDT
yes

5. remove NS_IMETHOD QueryInterface(const nsIID& uuid, void** result) = 0;
Comment 2 Jignesh Kakadiya [:jhk] 2012-04-10 12:29:06 PDT
Created attachment 613717 [details] [diff] [review]
Patch
Comment 3 alexander :surkov 2012-04-10 21:58:09 PDT
Jignesh, you should do hg rename instead
Comment 4 Jignesh Kakadiya [:jhk] 2012-04-11 01:24:54 PDT
Created attachment 613901 [details] [diff] [review]
Patch_2
Comment 5 alexander :surkov 2012-04-11 03:50:47 PDT
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] 2012-04-11 04:46:41 PDT
Created attachment 613948 [details] [diff] [review]
Patch_3

Fixed above comments.
Comment 7 Trevor Saunders (:tbsaunde) 2012-04-11 11:16:28 PDT
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] 2012-04-12 02:55:08 PDT
Created attachment 614317 [details] [diff] [review]
Patch_4

Fixed all above comments. we need nsCoreUtils.h for nsIFrame hence I kept it.
Comment 9 Jignesh Kakadiya [:jhk] 2012-04-12 03:00:45 PDT
Created attachment 614318 [details] [diff] [review]
Patch_5

Please review this one.
Comment 10 Trevor Saunders (:tbsaunde) 2012-04-12 03:34:10 PDT
(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) 2012-04-12 03:40:41 PDT
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] 2012-04-12 03:49:19 PDT
Created attachment 614326 [details] [diff] [review]
Patch_6

Good to go?
Comment 13 Jignesh Kakadiya [:jhk] 2012-04-12 04:33:53 PDT
Created attachment 614332 [details] [diff] [review]
Patch_7

Header position fixed.
Comment 14 alexander :surkov 2012-04-13 04:07:49 PDT
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 16 Marco Bonardo [::mak] 2012-04-14 06:42:01 PDT
https://hg.mozilla.org/mozilla-central/rev/ae42e4d0ec0b

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