-----------------------------------------------------------
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
>
>
