743676 - densify base/nsFormControlAccessible

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[alexander :surkov (:asurkov)]

similar bug 739889

1) move base/nsFormControlAccessible -> generic/FormControlAccessible
2) remove 'ns' prefixes from nsRadioButtonAccessible and put classes into mozilla:a11y namespace
3) remove typedef nsLeafAccessible nsFormControlAccessible (i.e. use nsLeafAccessible directly)

Comment 1

[Max Li [:maxli]]

Attachment: Patch (v1)

Comment 2

[alexander :surkov (:asurkov)]

Comment on attachment 613845 [details] [diff] [review]
Patch (v1)

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

since you wrap classes from nsHTMLFormControlAccessible and nsXULFormControlAccessible by a11y namespace then you might want to densify those classes as well (and rename those files). If not then please file follow up bug on this. Btw, pls remove nsHTMLFormControlAccessible.h from export list in xul/Makefile.in

::: accessible/src/base/nsFormControlAccessible.cpp
@@ +224,5 @@
>  {
>    return 1;
>  }
>  
> +NS_IMETHODIMP RadioButtonAccessible::GetActionName(PRUint8 aIndex, nsAString& aName)

nit: pls put NS_IMETHODIMP on own line

::: accessible/src/base/nsFormControlAccessible.h
@@ +47,5 @@
>  /**
>    * Generic class used for progress meters.
>    */
>  template<int Max>
> +class ProgressMeterAccessible: public nsLeafAccessible

nit: pls space before :

Comment 3

[Max Li [:maxli]]

Attachment: Patch v2

Fixed nits and densified nsHTMLFormControlAccessible and nsXULFormControlAccessible

Comment 4

[Trevor Saunders (:tbsaunde)]

Comment on attachment 614250 [details] [diff] [review]
Patch v2

>   nsAccessible* accessible = 
>-    new nsHTMLButtonAccessible(aContent, 
>+    new HTMLButtonAccessible(aContent, 
>                                nsAccUtils::GetDocAccessibleFor(aPresShell));

nit, wrong indentation here and below.

>diff --git a/accessible/src/base/nsFormControlAccessible.cpp b/accessible/src/generic/FormControlAccessible.cpp
>rename from accessible/src/base/nsFormControlAccessible.cpp
>rename to accessible/src/generic/FormControlAccessible.cpp
>--- a/accessible/src/base/nsFormControlAccessible.cpp
>+++ b/accessible/src/generic/FormControlAccessible.cpp
>@@ -36,17 +36,17 @@
>  * the terms of any one of the MPL, the GPL or the LGPL.
>  *
>  * ***** END LICENSE BLOCK ***** */
> 
> // NOTE: alphabetically ordered
> 
> #include "Role.h"
> 
>-#include "nsFormControlAccessible.h"
>+#include "FormControlAccessible.h"

please include it first.

Comment 5

[Max Li [:maxli]]

Attachment: Patch v3

Fixed nits

Comment 6

[alexander :surkov (:asurkov)]

Comment on attachment 614330 [details] [diff] [review]
Patch v3

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

I'll fix nits before landing (no new patch is needed)

::: accessible/src/base/nsAccessibilityService.cpp
@@ +1626,5 @@
>    // This method assumes we're in an HTML namespace.
>    nsIAtom* tag = aContent->Tag();
>    if (tag == nsGkAtoms::figcaption) {
>      nsAccessible* accessible =
> +      new HTMLFigcaptionAccessible(aContent, aDoc);

nit: keep it on one line

::: accessible/src/html/nsHTMLFormControlAccessible.cpp
@@ +185,1 @@
>                                                          PRInt32 *aSetSize)

nit: type* name, and align second line properly

::: accessible/src/xul/nsXULFormControlAccessible.cpp
@@ +629,1 @@
>                                                           PRInt32 *aSetSize)

same

Comment 7

[alexander :surkov (:asurkov)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/86845b70c936

Comment 8

[Boris Zbarsky [:bzbarsky]]

The code checked in for this bug does not compile on clang:

FormControlAccessible.cpp
In file included from ../../../../mozilla/accessible/src/generic/FormControlAccessible.cpp:1:
../../../../mozilla/accessible/src/generic/FormControlAccessible.cpp:56:16: error: explicit instantiation of 'mozilla::a11y::ProgressMeterAccessible' must occur in namespace 'a11y'
template class ProgressMeterAccessible<1>;
               ^
../../../../mozilla/accessible/src/generic/FormControlAccessible.h:51:7: note: explicit instantiation refers here
class ProgressMeterAccessible : public nsLeafAccessible
      ^

and likewise for the other instantiation with 100.

Comment 9

[Trevor Saunders (:tbsaunde)]

(In reply to Boris Zbarsky (:bz) from comment #8)
> The code checked in for this bug does not compile on clang:
> 
> FormControlAccessible.cpp
> In file included from
> ../../../../mozilla/accessible/src/generic/FormControlAccessible.cpp:1:
> ../../../../mozilla/accessible/src/generic/FormControlAccessible.cpp:56:16:
> error: explicit instantiation of 'mozilla::a11y::ProgressMeterAccessible'
> must occur in namespace 'a11y'
> template class ProgressMeterAccessible<1>;
>                ^
> ../../../../mozilla/accessible/src/generic/FormControlAccessible.h:51:7:
> note: explicit instantiation refers here
> class ProgressMeterAccessible : public nsLeafAccessible
>       ^
> 
> and likewise for the other instantiation with 100.

ok, that's kind of odd, so the using is good enough for methods but not template instantiations???  I guess the fix is to use
template class mozilla::a11y::ProgressMeterAccessible<n>;
or if that doesn't work
namespace a11y {
template class ProgressMeterAccessibel<n>;
}
but I don't have clang setup so can't really test either of those.

btw what is up with those paths? iirc we don't export that header.

Comment 10

[Jeff Walden [:Waldo]]

Attachment: Clang bustage fix

Yeah, mozilla::a11y:: works, tested that this builds for me.

C++98 and C++11 are not quite clear about |using namespace X| not being enough to permit specialization of a class X::Y via simply |template class Y|, but it seems about the right inference to make from the spec language (since otherwise it wouldn't be clear which Y was meant, if multiple Y templates were in scope via usings and namespace blocks and such).

Comment 11

[Trevor Saunders (:tbsaunde)]

(In reply to Jeff Walden (:Waldo) (remove +bmo to email) from comment #10)
> Created attachment 614910 [details] [diff] [review]
> Clang bustage fix
> 
> Yeah, mozilla::a11y:: works, tested that this builds for me.
> 
> C++98 and C++11 are not quite clear about |using namespace X| not being
> enough to permit specialization of a class X::Y via simply |template class
> Y|, but it seems about the right inference to make from the spec language
> (since otherwise it wouldn't be clear which Y was meant, if multiple Y
> templates were in scope via usings and namespace blocks and such).

using not being enough sort of makes sense, but I would think using shouldn't be enough for methods either then since you could have two class y's one in namespace x and another at the top level, or something similar with multiple using decls.

anyway this seems fine if we need it to fix the build for people.

Comment 12

[Boris Zbarsky [:bzbarsky]]

> btw what is up with those paths? 

Our build uses relative paths throughout so that ccache across srcdirs works.

Comment 13

[Jeff Walden [:Waldo]]

For what it's worth, here's the (C++11, but C++98 appears identical) spec language:

An explicit instantiation of a class or function template specialization is placed in the namespace in which
the template is defined. An explicit instantiation for a member of a class template is placed in the namespace where the enclosing class template is defined. An explicit instantiation for a member template is placed in the namespace where the enclosing class or class template is defined. [ Example:

namespace N {
  template<class T> class Y { void mf() { } };
}

template class Y<int>;            // error: class template Y not visible
                                  // in the global namespace

using N::Y;
template class Y<int>;            // error: explicit instantiation outside of the
                                  // namespace of the template

template class N::Y<char*>;       // OK: explicit instantiation in namespace N
template void N::Y<double>::mf(); // OK: explicit instantiation
                                  // in namespace N
— end example ]

So function templates are covered identically to class templates.

Comment 14

[Jeff Walden [:Waldo]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/6eca3199cbcf

Comment 15

[Trevor Saunders (:tbsaunde)]

(In reply to Boris Zbarsky (:bz) from comment #12)
> > btw what is up with those paths? 
> 
> Our build uses relative paths throughout so that ccache across srcdirs works.

yeah, my brain saw ../../mozilla/ and autoparsed it as ../../mozilla/a11y/blah

> So function templates are covered identically to class templates.

ok, I haven't looked much at the spec at this topic and since it works probably isn't worth spending time on, but what I meant was its funny that the the member definitions a few lines down from those template instantiations are fine.

Comment 16

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

https://hg.mozilla.org/mozilla-central/rev/6eca3199cbcf

Comment 17

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

and https://hg.mozilla.org/mozilla-central/rev/86845b70c936