> On 2011-12-06 23:03:02, Stanton Sievers wrote: > > Overall this seems OK. > > > > Have you looked at the performance impact here using lifecycle logging? > > > > The extra HTTP round trip could be costly, especially none of the HEAD > > request can be cached and reused by the subsequent GET. Just throwing it > > out there, how much sense would it make to do a GET instead of a HEAD > > request to check for page availability? My reasoning here is that at least > > the browser will have cached the page contents from the initial GET making > > the subsequent GET more performant. In the error case the HEAD and GET > > will take approximately the same amount of time to fail. > > Ryan Baxter wrote: > Would the browser actually cache the response of the osapi.http.get? I > am sure Shindig would but I am not sure the browser would. The request for > the get will be a post to the rpc servlet, the iframe will make a request to > the URL, would the browser be smart enough to know those are the same?
No, the browser is not that smart. > On 2011-12-06 23:03:02, Stanton Sievers wrote: > > http://svn.apache.org/repos/asf/shindig/trunk/features/src/main/javascript/features/container/container.js, > > line 934 > > <https://reviews.apache.org/r/3037/diff/2/?file=62529#file62529line934> > > > > I'd rather be checking for a status of any 2xx code. I'd rather treat > > redirects and other 3xx codes as failures if they are going to fail at > > render time anyway. > > > > Also, what is the difference between checking response.error and > > response.status. Shouldn't they indicate the same thing? > > Ryan Baxter wrote: > I ignored the 3xx codes because I figured the browser would be able to > handle them and redirect the iframe accordingly. > > I looked into the response.error case. It looks like that will happen if > something goes wrong in the containers proxy. The response.status is the > result of actually making the http request. Wouldn't we want to follow any 3xx redirects to ensure that the final content is available? Another idea I thought of that might be worth evaluating... what if you proxied the iframe urls through Shindig's proxy servlet and returned an error page from there? Maybe add a query param flag that the proxy servlet can read to know whether to return the original response or an error page. That could at least solve the issue of having several http requests. The response.error and response.status stuff makes sense. - Stanton ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/3037/#review3681 ----------------------------------------------------------- On 2011-12-06 22:36:38, Ryan Baxter wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/3037/ > ----------------------------------------------------------- > > (Updated 2011-12-06 22:36:38) > > > Review request for shindig, Dan Dumont and Stanton Sievers. > > > Summary > ------- > > When you call commoncontainer.navigateUrl if the URL cannot be reached the > caller has no way of knowing if the URL was navigated successfully or not. To > solve this we make a head request to the URL we are navigating to and add a > callback to the API. > > It is important to note that we will not be caching the response of the head > request. While this could possibly give us better performance we have no way > of guaranteeing the server will still be up next time and everything may > fail. This is different from the gadget case where we have the gadget XML > cached on the server. > > > This addresses bug SHINDIG-1669. > https://issues.apache.org/jira/browse/SHINDIG-1669 > > > Diffs > ----- > > > http://svn.apache.org/repos/asf/shindig/trunk/features/src/main/javascript/features/open-views/viewenhancements-container.js > 1211103 > > http://svn.apache.org/repos/asf/shindig/trunk/features/src/main/javascript/features/embeddedexperiences/embedded_experiences_container.js > 1211103 > > http://svn.apache.org/repos/asf/shindig/trunk/features/src/main/javascript/features/container/container.js > 1211103 > > http://svn.apache.org/repos/asf/shindig/trunk/features/src/main/javascript/features/container.util/util.js > 1211103 > > http://svn.apache.org/repos/asf/shindig/trunk/content/samplecontainer/examples/embeddedexperiences/PhotoList.xml > 1211103 > > http://svn.apache.org/repos/asf/shindig/trunk/features/src/test/javascript/features/container.url/container_url_test.js > 1211103 > > Diff: https://reviews.apache.org/r/3037/diff > > > Testing > ------- > > Tested in container as well as updating unit tests. > > > Thanks, > > Ryan > >
