On Wed, Oct 28, 2009 at 1:55 PM, Weygandt, Jon <[email protected]> wrote:
> We modify the rewriter slightly, plus have explored doing some inline > rendering. The only part we really need to change is the "rewrite" > method, provided the other methods are protected, while that is not a > great method for reuse, the other choice is to copy the entire class > which causes more problems to keep our custom code in sync with Shindig > versions. > Thanks for the info, that's what I was curious to know. I'm happy changing these to protected and will update with a patch soonish. > > Your changes are quite significant, what I would like to do, is to take > your patches and apply them to our build and see what the effect is, I > know there must be some impact. If I don't get to that before you are > ready to check in, that's OK. I'll simply recommend the > private/protected by a JIRA bug. > Yep, I suspect we'll have some impact on our side as well. Let me know if you run into any big blockers. Cheers, John > > -----Original Message----- > From: [email protected] [mailto:[email protected]] > Sent: Wednesday, October 28, 2009 1:15 PM > To: [email protected]; [email protected]; [email protected] > Subject: Re: Feature/JS System Rework > > Thanks for the comments, Jon. I'll update a patch shortly that > incorporates your fixes. What I haven't done (locally) yet is make > RenderingGadgetRewriter methods protected. What was the rationale there > again? Seems reasonable, just curious. > > > http://codereview.appspot.com/143046/diff/80/1043 > File > java/gadgets/src/main/java/org/apache/shindig/gadgets/features/FeatureRe > gistry.java > (right): > > http://codereview.appspot.com/143046/diff/80/1043#newcode177 > Line 177: public List<FeatureResource> getFeatureResources( Updated > patch forthcoming. I thought this logic was weird too when I ported it > over. > > http://codereview.appspot.com/143046/diff/80/1033 > File > java/gadgets/src/main/java/org/apache/shindig/gadgets/render/RenderingGa > dgetRewriter.java > (right): > > http://codereview.appspot.com/143046/diff/80/1033#newcode185 > Line 185: throw new RuntimeException(e); Agreed... I'll merge this in. > > http://codereview.appspot.com/143046 >
