> On 2011-06-03 18:45:20, johnfargo wrote: > > http://svn.apache.org/repos/asf/shindig/trunk/java/common/src/main/java/org/apache/shindig/common/PropertiesModule.java, > > line 82 > > <https://reviews.apache.org/r/792/diff/2/?file=20132#file20132line82> > > > > Why remove the Jetty properties? Not a big deal for me, but curious > > anyway.
it's moved to BasicAuthority. I have to do this since fallback is done there and host/port from request takes priority first. > On 2011-06-03 18:45:20, johnfargo wrote: > > http://svn.apache.org/repos/asf/shindig/trunk/java/common/src/main/java/org/apache/shindig/common/servlet/Authority.java, > > line 17 > > <https://reviews.apache.org/r/792/diff/2/?file=20133#file20133line17> > > > > Nit: could call this "getOrigin()" since that term is typically used > > for schema://authority done. > On 2011-06-03 18:45:20, johnfargo wrote: > > http://svn.apache.org/repos/asf/shindig/trunk/java/common/src/main/java/org/apache/shindig/common/servlet/AuthorityProvider.java, > > line 44 > > <https://reviews.apache.org/r/792/diff/2/?file=20134#file20134line44> > > > > Why not rename this BasicAuthorityProvider, and just @Inject a > > BasicAuthority into it? Then, BasicAuthority itself would have @Inject on > > its ctor w/ the shindig.host and shindig.port values. > > > > Others could still use the class w/o the annotations. > > done. > On 2011-06-03 18:45:20, johnfargo wrote: > > http://svn.apache.org/repos/asf/shindig/trunk/java/common/src/main/java/org/apache/shindig/common/servlet/BasicAuthority.java, > > line 18 > > <https://reviews.apache.org/r/792/diff/2/?file=20135#file20135line18> > > > > nits: prefer 2-space indent thru this file done. > On 2011-06-03 18:45:20, johnfargo wrote: > > http://svn.apache.org/repos/asf/shindig/trunk/java/common/src/main/java/org/apache/shindig/common/servlet/BasicAuthority.java, > > line 45 > > <https://reviews.apache.org/r/792/diff/2/?file=20135#file20135line45> > > > > I'd recommend pulling jetty.port and jetty.host strings into public > > static final Strings at the top of the class. done. > On 2011-06-03 18:45:20, johnfargo wrote: > > http://svn.apache.org/repos/asf/shindig/trunk/java/common/src/main/java/org/apache/shindig/common/servlet/ServletRequestContext.java, > > line 32 > > <https://reviews.apache.org/r/792/diff/2/?file=20136#file20136line32> > > > > For better consistency, I'd put all these Strings into public static > > finals as well so that their value is defined only once throughout the > > class. done. > On 2011-06-03 18:45:20, johnfargo wrote: > > http://svn.apache.org/repos/asf/shindig/trunk/java/common/src/main/java/org/apache/shindig/common/servlet/ServletRequestContext.java, > > line 33 > > <https://reviews.apache.org/r/792/diff/2/?file=20136#file20136line33> > > > > getServerPort().toString() int.toString() will not work, used ""+<int> to convert int to String. > On 2011-06-03 18:45:20, johnfargo wrote: > > http://svn.apache.org/repos/asf/shindig/trunk/java/common/src/main/java/org/apache/shindig/common/servlet/BasicAuthority.java, > > line 39 > > <https://reviews.apache.org/r/792/diff/2/?file=20135#file20135line39> > > > > Interesting, so you're just using the jetty values as a fallback in the > > default impl? Suppose that's fine. yes. system properties first, values in request 2nd, fallback to jetty if neither is available. test cases works with jetty host/port. - li ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/792/#review756 ----------------------------------------------------------- On 2011-06-03 21:37:58, li xu wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/792/ > ----------------------------------------------------------- > > (Updated 2011-06-03 21:37:58) > > > Review request for shindig. > > > Summary > ------- > > This is a follow up for Issue 1534: > https://issues.apache.org/jira/browse/SHINDIG-1534 > > As discussed, default AuthorityProvider would implement following logic with > a Provider pattern > > 1. shindig.host & shindig.port defined in web.xml or system property > 2. host/port from HttpServletRequest > 3. Jetty host/port > > link to JIRA: https://issues.apache.org/jira/browse/SHINDIG-1541 > > > Diffs > ----- > > http://svn.apache.org/repos/asf/shindig/trunk/config/container.js 1127686 > > http://svn.apache.org/repos/asf/shindig/trunk/java/common/src/main/java/org/apache/shindig/common/PropertiesModule.java > 1127686 > > http://svn.apache.org/repos/asf/shindig/trunk/java/common/src/main/java/org/apache/shindig/common/servlet/Authority.java > PRE-CREATION > > http://svn.apache.org/repos/asf/shindig/trunk/java/common/src/main/java/org/apache/shindig/common/servlet/BasicAuthority.java > PRE-CREATION > > http://svn.apache.org/repos/asf/shindig/trunk/java/common/src/main/java/org/apache/shindig/common/servlet/BasicAuthorityProvider.java > PRE-CREATION > > http://svn.apache.org/repos/asf/shindig/trunk/java/common/src/main/java/org/apache/shindig/common/servlet/ServletRequestContext.java > 1127686 > > http://svn.apache.org/repos/asf/shindig/trunk/java/common/src/test/java/org/apache/shindig/common/servlet/BasicAuthorityProviderTest.java > PRE-CREATION > > http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/DefaultGuiceModule.java > 1127686 > > http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/uri/DefaultConcatUriManager.java > 1127686 > > http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/uri/DefaultIframeUriManager.java > 1127686 > > http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/uri/DefaultJsUriManager.java > 1127686 > > http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/uri/DefaultProxyUriManager.java > 1127686 > > http://svn.apache.org/repos/asf/shindig/trunk/java/server/src/main/webapp/WEB-INF/web.xml > 1127686 > > Diff: https://reviews.apache.org/r/792/diff > > > Testing > ------- > > built, passed unit tests. tested with > /samplecontainer/examples/commoncontainer/index.html > > > Thanks, > > li > >
