Moving the Default/Negative/Primary Decorations into DefaultAppearance makes the API much cleaner.
First, I removed all of the static factory methods (but left some JavaDoc explaining how to use the different constructors). Users can just use the default constructor, then call setDecoration() to make the button a primary/negative button. Second, we now GWT.create(Appearance.class) instead of GWT.create(DefaultAppearance.class). This is much better because users should be able to provide skins by implementing Appearance, without having to subclass DefaultAppearance. In general, we should always push configuration into ButtonCellBase, and leave the Appearance to just render the configuration. http://gwt-code-reviews.appspot.com/1383806/diff/13/user/src/com/google/gwt/cell/client/ButtonCellBase.java File user/src/com/google/gwt/cell/client/ButtonCellBase.java (right): http://gwt-code-reviews.appspot.com/1383806/diff/13/user/src/com/google/gwt/cell/client/ButtonCellBase.java#newcode53 user/src/com/google/gwt/cell/client/ButtonCellBase.java:53: public static interface Appearance<C> { On 2011/03/23 17:27:30, rjrjr wrote:
all interfaces are static, that's noise. Please remove static from all
these
interfaces
Done. http://gwt-code-reviews.appspot.com/1383806/diff/13/user/src/com/google/gwt/cell/client/TextButtonCell.java File user/src/com/google/gwt/cell/client/TextButtonCell.java (right): http://gwt-code-reviews.appspot.com/1383806/diff/13/user/src/com/google/gwt/cell/client/TextButtonCell.java#newcode71 user/src/com/google/gwt/cell/client/TextButtonCell.java:71: super(SimpleSafeHtmlRenderer.getInstance()); On 2011/03/23 17:27:30, rjrjr wrote:
Are you sure that this shared instance of SimpleSafeHtmlRenderer is
worth the
bother? I wonder about unnecessary clinits and effects on inlining.
Did you
actually check that SimpleSafeHtmlRenderer.getInstance() is cheaper
than new
SimpleSafeHtmlRenderer()?
I doubt it makes any difference since Cells are singletons, so their won't be too many on a page and one extra object creation is insignificant. Can we remember to take a look into the general question of singletons versus new instance of simple objects? It seems like thats a bigger question than this change, and I need to submit this asap. http://gwt-code-reviews.appspot.com/1383806/diff/13/user/src/com/google/gwt/user/widget/client/ButtonBase.java File user/src/com/google/gwt/user/widget/client/ButtonBase.java (right): http://gwt-code-reviews.appspot.com/1383806/diff/13/user/src/com/google/gwt/user/widget/client/ButtonBase.java#newcode98 user/src/com/google/gwt/user/widget/client/ButtonBase.java:98: return addHandler(handler, BlurEvent.getType()); On 2011/03/23 17:03:21, tbroyer wrote:
Shouldn't this be addDomHandler?
Done. http://gwt-code-reviews.appspot.com/1383806/diff/13/user/src/com/google/gwt/user/widget/client/package-info.java File user/src/com/google/gwt/user/widget/client/package-info.java (right): http://gwt-code-reviews.appspot.com/1383806/diff/13/user/src/com/google/gwt/user/widget/client/package-info.java#newcode21 user/src/com/google/gwt/user/widget/client/package-info.java:21: package com.google.gwt.user.widget.client; On 2011/03/23 17:27:30, rjrjr wrote:
Any reason not to make this com.google.gwt.widget?
Done. http://gwt-code-reviews.appspot.com/1383806/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
