I like what jat said. Freeland? On Tue, Jun 9, 2009 at 5:33 PM, <[email protected]> wrote:
> > The alternative to this would be Bruce's suggestion of defining a > specific fallback value for a selection property rather than using > config properties for it. That would narrow the scope to exactly what > we know we want to support and simplify the linker changes and avoids > the breaking API change for existing linkers (where they have to supply > the config properties to avoid breaking, which means the same linker > can't work for GWT 1.6 and GWT 2.0). > > Something like: > <set-property-fallback name="locale" value="default/> > and the last set value wins. > > > http://gwt-code-reviews.appspot.com/34832/diff/1/2 > File > dev/core/src/com/google/gwt/core/ext/linker/impl/SelectionScriptLinker.java > (right): > > http://gwt-code-reviews.appspot.com/34832/diff/1/2#newcode194 > Line 194: new TreeSet<ConfigurationProperty>()); > It seems like allowing this could cause incorrect behavior. I know it > is a tradeoff of not breaking existing code, but if there is a linker > which calls this method, any code depending on config property lookup > will just break. > > http://gwt-code-reviews.appspot.com/34832/diff/1/2#newcode223 > Line 223: } > It looks like we are just substituting the empty string for unknown > properties, which is especially bad if the call is relayed through the > above method. > > http://gwt-code-reviews.appspot.com/34832/diff/1/4 > File dev/core/src/com/google/gwt/dev/cfg/StaticPropertyOracle.java > (right): > > http://gwt-code-reviews.appspot.com/34832/diff/1/4#newcode149 > Line 149: for (String v : prop.getAllowedValues()) { > setValues.addAll(cprop.getAllowedValues())? > > Also, maybe this should be computed in getPossibleValues() instead. > > http://gwt-code-reviews.appspot.com/34832/diff/1/5 > File > dev/core/src/com/google/gwt/dev/shell/ModuleSpacePropertyOracle.java > (right): > > http://gwt-code-reviews.appspot.com/34832/diff/1/5#newcode127 > Line 127: for (String v : cprop.getAllowedValues()) { > setValues.addAll(cprop.getAllowedValues())? > > Also, maybe this should be computed in getPossibleValues() instead. > > http://gwt-code-reviews.appspot.com/34832/diff/1/7 > File user/src/com/google/gwt/i18n/I18N.gwt.xml (right): > > http://gwt-code-reviews.appspot.com/34832/diff/1/7#newcode51 > Line 51: $wnd['__gwt_Locale'] = locale || > '/*-GWTCONFIGPROP(default.locale)-*/'; > I think we haven't nailed this down enough that we want anyone else > using it and expecting it to keep working. So, I would suggest a > comment here to that effect. > > http://gwt-code-reviews.appspot.com/34832/diff/1/7#newcode55 > Line 55: return "/*-GWTCONFIGPROP(default.locale)-*/"; > Since these can all be empty strings, I would suggest storing > "/*-GWTCONFIGPROP(default.locale)-*/" || 'default' in a variable and > reusing that. Things will fall over if the property provider returns a > value that is not valid. > > http://gwt-code-reviews.appspot.com/34832/diff/1/8 > File > user/src/com/google/gwt/i18n/rebind/AbstractLocalizableImplCreator.java > (right): > > http://gwt-code-reviews.appspot.com/34832/diff/1/8#newcode136 > Line 136: logger.log(Type.WARN, "default.locale has more than one value, > using " > Is this even possible since the config property definition says it isn't > multivalued? > > http://gwt-code-reviews.appspot.com/34832 > > > > --~--~---------~--~----~------------~-------~--~----~ http://groups.google.com/group/Google-Web-Toolkit-Contributors -~----------~----~----~----~------~----~------~--~---
