Closed Bug 758870 Opened 12 years ago Closed 12 years ago

de-ns-ify nsDocAccessible

Categories

(Core :: Disability Access APIs, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla15

People

(Reporter: surkov, Assigned: capella)

References

(Blocks 1 open bug)

Details

(Whiteboard: [good first bug][mentor=trev.saunders@gmail.com][lang=c++])

Attachments

(1 file, 1 obsolete file)

similar to bug 748724.
Assignee: nobody → markcapella
Status: NEW → ASSIGNED
Attached patch Patch (v1) (obsolete) — Splinter Review
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 ...
Attachment #627505 - Flags: feedback?(surkov.alexander)
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 :
Attachment #627505 - Flags: feedback?(surkov.alexander) → review+
(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.
Attached patch Patch (v2)Splinter Review
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
Attachment #627505 - Attachment is obsolete: true
Good TRY run, pushing to inbound ...

https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=f41434c72cb5
Target Milestone: --- → mozilla15
https://hg.mozilla.org/mozilla-central/rev/f41434c72cb5
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Flags: in-testsuite-
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.