You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@shindig.apache.org by Dan Dumont <dd...@us.ibm.com> on 2011/12/14 17:35:00 UTC
Re: Review Request: CommonContainer token refresh changes for a better UX
when tokens expire.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/3180/
-----------------------------------------------------------
(Updated 2011-12-14 16:35:00.374342)
Review request for shindig, Ryan Baxter, li xu, Jesse Ciancetta, Henry Saputra, and Stanton Sievers.
Changes
-------
Adding Henry.. I should probably just add the whole dev list at this point :) Was going to add the group when I got closer to final completion... oh well. Adding shindig too.
I'm glad people are interested :)
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
Re: Review Request: CommonContainer token refresh changes for a better UX
when tokens expire.
Posted by Dan Dumont <dd...@us.ibm.com>.
> 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
>
>
Re: Review Request: CommonContainer token refresh changes for a better UX
when tokens expire.
Posted by Dan Dumont <dd...@us.ibm.com>.
> 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
>
>
Re: Review Request: CommonContainer token refresh changes for a
better UX when tokens expire.
Posted by Henry Saputra <he...@gmail.com>.
I actually like the original idea of having one function to update/
initiate the container token. Because it doesnt really matter from
container page point of view whether common container doing init or
updating exisitng token
- Henry
On Wed, Dec 14, 2011 at 9:58 AM, Jesse Ciancetta <jc...@mitre.org> wrote:
>
> -----------------------------------------------------------
> 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
>>
>>
>
Re: Review Request: CommonContainer token refresh changes for a better UX
when tokens expire.
Posted by Jesse Ciancetta <jc...@mitre.org>.
-----------------------------------------------------------
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
>
>
Re: Review Request: CommonContainer token refresh changes for a better UX
when tokens expire.
Posted by Ryan Baxter <rb...@gmail.com>.
> 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...
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.
- Ryan
-----------------------------------------------------------
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
>
>
Re: Guice problem in persistence
Posted by Evgeny Bogdanov <ev...@epfl.ch>.
Thank you Stanton for the hint.
I went with the static field approach, since I do not explicitly
initialize UserDb class.
Best
Evgeny
On 22.12.11 14:49, Stanton Sievers wrote:
> Here are two options I can come up with off the top of my head, assuming I
> understand your problem correctly.
>
> You can do a static injection of UserDb in the JPASocialModule which means
> the field will have to be static. Or, you can modify the @Injected
> constructor for PersonServiceDb to have an
> @Named("shindig.signing.global-callback-url") param that you can then pass
> along to UserDb when you instanstiate it, assuming you are instanstiating
> it. If UserDb is a singleton then I would go with the static injection
> approach.
>
> Best regards,
> -Stanton
>
>
>
> From: Evgeny Bogdanov<ev...@epfl.ch>
> To: dev@shindig.apache.org,
> Date: 12/22/2011 08:42
> Subject: Re: Guice problem in persistence
>
>
>
>
>
> On 22.12.11 13:53, Stanton Sievers wrote:
>> Hi,
>>
>> How is your UserDb class being injected? Make sure it's not being
>> injected before the PropertiesModule, which by default is injected via
> the
>> web.xml.
> The thing is I am not very comfortable with Guice injection mechanism.
> So I do not know if it is injected or not. This I have currently
>
> 1. In web.xml (JPA module is after Properties)
> org.apache.shindig.common.PropertiesModule:
> org.apache.shindig.gadgets.DefaultGuiceModule:
> org.apache.shindig.social.core.config.SocialApiGuiceModule:
> org.apache.shindig.graaasp.jpa.spi.JPASocialModule:
>
> 2. In JPA I have explicit injection
> bind(PersonService.class).to(PersonServiceDb.class).in(Scopes.SINGLETON);
>
> 3. In PersonServiceDb I import the class UserDb.java:
> import org.apache.shindig.social.core.model.UserDb;
>
> So, I believe it does not do any injection for UserDb this way.
> Then the question is how do I inject UserDb or how do I pass the data
> from PropertiesModule
> to my UserDb.java class.
>> If your class isn't being injected at all you can request a static
>> injection in an existing guice module (or your own).
>> requestStaticInjection(UserDb.class)
> I tried to add it in JPASocialModule, but it does not seem to be
> working. The field CALLBACK_URL is still null.
>> I also prefer method injection to field injection for setting values.
> I'm
>> not sure if one is preferred over the other in the broader community or
>> not.
> Could you give a small example how you do it.
>> -Stanton
>>
>>
>>
>> From: Evgeny Bogdanov<ev...@epfl.ch>
>> To: dev@shindig.apache.org,
>> Date: 12/22/2011 06:30
>> Subject: Guice problem in persistence
>>
>>
>>
>> Hi
>>
>> I have small problem, maybe somebody can point out where it is.
>>
>> I have this line in shindig.properties:
>>
> shindig.signing.global-callback-url=http://localhost:8080/gadgets/oauthcallback
>> I want to access it from my db class.
>>
>> Properties file is read in PropertiesModule.java and Guice's Named
>> annotation is created with
>> Names.bindProperties(this.binder(), getProperties());
>>
>> Then I try to access it as following in my UserDb.java class
>> @Transient
>> @Inject @Named("shindig.signing.global-callback-url") String
>> CALLBACK_URL;
>>
>> For some reason, it is always null.
>> Could somebody explain why I can't do it? What is the workaround. I
>> don't want to read properties file again,
>> since it's done in PropertiesModule.java already.
>>
>> Any suggestions?
>>
>> Best
>> Evgeny
>>
>>
>>
>
>
Re: Guice problem in persistence
Posted by Stanton Sievers <ss...@us.ibm.com>.
Here are two options I can come up with off the top of my head, assuming I
understand your problem correctly.
You can do a static injection of UserDb in the JPASocialModule which means
the field will have to be static. Or, you can modify the @Injected
constructor for PersonServiceDb to have an
@Named("shindig.signing.global-callback-url") param that you can then pass
along to UserDb when you instanstiate it, assuming you are instanstiating
it. If UserDb is a singleton then I would go with the static injection
approach.
Best regards,
-Stanton
From: Evgeny Bogdanov <ev...@epfl.ch>
To: dev@shindig.apache.org,
Date: 12/22/2011 08:42
Subject: Re: Guice problem in persistence
On 22.12.11 13:53, Stanton Sievers wrote:
> Hi,
>
> How is your UserDb class being injected? Make sure it's not being
> injected before the PropertiesModule, which by default is injected via
the
> web.xml.
The thing is I am not very comfortable with Guice injection mechanism.
So I do not know if it is injected or not. This I have currently
1. In web.xml (JPA module is after Properties)
org.apache.shindig.common.PropertiesModule:
org.apache.shindig.gadgets.DefaultGuiceModule:
org.apache.shindig.social.core.config.SocialApiGuiceModule:
org.apache.shindig.graaasp.jpa.spi.JPASocialModule:
2. In JPA I have explicit injection
bind(PersonService.class).to(PersonServiceDb.class).in(Scopes.SINGLETON);
3. In PersonServiceDb I import the class UserDb.java:
import org.apache.shindig.social.core.model.UserDb;
So, I believe it does not do any injection for UserDb this way.
Then the question is how do I inject UserDb or how do I pass the data
from PropertiesModule
to my UserDb.java class.
>
> If your class isn't being injected at all you can request a static
> injection in an existing guice module (or your own).
> requestStaticInjection(UserDb.class)
I tried to add it in JPASocialModule, but it does not seem to be
working. The field CALLBACK_URL is still null.
>
> I also prefer method injection to field injection for setting values.
I'm
> not sure if one is preferred over the other in the broader community or
> not.
Could you give a small example how you do it.
>
> -Stanton
>
>
>
> From: Evgeny Bogdanov<ev...@epfl.ch>
> To: dev@shindig.apache.org,
> Date: 12/22/2011 06:30
> Subject: Guice problem in persistence
>
>
>
> Hi
>
> I have small problem, maybe somebody can point out where it is.
>
> I have this line in shindig.properties:
>
shindig.signing.global-callback-url=http://localhost:8080/gadgets/oauthcallback
> I want to access it from my db class.
>
> Properties file is read in PropertiesModule.java and Guice's Named
> annotation is created with
> Names.bindProperties(this.binder(), getProperties());
>
> Then I try to access it as following in my UserDb.java class
> @Transient
> @Inject @Named("shindig.signing.global-callback-url") String
> CALLBACK_URL;
>
> For some reason, it is always null.
> Could somebody explain why I can't do it? What is the workaround. I
> don't want to read properties file again,
> since it's done in PropertiesModule.java already.
>
> Any suggestions?
>
> Best
> Evgeny
>
>
>
Re: Guice problem in persistence
Posted by Evgeny Bogdanov <ev...@epfl.ch>.
On 22.12.11 13:53, Stanton Sievers wrote:
> Hi,
>
> How is your UserDb class being injected? Make sure it's not being
> injected before the PropertiesModule, which by default is injected via the
> web.xml.
The thing is I am not very comfortable with Guice injection mechanism.
So I do not know if it is injected or not. This I have currently
1. In web.xml (JPA module is after Properties)
org.apache.shindig.common.PropertiesModule:
org.apache.shindig.gadgets.DefaultGuiceModule:
org.apache.shindig.social.core.config.SocialApiGuiceModule:
org.apache.shindig.graaasp.jpa.spi.JPASocialModule:
2. In JPA I have explicit injection
bind(PersonService.class).to(PersonServiceDb.class).in(Scopes.SINGLETON);
3. In PersonServiceDb I import the class UserDb.java:
import org.apache.shindig.social.core.model.UserDb;
So, I believe it does not do any injection for UserDb this way.
Then the question is how do I inject UserDb or how do I pass the data
from PropertiesModule
to my UserDb.java class.
>
> If your class isn't being injected at all you can request a static
> injection in an existing guice module (or your own).
> requestStaticInjection(UserDb.class)
I tried to add it in JPASocialModule, but it does not seem to be
working. The field CALLBACK_URL is still null.
>
> I also prefer method injection to field injection for setting values. I'm
> not sure if one is preferred over the other in the broader community or
> not.
Could you give a small example how you do it.
>
> -Stanton
>
>
>
> From: Evgeny Bogdanov<ev...@epfl.ch>
> To: dev@shindig.apache.org,
> Date: 12/22/2011 06:30
> Subject: Guice problem in persistence
>
>
>
> Hi
>
> I have small problem, maybe somebody can point out where it is.
>
> I have this line in shindig.properties:
> shindig.signing.global-callback-url=http://localhost:8080/gadgets/oauthcallback
> I want to access it from my db class.
>
> Properties file is read in PropertiesModule.java and Guice's Named
> annotation is created with
> Names.bindProperties(this.binder(), getProperties());
>
> Then I try to access it as following in my UserDb.java class
> @Transient
> @Inject @Named("shindig.signing.global-callback-url") String
> CALLBACK_URL;
>
> For some reason, it is always null.
> Could somebody explain why I can't do it? What is the workaround. I
> don't want to read properties file again,
> since it's done in PropertiesModule.java already.
>
> Any suggestions?
>
> Best
> Evgeny
>
>
>
Re: Guice problem in persistence
Posted by Stanton Sievers <ss...@us.ibm.com>.
Hi,
How is your UserDb class being injected? Make sure it's not being
injected before the PropertiesModule, which by default is injected via the
web.xml.
If your class isn't being injected at all you can request a static
injection in an existing guice module (or your own).
requestStaticInjection(UserDb.class)
I also prefer method injection to field injection for setting values. I'm
not sure if one is preferred over the other in the broader community or
not.
-Stanton
From: Evgeny Bogdanov <ev...@epfl.ch>
To: dev@shindig.apache.org,
Date: 12/22/2011 06:30
Subject: Guice problem in persistence
Hi
I have small problem, maybe somebody can point out where it is.
I have this line in shindig.properties:
shindig.signing.global-callback-url=http://localhost:8080/gadgets/oauthcallback
I want to access it from my db class.
Properties file is read in PropertiesModule.java and Guice's Named
annotation is created with
Names.bindProperties(this.binder(), getProperties());
Then I try to access it as following in my UserDb.java class
@Transient
@Inject @Named("shindig.signing.global-callback-url") String
CALLBACK_URL;
For some reason, it is always null.
Could somebody explain why I can't do it? What is the workaround. I
don't want to read properties file again,
since it's done in PropertiesModule.java already.
Any suggestions?
Best
Evgeny
Guice problem in persistence
Posted by Evgeny Bogdanov <ev...@epfl.ch>.
Hi
I have small problem, maybe somebody can point out where it is.
I have this line in shindig.properties:
shindig.signing.global-callback-url=http://localhost:8080/gadgets/oauthcallback
I want to access it from my db class.
Properties file is read in PropertiesModule.java and Guice's Named
annotation is created with
Names.bindProperties(this.binder(), getProperties());
Then I try to access it as following in my UserDb.java class
@Transient
@Inject @Named("shindig.signing.global-callback-url") String
CALLBACK_URL;
For some reason, it is always null.
Could somebody explain why I can't do it? What is the workaround. I
don't want to read properties file again,
since it's done in PropertiesModule.java already.
Any suggestions?
Best
Evgeny
Re: Review Request: CommonContainer token refresh changes for a better UX
when tokens expire.
Posted by Henry Saputra <hs...@apache.org>.
> 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
>
>
Re: Review Request: CommonContainer token refresh changes for a better UX
when tokens expire.
Posted by Ryan Baxter <rb...@gmail.com>.
> 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?
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
-----------------------------------------------------------
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
>
>
Re: Review Request: CommonContainer token refresh changes for a better UX
when tokens expire.
Posted by Jesse Ciancetta <jc...@mitre.org>.
> 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.
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.
- Jesse
-----------------------------------------------------------
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
>
>
Re: Review Request: CommonContainer token refresh changes for a better UX
when tokens expire.
Posted by Jesse Ciancetta <jc...@mitre.org>.
> 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.
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.
- Jesse
-----------------------------------------------------------
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
>
>
Re: Review Request: CommonContainer token refresh changes for a better UX
when tokens expire.
Posted by Dan Dumont <dd...@us.ibm.com>.
> 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.
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.
- Dan
-----------------------------------------------------------
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
>
>
Re: Review Request: CommonContainer token refresh changes for a better UX
when tokens expire.
Posted by Stanton Sievers <si...@gmail.com>.
> 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
>
>
Re: Review Request: CommonContainer token refresh changes for a better UX
when tokens expire.
Posted by Dan Dumont <dd...@us.ibm.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/3180/#review3976
-----------------------------------------------------------
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?
- Dan
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
>
>
Re: Review Request: CommonContainer token refresh changes for a better UX
when tokens expire.
Posted by Dan Dumont <dd...@us.ibm.com>.
> On 2011-12-15 00:07:05, Henry Saputra wrote:
> > http://svn.apache.org/repos/asf/shindig/trunk/features/src/main/javascript/features/container/container.js, line 233
> > <https://reviews.apache.org/r/3180/diff/3/?file=64601#file64601line233>
> >
> > Shouldnt this call happen? The default shindig.auth contributor is using anonymous security token
I think I mentioned on the thread in the list that this change was breaking the jsunit tests. I'm not sure how to properly do a sync testing with our jsunit tests.
> On 2011-12-15 00:07:05, Henry Saputra wrote:
> > http://svn.apache.org/repos/asf/shindig/trunk/features/src/main/javascript/features/container/container.js, line 704
> > <https://reviews.apache.org/r/3180/diff/3/?file=64601#file64601line704>
> >
> > The declaration of updateContainerSecurityToken doesnt have to be inside the anon "(function() {" right?
It does because it needs scope of the helper functions and the variables associated with the refresh.
> On 2011-12-15 00:07:05, Henry Saputra wrote:
> > http://svn.apache.org/repos/asf/shindig/trunk/features/src/main/javascript/features/container/container.js, line 710
> > <https://reviews.apache.org/r/3180/diff/3/?file=64601#file64601line710>
> >
> > Shouldnt this 95/100?
The var is already 80% of the actual value. We need to convert it to 95% of the normal value.
- Dan
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/3180/#review3911
-----------------------------------------------------------
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
>
>
Re: Review Request: CommonContainer token refresh changes for a better UX
when tokens expire.
Posted by Henry Saputra <hs...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/3180/#review3911
-----------------------------------------------------------
http://svn.apache.org/repos/asf/shindig/trunk/features/src/main/javascript/features/container/container.js
<https://reviews.apache.org/r/3180/#comment8800>
Shouldnt this call happen? The default shindig.auth contributor is using anonymous security token
http://svn.apache.org/repos/asf/shindig/trunk/features/src/main/javascript/features/container/container.js
<https://reviews.apache.org/r/3180/#comment8787>
The declaration of updateContainerSecurityToken doesnt have to be inside the anon "(function() {" right?
http://svn.apache.org/repos/asf/shindig/trunk/features/src/main/javascript/features/container/container.js
<https://reviews.apache.org/r/3180/#comment8792>
Shouldnt this 95/100?
- Henry
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
>
>
Re: Review Request: CommonContainer token refresh changes for a better UX
when tokens expire.
Posted by Dan Dumont <dd...@us.ibm.com>.
> On 2012-01-10 13:00:45, Stanton Sievers wrote:
> > http://svn.apache.org/repos/asf/shindig/trunk/features/src/main/javascript/features/container.util/constant.js, line 1
> > <https://reviews.apache.org/r/3180/diff/7/?file=67350#file67350line1>
> >
> > Whether for this review or not, I think the ServiceConfig constants, like API_HOST and API_PATH should be added here as well.
I missed those, thanks.
> On 2012-01-10 13:00:45, Stanton Sievers wrote:
> > http://svn.apache.org/repos/asf/shindig/trunk/features/src/main/javascript/features/container/container.js, line 940
> > <https://reviews.apache.org/r/3180/diff/6-7/?file=65817#file65817line940>
> >
> > I'm having doubts about setting the fragment to be the module id. I thought we already used the fragment for the rpctoken in some cases (for like IFPC??). Will this overwrite that piece if it's set on the url? Can you use the "setFP" API instead of "setFragment"?
We could, but nothing right now uses the fragment part of a uri for token requests (which is the only place I'm currently using it).
Also, the code you mentioned in gadget_holder.js sets a fragment param for the iframe url. The moduleId I'm setting as the fragment here is on the gadget url for a token request. I don't think we'll have any collisions here.
- Dan
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/3180/#review4291
-----------------------------------------------------------
On 2012-01-09 21:18:06, Dan Dumont wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/3180/
> -----------------------------------------------------------
>
> (Updated 2012-01-09 21:18:06)
>
>
> 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/java/gadgets/src/test/java/org/apache/shindig/gadgets/servlet/GadgetsHandlerTest.java 1222407
> http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/test/java/org/apache/shindig/gadgets/servlet/GadgetsHandlerServiceTest.java 1222407
> http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/ModuleIdManager.java PRE-CREATION
> http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/ModuleIdManagerImpl.java PRE-CREATION
> http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/GadgetsHandlerApi.java 1222407
> http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/GadgetsHandlerService.java 1222407
> http://svn.apache.org/repos/asf/shindig/trunk/features/src/test/javascript/features/container/gadget_holder_test.js 1222407
> http://svn.apache.org/repos/asf/shindig/trunk/features/src/test/javascript/features/container/service_test.js 1222407
> http://svn.apache.org/repos/asf/shindig/trunk/java/common/src/main/java/org/apache/shindig/auth/AbstractSecurityToken.java 1222407
> http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/GadgetsHandler.java 1222407
> http://svn.apache.org/repos/asf/shindig/trunk/features/src/main/javascript/features/container/feature.xml 1222407
> http://svn.apache.org/repos/asf/shindig/trunk/features/src/main/javascript/features/container/service.js 1222407
> http://svn.apache.org/repos/asf/shindig/trunk/features/src/main/javascript/features/container.util/util.js 1222407
> http://svn.apache.org/repos/asf/shindig/trunk/features/src/main/javascript/features/container/container.js 1222407
> http://svn.apache.org/repos/asf/shindig/trunk/features/src/main/javascript/features/container.util/constant.js 1222407
> http://svn.apache.org/repos/asf/shindig/trunk/features/src/main/javascript/features/container.util/feature.xml 1222407
> http://svn.apache.org/repos/asf/shindig/trunk/features/src/main/javascript/features/container.gadget/gadget_site.js 1222407
> http://svn.apache.org/repos/asf/shindig/trunk/features/src/main/javascript/features/container.gadget/gadget_holder.js 1222407
> http://svn.apache.org/repos/asf/shindig/trunk/content/samplecontainer/examples/commoncontainer/assembler.js 1222407
>
> 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
>
>
Re: Review Request: CommonContainer token refresh changes for a better UX
when tokens expire.
Posted by Stanton Sievers <si...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/3180/#review4291
-----------------------------------------------------------
Overall this looks good to me. I have some questions about setting the module id as the fragment, since I think there are cases where the fragment gets used for an rpctoken or an ifpctok... you can see it in the gadget_holder feature code. I'm not sure how/why this is used but it's certainly worth looking into, as I think your code right now would kill off that piece of the fragment.
http://svn.apache.org/repos/asf/shindig/trunk/features/src/main/javascript/features/container/container.js
<https://reviews.apache.org/r/3180/#comment9667>
I'm having doubts about setting the fragment to be the module id. I thought we already used the fragment for the rpctoken in some cases (for like IFPC??). Will this overwrite that piece if it's set on the url? Can you use the "setFP" API instead of "setFragment"?
http://svn.apache.org/repos/asf/shindig/trunk/features/src/main/javascript/features/container.util/constant.js
<https://reviews.apache.org/r/3180/#comment9668>
Whether for this review or not, I think the ServiceConfig constants, like API_HOST and API_PATH should be added here as well.
- Stanton
On 2012-01-09 21:18:06, Dan Dumont wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/3180/
> -----------------------------------------------------------
>
> (Updated 2012-01-09 21:18:06)
>
>
> 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/java/gadgets/src/test/java/org/apache/shindig/gadgets/servlet/GadgetsHandlerTest.java 1222407
> http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/test/java/org/apache/shindig/gadgets/servlet/GadgetsHandlerServiceTest.java 1222407
> http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/ModuleIdManager.java PRE-CREATION
> http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/ModuleIdManagerImpl.java PRE-CREATION
> http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/GadgetsHandlerApi.java 1222407
> http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/GadgetsHandlerService.java 1222407
> http://svn.apache.org/repos/asf/shindig/trunk/features/src/test/javascript/features/container/gadget_holder_test.js 1222407
> http://svn.apache.org/repos/asf/shindig/trunk/features/src/test/javascript/features/container/service_test.js 1222407
> http://svn.apache.org/repos/asf/shindig/trunk/java/common/src/main/java/org/apache/shindig/auth/AbstractSecurityToken.java 1222407
> http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/GadgetsHandler.java 1222407
> http://svn.apache.org/repos/asf/shindig/trunk/features/src/main/javascript/features/container/feature.xml 1222407
> http://svn.apache.org/repos/asf/shindig/trunk/features/src/main/javascript/features/container/service.js 1222407
> http://svn.apache.org/repos/asf/shindig/trunk/features/src/main/javascript/features/container.util/util.js 1222407
> http://svn.apache.org/repos/asf/shindig/trunk/features/src/main/javascript/features/container/container.js 1222407
> http://svn.apache.org/repos/asf/shindig/trunk/features/src/main/javascript/features/container.util/constant.js 1222407
> http://svn.apache.org/repos/asf/shindig/trunk/features/src/main/javascript/features/container.util/feature.xml 1222407
> http://svn.apache.org/repos/asf/shindig/trunk/features/src/main/javascript/features/container.gadget/gadget_site.js 1222407
> http://svn.apache.org/repos/asf/shindig/trunk/features/src/main/javascript/features/container.gadget/gadget_holder.js 1222407
> http://svn.apache.org/repos/asf/shindig/trunk/content/samplecontainer/examples/commoncontainer/assembler.js 1222407
>
> 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
>
>
Re: Review Request: CommonContainer token refresh changes for a better UX
when tokens expire.
Posted by Dan Dumont <dd...@us.ibm.com>.
> On 2012-01-10 18:48:26, Jesse Ciancetta wrote:
> > http://svn.apache.org/repos/asf/shindig/trunk/features/src/main/javascript/features/container/container.js, line 911
> > <https://reviews.apache.org/r/3180/diff/8/?file=67462#file67462line911>
> >
> > In the patch I was working on for moduleId's I took a different approach here which I think you may be able to do as well. I think in my patch -- any all the gadget tokens we had already fetched either by pre-load or navigating a gadget (which would make them eligible for refresh) were cached in the Service object -- so it seemed to me that knowing what tokens needed refresh was a simple matter of getting the keys from the gadget token cache map in the Service object.
> >
> > It looks like your keying the gadget token cache in the Service object with the gadget spec URL + moduleId as well (or just a raw gadget spec URL in the case of no moduleId) so it *seems* like that strategy might work for you here too.
I agree this seems more straight forward, but I'm running out of time to work on it. Do you think you can port that change from you patch on top of these changes?
> On 2012-01-10 18:48:26, Jesse Ciancetta wrote:
> > http://svn.apache.org/repos/asf/shindig/trunk/features/src/main/javascript/features/container.gadget/gadget_site.js, line 202
> > <https://reviews.apache.org/r/3180/diff/8/?file=67458#file67458line202>
> >
> > This code (or something very similar) ends up repeated in a bunch of different places -- it might be worth making a utility function somewhere which takes the gadget URL and moduleId and returns the properly formatted string.
Thanks for pointing that out. I addressed all the related concerns with the next patch I'm about to post.
- Dan
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/3180/#review4298
-----------------------------------------------------------
On 2012-01-10 14:59:57, Dan Dumont wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/3180/
> -----------------------------------------------------------
>
> (Updated 2012-01-10 14:59:57)
>
>
> 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.
>
>
> This addresses bug SHINDIG-1681.
> https://issues.apache.org/jira/browse/SHINDIG-1681
>
>
> Diffs
> -----
>
> http://svn.apache.org/repos/asf/shindig/trunk/content/samplecontainer/examples/commoncontainer/assembler.js 1222407
> http://svn.apache.org/repos/asf/shindig/trunk/features/src/main/javascript/features/container.gadget/gadget_holder.js 1222407
> http://svn.apache.org/repos/asf/shindig/trunk/features/src/main/javascript/features/container.gadget/gadget_site.js 1222407
> http://svn.apache.org/repos/asf/shindig/trunk/features/src/main/javascript/features/container.util/constant.js 1222407
> http://svn.apache.org/repos/asf/shindig/trunk/features/src/main/javascript/features/container.util/feature.xml 1222407
> http://svn.apache.org/repos/asf/shindig/trunk/features/src/main/javascript/features/container.util/util.js 1222407
> http://svn.apache.org/repos/asf/shindig/trunk/features/src/main/javascript/features/container/container.js 1222407
> http://svn.apache.org/repos/asf/shindig/trunk/features/src/main/javascript/features/container/feature.xml 1222407
> http://svn.apache.org/repos/asf/shindig/trunk/features/src/main/javascript/features/container/service.js 1222407
> http://svn.apache.org/repos/asf/shindig/trunk/features/src/test/javascript/features/container/gadget_holder_test.js 1222407
> http://svn.apache.org/repos/asf/shindig/trunk/features/src/test/javascript/features/container/service_test.js 1222407
> http://svn.apache.org/repos/asf/shindig/trunk/java/common/src/main/java/org/apache/shindig/auth/AbstractSecurityToken.java 1222407
> http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/GadgetsHandler.java 1222407
> http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/GadgetsHandlerApi.java 1222407
> http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/GadgetsHandlerService.java 1222407
> http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/ModuleIdManager.java PRE-CREATION
> http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/ModuleIdManagerImpl.java PRE-CREATION
> http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/test/java/org/apache/shindig/gadgets/servlet/GadgetsHandlerServiceTest.java 1222407
> http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/test/java/org/apache/shindig/gadgets/servlet/GadgetsHandlerTest.java 1222407
>
> 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
>
>
Re: Review Request: CommonContainer token refresh changes for a better UX
when tokens expire.
Posted by Jesse Ciancetta <jc...@mitre.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/3180/#review4298
-----------------------------------------------------------
http://svn.apache.org/repos/asf/shindig/trunk/features/src/main/javascript/features/container.gadget/gadget_site.js
<https://reviews.apache.org/r/3180/#comment9687>
This code (or something very similar) ends up repeated in a bunch of different places -- it might be worth making a utility function somewhere which takes the gadget URL and moduleId and returns the properly formatted string.
http://svn.apache.org/repos/asf/shindig/trunk/features/src/main/javascript/features/container/container.js
<https://reviews.apache.org/r/3180/#comment9681>
You may be able to get away from using this function for gadget token refresh and maybe even eliminate it entirely -- see my next comment around osapi.container.Container.prototype.refreshTokens_()
http://svn.apache.org/repos/asf/shindig/trunk/features/src/main/javascript/features/container/container.js
<https://reviews.apache.org/r/3180/#comment9682>
In the patch I was working on for moduleId's I took a different approach here which I think you may be able to do as well. I think in my patch -- any all the gadget tokens we had already fetched either by pre-load or navigating a gadget (which would make them eligible for refresh) were cached in the Service object -- so it seemed to me that knowing what tokens needed refresh was a simple matter of getting the keys from the gadget token cache map in the Service object.
It looks like your keying the gadget token cache in the Service object with the gadget spec URL + moduleId as well (or just a raw gadget spec URL in the case of no moduleId) so it *seems* like that strategy might work for you here too.
http://svn.apache.org/repos/asf/shindig/trunk/features/src/main/javascript/features/container/service.js
<https://reviews.apache.org/r/3180/#comment9679>
This looks like it is supposed to be:
url.setFragment(moduleId)
http://svn.apache.org/repos/asf/shindig/trunk/features/src/main/javascript/features/container/service.js
<https://reviews.apache.org/r/3180/#comment9680>
Is this supposed to be calling url.toString() instead of keying the cache with the actual shindig.uri object?
- Jesse
On 2012-01-10 14:59:57, Dan Dumont wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/3180/
> -----------------------------------------------------------
>
> (Updated 2012-01-10 14:59:57)
>
>
> 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.
>
>
> This addresses bug SHINDIG-1681.
> https://issues.apache.org/jira/browse/SHINDIG-1681
>
>
> Diffs
> -----
>
> http://svn.apache.org/repos/asf/shindig/trunk/content/samplecontainer/examples/commoncontainer/assembler.js 1222407
> http://svn.apache.org/repos/asf/shindig/trunk/features/src/main/javascript/features/container.gadget/gadget_holder.js 1222407
> http://svn.apache.org/repos/asf/shindig/trunk/features/src/main/javascript/features/container.gadget/gadget_site.js 1222407
> http://svn.apache.org/repos/asf/shindig/trunk/features/src/main/javascript/features/container.util/constant.js 1222407
> http://svn.apache.org/repos/asf/shindig/trunk/features/src/main/javascript/features/container.util/feature.xml 1222407
> http://svn.apache.org/repos/asf/shindig/trunk/features/src/main/javascript/features/container.util/util.js 1222407
> http://svn.apache.org/repos/asf/shindig/trunk/features/src/main/javascript/features/container/container.js 1222407
> http://svn.apache.org/repos/asf/shindig/trunk/features/src/main/javascript/features/container/feature.xml 1222407
> http://svn.apache.org/repos/asf/shindig/trunk/features/src/main/javascript/features/container/service.js 1222407
> http://svn.apache.org/repos/asf/shindig/trunk/features/src/test/javascript/features/container/gadget_holder_test.js 1222407
> http://svn.apache.org/repos/asf/shindig/trunk/features/src/test/javascript/features/container/service_test.js 1222407
> http://svn.apache.org/repos/asf/shindig/trunk/java/common/src/main/java/org/apache/shindig/auth/AbstractSecurityToken.java 1222407
> http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/GadgetsHandler.java 1222407
> http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/GadgetsHandlerApi.java 1222407
> http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/GadgetsHandlerService.java 1222407
> http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/ModuleIdManager.java PRE-CREATION
> http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/ModuleIdManagerImpl.java PRE-CREATION
> http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/test/java/org/apache/shindig/gadgets/servlet/GadgetsHandlerServiceTest.java 1222407
> http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/test/java/org/apache/shindig/gadgets/servlet/GadgetsHandlerTest.java 1222407
>
> 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
>
>
Re: Review Request: CommonContainer token refresh changes for a better UX
when tokens expire.
Posted by Dan Dumont <dd...@us.ibm.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/3180/#review4362
-----------------------------------------------------------
Ship it!
Committed r1231211.
Thanks everyone for the reviews!
- Dan
On 2012-01-13 16:44:45, Dan Dumont wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/3180/
> -----------------------------------------------------------
>
> (Updated 2012-01-13 16:44:45)
>
>
> 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.
>
>
> This addresses bug SHINDIG-1681.
> https://issues.apache.org/jira/browse/SHINDIG-1681
>
>
> Diffs
> -----
>
> http://svn.apache.org/repos/asf/shindig/trunk/content/samplecontainer/examples/commoncontainer/assembler.js 1231165
> http://svn.apache.org/repos/asf/shindig/trunk/features/src/main/javascript/features/container.gadget/gadget_holder.js 1231165
> http://svn.apache.org/repos/asf/shindig/trunk/features/src/main/javascript/features/container.gadget/gadget_site.js 1231165
> http://svn.apache.org/repos/asf/shindig/trunk/features/src/main/javascript/features/container.util/constant.js 1231165
> http://svn.apache.org/repos/asf/shindig/trunk/features/src/main/javascript/features/container.util/feature.xml 1231165
> http://svn.apache.org/repos/asf/shindig/trunk/features/src/main/javascript/features/container.util/util.js 1231165
> http://svn.apache.org/repos/asf/shindig/trunk/features/src/main/javascript/features/container/container.js 1231165
> http://svn.apache.org/repos/asf/shindig/trunk/features/src/main/javascript/features/container/feature.xml 1231165
> http://svn.apache.org/repos/asf/shindig/trunk/features/src/main/javascript/features/container/service.js 1231165
> http://svn.apache.org/repos/asf/shindig/trunk/features/src/test/javascript/features/container/gadget_holder_test.js 1231165
> http://svn.apache.org/repos/asf/shindig/trunk/features/src/test/javascript/features/container/service_test.js 1231165
> http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/GadgetsHandler.java 1231165
> http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/GadgetsHandlerApi.java 1231165
> http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/GadgetsHandlerService.java 1231165
> http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/ModuleIdManager.java PRE-CREATION
> http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/ModuleIdManagerImpl.java PRE-CREATION
> http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/test/java/org/apache/shindig/gadgets/servlet/GadgetsHandlerServiceTest.java 1231165
> http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/test/java/org/apache/shindig/gadgets/servlet/GadgetsHandlerTest.java 1231165
>
> 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
>
>
Re: Review Request: CommonContainer token refresh changes for a better UX
when tokens expire.
Posted by li xu <le...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/3180/#review4361
-----------------------------------------------------------
Ship it!
Thanks!
- li
On 2012-01-13 16:44:45, Dan Dumont wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/3180/
> -----------------------------------------------------------
>
> (Updated 2012-01-13 16:44:45)
>
>
> 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.
>
>
> This addresses bug SHINDIG-1681.
> https://issues.apache.org/jira/browse/SHINDIG-1681
>
>
> Diffs
> -----
>
> http://svn.apache.org/repos/asf/shindig/trunk/content/samplecontainer/examples/commoncontainer/assembler.js 1231165
> http://svn.apache.org/repos/asf/shindig/trunk/features/src/main/javascript/features/container.gadget/gadget_holder.js 1231165
> http://svn.apache.org/repos/asf/shindig/trunk/features/src/main/javascript/features/container.gadget/gadget_site.js 1231165
> http://svn.apache.org/repos/asf/shindig/trunk/features/src/main/javascript/features/container.util/constant.js 1231165
> http://svn.apache.org/repos/asf/shindig/trunk/features/src/main/javascript/features/container.util/feature.xml 1231165
> http://svn.apache.org/repos/asf/shindig/trunk/features/src/main/javascript/features/container.util/util.js 1231165
> http://svn.apache.org/repos/asf/shindig/trunk/features/src/main/javascript/features/container/container.js 1231165
> http://svn.apache.org/repos/asf/shindig/trunk/features/src/main/javascript/features/container/feature.xml 1231165
> http://svn.apache.org/repos/asf/shindig/trunk/features/src/main/javascript/features/container/service.js 1231165
> http://svn.apache.org/repos/asf/shindig/trunk/features/src/test/javascript/features/container/gadget_holder_test.js 1231165
> http://svn.apache.org/repos/asf/shindig/trunk/features/src/test/javascript/features/container/service_test.js 1231165
> http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/GadgetsHandler.java 1231165
> http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/GadgetsHandlerApi.java 1231165
> http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/GadgetsHandlerService.java 1231165
> http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/ModuleIdManager.java PRE-CREATION
> http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/ModuleIdManagerImpl.java PRE-CREATION
> http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/test/java/org/apache/shindig/gadgets/servlet/GadgetsHandlerServiceTest.java 1231165
> http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/test/java/org/apache/shindig/gadgets/servlet/GadgetsHandlerTest.java 1231165
>
> 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
>
>
Re: Review Request: CommonContainer token refresh changes for a better UX
when tokens expire.
Posted by Dan Dumont <dd...@us.ibm.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/3180/
-----------------------------------------------------------
(Updated 2012-01-13 16:44:45.741871)
Review request for shindig, Henry Saputra, Ryan Baxter, li xu, Jesse Ciancetta, and Stanton Sievers.
Changes
-------
Update patch against latest shindig revision
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.
This addresses bug SHINDIG-1681.
https://issues.apache.org/jira/browse/SHINDIG-1681
Diffs (updated)
-----
http://svn.apache.org/repos/asf/shindig/trunk/content/samplecontainer/examples/commoncontainer/assembler.js 1231165
http://svn.apache.org/repos/asf/shindig/trunk/features/src/main/javascript/features/container.gadget/gadget_holder.js 1231165
http://svn.apache.org/repos/asf/shindig/trunk/features/src/main/javascript/features/container.gadget/gadget_site.js 1231165
http://svn.apache.org/repos/asf/shindig/trunk/features/src/main/javascript/features/container.util/constant.js 1231165
http://svn.apache.org/repos/asf/shindig/trunk/features/src/main/javascript/features/container.util/feature.xml 1231165
http://svn.apache.org/repos/asf/shindig/trunk/features/src/main/javascript/features/container.util/util.js 1231165
http://svn.apache.org/repos/asf/shindig/trunk/features/src/main/javascript/features/container/container.js 1231165
http://svn.apache.org/repos/asf/shindig/trunk/features/src/main/javascript/features/container/feature.xml 1231165
http://svn.apache.org/repos/asf/shindig/trunk/features/src/main/javascript/features/container/service.js 1231165
http://svn.apache.org/repos/asf/shindig/trunk/features/src/test/javascript/features/container/gadget_holder_test.js 1231165
http://svn.apache.org/repos/asf/shindig/trunk/features/src/test/javascript/features/container/service_test.js 1231165
http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/GadgetsHandler.java 1231165
http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/GadgetsHandlerApi.java 1231165
http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/GadgetsHandlerService.java 1231165
http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/ModuleIdManager.java PRE-CREATION
http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/ModuleIdManagerImpl.java PRE-CREATION
http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/test/java/org/apache/shindig/gadgets/servlet/GadgetsHandlerServiceTest.java 1231165
http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/test/java/org/apache/shindig/gadgets/servlet/GadgetsHandlerTest.java 1231165
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
Re: Review Request: CommonContainer token refresh changes for a better UX
when tokens expire.
Posted by Jesse Ciancetta <jc...@mitre.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/3180/#review4360
-----------------------------------------------------------
Ship it!
LGTM
Thanks Dan!
- Jesse
On 2012-01-13 16:20:24, Dan Dumont wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/3180/
> -----------------------------------------------------------
>
> (Updated 2012-01-13 16:20:24)
>
>
> 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.
>
>
> This addresses bug SHINDIG-1681.
> https://issues.apache.org/jira/browse/SHINDIG-1681
>
>
> Diffs
> -----
>
> http://svn.apache.org/repos/asf/shindig/trunk/content/samplecontainer/examples/commoncontainer/assembler.js 1222407
> http://svn.apache.org/repos/asf/shindig/trunk/features/src/main/javascript/features/container.gadget/gadget_holder.js 1222407
> http://svn.apache.org/repos/asf/shindig/trunk/features/src/main/javascript/features/container.gadget/gadget_site.js 1222407
> http://svn.apache.org/repos/asf/shindig/trunk/features/src/main/javascript/features/container.util/constant.js 1222407
> http://svn.apache.org/repos/asf/shindig/trunk/features/src/main/javascript/features/container.util/feature.xml 1222407
> http://svn.apache.org/repos/asf/shindig/trunk/features/src/main/javascript/features/container.util/util.js 1222407
> http://svn.apache.org/repos/asf/shindig/trunk/features/src/main/javascript/features/container/container.js 1222407
> http://svn.apache.org/repos/asf/shindig/trunk/features/src/main/javascript/features/container/feature.xml 1222407
> http://svn.apache.org/repos/asf/shindig/trunk/features/src/main/javascript/features/container/service.js 1222407
> http://svn.apache.org/repos/asf/shindig/trunk/features/src/test/javascript/features/container/gadget_holder_test.js 1222407
> http://svn.apache.org/repos/asf/shindig/trunk/features/src/test/javascript/features/container/service_test.js 1222407
> http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/GadgetsHandler.java 1222407
> http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/GadgetsHandlerApi.java 1222407
> http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/GadgetsHandlerService.java 1222407
> http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/ModuleIdManager.java PRE-CREATION
> http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/ModuleIdManagerImpl.java PRE-CREATION
> http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/test/java/org/apache/shindig/gadgets/servlet/GadgetsHandlerServiceTest.java 1222407
> http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/test/java/org/apache/shindig/gadgets/servlet/GadgetsHandlerTest.java 1222407
>
> 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
>
>
Re: Review Request: CommonContainer token refresh changes for a better UX
when tokens expire.
Posted by Dan Dumont <dd...@us.ibm.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/3180/
-----------------------------------------------------------
(Updated 2012-01-13 16:20:24.912530)
Review request for shindig, Henry Saputra, Ryan Baxter, li xu, Jesse Ciancetta, and Stanton Sievers.
Changes
-------
Adding more docs and removing incorrect docs.
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.
This addresses bug SHINDIG-1681.
https://issues.apache.org/jira/browse/SHINDIG-1681
Diffs (updated)
-----
http://svn.apache.org/repos/asf/shindig/trunk/content/samplecontainer/examples/commoncontainer/assembler.js 1222407
http://svn.apache.org/repos/asf/shindig/trunk/features/src/main/javascript/features/container.gadget/gadget_holder.js 1222407
http://svn.apache.org/repos/asf/shindig/trunk/features/src/main/javascript/features/container.gadget/gadget_site.js 1222407
http://svn.apache.org/repos/asf/shindig/trunk/features/src/main/javascript/features/container.util/constant.js 1222407
http://svn.apache.org/repos/asf/shindig/trunk/features/src/main/javascript/features/container.util/feature.xml 1222407
http://svn.apache.org/repos/asf/shindig/trunk/features/src/main/javascript/features/container.util/util.js 1222407
http://svn.apache.org/repos/asf/shindig/trunk/features/src/main/javascript/features/container/container.js 1222407
http://svn.apache.org/repos/asf/shindig/trunk/features/src/main/javascript/features/container/feature.xml 1222407
http://svn.apache.org/repos/asf/shindig/trunk/features/src/main/javascript/features/container/service.js 1222407
http://svn.apache.org/repos/asf/shindig/trunk/features/src/test/javascript/features/container/gadget_holder_test.js 1222407
http://svn.apache.org/repos/asf/shindig/trunk/features/src/test/javascript/features/container/service_test.js 1222407
http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/GadgetsHandler.java 1222407
http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/GadgetsHandlerApi.java 1222407
http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/GadgetsHandlerService.java 1222407
http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/ModuleIdManager.java PRE-CREATION
http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/ModuleIdManagerImpl.java PRE-CREATION
http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/test/java/org/apache/shindig/gadgets/servlet/GadgetsHandlerServiceTest.java 1222407
http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/test/java/org/apache/shindig/gadgets/servlet/GadgetsHandlerTest.java 1222407
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
Re: Review Request: CommonContainer token refresh changes for a better UX
when tokens expire.
Posted by Dan Dumont <dd...@us.ibm.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/3180/#review4335
-----------------------------------------------------------
I'd like to commit this by EOD tomorrow (Friday). Please let me know if anyone would like more time to review and I can hold off.
- Dan
On 2012-01-10 20:53:59, Dan Dumont wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/3180/
> -----------------------------------------------------------
>
> (Updated 2012-01-10 20:53:59)
>
>
> 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.
>
>
> This addresses bug SHINDIG-1681.
> https://issues.apache.org/jira/browse/SHINDIG-1681
>
>
> Diffs
> -----
>
> http://svn.apache.org/repos/asf/shindig/trunk/content/samplecontainer/examples/commoncontainer/assembler.js 1222407
> http://svn.apache.org/repos/asf/shindig/trunk/features/src/main/javascript/features/container.gadget/gadget_holder.js 1222407
> http://svn.apache.org/repos/asf/shindig/trunk/features/src/main/javascript/features/container.gadget/gadget_site.js 1222407
> http://svn.apache.org/repos/asf/shindig/trunk/features/src/main/javascript/features/container.util/constant.js 1222407
> http://svn.apache.org/repos/asf/shindig/trunk/features/src/main/javascript/features/container.util/feature.xml 1222407
> http://svn.apache.org/repos/asf/shindig/trunk/features/src/main/javascript/features/container.util/util.js 1222407
> http://svn.apache.org/repos/asf/shindig/trunk/features/src/main/javascript/features/container/container.js 1222407
> http://svn.apache.org/repos/asf/shindig/trunk/features/src/main/javascript/features/container/feature.xml 1222407
> http://svn.apache.org/repos/asf/shindig/trunk/features/src/main/javascript/features/container/service.js 1222407
> http://svn.apache.org/repos/asf/shindig/trunk/features/src/test/javascript/features/container/gadget_holder_test.js 1222407
> http://svn.apache.org/repos/asf/shindig/trunk/features/src/test/javascript/features/container/service_test.js 1222407
> http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/GadgetsHandler.java 1222407
> http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/GadgetsHandlerApi.java 1222407
> http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/GadgetsHandlerService.java 1222407
> http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/ModuleIdManager.java PRE-CREATION
> http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/ModuleIdManagerImpl.java PRE-CREATION
> http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/test/java/org/apache/shindig/gadgets/servlet/GadgetsHandlerServiceTest.java 1222407
> http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/test/java/org/apache/shindig/gadgets/servlet/GadgetsHandlerTest.java 1222407
>
> 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
>
>
Re: Review Request: CommonContainer token refresh changes for a better UX
when tokens expire.
Posted by Dan Dumont <dd...@us.ibm.com>.
> On 2012-01-12 23:25:46, Ryan Baxter wrote:
> > Looks like you added some additional methods to gadget site and container util, do you want to also make the corresponding changes in the feature.xml files as well?
I think I've declared all of the ones I wanted to be public in the features.xml.
That file is just to assist in exports and closure process, right? Or do all methods used in other files need to be called out there?
> On 2012-01-12 23:25:46, Ryan Baxter wrote:
> > http://svn.apache.org/repos/asf/shindig/trunk/features/src/main/javascript/features/container.gadget/gadget_site.js, line 84
> > <https://reviews.apache.org/r/3180/diff/9/?file=67635#file67635line84>
> >
> > Maybe I am missing something here but you seem to be saying you can pass in the module id in args but here you are setting it to 0 instead of checking if its in the args
Whoops. I'll edit that comment.
- Dan
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/3180/#review4339
-----------------------------------------------------------
On 2012-01-10 20:53:59, Dan Dumont wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/3180/
> -----------------------------------------------------------
>
> (Updated 2012-01-10 20:53:59)
>
>
> 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.
>
>
> This addresses bug SHINDIG-1681.
> https://issues.apache.org/jira/browse/SHINDIG-1681
>
>
> Diffs
> -----
>
> http://svn.apache.org/repos/asf/shindig/trunk/content/samplecontainer/examples/commoncontainer/assembler.js 1222407
> http://svn.apache.org/repos/asf/shindig/trunk/features/src/main/javascript/features/container.gadget/gadget_holder.js 1222407
> http://svn.apache.org/repos/asf/shindig/trunk/features/src/main/javascript/features/container.gadget/gadget_site.js 1222407
> http://svn.apache.org/repos/asf/shindig/trunk/features/src/main/javascript/features/container.util/constant.js 1222407
> http://svn.apache.org/repos/asf/shindig/trunk/features/src/main/javascript/features/container.util/feature.xml 1222407
> http://svn.apache.org/repos/asf/shindig/trunk/features/src/main/javascript/features/container.util/util.js 1222407
> http://svn.apache.org/repos/asf/shindig/trunk/features/src/main/javascript/features/container/container.js 1222407
> http://svn.apache.org/repos/asf/shindig/trunk/features/src/main/javascript/features/container/feature.xml 1222407
> http://svn.apache.org/repos/asf/shindig/trunk/features/src/main/javascript/features/container/service.js 1222407
> http://svn.apache.org/repos/asf/shindig/trunk/features/src/test/javascript/features/container/gadget_holder_test.js 1222407
> http://svn.apache.org/repos/asf/shindig/trunk/features/src/test/javascript/features/container/service_test.js 1222407
> http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/GadgetsHandler.java 1222407
> http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/GadgetsHandlerApi.java 1222407
> http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/GadgetsHandlerService.java 1222407
> http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/ModuleIdManager.java PRE-CREATION
> http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/ModuleIdManagerImpl.java PRE-CREATION
> http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/test/java/org/apache/shindig/gadgets/servlet/GadgetsHandlerServiceTest.java 1222407
> http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/test/java/org/apache/shindig/gadgets/servlet/GadgetsHandlerTest.java 1222407
>
> 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
>
>
Re: Review Request: CommonContainer token refresh changes for a better UX
when tokens expire.
Posted by Ryan Baxter <rb...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/3180/#review4339
-----------------------------------------------------------
Looks like you added some additional methods to gadget site and container util, do you want to also make the corresponding changes in the feature.xml files as well?
http://svn.apache.org/repos/asf/shindig/trunk/features/src/main/javascript/features/container.gadget/gadget_site.js
<https://reviews.apache.org/r/3180/#comment9769>
Maybe I am missing something here but you seem to be saying you can pass in the module id in args but here you are setting it to 0 instead of checking if its in the args
- Ryan
On 2012-01-10 20:53:59, Dan Dumont wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/3180/
> -----------------------------------------------------------
>
> (Updated 2012-01-10 20:53:59)
>
>
> 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.
>
>
> This addresses bug SHINDIG-1681.
> https://issues.apache.org/jira/browse/SHINDIG-1681
>
>
> Diffs
> -----
>
> http://svn.apache.org/repos/asf/shindig/trunk/content/samplecontainer/examples/commoncontainer/assembler.js 1222407
> http://svn.apache.org/repos/asf/shindig/trunk/features/src/main/javascript/features/container.gadget/gadget_holder.js 1222407
> http://svn.apache.org/repos/asf/shindig/trunk/features/src/main/javascript/features/container.gadget/gadget_site.js 1222407
> http://svn.apache.org/repos/asf/shindig/trunk/features/src/main/javascript/features/container.util/constant.js 1222407
> http://svn.apache.org/repos/asf/shindig/trunk/features/src/main/javascript/features/container.util/feature.xml 1222407
> http://svn.apache.org/repos/asf/shindig/trunk/features/src/main/javascript/features/container.util/util.js 1222407
> http://svn.apache.org/repos/asf/shindig/trunk/features/src/main/javascript/features/container/container.js 1222407
> http://svn.apache.org/repos/asf/shindig/trunk/features/src/main/javascript/features/container/feature.xml 1222407
> http://svn.apache.org/repos/asf/shindig/trunk/features/src/main/javascript/features/container/service.js 1222407
> http://svn.apache.org/repos/asf/shindig/trunk/features/src/test/javascript/features/container/gadget_holder_test.js 1222407
> http://svn.apache.org/repos/asf/shindig/trunk/features/src/test/javascript/features/container/service_test.js 1222407
> http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/GadgetsHandler.java 1222407
> http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/GadgetsHandlerApi.java 1222407
> http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/GadgetsHandlerService.java 1222407
> http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/ModuleIdManager.java PRE-CREATION
> http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/ModuleIdManagerImpl.java PRE-CREATION
> http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/test/java/org/apache/shindig/gadgets/servlet/GadgetsHandlerServiceTest.java 1222407
> http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/test/java/org/apache/shindig/gadgets/servlet/GadgetsHandlerTest.java 1222407
>
> 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
>
>
Re: Review Request: CommonContainer token refresh changes for a better UX
when tokens expire.
Posted by li xu <le...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/3180/#review4354
-----------------------------------------------------------
Looks good. The new interface is neat. One minor comment, please see below.
Also I have problem applying patch in my eclipse workspace, there're lines unmatching for assembler.js and container.js.
http://svn.apache.org/repos/asf/shindig/trunk/features/src/main/javascript/features/container/container.js
<https://reviews.apache.org/r/3180/#comment9801>
Could you please add more js doc here?
The sample in assembler.js is helpful.
It would be nice to give an overview with example in one place.
For example:
1. container page need to define function to fetch container token when creating cc.
how common container would use this function?
2. what need to be done in that function? fetch container token and call the callback
3. Callback arguments that's passed to the function: token, ttl
- li
On 2012-01-10 20:53:59, Dan Dumont wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/3180/
> -----------------------------------------------------------
>
> (Updated 2012-01-10 20:53:59)
>
>
> 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.
>
>
> This addresses bug SHINDIG-1681.
> https://issues.apache.org/jira/browse/SHINDIG-1681
>
>
> Diffs
> -----
>
> http://svn.apache.org/repos/asf/shindig/trunk/content/samplecontainer/examples/commoncontainer/assembler.js 1222407
> http://svn.apache.org/repos/asf/shindig/trunk/features/src/main/javascript/features/container.gadget/gadget_holder.js 1222407
> http://svn.apache.org/repos/asf/shindig/trunk/features/src/main/javascript/features/container.gadget/gadget_site.js 1222407
> http://svn.apache.org/repos/asf/shindig/trunk/features/src/main/javascript/features/container.util/constant.js 1222407
> http://svn.apache.org/repos/asf/shindig/trunk/features/src/main/javascript/features/container.util/feature.xml 1222407
> http://svn.apache.org/repos/asf/shindig/trunk/features/src/main/javascript/features/container.util/util.js 1222407
> http://svn.apache.org/repos/asf/shindig/trunk/features/src/main/javascript/features/container/container.js 1222407
> http://svn.apache.org/repos/asf/shindig/trunk/features/src/main/javascript/features/container/feature.xml 1222407
> http://svn.apache.org/repos/asf/shindig/trunk/features/src/main/javascript/features/container/service.js 1222407
> http://svn.apache.org/repos/asf/shindig/trunk/features/src/test/javascript/features/container/gadget_holder_test.js 1222407
> http://svn.apache.org/repos/asf/shindig/trunk/features/src/test/javascript/features/container/service_test.js 1222407
> http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/GadgetsHandler.java 1222407
> http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/GadgetsHandlerApi.java 1222407
> http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/GadgetsHandlerService.java 1222407
> http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/ModuleIdManager.java PRE-CREATION
> http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/ModuleIdManagerImpl.java PRE-CREATION
> http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/test/java/org/apache/shindig/gadgets/servlet/GadgetsHandlerServiceTest.java 1222407
> http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/test/java/org/apache/shindig/gadgets/servlet/GadgetsHandlerTest.java 1222407
>
> 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
>
>
Re: Review Request: CommonContainer token refresh changes for a better UX
when tokens expire.
Posted by Dan Dumont <dd...@us.ibm.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/3180/
-----------------------------------------------------------
(Updated 2012-01-10 20:53:59.258090)
Review request for shindig, Henry Saputra, Ryan Baxter, li xu, Jesse Ciancetta, and Stanton Sievers.
Changes
-------
Changes from Jesse's feedback.
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.
This addresses bug SHINDIG-1681.
https://issues.apache.org/jira/browse/SHINDIG-1681
Diffs (updated)
-----
http://svn.apache.org/repos/asf/shindig/trunk/content/samplecontainer/examples/commoncontainer/assembler.js 1222407
http://svn.apache.org/repos/asf/shindig/trunk/features/src/main/javascript/features/container.gadget/gadget_holder.js 1222407
http://svn.apache.org/repos/asf/shindig/trunk/features/src/main/javascript/features/container.gadget/gadget_site.js 1222407
http://svn.apache.org/repos/asf/shindig/trunk/features/src/main/javascript/features/container.util/constant.js 1222407
http://svn.apache.org/repos/asf/shindig/trunk/features/src/main/javascript/features/container.util/feature.xml 1222407
http://svn.apache.org/repos/asf/shindig/trunk/features/src/main/javascript/features/container.util/util.js 1222407
http://svn.apache.org/repos/asf/shindig/trunk/features/src/main/javascript/features/container/container.js 1222407
http://svn.apache.org/repos/asf/shindig/trunk/features/src/main/javascript/features/container/feature.xml 1222407
http://svn.apache.org/repos/asf/shindig/trunk/features/src/main/javascript/features/container/service.js 1222407
http://svn.apache.org/repos/asf/shindig/trunk/features/src/test/javascript/features/container/gadget_holder_test.js 1222407
http://svn.apache.org/repos/asf/shindig/trunk/features/src/test/javascript/features/container/service_test.js 1222407
http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/GadgetsHandler.java 1222407
http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/GadgetsHandlerApi.java 1222407
http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/GadgetsHandlerService.java 1222407
http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/ModuleIdManager.java PRE-CREATION
http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/ModuleIdManagerImpl.java PRE-CREATION
http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/test/java/org/apache/shindig/gadgets/servlet/GadgetsHandlerServiceTest.java 1222407
http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/test/java/org/apache/shindig/gadgets/servlet/GadgetsHandlerTest.java 1222407
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
Re: Review Request: CommonContainer token refresh changes for a better UX
when tokens expire.
Posted by Dan Dumont <dd...@us.ibm.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/3180/
-----------------------------------------------------------
(Updated 2012-01-10 14:59:57.500691)
Review request for shindig, Henry Saputra, Ryan Baxter, li xu, Jesse Ciancetta, and Stanton Sievers.
Changes
-------
Adding Jira
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.
This addresses bug SHINDIG-1681.
https://issues.apache.org/jira/browse/SHINDIG-1681
Diffs
-----
http://svn.apache.org/repos/asf/shindig/trunk/content/samplecontainer/examples/commoncontainer/assembler.js 1222407
http://svn.apache.org/repos/asf/shindig/trunk/features/src/main/javascript/features/container.gadget/gadget_holder.js 1222407
http://svn.apache.org/repos/asf/shindig/trunk/features/src/main/javascript/features/container.gadget/gadget_site.js 1222407
http://svn.apache.org/repos/asf/shindig/trunk/features/src/main/javascript/features/container.util/constant.js 1222407
http://svn.apache.org/repos/asf/shindig/trunk/features/src/main/javascript/features/container.util/feature.xml 1222407
http://svn.apache.org/repos/asf/shindig/trunk/features/src/main/javascript/features/container.util/util.js 1222407
http://svn.apache.org/repos/asf/shindig/trunk/features/src/main/javascript/features/container/container.js 1222407
http://svn.apache.org/repos/asf/shindig/trunk/features/src/main/javascript/features/container/feature.xml 1222407
http://svn.apache.org/repos/asf/shindig/trunk/features/src/main/javascript/features/container/service.js 1222407
http://svn.apache.org/repos/asf/shindig/trunk/features/src/test/javascript/features/container/gadget_holder_test.js 1222407
http://svn.apache.org/repos/asf/shindig/trunk/features/src/test/javascript/features/container/service_test.js 1222407
http://svn.apache.org/repos/asf/shindig/trunk/java/common/src/main/java/org/apache/shindig/auth/AbstractSecurityToken.java 1222407
http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/GadgetsHandler.java 1222407
http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/GadgetsHandlerApi.java 1222407
http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/GadgetsHandlerService.java 1222407
http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/ModuleIdManager.java PRE-CREATION
http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/ModuleIdManagerImpl.java PRE-CREATION
http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/test/java/org/apache/shindig/gadgets/servlet/GadgetsHandlerServiceTest.java 1222407
http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/test/java/org/apache/shindig/gadgets/servlet/GadgetsHandlerTest.java 1222407
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
Re: Review Request: CommonContainer token refresh changes for a better UX
when tokens expire.
Posted by Dan Dumont <dd...@us.ibm.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/3180/
-----------------------------------------------------------
(Updated 2012-01-10 14:08:09.496346)
Review request for shindig, Henry Saputra, Ryan Baxter, li xu, Jesse Ciancetta, and Stanton Sievers.
Changes
-------
Moving some left-behind constants to the constants file.
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 (updated)
-----
http://svn.apache.org/repos/asf/shindig/trunk/content/samplecontainer/examples/commoncontainer/assembler.js 1222407
http://svn.apache.org/repos/asf/shindig/trunk/features/src/main/javascript/features/container.gadget/gadget_holder.js 1222407
http://svn.apache.org/repos/asf/shindig/trunk/features/src/main/javascript/features/container.gadget/gadget_site.js 1222407
http://svn.apache.org/repos/asf/shindig/trunk/features/src/main/javascript/features/container.util/constant.js 1222407
http://svn.apache.org/repos/asf/shindig/trunk/features/src/main/javascript/features/container.util/feature.xml 1222407
http://svn.apache.org/repos/asf/shindig/trunk/features/src/main/javascript/features/container.util/util.js 1222407
http://svn.apache.org/repos/asf/shindig/trunk/features/src/main/javascript/features/container/container.js 1222407
http://svn.apache.org/repos/asf/shindig/trunk/features/src/main/javascript/features/container/feature.xml 1222407
http://svn.apache.org/repos/asf/shindig/trunk/features/src/main/javascript/features/container/service.js 1222407
http://svn.apache.org/repos/asf/shindig/trunk/features/src/test/javascript/features/container/gadget_holder_test.js 1222407
http://svn.apache.org/repos/asf/shindig/trunk/features/src/test/javascript/features/container/service_test.js 1222407
http://svn.apache.org/repos/asf/shindig/trunk/java/common/src/main/java/org/apache/shindig/auth/AbstractSecurityToken.java 1222407
http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/GadgetsHandler.java 1222407
http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/GadgetsHandlerApi.java 1222407
http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/GadgetsHandlerService.java 1222407
http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/ModuleIdManager.java PRE-CREATION
http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/ModuleIdManagerImpl.java PRE-CREATION
http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/test/java/org/apache/shindig/gadgets/servlet/GadgetsHandlerServiceTest.java 1222407
http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/test/java/org/apache/shindig/gadgets/servlet/GadgetsHandlerTest.java 1222407
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
Re: Review Request: CommonContainer token refresh changes for a better UX
when tokens expire.
Posted by Dan Dumont <dd...@us.ibm.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/3180/
-----------------------------------------------------------
(Updated 2012-01-09 21:18:06.698353)
Review request for shindig, Henry Saputra, Ryan Baxter, li xu, Jesse Ciancetta, and Stanton Sievers.
Changes
-------
Hello again,
In today's installment of "ModuleIds - Things I wish I never knew" (version 6 -> 7) we have some constants moving to the constants cleanup, and some changes to the navigate chain to (hopefully) account for a moduleId being set in the RENDER_PARAMS bag during navigate.
ModuleId < 0: Please generate me a moduleId with a new token and let me know what they are
ModuleId == 0: No token extra token request.
ModuleId > 0: Get a token for a moduleId I've previously persisted.
If all goes according to plan, the moduleId would be parsed out, and if non-zero (default: 0) it would result in another request to get a security token for the gadget before the render is complete.
Jesse, I'd really appreciate it if you could go over all of this... Also, I think I might let you make the update to batch the metadata and token requests(if you don't mind)... because I think I'm starting to go cross-eyed looking at this code...
So for now it results in at worst 1 extra transaction for gadgets that specify a moduleId. But since that code path never existed before, I guess it shouldn't hurt shindig performance as-is.
This hasn't really been tested (the moduleId path which shindig doesn't use yet) because shindig doesn't have an impl for persisting moduleIds, so until I get that infra locally for our versions of shindig, I don't know if I'll really be able to pound on this. I hope that's ok.
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 (updated)
-----
http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/test/java/org/apache/shindig/gadgets/servlet/GadgetsHandlerTest.java 1222407
http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/test/java/org/apache/shindig/gadgets/servlet/GadgetsHandlerServiceTest.java 1222407
http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/ModuleIdManager.java PRE-CREATION
http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/ModuleIdManagerImpl.java PRE-CREATION
http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/GadgetsHandlerApi.java 1222407
http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/GadgetsHandlerService.java 1222407
http://svn.apache.org/repos/asf/shindig/trunk/features/src/test/javascript/features/container/gadget_holder_test.js 1222407
http://svn.apache.org/repos/asf/shindig/trunk/features/src/test/javascript/features/container/service_test.js 1222407
http://svn.apache.org/repos/asf/shindig/trunk/java/common/src/main/java/org/apache/shindig/auth/AbstractSecurityToken.java 1222407
http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/GadgetsHandler.java 1222407
http://svn.apache.org/repos/asf/shindig/trunk/features/src/main/javascript/features/container/feature.xml 1222407
http://svn.apache.org/repos/asf/shindig/trunk/features/src/main/javascript/features/container/service.js 1222407
http://svn.apache.org/repos/asf/shindig/trunk/features/src/main/javascript/features/container.util/util.js 1222407
http://svn.apache.org/repos/asf/shindig/trunk/features/src/main/javascript/features/container/container.js 1222407
http://svn.apache.org/repos/asf/shindig/trunk/features/src/main/javascript/features/container.util/constant.js 1222407
http://svn.apache.org/repos/asf/shindig/trunk/features/src/main/javascript/features/container.util/feature.xml 1222407
http://svn.apache.org/repos/asf/shindig/trunk/features/src/main/javascript/features/container.gadget/gadget_site.js 1222407
http://svn.apache.org/repos/asf/shindig/trunk/features/src/main/javascript/features/container.gadget/gadget_holder.js 1222407
http://svn.apache.org/repos/asf/shindig/trunk/content/samplecontainer/examples/commoncontainer/assembler.js 1222407
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
Re: Review Request: CommonContainer token refresh changes for a better UX
when tokens expire.
Posted by Dan Dumont <dd...@us.ibm.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/3180/
-----------------------------------------------------------
(Updated 2011-12-23 21:20:36.791685)
Review request for shindig, Henry Saputra, Ryan Baxter, li xu, Jesse Ciancetta, and Stanton Sievers.
Changes
-------
New changes: 5 -> 6
* GadgetHolder is now instantiated with a site, instead of a siteId. GadgetSite.getId() and GadgetSite.getModuleId() are used by this class now.
* The response to a token refresh call now includes the moduleId and the token's TTL. We (Jesse and I) plan to phase out the security tokens in MetadataRequests. See below for more detail.
* You can now request that a new moduleId be generated for a gadget when asking for a new token by setting it's moduleId to a negative number and fetching a new token. The response will come back with a moduleId you can save for persistence. There is a corresponding new class in Java to override if you plan on persisting moduleIds. It has 2 methods: validate and generate which are used to verify the validity of a moduleId, viewerId, gadgetUrl combination or to generate (and persist) a new moduleId for that combination.
I was talking with Jesse today and he had an idea that we could batch the metadata and security token requests into 1 request to remove the hard tie of having security tokens embedded in the metadata responses. I think this is probably a good idea and he got a little proof of concept going. I am hoping that he'll have time after the holidays to work on and post a patch (on top of this one) that, once reviewed I can include here.
For gadgets that require no token, we can probably just return null for their tokens in the token request part of the batch.
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 (updated)
-----
http://svn.apache.org/repos/asf/shindig/trunk/content/samplecontainer/examples/commoncontainer/assembler.js 1222407
http://svn.apache.org/repos/asf/shindig/trunk/features/src/main/javascript/features/container.gadget/gadget_holder.js 1222407
http://svn.apache.org/repos/asf/shindig/trunk/features/src/main/javascript/features/container.gadget/gadget_site.js 1222407
http://svn.apache.org/repos/asf/shindig/trunk/features/src/main/javascript/features/container.util/util.js 1222407
http://svn.apache.org/repos/asf/shindig/trunk/features/src/main/javascript/features/container/container.js 1222407
http://svn.apache.org/repos/asf/shindig/trunk/features/src/main/javascript/features/container/service.js 1222407
http://svn.apache.org/repos/asf/shindig/trunk/features/src/test/javascript/features/container/gadget_holder_test.js 1222407
http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/GadgetsHandler.java 1222407
http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/GadgetsHandlerApi.java 1222407
http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/GadgetsHandlerService.java 1222407
http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/ModuleIdManager.java PRE-CREATION
http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/ModuleIdManagerImpl.java PRE-CREATION
http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/test/java/org/apache/shindig/gadgets/servlet/GadgetsHandlerServiceTest.java 1222407
http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/test/java/org/apache/shindig/gadgets/servlet/GadgetsHandlerTest.java 1222407
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
Re: Review Request: CommonContainer token refresh changes for a better UX
when tokens expire.
Posted by Jesse Ciancetta <jc...@mitre.org>.
> On 2011-12-23 14:36:01, Jesse Ciancetta wrote:
> > I'm trying to respond to the questions Dan posted with his last update but I dont see a way to comment there -- so I guess I'll just put my comments here... Going to copy/paste Dan's questions and respond to them inline below.
> >
> > ---------------------------------------
> >
> > > Started porting of Jesse's review here: https://reviews.apache.org/r/1632
> > > and ran into some questions. (todo comments in gadget_site.js)
> >
> > > I'm wondering how a container would choose to persist a gadget, and what that would mean for it's token.
> > > Perhaps some other xhr to save the gadget and get a moduleId and then the container would set the moduleId of the site,
> > > which could trigger a token refresh (with the new moduleId).
> >
> > This doesnt seem like a common use case to me, but maybe I'm missing something. I would think that if a container was rendering transient gadgets they would stay transient and if a container was rendering persistent gadgets they would stay persistent.
> >
> > So for the transient case -- I could envision rendering a gadget on a sidebar somewhere that has something to do with the users current context.
> >
> > And for the persistent case -- I'm thinking igoogle or rave -- the user adds a gadget to their page which is persistent from the start.
> >
> > Do you have another use case in mind where a transient-but-already-rendered gadget is made persistent?
> >
> > > Some other interesting questions I have from reading Jesse's review:
> > > How would we tell the server to use a certain moduleId in a metadata request?
> >
> > I ran into this issue too. It would be easy enough to just change the metadata request format to gadgetUrl:moduleId but the problem with that is that it you make a metadata request for 5 instances of the same gadget (same gadget spec url but different moduleId's) you'd end up with 5 full metadata responses -- and the only difference between them all would be the security token. So in my patch I'd left the metadata request it as is. Of course that means you now have the potential for two XHR's per gadget navigate - one to fetch metadata and another to fetch a token. I think the only other option would be to change the structure of the metadata.
> >
> > Or actually -- come to think of it -- cant we send more than one request at a time? Couldnt we send a metadata request and token request together? Mahybe that would help.
> >
> > > Should we do that, or should we let any instances of the gadget which are not persisted use a default token with no moduleId in it?
> > > Should we fetch specific tokens, post metadata request, for a gadget if we know we are "restoring" a saved instance?
Following up on my earlier comment about batching metadata and token requests together so we only make one XHR -- I dug into and it looks like we can do it. From a freshly loaded common container page running off of shindig trunk this code seems to do it:
-----------------
var gadgetUrl = 'http://localhost:8080/samplecontainer/examples/media/Media.xml';
var metadataRequest = osapi.container.util.newMetadataRequest(gadgetUrl);
var tokenRequest = osapi.container.util.newTokenRequest(gadgetUrl);
var batch = osapi.newBatch().
add('metadata', osapi['gadgets']['metadata'](metadataRequest)).
add('token', osapi['gadgets']['token'](tokenRequest));
batch.execute(function(result) {
console.dir(result);
});
- Jesse
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/3180/#review4105
-----------------------------------------------------------
On 2011-12-22 21:00:32, Dan Dumont wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/3180/
> -----------------------------------------------------------
>
> (Updated 2011-12-22 21:00:32)
>
>
> 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/content/samplecontainer/examples/commoncontainer/assembler.js 1222407
> http://svn.apache.org/repos/asf/shindig/trunk/features/src/main/javascript/features/container.gadget/gadget_site.js 1222407
> http://svn.apache.org/repos/asf/shindig/trunk/features/src/main/javascript/features/container/container.js 1222407
> http://svn.apache.org/repos/asf/shindig/trunk/features/src/main/javascript/features/container/service.js 1222407
>
> 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
>
>
Re: Review Request: CommonContainer token refresh changes for a better UX
when tokens expire.
Posted by Jesse Ciancetta <jc...@mitre.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/3180/#review4105
-----------------------------------------------------------
I'm trying to respond to the questions Dan posted with his last update but I dont see a way to comment there -- so I guess I'll just put my comments here... Going to copy/paste Dan's questions and respond to them inline below.
---------------------------------------
> Started porting of Jesse's review here: https://reviews.apache.org/r/1632
> and ran into some questions. (todo comments in gadget_site.js)
> I'm wondering how a container would choose to persist a gadget, and what that would mean for it's token.
> Perhaps some other xhr to save the gadget and get a moduleId and then the container would set the moduleId of the site,
> which could trigger a token refresh (with the new moduleId).
This doesnt seem like a common use case to me, but maybe I'm missing something. I would think that if a container was rendering transient gadgets they would stay transient and if a container was rendering persistent gadgets they would stay persistent.
So for the transient case -- I could envision rendering a gadget on a sidebar somewhere that has something to do with the users current context.
And for the persistent case -- I'm thinking igoogle or rave -- the user adds a gadget to their page which is persistent from the start.
Do you have another use case in mind where a transient-but-already-rendered gadget is made persistent?
> Some other interesting questions I have from reading Jesse's review:
> How would we tell the server to use a certain moduleId in a metadata request?
I ran into this issue too. It would be easy enough to just change the metadata request format to gadgetUrl:moduleId but the problem with that is that it you make a metadata request for 5 instances of the same gadget (same gadget spec url but different moduleId's) you'd end up with 5 full metadata responses -- and the only difference between them all would be the security token. So in my patch I'd left the metadata request it as is. Of course that means you now have the potential for two XHR's per gadget navigate - one to fetch metadata and another to fetch a token. I think the only other option would be to change the structure of the metadata.
Or actually -- come to think of it -- cant we send more than one request at a time? Couldnt we send a metadata request and token request together? Mahybe that would help.
> Should we do that, or should we let any instances of the gadget which are not persisted use a default token with no moduleId in it?
> Should we fetch specific tokens, post metadata request, for a gadget if we know we are "restoring" a saved instance?
- Jesse
On 2011-12-22 21:00:32, Dan Dumont wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/3180/
> -----------------------------------------------------------
>
> (Updated 2011-12-22 21:00:32)
>
>
> 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/content/samplecontainer/examples/commoncontainer/assembler.js 1222407
> http://svn.apache.org/repos/asf/shindig/trunk/features/src/main/javascript/features/container.gadget/gadget_site.js 1222407
> http://svn.apache.org/repos/asf/shindig/trunk/features/src/main/javascript/features/container/container.js 1222407
> http://svn.apache.org/repos/asf/shindig/trunk/features/src/main/javascript/features/container/service.js 1222407
>
> 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
>
>
Re: Review Request: CommonContainer token refresh changes for a better UX
when tokens expire.
Posted by Dan Dumont <dd...@us.ibm.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/3180/
-----------------------------------------------------------
(Updated 2011-12-22 21:00:32.001973)
Review request for shindig, Henry Saputra, Ryan Baxter, li xu, Jesse Ciancetta, and Stanton Sievers.
Changes
-------
Added container token refresh function to sample container.
Started porting of Jesse's review here: https://reviews.apache.org/r/1632
and ran into some questions. (todo comments in gadget_site.js)
I'm wondering how a container would choose to persist a gadget, and what that would mean for it's token.
Perhaps some other xhr to save the gadget and get a moduleId and then the container would set the moduleId of the site, which could trigger a token refresh (with the new moduleId).
Some other interesting questions I have from reading Jesse's review:
How would we tell the server to use a certain moduleId in a metadata request?
Should we do that, or should we let any instances of the gadget which are not persisted use a default token with no moduleId in it?
Should we fetch specific tokens, post metadata request, for a gadget if we know we are "restoring" a saved instance?
(Compare patch versions 4 and 5 to see the changes in this update.)
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 (updated)
-----
http://svn.apache.org/repos/asf/shindig/trunk/content/samplecontainer/examples/commoncontainer/assembler.js 1222407
http://svn.apache.org/repos/asf/shindig/trunk/features/src/main/javascript/features/container.gadget/gadget_site.js 1222407
http://svn.apache.org/repos/asf/shindig/trunk/features/src/main/javascript/features/container/container.js 1222407
http://svn.apache.org/repos/asf/shindig/trunk/features/src/main/javascript/features/container/service.js 1222407
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
Re: Review Request: CommonContainer token refresh changes for a better UX
when tokens expire.
Posted by Dan Dumont <dd...@us.ibm.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/3180/
-----------------------------------------------------------
(Updated 2011-12-22 19:58:30.507072)
Review request for shindig, Henry Saputra, Ryan Baxter, li xu, Jesse Ciancetta, and Stanton Sievers.
Changes
-------
Moved container token refresh impl to service so that it could more easily be used by the getGadgetMetadata function (which is where it's really needed, not surrounding navigate).
Left the public api on Container which calls into service for the impl.
Adjusted if/else line style to: } else {
Tests are now working with the getGadgetMetadata call wrapped in the updateContainerSecurityToken call.
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 (updated)
-----
http://svn.apache.org/repos/asf/shindig/trunk/features/src/main/javascript/features/container/service.js 1222407
http://svn.apache.org/repos/asf/shindig/trunk/features/src/main/javascript/features/container/container.js 1222407
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