LGTM This is pretty exciting.
On Wed, Mar 23, 2011 at 11:05 AM, <[email protected]> wrote: > 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
