Thanks for the quick update, looking now. One thought (doesn't gate this patch): I wonder if your code bloat problem would go away if your Widgets classes were JSOs.
On Wed, Apr 20, 2011 at 9:03 AM, <her...@google.com> wrote: > > > http://gwt-code-reviews.appspot.com/1420804/diff/1/user/src/com/google/gwt/uibinder/client/UiBinderUtil.java > File user/src/com/google/gwt/uibinder/client/UiBinderUtil.java (right): > > > http://gwt-code-reviews.appspot.com/1420804/diff/1/user/src/com/google/gwt/uibinder/client/UiBinderUtil.java#newcode44 > user/src/com/google/gwt/uibinder/client/UiBinderUtil.java:44: element = > com.google.gwt.dom.client.Document.get() > On 2011/04/19 21:07:20, rjrjr wrote: > >> no need for the long name, this is imported >> > > Done. > > > > http://gwt-code-reviews.appspot.com/1420804/diff/1/user/src/com/google/gwt/uibinder/elementparsers/LazyPanelParser.java > File > user/src/com/google/gwt/uibinder/elementparsers/LazyPanelParser.java > (right): > > > http://gwt-code-reviews.appspot.com/1420804/diff/1/user/src/com/google/gwt/uibinder/elementparsers/LazyPanelParser.java#newcode38 > user/src/com/google/gwt/uibinder/elementparsers/LazyPanelParser.java:38: > > On 2011/04/19 21:07:20, rjrjr wrote: > >> assert writer.useAttachableStrategyMabobThing(); >> > > Well, the parser is not registered if the flag is disabled so this point > is never reached. But did the change anyway since this way I can spit > out a better warning message. > > > > http://gwt-code-reviews.appspot.com/1420804/diff/1/user/src/com/google/gwt/uibinder/elementparsers/LazyPanelParser.java#newcode40 > user/src/com/google/gwt/uibinder/elementparsers/LazyPanelParser.java:40: > String widgetClassPath = writer.getClassPath(Widget.class); > On 2011/04/19 21:07:20, rjrjr wrote: > >> just use Widget.class.getName(), getClassPath adds no value I can see. >> > > Done. > > > > http://gwt-code-reviews.appspot.com/1420804/diff/1/user/src/com/google/gwt/uibinder/elementparsers/LazyPanelParser.java#newcode42 > user/src/com/google/gwt/uibinder/elementparsers/LazyPanelParser.java:42: > String code = null; > On 2011/04/19 21:07:20, rjrjr wrote: > >> Use XMLElement#consumeSingleChildElement and you don't need your own >> one-and-only-one checks. >> > > Done. > > > > http://gwt-code-reviews.appspot.com/1420804/diff/1/user/src/com/google/gwt/uibinder/elementparsers/WidgetInterpreter.java > File > user/src/com/google/gwt/uibinder/elementparsers/WidgetInterpreter.java > (right): > > > http://gwt-code-reviews.appspot.com/1420804/diff/1/user/src/com/google/gwt/uibinder/elementparsers/WidgetInterpreter.java#newcode95 > user/src/com/google/gwt/uibinder/elementparsers/WidgetInterpreter.java:95: > "com.google.gwt.uibinder.client.UiBinderUtil.LazyDomElement", > elementPointer); > On 2011/04/19 21:07:20, rjrjr wrote: > >> Please use LazyDomElement.class.getName() >> > > Done. > > > > http://gwt-code-reviews.appspot.com/1420804/diff/1/user/src/com/google/gwt/uibinder/elementparsers/WidgetInterpreter.java#newcode98 > user/src/com/google/gwt/uibinder/elementparsers/WidgetInterpreter.java:98: > "new com.google.gwt.uibinder.client.UiBinderUtil.LazyDomElement(%s)", > On 2011/04/19 21:07:20, rjrjr wrote: > >> ditto >> > > Done. > > > > http://gwt-code-reviews.appspot.com/1420804/diff/1/user/src/com/google/gwt/uibinder/rebind/FieldManager.java > File user/src/com/google/gwt/uibinder/rebind/FieldManager.java (right): > > > http://gwt-code-reviews.appspot.com/1420804/diff/1/user/src/com/google/gwt/uibinder/rebind/FieldManager.java#newcode98 > user/src/com/google/gwt/uibinder/rebind/FieldManager.java:98: public > void addAttachStatement(String fieldName, String format, Object... args) > On 2011/04/19 21:07:20, rjrjr wrote: > >> Don't duplicate the FieldWriter api. To get them add a new >> > require(fieldName) > >> method that wraps lookup() and does the die thing. >> > > Done. > > > > http://gwt-code-reviews.appspot.com/1420804/diff/1/user/src/com/google/gwt/uibinder/rebind/FieldManager.java#newcode102 > user/src/com/google/gwt/uibinder/rebind/FieldManager.java:102: > logger.die("Failing adding attach statement. FieldName %s has no > FieldWriter associated.", > On 2011/04/19 21:07:20, rjrjr wrote: > >> die is for user error. If this is really developer error, make it a >> RuntimeException. >> > > Done. > > > > http://gwt-code-reviews.appspot.com/1420804/diff/1/user/src/com/google/gwt/uibinder/rebind/FieldManager.java#newcode112 > user/src/com/google/gwt/uibinder/rebind/FieldManager.java:112: public > void addDetachStatement(String fieldName, String format, Object... args) > On 2011/04/19 21:07:20, rjrjr wrote: > >> ditto >> > > Done. > > > > http://gwt-code-reviews.appspot.com/1420804/diff/1/user/src/com/google/gwt/uibinder/rebind/FieldManager.java#newcode126 > user/src/com/google/gwt/uibinder/rebind/FieldManager.java:126: public > FieldWriter addFieldStatement(String fieldName, String format, Object... > args) > On 2011/04/19 21:07:20, rjrjr wrote: > >> ditto >> > > Done. > > > > http://gwt-code-reviews.appspot.com/1420804/diff/1/user/src/com/google/gwt/uibinder/rebind/FieldManager.java#newcode143 > user/src/com/google/gwt/uibinder/rebind/FieldManager.java:143: public > String convertFieldToGetter(String fieldName) { > On 2011/04/19 21:07:20, rjrjr wrote: > >> Can this be a FieldWriter instance method? >> > > No because convertFieldToGetter() might be called before FieldWriter is > actually created, so for now this is the easier way. > > > > http://gwt-code-reviews.appspot.com/1420804/diff/1/user/src/com/google/gwt/uibinder/rebind/FieldManager.java#newcode155 > user/src/com/google/gwt/uibinder/rebind/FieldManager.java:155: * There > are 3 possible situations: > On 2011/04/19 21:07:20, rjrjr wrote: > >> Lists like this are more readable if you use <dl>, and drop the >> > colons. e.g. > > <dl> >> <dt>getter never called >> <dd> write builder only if... >> </dl> >> > > Done. > > > > http://gwt-code-reviews.appspot.com/1420804/diff/1/user/src/com/google/gwt/uibinder/rebind/FieldManager.java#newcode177 > user/src/com/google/gwt/uibinder/rebind/FieldManager.java:177: Integer > count = getGetterCounter(field.getName()); > On 2011/04/19 21:07:20, rjrjr wrote: > >> And this block can be an instance method on FieldWriter >> > > Done. > > > > http://gwt-code-reviews.appspot.com/1420804/diff/1/user/src/com/google/gwt/uibinder/rebind/FieldManager.java#newcode314 > user/src/com/google/gwt/uibinder/rebind/FieldManager.java:314: public > void setFieldInitializer(String fieldName, String format, Object... > args) > On 2011/04/19 21:07:20, rjrjr wrote: > >> ditto >> > > Done. > > > > http://gwt-code-reviews.appspot.com/1420804/diff/1/user/src/com/google/gwt/uibinder/rebind/FieldManager.java#newcode379 > user/src/com/google/gwt/uibinder/rebind/FieldManager.java:379: private > int getGetterCounter(String fieldName) { > On 2011/04/19 21:07:20, rjrjr wrote: > >> Wouldn't this count be more natural as a field on the FieldWriter? >> > > see my previous comment. > > > > http://gwt-code-reviews.appspot.com/1420804/diff/1/user/src/com/google/gwt/uibinder/rebind/FieldWriter.java > File user/src/com/google/gwt/uibinder/rebind/FieldWriter.java (right): > > > http://gwt-code-reviews.appspot.com/1420804/diff/1/user/src/com/google/gwt/uibinder/rebind/FieldWriter.java#newcode41 > user/src/com/google/gwt/uibinder/rebind/FieldWriter.java:41: * Add a > statement to be executed right after the current field is attached. > On 2011/04/19 21:07:20, rjrjr wrote: > >> Thanks, this is helpful. >> > > Done. > > > > http://gwt-code-reviews.appspot.com/1420804/diff/1/user/src/com/google/gwt/uibinder/rebind/FieldWriter.java#newcode66 > user/src/com/google/gwt/uibinder/rebind/FieldWriter.java:66: * Add a > statement to be executed right after the current field is detached. > Example: > On 2011/04/19 21:07:20, rjrjr wrote: > >> lose "Example:" >> > > Done. > > > > http://gwt-code-reviews.appspot.com/1420804/diff/1/user/src/com/google/gwt/uibinder/rebind/FieldWriter.java#newcode155 > user/src/com/google/gwt/uibinder/rebind/FieldWriter.java:155: * private > WidgetX get_widgetX() { > On 2011/04/19 21:07:20, rjrjr wrote: > >> I'm wondering if some of your code size penalty will go away if you >> > simply don't > >> write get_ methods for only-once widgets. Make them work as if >> useAttachableStrategy is false. Needn't gate submitting this since you >> > put the > >> switch in, just something to think about. >> > > OK. > > > > http://gwt-code-reviews.appspot.com/1420804/diff/1/user/src/com/google/gwt/uibinder/rebind/FieldWriter.java#newcode164 > user/src/com/google/gwt/uibinder/rebind/FieldWriter.java:164: * Notice > that there's no field class and the getter just fallback to > On 2011/04/19 21:07:20, rjrjr wrote: > >> "field class" You mean just "field"? >> > > Done. > > > > http://gwt-code-reviews.appspot.com/1420804/diff/1/user/src/com/google/gwt/uibinder/rebind/UiBinderGenerator.java > File user/src/com/google/gwt/uibinder/rebind/UiBinderGenerator.java > (right): > > > http://gwt-code-reviews.appspot.com/1420804/diff/1/user/src/com/google/gwt/uibinder/rebind/UiBinderGenerator.java#newcode50 > user/src/com/google/gwt/uibinder/rebind/UiBinderGenerator.java:50: > private static final String ELEMENT_FACTORY_PROPERTY = > "uibinder.html.elementfactory"; > On 2011/04/19 21:07:20, rjrjr wrote: > >> You're stale, this field is gone now. >> > > Done. > > > > http://gwt-code-reviews.appspot.com/1420804/diff/1/user/src/com/google/gwt/uibinder/rebind/UiBinderGenerator.java#newcode166 > user/src/com/google/gwt/uibinder/rebind/UiBinderGenerator.java:166: // > TODO(hermes): poor naming > On 2011/04/19 21:07:20, rjrjr wrote: > >> How about useLazyWidgetBuilders >> > > Done. > > > > http://gwt-code-reviews.appspot.com/1420804/diff/1/user/src/com/google/gwt/uibinder/rebind/UiBinderWriter.java > File user/src/com/google/gwt/uibinder/rebind/UiBinderWriter.java > (right): > > > http://gwt-code-reviews.appspot.com/1420804/diff/1/user/src/com/google/gwt/uibinder/rebind/UiBinderWriter.java#newcode129 > user/src/com/google/gwt/uibinder/rebind/UiBinderWriter.java:129: public > static String extractFieldFromCode(String code) { > On 2011/04/19 21:07:20, rjrjr wrote: > >> Does this really need to be public? >> > > Moved to where it's called. > > > > http://gwt-code-reviews.appspot.com/1420804/diff/1/user/src/com/google/gwt/uibinder/rebind/UiBinderWriter.java#newcode322 > user/src/com/google/gwt/uibinder/rebind/UiBinderWriter.java:322: // I'm > expliciting over-simplifying this and assuming that the input > On 2011/04/19 21:07:20, rjrjr wrote: > >> Use /* for long comments. And doesn't this comment belong in >> extractFieldFromCode? >> > > I'd rather having them together, so I'm moving the code from > extractFieldFromCode to here. > > > > http://gwt-code-reviews.appspot.com/1420804/diff/1/user/src/com/google/gwt/uibinder/rebind/UiBinderWriter.java#newcode329 > user/src/com/google/gwt/uibinder/rebind/UiBinderWriter.java:329: } catch > (UnableToCompleteException e) { > On 2011/04/19 21:07:20, rjrjr wrote: > >> Why is this okay? >> > > Not OK, sort of done. > > > > http://gwt-code-reviews.appspot.com/1420804/diff/1/user/src/com/google/gwt/uibinder/rebind/UiBinderWriter.java#newcode364 > user/src/com/google/gwt/uibinder/rebind/UiBinderWriter.java:364: * > @param fieldName The name of the field being declared > On 2011/04/19 21:07:20, rjrjr wrote: > >> @param ancestorField ... >> > > Done. > > > > http://gwt-code-reviews.appspot.com/1420804/diff/1/user/src/com/google/gwt/uibinder/rebind/UiBinderWriter.java#newcode404 > user/src/com/google/gwt/uibinder/rebind/UiBinderWriter.java:404: > domField.setBuildPrecendence(2); > On 2011/04/19 21:07:20, rjrjr wrote: > >> comment explaining why >> > > Done. I really hated this but it's the easier way of determining which > builders should come first. > > > > http://gwt-code-reviews.appspot.com/1420804/diff/1/user/src/com/google/gwt/uibinder/rebind/UiBinderWriter.java#newcode624 > user/src/com/google/gwt/uibinder/rebind/UiBinderWriter.java:624: public > String getClassPath(Class clazz) { > On 2011/04/19 21:07:20, rjrjr wrote: > >> Really? What's wrong with clazz.getName()? Why have this method at >> > all? > > Not needed, old shit I forgot to get rid of. > > > > http://gwt-code-reviews.appspot.com/1420804/diff/1/user/src/com/google/gwt/uibinder/rebind/UiBinderWriter.java#newcode713 > user/src/com/google/gwt/uibinder/rebind/UiBinderWriter.java:713: return > (returnGetter ? fieldManager.convertFieldToGetter(fieldName) : > fieldName); > On 2011/04/19 21:07:20, rjrjr wrote: > >> Can convertFieldToGetter be an instant method on FieldWriter? >> > > If so, instead of the hacky returnGetter arg, you can make this method >> > return > >> the FieldWriter itself (still leaving the old one as a wrapper around >> > it to > >> avoid mass parser update -- maybe this is just >> > parseElement(XMLElement)?). Now > >> that you've made FieldManager more public might as well go whole hog. >> > > Either way, please update the javadoc here and below. >> > > See my previous comment, convertFieldToGetter() cannot be FieldWriter, > at least for now. Sometimes it is called before the field writer is > actually created. > > Can I leave a TODO? We must revisit this in the future when > useLazyWidgetBuilders become the main branch. > > > http://gwt-code-reviews.appspot.com/1420804/ > -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
