Yes, he and I already discussed it. I was initially trying to avoid needing a new XML tag when the existing one was such a near fit, but the legacy linker support issues convinced me we needed it.
On Tue, Jun 9, 2009 at 5:37 PM, Bruce Johnson <[email protected]> wrote: > 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 -~----------~----~----~----~------~----~------~--~---
