Last Comment Bug 758870 - de-ns-ify nsDocAccessible
: de-ns-ify nsDocAccessible
Status: RESOLVED FIXED
[good first bug][mentor=trev.saunders...
:
Product: Core
Classification: Components
Component: Disability Access APIs (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla15
Assigned To: Mark Capella [:capella]
:
: alexander :surkov
Mentors:
Depends on:
Blocks: densifya11y
  Show dependency treegraph
 
Reported: 2012-05-26 04:41 PDT by alexander :surkov
Modified: 2012-05-27 18:46 PDT (History)
3 users (show)
ryanvm: in‑testsuite-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch (v1) (352.32 KB, patch)
2012-05-26 13:52 PDT, Mark Capella [:capella]
surkov.alexander: review+
Details | Diff | Splinter Review
Patch (v2) (355.70 KB, patch)
2012-05-26 22:42 PDT, Mark Capella [:capella]
no flags Details | Diff | Splinter Review

Description alexander :surkov 2012-05-26 04:41:39 PDT
similar to bug 748724.
Comment 1 Mark Capella [:capella] 2012-05-26 13:52:01 PDT
Created attachment 627505 [details] [diff] [review]
Patch (v1)

Builds and tests locally, push to TRY: https://tbpl.mozilla.org/?tree=Try&rev=738741efa386

This doesn't include moving the classes into the a11y namespace ... originally I coded that change but ran into build problems involving the DocAccessible cache, so I pulled just that portion out to verify it as the cause of the problems.

I'm posting it here for feedback, and wondering it we might move the patch along as-is and do a follow-up for the namespace change later ... allows us to move this large change quickly and cleanly without modifying too much code other than nsDocAccessible to DocAccessible renames ...
Comment 2 alexander :surkov 2012-05-26 20:54:13 PDT
Comment on attachment 627505 [details] [diff] [review]
Patch (v1)

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

r=me, please fix style nits and land it asap

::: accessible/src/atk/nsDocAccessibleWrap.cpp
@@ +16,1 @@
>                        nsIPresShell* aPresShell) :

nit: correct indent

::: accessible/src/atk/nsDocAccessibleWrap.h
@@ +15,2 @@
>  
> +class DocAccessibleWrap: public DocAccessible

nit: space before :

::: accessible/src/base/nsAccDocManager.h
@@ +32,5 @@
>  
>    /**
>     * Return document accessible for the given DOM node.
>     */
> +  DocAccessible* GetDocAccessible(nsIDocument *aDocument);

nit: nsIDocument* aDocument

@@ +105,5 @@
>  
>    /**
>     * Create document or root accessible.
>     */
> +  DocAccessible* CreateDocOrRootAccessible(nsIDocument *aDocument);

type* name

::: accessible/src/base/nsAccUtils.h
@@ +106,5 @@
>  
>    /**
>     * Return document accessible for the given DOM node.
>     */
> +  static DocAccessible* GetDocAccessibleFor(nsINode *aNode)

type* name

@@ +115,5 @@
>  
>    /**
>     * Return document accessible for the given docshell.
>     */
> +  static DocAccessible* GetDocAccessibleFor(nsIDocShellTreeItem *aContainer)

type* name

::: accessible/src/base/nsEventShell.cpp
@@ +5,5 @@
>  
>  #include "nsEventShell.h"
>  
>  #include "nsAccUtils.h"
> +//#include "DocAccessible.h"

nit: pls remove the comment

::: accessible/src/base/nsDocAccessible.cpp
@@ +325,5 @@
>  
>  }
>  
>  NS_IMETHODIMP
> +DocAccessible::GetAttributes(nsIPersistentProperties **aAttributes)

type* name

@@ +342,5 @@
>    // be contained within the current document.
>    return FocusMgr()->FocusedAccessible();
>  }
>  
> +NS_IMETHODIMP DocAccessible::TakeFocus()

nit: NS_IMETHODIMP on new line pls

@@ +361,5 @@
>  
>  ////////////////////////////////////////////////////////////////////////////////
>  // nsIAccessibleDocument
>  
> +NS_IMETHODIMP DocAccessible::GetURL(nsAString& aURL)

same

@@ +418,5 @@
>  
>    return NS_ERROR_FAILURE;
>  }
>  
> +NS_IMETHODIMP DocAccessible::GetNameSpaceURIForID(PRInt16 aNameSpaceID, nsAString& aNameSpaceURI)

same

@@ +429,5 @@
>    }
>    return NS_ERROR_FAILURE;
>  }
>  
> +NS_IMETHODIMP DocAccessible::GetWindowHandle(void **aWindow)

return type on own line, type* name

@@ +436,5 @@
>    *aWindow = GetNativeWindow();
>    return NS_OK;
>  }
>  
> +NS_IMETHODIMP DocAccessible::GetWindow(nsIDOMWindow **aDOMWin)

same

@@ +453,5 @@
>    return NS_OK;
>  }
>  
>  NS_IMETHODIMP
> +DocAccessible::GetDOMDocument(nsIDOMDocument **aDOMDocument)

type* name

@@ +494,1 @@
>                                      nsIAccessibleDocument** aDocument)

wrong indentation

@@ +671,5 @@
>    return root;
>  }
>  
> +// DocAccessible protected member
> +void DocAccessible::GetBoundsRect(nsRect& aBounds, nsIFrame** aRelativeFrame)

return type on own line

@@ +712,5 @@
>    }
>  }
>  
> +// DocAccessible protected member
> +nsresult DocAccessible::AddEventListeners()

same

@@ +755,5 @@
>    return NS_OK;
>  }
>  
> +// DocAccessible protected member
> +nsresult DocAccessible::RemoveEventListeners()

same

@@ +798,5 @@
>  
>    return NS_OK;
>  }
>  
> +void DocAccessible::ScrollTimerCallback(nsITimer *aTimer, void *aClosure)

same plus type* name

@@ +820,5 @@
>    }
>  }
>  
> +// DocAccessible protected member
> +void DocAccessible::AddScrollListener()

too many of them, it's probably worth to run through the file and fix all of them

@@ +878,1 @@
>                                         const PRUnichar *aData)

wrong indentation

@@ +898,2 @@
>                                  nsIAccessible* aOldAccessible,
>                                  PRInt32 aOldStart, PRInt32 aOldEnd)

same

@@ +914,3 @@
>  
>  void
> +DocAccessible::AttributeWillChange(nsIDocument *aDocument,

type* name

@@ +953,3 @@
>                                    dom::Element* aElement,
>                                    PRInt32 aNameSpaceID, nsIAtom* aAttribute,
>                                    PRInt32 aModType)

wrong indentation

@@ +1210,3 @@
>                                        nsIContent* aContainer,
>                                        nsIContent* aFirstNewContent,
>                                        PRInt32 /* unused */)

indent it

@@ +1217,2 @@
>                                            nsIContent* aContent,
>                                            nsEventStates aStateMask)

same and void on own line

@@ +1243,1 @@
>                                              nsEventStates aStateMask)

please run through the file and fix broken indentation

::: accessible/src/base/nsDocAccessible.h
@@ +54,5 @@
>  public:
>    using nsAccessible::GetParent;
>  
> +  DocAccessible(nsIDocument *aDocument, nsIContent *aRootContent,
> +                nsIPresShell* aPresShell);

nit: type* name

::: accessible/src/mac/nsDocAccessibleWrap.h
@@ +10,2 @@
>  
> +class DocAccessibleWrap: public DocAccessible

nit: space before :

::: accessible/src/msaa/nsDocAccessibleWrap.cpp
@@ +48,5 @@
>  
>  //-----------------------------------------------------
>  // IUnknown interface methods - see iunknown.h for documentation
>  //-----------------------------------------------------
> +STDMETHODIMP_(ULONG) DocAccessibleWrap::AddRef()

return type on ow line please (through this file)

::: accessible/src/msaa/nsDocAccessibleWrap.h
@@ +17,3 @@
>  #include "nsIDocShellTreeItem.h"
>  
> +class DocAccessibleWrap: public DocAccessible,

nit: space before :
Comment 3 alexander :surkov 2012-05-26 20:56:29 PDT
(In reply to Mark Capella [:capella] from comment #1)
> Created attachment 627505 [details] [diff] [review]
> Patch (v1)
> 
> Builds and tests locally, push to TRY:
> https://tbpl.mozilla.org/?tree=Try&rev=738741efa386
> 
> This doesn't include moving the classes into the a11y namespace ...
> originally I coded that change but ran into build problems involving the
> DocAccessible cache, so I pulled just that portion out to verify it as the
> cause of the problems.
> 
> I'm posting it here for feedback, and wondering it we might move the patch
> along as-is and do a follow-up for the namespace change later ... allows us
> to move this large change quickly and cleanly without modifying too much
> code other than nsDocAccessible to DocAccessible renames ...

fine with me. I'd say we should put DocAccessible into namespace after we put all other accessible classes (except nsAccessible perhaps) into that namespace, otherwise you need to add mozilla::a11y for every header file and then remove them.
Comment 4 Mark Capella [:capella] 2012-05-26 22:42:29 PDT
Created attachment 627533 [details] [diff] [review]
Patch (v2)

Latest version, nits addressed ... patch merged to inbound this time ... minor changes due to clash with msaa/nsAccessibleWrap.cpp and .h class constructor / destructor ...

Pushing to try ...
https://tbpl.mozilla.org/?tree=Try&rev=0d202956a8a0
Comment 5 Mark Capella [:capella] 2012-05-27 02:19:09 PDT
Good TRY run, pushing to inbound ...

https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=f41434c72cb5
Comment 6 Ryan VanderMeulen [:RyanVM] 2012-05-27 18:46:54 PDT
https://hg.mozilla.org/mozilla-central/rev/f41434c72cb5

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