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