> On 2011-12-08 15:24:35, Jesse Ciancetta wrote: > > 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. > > Ryan Baxter wrote: > All great points Jesse. > > I thought of the iframe onload event as well, but when I was thinking > about it I was hoping there was an event passed into the onload function > which would tell me whether the iframe loaded properly or not, but there > wasn't (At least I couldn't find any information on it.). I didn't think > about doing it the way you described. I did a quick test and FF does not > fire the onload event when the URL cannot load the page, but Chrom and IE8 > do, so the approach of a timeout would not work. > > I can certainly make the head request an opt in approach and only make > the head request when the called supplies a callback into the navigateUrl > call, if not, there is no point in making the head request anyways.
Ah -- ok... I'd seen some people saying onload wouldn't get fired for iframes when the content failed to load but hadn't had a chance to test it out -- too bad it isn't consistent across browsers. With the option to opt-out by not passing the callback I think this looks good. - Jesse ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/3037/#review3739 ----------------------------------------------------------- On 2011-12-08 17:58:33, Ryan Baxter wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/3037/ > ----------------------------------------------------------- > > (Updated 2011-12-08 17:58:33) > > > 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/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 > > http://svn.apache.org/repos/asf/shindig/trunk/features/src/test/javascript/features/container.url/container_url_test.js > 1211913 > > Diff: https://reviews.apache.org/r/3037/diff > > > Testing > ------- > > Tested in container as well as updating unit tests. > > > Thanks, > > Ryan > >
