> On 2011-12-19 18:23:54, Dan Dumont wrote: > > So far I have 1 vote for splitting the update function into 2 parts (from > > Jesse), and 1 for leaving it as 1 function (from Henry, I think on the dev > > list). > > > > Are there any other opinions on the matter? > > Ryan Baxter wrote: > I agree with Jesse on this one. Splitting this up may help make this > more understandable. I see your point about not having to set the token but > this can be made clear in the specification for the common container. It > took me a little bit to figure out what was happening, and I am still not > sure I completely understand it all... > > Ryan Baxter wrote: > Just so it is clear, I don't think it will be the end of the world if we > don't have an separate set function. I would really like to see more helper > functions. We all have to read through the code to understand things in > Shindig and reading this code is kind of difficult, so anything we can do to > make it more strait forward would be good. > > Jesse Ciancetta wrote: > Agreed -- I think having two functions would make the implementation > simpler, which was actually another motivator for my comment about breaking > the function up in the first place (although I didn't call that out > explicitly, which I guess I probably should have). It took me a while to > figure out what was going on in the current implementation too. > > Dan Dumont wrote: > I can see your point, but I disagree it would be simpler in the sense > that it would introduce more places for bugs to occur. > > Again, I would rather clarify with comments on areas you found > particularly troubling. > From the standpoint of the API consumer it's fairly straight forward: > * I can call updateContainerSecurityToken(callback) and I will get a call > back when the token is updated. > * I can call updateContainerSecurityToken(callback, token, ttl) and get a > call back when the token is updated with the info i've provided. > > The API is always async, and always behaves the same. In either case, > the token is updated and is then scheduled for future updates. > > From a maintenance point of view. This impl is fairly small and well > contained and scoped. > There is a well defined helper function that handles the refresh. > And there's comments around when / why we may queue callbacks or run them > immediately. > > I personally think that creating extra functions does not always simplify > things. > I'd rather not potentially duplicate code or logic for the sake of > avoiding non-recursive or difficult to understand functions. > > If you do have questions or need more clarifications on certain parts of > the code, or think that a certain area is confusing, I can try my best to > make sure the comments there explain why we do what we are doing. >
More helper functions help if they do different functionalities. In this case, doesnt really matter if the security token is actually newly created or updated from caller point of view. So I am +1 to keep it as one function. - Henry ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/3180/#review3976 ----------------------------------------------------------- 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, Henry Saputra, Ryan Baxter, li xu, Jesse > Ciancetta, 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 > >
