On Tue, Sep 1, 2009 at 2:11 PM, <[email protected]> wrote: > On 2009/09/01 17:25:49, johnfargo wrote: > >> This is looking great. A few questions: >> 1. Do you have a test for this? >> > > I have been working on the test today, but it seems to be more > complicated than I thought. What exactly do we want to test? On the > rendering side, it is actually exactly the same as > RenderngGadgetRewritter, and I don't think we really want to load the > actual data constant from resource to do the test. The only additional > work in this class is the logic of figuring out which file to load based > on the locale. Shall we test on that only?
That sounds reasonable. It's a shame we have so many static methods (DomUtil et al) in here making it tougher to test the DOM manipulation. > > > 2. What happens when an invalid/unmatched locale is passed? >> > > The data files I checked in covers all language/country that OpenSocial > support. For invalid/unmatched ones, it should fall back to ALL, and we > have the logic in this file to deal with that, so there shouldn't be of > any issue. True, but I also want to ensure that someone can't blow up a gadget server by passing in &lang=foo. > > > 3. Please add a simple cache (Map<Locale, String> will do) for the >> > retrieved > >> data so you don't need to reload the data from JARs all the time. >> > > > That is a great suggestion. I have made the change. > > Thanks! >> John >> > > > > http://codereview.appspot.com/109114 >
