Hi Gagan, I've uploaded my patch for review here:
http://codereview.appspot.com/2004042/ The first patch set I added wasn't based right -- the second patch set should be properly based from the current trunk. Thanks, --Jesse -----Original Message----- From: Gagandeep singh [mailto:[email protected]] Sent: Wednesday, August 18, 2010 1:20 PM To: [email protected] Subject: Re: [VOTE] Release Apache Shindig Version 2.0.0-RC2 Hi Jesse * *We have also found similar issues, and started on a similar codereview<http://codereview.appspot.com/1888046/> to address the issue. As you rightly mentioned, the problem is that makeGadget(Uri) does not have the gadget context, and hence the container information. We were pondering over the idea of removing makeGadget(Uri) method completely as its really confusing. It masks the subtle bug of not passing the gadget context, and we decided to give it more thought and review before opening up the patch to dev community. I took a quick glance at the patch you have mentioned in the jira issue and its in a nice shape. Some small nitpicks regarding to accel handling though. Would be very nice if you could upload it on http://codereview.appspot.com/. This would help the shindig community review it better. On Wed, Aug 18, 2010 at 1:49 AM, Ciancetta, Jesse E. <[email protected]>wrote: > I ran into another container configuration related issue while testing out > RC2, which is that the CSS rewriter doesn't seem to be taking the container > into account when rewriting embedded URL's -- it always uses the default > container configuration which ends up giving me invalid proxy URL's in my > rewritten CSS. This appears to be due to the fact that the > CssResponseRewriter uses the DomWalker.makeGadget method to create a Gadget > instance to pass to the ProxyUriManager.ProxyUri constructor, but the > GadgetContext inside that Gadget instance doesn't have the container set, > which makes code that runs later in the ProxyUriManager to generate the > proxy URL use the default container configuration instead of my custom > container configuration. > > I was able to come up with a patch to work around this issue, but I'm not > sure if it's the best way to go about it... There is a lot of code around > the whole gadget rewriting process and I definitely don't have my head > wrapped around all of it -- but I thought I'd take a crack at it anyway, if > for nothing else to at least highlight where the issue is occurring. The > two major changes I made were to add another parameter to the > DomWalker.makeGadget method for the container, and passing the container > down through the call stack in the CssResponseRewriter. For any other > usages of the affected classes which broke -- if there was an obvious place > to pull the container from (like a Gadget instance) I used it -- otherwise I > just passed in null to trigger the default behavior (which is to return the > default container). > > I uploaded the patch file (which I based against the RC2 sources) to JIRA > here: > > https://issues.apache.org/jira/browse/SHINDIG-1411 > > --Jesse > > -----Original Message----- > From: Ciancetta, Jesse E. [mailto:[email protected]] > Sent: Tuesday, August 17, 2010 8:50 AM > To: [email protected] > Subject: RE: [VOTE] Release Apache Shindig Version 2.0.0-RC2 > > That was my problem... Updating those values in my extended container > configuration resolved the issue. > > I had started out trying to take the RC2 jar files and dropping them into > my custom deployment and was having problems, so I decided to just drop back > to using the out of the box WAR file without modifications. And since > rendering with "container=default" seemed to be working I never really took > a close look at what the default values were in container.js. What still > seems odd though is that things *do* seem to work even with those default > values when using the default container, but when I added another container > which inherited from default things broke. I don't think it's worth trying > to chase down why that is though unless someone might happen to know > offhand... > > Thanks for the pointer in the right direction! > > --Jesse > > -----Original Message----- > From: John Hjelmstad [mailto:[email protected]] > Sent: Monday, August 16, 2010 5:32 PM > To: [email protected] > Subject: Re: [VOTE] Release Apache Shindig Version 2.0.0-RC2 > > What are your configured values for keys > > "gadgets.uri.proxy.host" > > "gadgets.uri.proxy.path" > > ? > > > On Mon, Aug 16, 2010 at 1:13 PM, Ciancetta, Jesse E. <[email protected] > >wrote: > > > Hi, > > > > I spent some time this morning trying to test out the RC2 build and ran > > into an issue trying to define additional containers (our internal > > implementation defines additional containers beyond "default" and I was > > trying to get that working). As a test I defined a new container called > > "foo" and tried using it to render gadgets. Rendering gadgets with the > > default container seemed to be mostly working, but whenever I specified > the > > foo container the gadget rewriters seemed to spit back invalid links. > > > > Here is what I did to test: > > > > -- Downloaded the RC2 WAR file from here: > > > > > https://repository.apache.org/content/repositories/orgapacheshindig-091/org/apache/shindig/shindig-server/2.0.0-RC2/shindig-server-2.0.0-RC2.war > > > > -- Renamed it to ROOT.war and dropped it into my Tomcat 6 webapps > directory > > > > -- Added my new container definition to the shindig.properties file: > > > > > > shindig.containers.default=res://containers/default/container.js,res://foo_container.js > > > > -- Created the foo_container.js container definition under > WEB-INF/classes > > with the following content (inheriting everything from the default > > container): > > {"gadgets.container" : ["foo"]} > > > > -- Tried rendering the New York Times gadget with the following URL's: > > > > > http://localhost:8080/gadgets/ifr?container=default&url=http://widgets.nytimes.com/packages/html/igoogle/topstories.xml(defaultcontainer) > > > > > http://localhost:8080/gadgets/ifr?container=foo&url=http://widgets.nytimes.com/packages/html/igoogle/topstories.xml(foocontainer) > > > > When rendering with the default container things looked mostly right > > (except it seemed to be missing some CSS styling), but with the foo > > container I got links back in the rendered gadget code that look > something > > like this: > > > > <link href="http://:/gadgets/proxy?container=foo&gadget=http%3A%2F% > > 2Fwidgets.nytimes.com > > > %2Fpackages%2Fhtml%2Figoogle%2Ftopstories.xml&debug=0&nocache=0&refresh=86400&url=http%3A%2F% > > 2Fgraphics8.nytimes.com%2Fcss%2Fapp%2Figoogle%2Fhome-merged.css" > > rel="stylesheet" type="text/css"> > > > > I thought maybe the inheritance from the default container configuration > > wasn't working properly so I tried copying all of its content directly > into > > the foo_container.js file (changing the container name from default to > foo) > > but I still ended up with the same issue. > > > > Anyone else seeing similar issues or have any other suggestions on what > > else to try? > > > > Thanks, > > > > --Jesse > > > > -----Original Message----- > > From: Paul Lindner [mailto:[email protected]] > > Sent: Tuesday, August 10, 2010 3:38 PM > > To: [email protected] > > Subject: [VOTE] Release Apache Shindig Version 2.0.0-RC2 > > > > Hi, > > > > We solved many issues: > > > > > https://issues.apache.org/jira/secure/ReleaseNote.jspa?version=12314413&styleName=Html&projectId=12310741 > > > > Project reports will get pushed to the main site soon. For now you can > > see: > > http://people.apache.org/~lindner/shindig-2.0.x/ > > > > Including the clean RAT > > report<http://people.apache.org/~lindner/shindig-2.0.x/rat-report.html> > > > > > > Staging repo: > > https://repository.apache.org/content/repositories/orgapacheshindig-091/ > > > > Sources: > > > > > https://repository.apache.org/content/repositories/orgapacheshindig-091/org/apache/shindig/shindig/2.0.0-RC2/ > > > > > > Web site: > > http://shindig.apache.org/ > > > > Vote open for 72 hours. > > > > [ ] +1 > > [ ] +0 > > [ ] -1 > > > > -- > > Paul Lindner -- [email protected] -- linkedin.com/in/plindner > > > >
