----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/3037/#review3739 -----------------------------------------------------------
Sorry for being late to the review -- this is the first chance I've had to really look this over. I have a few concerns with this approach that make me wonder if an alternate approach might be more appropriate -- although the alternate I have in mind has some issues of its own too... I'll list my concerns first and then offer the potential alternate solution. -- Performance (of course, as others have pointed out) on both the client side and server side. On the client side -- if the server takes a while to process the request (say two seconds) then that means the client has to wait four seconds before they actually see the content. On the server side the server will end up seeing twice the load. -- Side effects of two calls. I know an HTTP GET for an iframe url should be idempotent, but what if its not? We should at least document that the call gets made twice to be sure callers are aware. -- Authentication. What if the iframe url requires some sort of authentication? Maybe the iframe url returns a a 401 with a basic auth challenge. Or maybe the url is protected with SSO and the clients browser has a valid SSO cookie to send, but shindig would not. In either of these cases we'd say that the navigate failed (since shindig would not be able to auth) but the client may have been able to auth just fine. -- There's really no *guarantee* that just because the head request succeeds that the subsequent get will succeed too. It's extremely likely to succeed, but not guaranteed. -- If we go with this approach I think we should offer some way for the caller to opt-out of the head request in case they need to avoid any of the issues I've raised above. As an alternative -- it seems to me that an onload listener on the iframe with a caller-configurable-timeout might actually be a more useful way to approach this. I think more often than not we have some well known set amount of time that we're willing to wait for the content of the iframe to load before we just give up and call it a failure -- so I'm wondering if we could just register an onload listener on the iframe and if the onload event hasn't fired within the designated amount of time we consider the navigate as a failure. The issues I see with this approach are: -- It needs to be tested and proven that onload on the iframe works the way I think it does in all major browsers (which is that it only fires if the iframe loaded successfully). -- If the iframe fails immediately the user would see the error displayed in the frame until the timeout expired (assuming on timeout we put a nice error message in place of the iframe). Maybe the iframe could be hidden until successful? That could degrade from the experience though too... Not sure what the best approach is here. If the consensus is that the head method is best I'm fine with it (as long as we add the ability for the caller to opt-out) -- I just wanted to be sure to point out all the issues I see and offer this potential alternative for consideration. - Jesse On 2011-12-08 15:07:59, Ryan Baxter wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/3037/ > ----------------------------------------------------------- > > (Updated 2011-12-08 15:07:59) > > > 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/test/javascript/features/container.url/container_url_test.js > 1211913 > > http://svn.apache.org/repos/asf/shindig/trunk/content/samplecontainer/examples/embeddedexperiences/PhotoList.xml > 1211913 > > http://svn.apache.org/repos/asf/shindig/trunk/features/src/main/javascript/features/container.util/util.js > 1211913 > > http://svn.apache.org/repos/asf/shindig/trunk/features/src/main/javascript/features/container/container.js > 1211913 > > http://svn.apache.org/repos/asf/shindig/trunk/features/src/main/javascript/features/embeddedexperiences/embedded_experiences_container.js > 1211913 > > http://svn.apache.org/repos/asf/shindig/trunk/features/src/main/javascript/features/open-views/viewenhancements-container.js > 1211913 > > Diff: https://reviews.apache.org/r/3037/diff > > > Testing > ------- > > Tested in container as well as updating unit tests. > > > Thanks, > > Ryan > >
