I actually like the original idea of having one function to update/ initiate the container token. Because it doesnt really matter from container page point of view whether common container doing init or updating exisitng token
- Henry On Wed, Dec 14, 2011 at 9:58 AM, Jesse Ciancetta <jc...@mitre.org> wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/3180/#review3908 > ----------------------------------------------------------- > > > Some observations after a first pass: > > IMO the API would make more sense and be a little more intuitive if we broke > it up into a couple of more specific public functions instead of having just > one multi-purpose function. Right now without looking at the code I'd assume > that updateContainerSecurityToken would be a function that I would call if I > literally wanted the container security token to be updated -- but it's > really a function that I can use for either setting the initial token or > wrapping a token-needing-function call with a valid token check before > execution. > > I think things would be a little clearer if we had: > > osapi.container.Container.prototype.setContainerSecurityToken = > function(token, ttl) { > //Make it clear in the documentation that this should be > called before you start any real work with a freshly constructed container. > //Maybe even add another optional property to the opt_config > object passed to the container constructor for the token and ttl and > //then call this function automatically as part of the > container initialization. Either way it might still be useful to have a > //function that can be called to set the token on-demand if > needed. This function would: > > //set the initial token > //schedule the next refresh > } > > osapi.container.Container.prototype.ensureContainerSecurityToken = > function(callback) { > //Do pretty much what updateContainerSecurityToken does now > sans the handling of the initial token. > } > > I'm guessing your also going to want to have a function for ensuring a gadget > token too which is going to also use ensureContainerSecurityToken if it finds > that it needs to update the gadget token: > > osapi.container.Container.prototype.ensureGadgetSecurityToken = > function(gadgetUrl, moduleId, callback) { > //If we find that we need a gadget token then we'll wrap the > code to fetch the gadget token in an ensureContainerSecurityToken call > } > > Anyone else have an opinion on this? > > Code style nit -- all of the other JS code that I see on a quick scan of > container.js has braces on the same line as the if/else statements which I > think helps to improve readability a bit. > > - Jesse > > > On 2011-12-14 16:35:00, Dan Dumont wrote: >> >> ----------------------------------------------------------- >> This is an automatically generated e-mail. To reply, visit: >> https://reviews.apache.org/r/3180/ >> ----------------------------------------------------------- >> >> (Updated 2011-12-14 16:35:00) >> >> >> Review request for shindig, Ryan Baxter, li xu, Jesse Ciancetta, Henry >> Saputra, and Stanton Sievers. >> >> >> Summary >> ------- >> >> Initial review of 1st change. Allowing common container to manage container >> token refreshes. Also, refresh of gadget security tokens will now wait for >> valid container security token before trying to refresh. >> >> >> Diffs >> ----- >> >> >> http://svn.apache.org/repos/asf/shindig/trunk/features/src/main/javascript/features/container/container.js >> 1213887 >> >> Diff: https://reviews.apache.org/r/3180/diff >> >> >> Testing >> ------- >> >> Tested code in a private container with some examples of setting no refresh >> (ttl = 0) and setting an initial token (if it was written by jsp page to >> avoid transaction) etc.. >> >> >> Thanks, >> >> Dan >> >> >
