Last Comment Bug 743676 - densify base/nsFormControlAccessible
: densify base/nsFormControlAccessible
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)
: mozilla14
Assigned To: Max Li [:maxli]
:
Mentors:
Depends on:
Blocks: densifya11y
  Show dependency treegraph
 
Reported: 2012-04-09 07:56 PDT by alexander :surkov
Modified: 2012-04-14 06:41 PDT (History)
6 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch (v1) (19.39 KB, patch)
2012-04-10 18:51 PDT, Max Li [:maxli]
surkov.alexander: feedback+
Details | Diff | Splinter Review
Patch v2 (84.13 KB, patch)
2012-04-11 18:53 PDT, Max Li [:maxli]
tbsaunde+mozbugs: review+
Details | Diff | Splinter Review
Patch v3 (84.76 KB, patch)
2012-04-12 04:28 PDT, Max Li [:maxli]
no flags Details | Diff | Splinter Review
Clang bustage fix (856 bytes, patch)
2012-04-13 14:21 PDT, Jeff Walden [:Waldo] (remove +bmo to email)
tbsaunde+mozbugs: review+
Details | Diff | Splinter Review

Description alexander :surkov 2012-04-09 07:56:41 PDT
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] 2012-04-10 18:51:44 PDT
Created attachment 613845 [details] [diff] [review]
Patch (v1)
Comment 2 alexander :surkov 2012-04-10 23:08:17 PDT
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] 2012-04-11 18:53:03 PDT
Created attachment 614250 [details] [diff] [review]
Patch v2

Fixed nits and densified nsHTMLFormControlAccessible and nsXULFormControlAccessible
Comment 4 Trevor Saunders (:tbsaunde) 2012-04-12 00:33:34 PDT
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] 2012-04-12 04:28:16 PDT
Created attachment 614330 [details] [diff] [review]
Patch v3

Fixed nits
Comment 6 alexander :surkov 2012-04-13 03:32:31 PDT
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 8 Boris Zbarsky [:bz] 2012-04-13 13:46:19 PDT
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) 2012-04-13 14:07:43 PDT
(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] (remove +bmo to email) 2012-04-13 14:21:47 PDT
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).
Comment 11 Trevor Saunders (:tbsaunde) 2012-04-13 14:31:18 PDT
(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 [:bz] 2012-04-13 14:34:59 PDT
> btw what is up with those paths? 

Our build uses relative paths throughout so that ccache across srcdirs works.
Comment 13 Jeff Walden [:Waldo] (remove +bmo to email) 2012-04-13 14:37:56 PDT
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] (remove +bmo to email) 2012-04-13 14:42:06 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/6eca3199cbcf
Comment 15 Trevor Saunders (:tbsaunde) 2012-04-13 14:55:32 PDT
(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 (⌚ UTC+1/+2) 2012-04-14 06:40:34 PDT
https://hg.mozilla.org/mozilla-central/rev/6eca3199cbcf
Comment 17 Marco Bonardo [::mak] (Away 6-20 Aug) 2012-04-14 06:41:44 PDT
and https://hg.mozilla.org/mozilla-central/rev/86845b70c936

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