> 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. > > > Henry Saputra wrote: > 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. > > Jesse Ciancetta wrote: > I think it just comes down to a difference of style and opinion. I was > just offering my opinion on what I thought might make things cleaner and > simpler -- I wasn't trying to say it had to be changed. > > Like Ryan said -- I don't think it would be the end of the world to leave > it as is, so if that's what you choose to do its fine by me.
+1 for keeping it as one function. I'm a bit late to the game here, but I can see the argument going both ways. However, at the end of the day I'd rather keep the CC APIs as simple as possible and I think keeping it as one function achieves that. As long as the documentation is clear then I don't see an issue. - Stanton ----------------------------------------------------------- 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 > >
