I suggest moving the aria stuff out of client/ui.  Maybe
com.google.gwt.aria?


http://gwt-code-reviews.appspot.com/1624803/diff/1/user/src/com/google/gwt/user/cellview/client/SimplePager.java
File user/src/com/google/gwt/user/cellview/client/SimplePager.java
(right):

http://gwt-code-reviews.appspot.com/1624803/diff/1/user/src/com/google/gwt/user/cellview/client/SimplePager.java#newcode157
user/src/com/google/gwt/user/cellview/client/SimplePager.java:157:
extra spaces

http://gwt-code-reviews.appspot.com/1624803/diff/1/user/src/com/google/gwt/user/cellview/client/SimplePager.java#newcode186
user/src/com/google/gwt/user/cellview/client/SimplePager.java:186:
spaces

http://gwt-code-reviews.appspot.com/1624803/diff/1/user/src/com/google/gwt/user/cellview/client/SimplePager.java#newcode203
user/src/com/google/gwt/user/cellview/client/SimplePager.java:203:
static final String FAST_FORWARD_BUTTON_LABEL = "Fast forward";
These should be user definable, maybe in a Constants interface so they
can be internationalized.

http://gwt-code-reviews.appspot.com/1624803/diff/1/user/src/com/google/gwt/user/client/ui/aria/Attribute.java
File user/src/com/google/gwt/user/client/ui/aria/Attribute.java (right):

http://gwt-code-reviews.appspot.com/1624803/diff/1/user/src/com/google/gwt/user/client/ui/aria/Attribute.java#newcode34
user/src/com/google/gwt/user/client/ui/aria/Attribute.java:34: private
Set<String> allowedValues;
Instead of taking a Set of allowedValues, Attribute can be parameterized
to a type.  Each set of allowed values can be an enum type.  Open ended
strings and integers can just use String and Integer.  You can create a
TriState enum the is shared across tristate values, or just use Boolean
and have an option to allow/disallow null.

http://gwt-code-reviews.appspot.com/1624803/diff/1/user/src/com/google/gwt/user/client/ui/aria/Attribute.java#newcode110
user/src/com/google/gwt/user/client/ui/aria/Attribute.java:110: assert
element != null : "Element to cannot be null.";
/r/Element to/Element

http://gwt-code-reviews.appspot.com/1624803/diff/1/user/src/com/google/gwt/user/client/ui/aria/AttributeValueType.java
File user/src/com/google/gwt/user/client/ui/aria/AttributeValueType.java
(right):

http://gwt-code-reviews.appspot.com/1624803/diff/1/user/src/com/google/gwt/user/client/ui/aria/AttributeValueType.java#newcode67
user/src/com/google/gwt/user/client/ui/aria/AttributeValueType.java:67:
+ value.getClass().getName();
Calling getClass().getName() will cause the compiler to include the name
of ALL classes in the compiled output.  The compiler might be smart
enough to realize that this cannot be called because assertions are
disabled, but we should check that.

http://gwt-code-reviews.appspot.com/1624803/diff/1/user/src/com/google/gwt/user/client/ui/aria/Property.java
File user/src/com/google/gwt/user/client/ui/aria/Property.java (right):

http://gwt-code-reviews.appspot.com/1624803/diff/1/user/src/com/google/gwt/user/client/ui/aria/Property.java#newcode46
user/src/com/google/gwt/user/client/ui/aria/Property.java:46:
AUTOCOMPLETE("aria-autocomplete", "none", new String [] {"inline",
"list", "both", "none"}),
Instead of limiting the allowed values to a list of strings, we should
create an enum for each attribute that has a specific set of values.
Then it would be a compile time check instead of a runtime check.

http://gwt-code-reviews.appspot.com/1624803/diff/1/user/src/com/google/gwt/user/client/ui/aria/Role.java
File user/src/com/google/gwt/user/client/ui/aria/Role.java (right):

http://gwt-code-reviews.appspot.com/1624803/diff/1/user/src/com/google/gwt/user/client/ui/aria/Role.java#newcode53
user/src/com/google/gwt/user/client/ui/aria/Role.java:53: * @param
patentRoles Parent roles. One role can inherit from one or more patent
roles
/r/patentRoles/parentRoles

http://gwt-code-reviews.appspot.com/1624803/diff/1/user/src/com/google/gwt/user/client/ui/aria/Roles.java
File user/src/com/google/gwt/user/client/ui/aria/Roles.java (right):

http://gwt-code-reviews.appspot.com/1624803/diff/1/user/src/com/google/gwt/user/client/ui/aria/Roles.java#newcode61
user/src/com/google/gwt/user/client/ui/aria/Roles.java:61: public final
class Roles {
These Roles would be much better as a set of interfaces that implement
each other.  That would make the hierarchy clearer in Java.  I think its
weird to pass parent roles as an argument, which essentially turns a
compile time Java hierarchy into a Runtime hierarchy.

We do similar things for Elements.  Each element has its own interface,
and they extend from each other.

http://gwt-code-reviews.appspot.com/1624803/

--
http://groups.google.com/group/Google-Web-Toolkit-Contributors

Reply via email to