LGTM2
On 2011/03/23 18:07:02, rjrjr wrote:
LGTM
This is pretty exciting.
On Wed, Mar 23, 2011 at 11:05 AM, <mailto:jlaba...@google.com> 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://gwt-code-reviews.appspot.com/1383806/
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
