Note: There are a few cases of duplicates in user autocompletion which are being worked on.

densify base/nsFormControlAccessible

RESOLVED FIXED in mozilla14

Status

()

Core
Disability Access APIs
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: surkov, Assigned: maxli)

Tracking

(Blocks: 1 bug)

unspecified
mozilla14
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

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

Attachments

(2 attachments, 2 obsolete attachments)

(Reporter)

Description

5 years ago
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)
(Assignee)

Updated

5 years ago
Assignee: nobody → maxli
(Assignee)

Comment 1

5 years ago
Created attachment 613845 [details] [diff] [review]
Patch (v1)
Attachment #613845 - Flags: review?(trev.saunders)
(Reporter)

Comment 2

5 years ago
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 :
Attachment #613845 - Flags: feedback+
(Assignee)

Comment 3

5 years ago
Created attachment 614250 [details] [diff] [review]
Patch v2

Fixed nits and densified nsHTMLFormControlAccessible and nsXULFormControlAccessible
Attachment #613845 - Attachment is obsolete: true
Attachment #613845 - Flags: review?(trev.saunders)
Attachment #614250 - Flags: review?(trev.saunders)
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.
Attachment #614250 - Flags: review?(trev.saunders) → review+
(Assignee)

Comment 5

5 years ago
Created attachment 614330 [details] [diff] [review]
Patch v3

Fixed nits
Attachment #614250 - Attachment is obsolete: true
(Reporter)

Comment 6

5 years ago
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
(Reporter)

Comment 7

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/86845b70c936
Target Milestone: --- → mozilla14

Comment 8

5 years ago
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.
(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.
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).
Attachment #614910 - Flags: review?(trev.saunders)
(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.
Attachment #614910 - Flags: review?(trev.saunders) → review+

Comment 12

5 years ago
> btw what is up with those paths? 

Our build uses relative paths throughout so that ccache across srcdirs works.
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.
https://hg.mozilla.org/integration/mozilla-inbound/rev/6eca3199cbcf
(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.
https://hg.mozilla.org/mozilla-central/rev/6eca3199cbcf
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
and https://hg.mozilla.org/mozilla-central/rev/86845b70c936
You need to log in before you can comment on or make changes to this bug.