> On 2011-12-14 17:58:55, Jesse Ciancetta wrote:
> > 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.
>
> Dan Dumont wrote:
> I suppose that wouldn't really hurt anything. I was hesitant to add it
> though as explicitly SETTING the token is something that some containers may
> choose never to do. It's perfectly valid to never do that, the refresh
> mechanism will launch the first refresh for you on demand.
> However if you're writing out the page with a JSP or something and want
> to avoid a round trip for the container token, this provides you simple means
> to do that without introducing a potentially infrequently used function to
> maintain.
>
> I won't hold it against anyone if they really don't like baking it all in
> one. Having 2 functions seems doable, I'm just trying to be conscious of the
> surface area of the public CC api and how to keep it concise and
> understandable. Making people juggle a few different apis to do 1 task is
> something most JS apis avoid. Dojo and JQuery both massively overload js
> functions that all do a task in different ways depending on the arguments
> supplied.
>
> As for the last comment, I agree with you and for some reason had it
> stuck in my head that it was shindig convention to not do that. But I can't
> recall any specific code I've run into that made me think that. I'll clean
> that up on my next run through of changes.
I actually started out that way (with the name ensureSecurityToken) and thought
that update could serve both use cases nicely especially if the documentation
stated that updating an up-to-date token was a no-op.
- Dan
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/3180/#review3908
-----------------------------------------------------------
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
>
>
