748724 - de-ns-ify nsRootAccessible

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[alexander :surkov (:asurkov)]

Similar to bug 742695 with base/nsRootAccessible.cpp/h this time.  Make sure to update the nsRootAccessibleWraps.cpp/.h in msaa/ atk/ mac/ and other/

Comment 1

[alexander :surkov (:asurkov)]

also please put nsRootAccessible into mozilla::a11y namespace

Comment 2

[Mark Capella [:capella]]

Attachment: Patch (v1)

I thought this would be another simple de-ns-ify bug, but it has a weird twist ... build system fails while putting together widget/windows modules.

for ex: c:/Users/Master/mozilla-central/widget/windows/nsNativeDragTarget.cpp' failed, return code 2
c:\Users\Master\mozilla-central\obj\dist\include\nsAccessible.h(471) : error C2143: syntax error : missing ';' before '*'

It doesn't seem to recognize RootAccessible as a type ... points to the line :  RootAccessible* AsRoot(); which we added for downcasting ... thats the only thing that seems to make this unique ...

I've tried hacking the build system, (adding explicit include path and such), referencing the function via namespace::, adding a #include "rootaccessible.h" to nsaccessible.h, class forwarding RootAccessible ... but basically I'm guessing. 

Maybe something will occur to you? I can describe it further if it will help, or push to try so the build is viewable...

Comment 3

[alexander :surkov (:asurkov)]

I think because you put RootAccessible under mozilla::a11y but nsAccessNode.h defines it as class RootAccessible out of that namespace so that inline is undefined.

Comment 4

[alexander :surkov (:asurkov)]

perhaps that inline should be out of mozilla::a11y namespace too (RootAccessible.h)

Comment 5

[Mark Capella [:capella]]

Attachment: Patch (v2)

I've detailed the Build errors as pastebin http://pastebin.mozilla.org/1609068

Comment 6

[alexander :surkov (:asurkov)]

Comment on attachment 619835 [details] [diff] [review]
Patch (v2)

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

::: accessible/src/base/nsAccessNode.h
@@ +96,5 @@
>  
>    /**
>     * Return the root document accessible for this accessnode.
>     */
> +  RootAccessible* RootAccessible() const;

mozilla::a11y::RootAccessible

::: accessible/src/base/nsRootAccessible.h
@@ +35,5 @@
>   *
>   * ***** END LICENSE BLOCK ***** */
>  
> +#ifndef MOZILLA_A11Y_RootAccessible_H_
> +#define MOZILLA_A11Y_RootAccessible_H_

I know that's something we were agree previously but it'd be nicer to follow other modules here and do
mozilla_a11y_RootAccessible_h__ like in Element.h

up to you.

@@ +124,5 @@
>  
> +} // namespace a11y
> +} // namespace mozilla
> +
> +inline RootAccessible*

mozilla::a11y::RootAccessible

@@ +129,4 @@
>  nsAccessible::AsRoot()
>  {
>    return mFlags & eRootAccessible ?
> +    static_cast<RootAccessible*>(this) : nsnull;

same here

Comment 7

[Mark Capella [:capella]]

Attachment: Patch (v3)

I finally have a version for review that builds and tests ok. Thanks to Surkov for straightening me out on namespace usage. 

Note that I expect to have to re-base this after Bug 748716 - don't export ApplicationAccessibleWrap.h and Bug 740747 - dexpcom nsAccessible::GetName get pulled down to me in nightly in the next day or so.

You'll note I had to tweak the makefiles a little. Also, I tried to incorporate all standards as defined in the past. I may have missed on a few, but thats what feedbacks for, so let me know what you find -- mark

Comment 8

[alexander :surkov (:asurkov)]

(In reply to Mark Capella [:capella] from comment #7)
> Note that I expect to have to re-base this after Bug 748716 - don't export
> ApplicationAccessibleWrap.h and Bug 740747 - dexpcom nsAccessible::GetName
> get pulled down to me in nightly in the next day or so.
> 
> You'll note I had to tweak the makefiles a little. Also, I tried to
> incorporate all standards as defined in the past. I may have missed on a
> few, but thats what feedbacks for, so let me know what you find -- mark

Just start review process. You (or somebody who lands) can do merging later if necessary.

Comment 9

[Mark Capella [:capella]]

Comment on attachment 619870 [details] [diff] [review]
Patch (v3)

Starting review process with feedback request from mentor.

Comment 10

[Trevor Saunders (:tbsaunde)]

Comment on attachment 619870 [details] [diff] [review]
Patch (v3)

>-nsNativeRootAccessibleWrap::nsNativeRootAccessibleWrap(AtkObject *aAccessible):
>-    nsRootAccessible(nsnull, nsnull, nsnull)
>+nsNativeRootAccessibleWrap::nsNativeRootAccessibleWrap(AtkObject* aAccessible):

this should probably get renamed too, but it doesn't much matter to me if that's in a different bug.

>     case nsIAccessibleEvent::EVENT_WINDOW_ACTIVATE:
>       {
>         MAI_LOG_DEBUG(("\n\nReceived: EVENT_WINDOW_ACTIVATED\n"));
>-        nsRootAccessible *rootAcc =
>-          static_cast<nsRootAccessible *>(accessible);
>+        RootAccessible* rootAcc =
>+          static_cast<RootAccessible *>(accessible);

while your changingthis use AsRootAccessible() instead of direct static cast.

>     case nsIAccessibleEvent::EVENT_WINDOW_DEACTIVATE:
>       {
>         MAI_LOG_DEBUG(("\n\nReceived: EVENT_WINDOW_DEACTIVATED\n"));
>-        nsRootAccessible *rootAcc =
>-          static_cast<nsRootAccessible *>(accessible);
>+        RootAccessible* rootAcc =
>+          static_cast<RootAccessible *>(accessible);

same

>-nsRootAccessible*
>+mozilla::a11y::RootAccessible*
> nsAccessNode::RootAccessible() const

you there should be using namespace mozilla::a11y; at the top of the file if not add it then you can just use RootAccessible*

> nsAccessibilityService::PresShellActivated(nsIPresShell* aPresShell)
> {
>   nsIDocument* DOMDoc = aPresShell->GetDocument();
>   if (DOMDoc) {
>     nsDocAccessible* document = GetDocAccessibleFromCache(DOMDoc);
>     if (document) {
>-      nsRootAccessible* rootDocument = document->RootAccessible();
>+      mozilla:a11y::RootAccessible* rootDocument = document->RootAccessible();

same

> nsAccessible::GetRootDocument(nsIAccessibleDocument **aRootDocument)
> {
>   NS_ENSURE_ARG_POINTER(aRootDocument);
> 
>-  nsRootAccessible* rootDocument = RootAccessible();
>+  mozilla::a11y::RootAccessible* rootDocument = RootAccessible();
>   NS_IF_ADDREF(*aRootDocument = rootDocument);

same, and you could get rid local var all together if you like

>-nsCaretAccessible::nsCaretAccessible( nsRootAccessible *aRootAccessible):
>+nsCaretAccessible::nsCaretAccessible(mozilla::a11y::RootAccessible* aRootAccessible):

same

>@@ -759,17 +759,17 @@ nsresult nsDocAccessible::AddEventListen
>     if (commandManager) {
>       commandManager->AddCommandObserver(this, "obs_documentCreated");
>     }
>   }
> 
>   nsCOMPtr<nsIDocShellTreeItem> rootTreeItem;
>   docShellTreeItem->GetRootTreeItem(getter_AddRefs(rootTreeItem));
>   if (rootTreeItem) {
>-    nsRootAccessible* rootAccessible = RootAccessible();
>+    mozilla::a11y::RootAccessible* rootAccessible = RootAccessible();

same here and below


>+++ b/accessible/src/generic/RootAccessible.cpp
>@@ -71,55 +71,57 @@
> #include "nsIFrame.h"
> #include "nsIHTMLDocument.h"
> #include "nsIInterfaceRequestorUtils.h"
> #include "nsISelectionPrivate.h"
> #include "nsIServiceManager.h"
> #include "nsPIDOMWindow.h"
> #include "nsIWebBrowserChrome.h"
> #include "nsReadableUtils.h"
>-#include "nsRootAccessible.h"
> #include "nsIPrivateDOMEvent.h"
> #include "nsFocusManager.h"
>+#include "RootAccessible.h"

make it first

>-nsresult nsRootAccessible::RemoveEventListeners()
>+nsresult RootAccessible::RemoveEventListeners()

put return type on own line while you touch this line.

>+static id <mozAccessible, mozView> getNativeViewFromRootAccessible (nsAccessible* accessible)
> {
>-  nsRootAccessibleWrap *root = static_cast<nsRootAccessibleWrap*>(accessible);
>+  RootAccessibleWrap* root = static_cast<RootAccessibleWrap*>(accessible);

use AsRootAccessible() here too

> 
>-class nsRootAccessibleWrap: public nsRootAccessible
>+namespace mozilla {
>+namespace a11y {
>+
>+class RootAccessibleWrap: public RootAccessible
> {
> public:
>-  nsRootAccessibleWrap(nsIDocument* aDocument, nsIContent* aRootContent,
>-                       nsIPresShell* aPresShell);
>-  virtual ~nsRootAccessibleWrap();
>+  RootAccessibleWrap(nsIDocument* aDocument, nsIContent* aRootContent,
>+                     nsIPresShell* aPresShell);
>+  virtual ~RootAccessibleWrap();
> };

it seems like we should be able to replace this with a typedef but it doesn't seem very important.

Comment 11

[Mark Capella [:capella]]

Attachment: Patch (v4)

Updated patch ... I renamed that extra class and used the available AsRoot() function where pointed out instead of the direct cast approach.

I wasn't able to remove all the explicit mozilla::a11y:: references but got most of them. nsDocAccessible.cpp for example has a "using namespace mozilla::a11y" definition but still requires the mozilla::a11y::RootAccessible reference in a couple places for some reason.

And nsCaretAccessible.cpp/.h builds as-is but looks like maybe something could be cleaner and still avoid ambiguous name clashes.

Finally I didn't do anything with the typedef suggestion... as I'd need further information on that. All else looks good ... I'll flag Surkov for review and hope for the best :)

Comment 12

[Mark Capella [:capella]]

Ah sorry Trev, I thought you'd feedback+ it, please continue to comment as appropriate.

Comment 13

[alexander :surkov (:asurkov)]

Comment on attachment 620059 [details] [diff] [review]
Patch (v4)

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

btw, would you mind to file a bug to remove accessible-docs.html from sources, it's out of date and all docs should be on devmo.

r=me with comments addressed

::: accessible/src/atk/Makefile.in
@@ +80,5 @@
>    nsXULTreeGridAccessibleWrap.h \
>    nsHyperTextAccessibleWrap.h \
>    nsHTMLImageAccessibleWrap.h \
>    nsHTMLTableAccessibleWrap.h \
> +  RootAccessibleWrap.h \

you don't need to export it, right?

::: accessible/src/atk/nsRootAccessibleWrap.cpp
@@ +43,1 @@
>  

using namespace mozilla::a11y I think

::: accessible/src/atk/nsRootAccessibleWrap.h
@@ +38,5 @@
>   *
>   * ***** END LICENSE BLOCK ***** */
>  
> +#ifndef mozilla_a11y_Root_Accessible_Wrap_h__
> +#define mozilla_a11y_Root_Accessible_Wrap_h__

nit: mozilla_a11y_RootAccessibleWrap_h__

@@ +53,4 @@
>   * It is added into root when the toplevel window is created, and removed
>   * from root when the toplevel window is destroyed.
>   */
> +class NativeRootAccessibleWrap: public RootAccessible

nit: space before :

@@ +57,4 @@
>  {
>  public:
> +  NativeRootAccessibleWrap(AtkObject* aAccessible);
> +  ~NativeRootAccessibleWrap();

make the destructor virtual please

@@ +62,5 @@
>  
> +} // namespace a11y
> +} // namespace mozilla
> +
> +#endif   /* mozilla_a11y_Root_Accessible_Wrap_h__ */

nit: make sure there empty line in the end

::: accessible/src/base/nsAccessibilityService.cpp
@@ +1729,5 @@
>    if (!applicationAcc)
>      return nsnull;
>  
> +  nsRefPtr<NativeRootAccessibleWrap> nativeRootAcc =
> +     new NativeRootAccessibleWrap((AtkObject*)aAtkAccessible);

nit: two spaces indent (not 4)
please use static_cast<>

::: accessible/src/base/nsCaretAccessible.cpp
@@ +52,3 @@
>  #include "nsISelectionPrivate.h"
>  #include "nsServiceManagerUtils.h"
> +#include "RootAccessible.h"

please keep it in a11y include block

@@ +60,3 @@
>  NS_IMPL_ISUPPORTS1(nsCaretAccessible, nsISelectionListener)
>    
> +nsCaretAccessible::nsCaretAccessible(RootAccessible* aRootAccessible):

nit: space before :

::: accessible/src/base/nsCaretAccessible.h
@@ +46,5 @@
> +namespace mozilla {
> +namespace a11y {
> +class RootAccessible;
> +}
> +}

it's defined in nsAccessNode which is included by nsHyperTextAccesible.h, so it appears you don't need to prototype RootAccessible here

::: accessible/src/base/nsDocAccessible.cpp
@@ +763,5 @@
>  
>    nsCOMPtr<nsIDocShellTreeItem> rootTreeItem;
>    docShellTreeItem->GetRootTreeItem(getter_AddRefs(rootTreeItem));
>    if (rootTreeItem) {
> +    mozilla::a11y::RootAccessible* rootAccessible = RootAccessible();

so you have names conflict here? then do a11y::RootAccessible

@@ +810,5 @@
>      mScrollWatchTimer = nsnull;
>      NS_RELEASE_THIS(); // Kung fu death grip
>    }
>  
> +  mozilla::a11y::RootAccessible* rootAccessible = RootAccessible();

same

::: accessible/src/base/nsRootAccessible.cpp
@@ +92,5 @@
>  
>  ////////////////////////////////////////////////////////////////////////////////
>  // nsISupports
>  
> +NS_IMPL_ISUPPORTS_INHERITED1(mozilla::a11y::RootAccessible,

do you have name conflicts here as well? if yes what kind of error do you get?

::: accessible/src/base/nsRootAccessible.h
@@ +49,5 @@
>  
>  class nsXULTreeAccessible;
>  class Relation;
>  
>  const PRInt32 SCROLL_HASH_START_SIZE = 6;

nit: it'd be nice if you remove it since nobody uses it

::: accessible/src/mac/Makefile.in
@@ +79,5 @@
>    mozAccessibleProtocol.h \
>    mozActionElements.h \
>    mozTextAccessible.h \
>    nsRoleMap.h \
> +  RootAccessibleWrap.h \

no need to export it, right?

::: accessible/src/mac/nsRootAccessibleWrap.mm
@@ +42,5 @@
>  
>  #include "nsCOMPtr.h"
>  #include "nsObjCExceptions.h"
>  #include "nsIWidget.h"
>  #include "nsIViewManager.h"

should you add using namespace mozilla::a11y?

@@ +56,5 @@
>  {
>  }
>  
>  Class
> +RootAccessibleWrap::GetNativeType ()

nit: no space before ()

@@ +66,5 @@
>    NS_OBJC_END_TRY_ABORT_BLOCK_NIL;
>  }
>  
>  void
> +RootAccessibleWrap::GetNativeWidget (void **aOutView)

same and type* name please

::: accessible/src/mac/mozDocAccessible.mm
@@ +41,4 @@
>  
>  #import "mozDocAccessible.h"
>  
>  #import "mozView.h"

you should add using namespace mozilla::a11y I think

@@ +42,5 @@
>  #import "mozDocAccessible.h"
>  
>  #import "mozView.h"
>  
> +static id <mozAccessible, mozView> getNativeViewFromRootAccessible (nsAccessible* accessible)

please no space before ( and 'accessible' -> aAccessible
function name on new line

::: accessible/src/msaa/Makefile.in
@@ +100,5 @@
>    ia2AccessibleHypertext.h \
>    CAccessibleTable.h \
>    CAccessibleTableCell.h \
>    CAccessibleValue.h \
> +  RootAccessibleWrap.h \

no export?

::: accessible/src/msaa/nsAccessibleWrap.cpp
@@ +66,5 @@
>  #include "nsIViewManager.h"
>  #include "nsRoleMap.h"
>  #include "nsEventMap.h"
>  #include "nsArrayUtils.h"
> +#include "RootAccessible.h"

please keep it in accessible inlcudes block

@@ +1857,5 @@
>    // Move the system caret so that Windows Tablet Edition and tradional ATs with 
>    // off-screen model can follow the caret
>    ::DestroyCaret();
>  
> +  mozilla::a11y::RootAccessible* rootAccessible = RootAccessible();

name conflict? if yes then a11y::RootAccessible

::: accessible/src/msaa/nsDocAccessibleWrap.cpp
@@ +279,5 @@
>      if (nsCoreUtils::IsTabDocument(mDocument)) {
>        mozilla::dom::TabChild* tabChild =
>          mozilla::dom::GetTabChildFrom(mDocument->GetShell());
>  
> +      mozilla::a11y::RootAccessible* rootDocument = RootAccessible();

same

::: accessible/src/msaa/nsWinUtils.cpp
@@ +41,5 @@
>  #include "nsWinUtils.h"
>  
>  #include "Compatibility.h"
>  #include "nsIWinAccessNode.h"
> +#include "RootAccessible.h"

it appears it's not used in this file, makes sense to remove this include

::: accessible/src/other/Makefile.in
@@ +65,5 @@
>    nsXULTreeGridAccessibleWrap.h \
>    nsHyperTextAccessibleWrap.h \
>    nsHTMLImageAccessibleWrap.h \
>    nsHTMLTableAccessibleWrap.h \
> +  RootAccessibleWrap.h \

no export?

::: accessible/src/other/nsRootAccessibleWrap.h
@@ +47,5 @@
>  
> +namespace mozilla {
> +namespace a11y {
> +
> +class RootAccessibleWrap: public RootAccessible

space before :

Comment 14

[Mark Capella [:capella]]

FYI:

::: accessible/src/msaa/nsWinUtils.cpp
@@ +41,5 @@
> +#include "RootAccessible.h"
it appears it's not used in this file, makes sense to remove this include

Build fails without it, lotsa missing stuff, so i put it back.

Comment 15

[Mark Capella [:capella]]

FYI:

Trying to resolve name conflicts from mozilla::a11y::RootAccessible* rootAccessible = RootAccessible();
To: a11y::RootAccessible* rootAccessible = RootAccessible();

Results in foo.cpp(###) : error C2653: 'a11y' : is not a class or namespace name

Comment 16

[alexander :surkov (:asurkov)]

(In reply to Mark Capella [:capella] from comment #14)
> > +#include "RootAccessible.h"
> it appears it's not used in this file, makes sense to remove this include
> 
> Build fails without it, lotsa missing stuff, so i put it back.

ok, you could add missed dependencies but it's up to you.

(In reply to Mark Capella [:capella] from comment #15)
> Trying to resolve name conflicts from mozilla::a11y::RootAccessible*
> rootAccessible = RootAccessible();
> To: a11y::RootAccessible* rootAccessible = RootAccessible();
> 
> Results in foo.cpp(###) : error C2653: 'a11y' : is not a class or namespace
> name

please make sure there's "using namespace mozilla" there.

Comment 17

[Mark Capella [:capella]]

Attachment: Patch (v5)

Re: Trying to resolve name conflicts by abbreviating mozilla::a11y:: to a11y:: 

I did verify that there is a "using namespace" in the files so I'm not sure why this seems to need to be coded as is.

Re: nsRefPtr<NativeRootAccessibleWrap> nativeRootAcc = new NativeRootAccessibleWrap((AtkObject*)aAtkAccessible);
please use static_cast<>

Can you tell me explicitly how? I'm not familiar here ...

Re: ::: accessible/src/base/nsRootAccessible.cpp
> +NS_IMPL_ISUPPORTS_INHERITED1(mozilla::a11y::RootAccessible,
do you have name conflicts here as well? if yes what kind of error do you get?

Seems to be something left over from test-building that I forgot to remove ... it's fixed back now.

All other issues have been addressed / changed, and are included in this version of the patch.

Comment 18

[alexander :surkov (:asurkov)]

(In reply to Mark Capella [:capella] from comment #17)
> Created attachment 620298 [details] [diff] [review]
> Patch (v5)
> 
> Re: Trying to resolve name conflicts by abbreviating mozilla::a11y:: to
> a11y:: 
> 
> I did verify that there is a "using namespace" in the files so I'm not sure
> why this seems to need to be coded as is.

ok, ignore that.

> Re: nsRefPtr<NativeRootAccessibleWrap> nativeRootAcc = new
> NativeRootAccessibleWrap((AtkObject*)aAtkAccessible);
> please use static_cast<>
> 
> Can you tell me explicitly how? I'm not familiar here ...

static_cast<AtkObject*>(aAtkAccessible) instead implict c style AtkObject*)aAtkAccessible

Comment 19

[Trevor Saunders (:tbsaunde)]

Comment on attachment 620298 [details] [diff] [review]
Patch (v5)

> 
> #include "nsMai.h"
>-#include "nsRootAccessibleWrap.h"
>+#include "RootAccessibleWrap.h"

it should come first

> 
>-  nsRootAccessible* rootAccessible = RootAccessible();
>+  mozilla::a11y::RootAccessible* rootAccessible = RootAccessible();

I suspect you need to namespace this to distinguish the variable from the type.  I think you need mozilla:: because this file only has using mozilla::a11y and not using mozilla

>         mozilla::dom::GetTabChildFrom(mDocument->GetShell());
> 
>-      nsRootAccessible* rootDocument = RootAccessible();
>+      mozilla::a11y::RootAccessible* rootDocument = RootAccessible();

also not using mozilla in this file

>--- a/accessible/src/msaa/nsWinUtils.cpp
>+++ b/accessible/src/msaa/nsWinUtils.cpp
>@@ -37,17 +37,17 @@
>  * the terms of any one of the MPL, the GPL or the LGPL.
>  *
>  * ***** END LICENSE BLOCK ***** */
> 
> #include "nsWinUtils.h"
> 
> #include "Compatibility.h"
> #include "nsIWinAccessNode.h"
>-#include "nsRootAccessible.h"
>+#include "RootAccessible.h"

someone want to file a bug to directly include what we actually need?

> 
> #include "nsCOMPtr.h"
>-#include "nsRootAccessibleWrap.h"
> #include "nsIServiceManager.h"
> #include "nsIAccessibilityService.h"
>+#include "RootAccessibleWrap.h"
> 
> ////////////////////////////////////////////////////////////////////////////////
>-// nsRootAccessibleWrap
>+// RootAccessibleWrap

this file's header should come first

However we really should just make this class a typedef to RootAccessible

Comment 20

[alexander :surkov (:asurkov)]

I'll fix these before landing

Comment 21

[Mark Capella [:capella]]

Thank-you both !  :)

Comment 22

[alexander :surkov (:asurkov)]

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

Comment 23

[(no longer active)]

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