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/08/31 22:07:34 UTC
Review Request: Implement Specification: Issue 1210: Extend actions feature
to allow container to register listeners for when actions are run
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/1689/
-----------------------------------------------------------
Review request for shindig, Ryan Baxter and Stanton Sievers.
Summary
-------
Specification: http://code.google.com/p/opensocial-resources/issues/detail?id=1210
Add 2 functions to the actions feature.
1 to allow the container to register a listener for an action being run. The listener would be run whenever an action matching the id provided was run.
1 to allow the container to unregister(remove) any listener added with the previous function.
This addresses bug SHINDIG-1612.
https://issues.apache.org/jira/browse/SHINDIG-1612
Diffs
-----
http://svn.apache.org/repos/asf/shindig/trunk/features/src/main/javascript/features/actions/actions_container.js 1163664
Diff: https://reviews.apache.org/r/1689/diff
Testing
-------
If anyone has any suggestions as to how to approach adding unit tests for this, I'm all ears.
It looks like that in order to get the actions api to hit this code, by calling runAction, I'd need to somehow fake setting up a gadget site and register a fake action.
Thanks,
Dan
Re: Review Request: Implement Specification: Issue 1210: Extend actions
feature to allow container to register listeners for when actions are run
Posted by Dan Dumont <dd...@us.ibm.com>.
> On 2011-08-31 21:30:42, Stanton Sievers wrote:
> > http://svn.apache.org/repos/asf/shindig/trunk/features/src/main/javascript/features/actions/actions_container.js, line 785
> > <https://reviews.apache.org/r/1689/diff/2/?file=37008#file37008line785>
> >
> > Can you also verify typeof callback is a Function? If I push a non-Function into your actionListenerMap, it'll just get called in the runAction code and puke.
To some degree, that kind of error might be useful for a container author. It's not like random gadgets will be able to call this, it'a container only method.
> On 2011-08-31 21:30:42, Stanton Sievers wrote:
> > http://svn.apache.org/repos/asf/shindig/trunk/features/src/main/javascript/features/actions/actions_container.js, line 789
> > <https://reviews.apache.org/r/1689/diff/2/?file=37008#file37008line789>
> >
> > My gut feeling is that the token being an object with a function in it is a bad idea. Why can't the token be a number? Are you concerned that malicious/buggy code will accidentally remove a listener it didn't add?
Why is that? the function inside is a nicely packed closure that was created in scope of the variables in which the callbacks are stored. It's pretty tamper-proof and makes removing the a breeze because a reference to the callback passed in originally is saved. It allows containers to use anonymous functions instead of being forced to save a reference to them and pass them back. It also prevents the need of having to maintain a sparse array of callback mappings.
> On 2011-08-31 21:30:42, Stanton Sievers wrote:
> > http://svn.apache.org/repos/asf/shindig/trunk/features/src/main/javascript/features/actions/actions_container.js, line 810
> > <https://reviews.apache.org/r/1689/diff/2/?file=37008#file37008line810>
> >
> > Is it necessary to check the typeof regToken.remove?
I suppose not. The api is pretty clear here that you must pass in the object you got. If they're that determined to shoot themselves in the foot at this point, I might as well let them :)
- Dan
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/1689/#review1706
-----------------------------------------------------------
On 2011-08-31 20:07:34, Dan Dumont wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/1689/
> -----------------------------------------------------------
>
> (Updated 2011-08-31 20:07:34)
>
>
> Review request for shindig, Ryan Baxter and Stanton Sievers.
>
>
> Summary
> -------
>
> Specification: http://code.google.com/p/opensocial-resources/issues/detail?id=1210
>
> Add 2 functions to the actions feature.
> 1 to allow the container to register a listener for an action being run. The listener would be run whenever an action matching the id provided was run.
> 1 to allow the container to unregister(remove) any listener added with the previous function.
>
>
> This addresses bug SHINDIG-1612.
> https://issues.apache.org/jira/browse/SHINDIG-1612
>
>
> Diffs
> -----
>
> http://svn.apache.org/repos/asf/shindig/trunk/features/src/main/javascript/features/actions/actions_container.js 1163664
>
> Diff: https://reviews.apache.org/r/1689/diff
>
>
> Testing
> -------
>
> If anyone has any suggestions as to how to approach adding unit tests for this, I'm all ears.
>
> It looks like that in order to get the actions api to hit this code, by calling runAction, I'd need to somehow fake setting up a gadget site and register a fake action.
>
>
> Thanks,
>
> Dan
>
>
Re: Review Request: Implement Specification: Issue 1210: Extend actions
feature to allow container to register listeners for when actions are run
Posted by Dan Dumont <dd...@us.ibm.com>.
> On 2011-08-31 21:30:42, Stanton Sievers wrote:
> > http://svn.apache.org/repos/asf/shindig/trunk/features/src/main/javascript/features/actions/actions_container.js, line 785
> > <https://reviews.apache.org/r/1689/diff/2/?file=37008#file37008line785>
> >
> > Can you also verify typeof callback is a Function? If I push a non-Function into your actionListenerMap, it'll just get called in the runAction code and puke.
>
> Dan Dumont wrote:
> To some degree, that kind of error might be useful for a container author. It's not like random gadgets will be able to call this, it'a container only method.
I do suppose I can have the code puke earlier in the initial register call so that the error isn't deferred. Which is actually the point you were probably trying to make.
- Dan
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/1689/#review1706
-----------------------------------------------------------
On 2011-08-31 20:07:34, Dan Dumont wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/1689/
> -----------------------------------------------------------
>
> (Updated 2011-08-31 20:07:34)
>
>
> Review request for shindig, Ryan Baxter and Stanton Sievers.
>
>
> Summary
> -------
>
> Specification: http://code.google.com/p/opensocial-resources/issues/detail?id=1210
>
> Add 2 functions to the actions feature.
> 1 to allow the container to register a listener for an action being run. The listener would be run whenever an action matching the id provided was run.
> 1 to allow the container to unregister(remove) any listener added with the previous function.
>
>
> This addresses bug SHINDIG-1612.
> https://issues.apache.org/jira/browse/SHINDIG-1612
>
>
> Diffs
> -----
>
> http://svn.apache.org/repos/asf/shindig/trunk/features/src/main/javascript/features/actions/actions_container.js 1163664
>
> Diff: https://reviews.apache.org/r/1689/diff
>
>
> Testing
> -------
>
> If anyone has any suggestions as to how to approach adding unit tests for this, I'm all ears.
>
> It looks like that in order to get the actions api to hit this code, by calling runAction, I'd need to somehow fake setting up a gadget site and register a fake action.
>
>
> Thanks,
>
> Dan
>
>
Re: Review Request: Implement Specification: Issue 1210: Extend actions
feature to allow container to register listeners for when actions are run
Posted by Stanton Sievers <si...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/1689/#review1706
-----------------------------------------------------------
http://svn.apache.org/repos/asf/shindig/trunk/features/src/main/javascript/features/actions/actions_container.js
<https://reviews.apache.org/r/1689/#comment3876>
Can you also verify typeof callback is a Function? If I push a non-Function into your actionListenerMap, it'll just get called in the runAction code and puke.
http://svn.apache.org/repos/asf/shindig/trunk/features/src/main/javascript/features/actions/actions_container.js
<https://reviews.apache.org/r/1689/#comment3878>
My gut feeling is that the token being an object with a function in it is a bad idea. Why can't the token be a number? Are you concerned that malicious/buggy code will accidentally remove a listener it didn't add?
http://svn.apache.org/repos/asf/shindig/trunk/features/src/main/javascript/features/actions/actions_container.js
<https://reviews.apache.org/r/1689/#comment3877>
Is it necessary to check the typeof regToken.remove?
- Stanton
On 2011-08-31 20:07:34, Dan Dumont wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/1689/
> -----------------------------------------------------------
>
> (Updated 2011-08-31 20:07:34)
>
>
> Review request for shindig, Ryan Baxter and Stanton Sievers.
>
>
> Summary
> -------
>
> Specification: http://code.google.com/p/opensocial-resources/issues/detail?id=1210
>
> Add 2 functions to the actions feature.
> 1 to allow the container to register a listener for an action being run. The listener would be run whenever an action matching the id provided was run.
> 1 to allow the container to unregister(remove) any listener added with the previous function.
>
>
> This addresses bug SHINDIG-1612.
> https://issues.apache.org/jira/browse/SHINDIG-1612
>
>
> Diffs
> -----
>
> http://svn.apache.org/repos/asf/shindig/trunk/features/src/main/javascript/features/actions/actions_container.js 1163664
>
> Diff: https://reviews.apache.org/r/1689/diff
>
>
> Testing
> -------
>
> If anyone has any suggestions as to how to approach adding unit tests for this, I'm all ears.
>
> It looks like that in order to get the actions api to hit this code, by calling runAction, I'd need to somehow fake setting up a gadget site and register a fake action.
>
>
> Thanks,
>
> Dan
>
>
Re: Review Request: Implement Specification: Issue 1210: Extend actions
feature to allow container to register listeners for when actions are run
Posted by Dan Dumont <dd...@us.ibm.com>.
> On 2011-08-31 21:43:06, Ryan Baxter wrote:
> > http://svn.apache.org/repos/asf/shindig/trunk/features/src/main/javascript/features/actions/actions_container.js, line 789
> > <https://reviews.apache.org/r/1689/diff/2/?file=37008#file37008line789>
> >
> > Any particular reason why you chose to do the removeListener this way? I would expect the API to take in the listener you want to remove, not a token....
> > One disadvantage I could think of is you have to save off this token somewhere to use it later on if you want to remove it. Then again you also need to save off your listener if you want to remove it as well, so maybe there really is no difference. Also as far as consistency, are there other listener APIs in the container today? How do they do it?
The examples of registering handlers in actions are all singletons. So is the container's addGadgetLifecycleCallback.
I was looking for something a little more flexible for the actions listeners. Allowing the container to register different functions to handle different action invocations (or the same action invocation).
I also did not want to have 1 single function callback for all actions, as actions can happen many times... and there is a cost to executing a function. Checking a boolean expression first to see if we should even call the callback based on the id saves us that unneeded function call.
If there's significant pressure from the community on conforming to the existing model I'll follow suit, but I think actions are a specific, dynamic, and frequent enough occurrence to justify a more flexible impl.
- Dan
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/1689/#review1705
-----------------------------------------------------------
On 2011-08-31 20:07:34, Dan Dumont wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/1689/
> -----------------------------------------------------------
>
> (Updated 2011-08-31 20:07:34)
>
>
> Review request for shindig, Ryan Baxter and Stanton Sievers.
>
>
> Summary
> -------
>
> Specification: http://code.google.com/p/opensocial-resources/issues/detail?id=1210
>
> Add 2 functions to the actions feature.
> 1 to allow the container to register a listener for an action being run. The listener would be run whenever an action matching the id provided was run.
> 1 to allow the container to unregister(remove) any listener added with the previous function.
>
>
> This addresses bug SHINDIG-1612.
> https://issues.apache.org/jira/browse/SHINDIG-1612
>
>
> Diffs
> -----
>
> http://svn.apache.org/repos/asf/shindig/trunk/features/src/main/javascript/features/actions/actions_container.js 1163664
>
> Diff: https://reviews.apache.org/r/1689/diff
>
>
> Testing
> -------
>
> If anyone has any suggestions as to how to approach adding unit tests for this, I'm all ears.
>
> It looks like that in order to get the actions api to hit this code, by calling runAction, I'd need to somehow fake setting up a gadget site and register a fake action.
>
>
> Thanks,
>
> Dan
>
>
Re: Review Request: Implement Specification: Issue 1210: Extend actions
feature to allow container to register listeners for when actions are run
Posted by Ryan Baxter <rb...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/1689/#review1705
-----------------------------------------------------------
http://svn.apache.org/repos/asf/shindig/trunk/features/src/main/javascript/features/actions/actions_container.js
<https://reviews.apache.org/r/1689/#comment3875>
Small nit, I would prefer you call the second parameter listener instead of callback, since the name of your method has the word listener in it.
http://svn.apache.org/repos/asf/shindig/trunk/features/src/main/javascript/features/actions/actions_container.js
<https://reviews.apache.org/r/1689/#comment3874>
Any particular reason why you chose to do the removeListener this way? I would expect the API to take in the listener you want to remove, not a token....
One disadvantage I could think of is you have to save off this token somewhere to use it later on if you want to remove it. Then again you also need to save off your listener if you want to remove it as well, so maybe there really is no difference. Also as far as consistency, are there other listener APIs in the container today? How do they do it?
- Ryan
On 2011-08-31 20:07:34, Dan Dumont wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/1689/
> -----------------------------------------------------------
>
> (Updated 2011-08-31 20:07:34)
>
>
> Review request for shindig, Ryan Baxter and Stanton Sievers.
>
>
> Summary
> -------
>
> Specification: http://code.google.com/p/opensocial-resources/issues/detail?id=1210
>
> Add 2 functions to the actions feature.
> 1 to allow the container to register a listener for an action being run. The listener would be run whenever an action matching the id provided was run.
> 1 to allow the container to unregister(remove) any listener added with the previous function.
>
>
> This addresses bug SHINDIG-1612.
> https://issues.apache.org/jira/browse/SHINDIG-1612
>
>
> Diffs
> -----
>
> http://svn.apache.org/repos/asf/shindig/trunk/features/src/main/javascript/features/actions/actions_container.js 1163664
>
> Diff: https://reviews.apache.org/r/1689/diff
>
>
> Testing
> -------
>
> If anyone has any suggestions as to how to approach adding unit tests for this, I'm all ears.
>
> It looks like that in order to get the actions api to hit this code, by calling runAction, I'd need to somehow fake setting up a gadget site and register a fake action.
>
>
> Thanks,
>
> Dan
>
>
Re: Review Request: Implement Specification: Issue 1210: Extend actions
feature to allow container to register listeners for when actions are run
Posted by Stanton Sievers <si...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/1689/#review1716
-----------------------------------------------------------
Ship it!
LGTM
- Stanton
On 2011-08-31 22:12:15, Dan Dumont wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/1689/
> -----------------------------------------------------------
>
> (Updated 2011-08-31 22:12:15)
>
>
> Review request for shindig, Ryan Baxter and Stanton Sievers.
>
>
> Summary
> -------
>
> Specification: http://code.google.com/p/opensocial-resources/issues/detail?id=1210
>
> Add 2 functions to the actions feature.
> 1 to allow the container to register a listener for an action being run. The listener would be run whenever an action matching the id provided was run.
> 1 to allow the container to unregister(remove) any listener added with the previous function.
>
>
> This addresses bug SHINDIG-1612.
> https://issues.apache.org/jira/browse/SHINDIG-1612
>
>
> Diffs
> -----
>
> http://svn.apache.org/repos/asf/shindig/trunk/features/src/main/javascript/features/actions/actions_container.js 1163664
>
> Diff: https://reviews.apache.org/r/1689/diff
>
>
> Testing
> -------
>
> If anyone has any suggestions as to how to approach adding unit tests for this, I'm all ears.
>
> It looks like that in order to get the actions api to hit this code, by calling runAction, I'd need to somehow fake setting up a gadget site and register a fake action.
>
>
> Thanks,
>
> Dan
>
>
Re: Review Request: Implement Specification: Issue 1210: Extend actions
feature to allow container to register listeners for when actions are run
Posted by Ryan Baxter <rb...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/1689/#review1978
-----------------------------------------------------------
Ship it!
Committed revision 1173325.
- Ryan
On 2011-09-16 20:36:03, Dan Dumont wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/1689/
> -----------------------------------------------------------
>
> (Updated 2011-09-16 20:36:03)
>
>
> Review request for shindig, Ryan Baxter, Matthew Hatem, and Stanton Sievers.
>
>
> Summary
> -------
>
> Specification: http://code.google.com/p/opensocial-resources/issues/detail?id=1210
>
> Add 2 functions to the actions feature.
> 1 to allow the container to register a listener for an action being run. The listener would be run whenever an action matching the id provided was run.
> 1 to allow the container to unregister(remove) any listener added with the previous function.
>
>
> This addresses bug SHINDIG-1612.
> https://issues.apache.org/jira/browse/SHINDIG-1612
>
>
> Diffs
> -----
>
> http://svn.apache.org/repos/asf/shindig/trunk/features/src/main/javascript/features/actions/actions_container.js 1171739
>
> Diff: https://reviews.apache.org/r/1689/diff
>
>
> Testing
> -------
>
> If anyone has any suggestions as to how to approach adding unit tests for this, I'm all ears.
>
> It looks like that in order to get the actions api to hit this code, by calling runAction, I'd need to somehow fake setting up a gadget site and register a fake action.
>
>
> Thanks,
>
> Dan
>
>
Re: Review Request: Implement Specification: Issue 1210: Extend actions
feature to allow container to register listeners for when actions are run
Posted by Dan Dumont <dd...@us.ibm.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/1689/
-----------------------------------------------------------
(Updated 2011-09-16 20:36:03.558948)
Review request for shindig, Ryan Baxter, Matthew Hatem, and Stanton Sievers.
Changes
-------
Added some nifty jsdoc annotations for closure compiler and better clarification for some types.
Changed impl based on patch from Matt Hatem.
One noteworthy change is that the caller of addListener is expected to keep a reference to the exact function passed into addListener in order to pass it in to removeListener later. Remove tokens are no longer provided.
Summary
-------
Specification: http://code.google.com/p/opensocial-resources/issues/detail?id=1210
Add 2 functions to the actions feature.
1 to allow the container to register a listener for an action being run. The listener would be run whenever an action matching the id provided was run.
1 to allow the container to unregister(remove) any listener added with the previous function.
This addresses bug SHINDIG-1612.
https://issues.apache.org/jira/browse/SHINDIG-1612
Diffs (updated)
-----
http://svn.apache.org/repos/asf/shindig/trunk/features/src/main/javascript/features/actions/actions_container.js 1171739
Diff: https://reviews.apache.org/r/1689/diff
Testing
-------
If anyone has any suggestions as to how to approach adding unit tests for this, I'm all ears.
It looks like that in order to get the actions api to hit this code, by calling runAction, I'd need to somehow fake setting up a gadget site and register a fake action.
Thanks,
Dan
Re: Review Request: Implement Specification: Issue 1210: Extend actions
feature to allow container to register listeners for when actions are run
Posted by Dan Dumont <dd...@us.ibm.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/1689/
-----------------------------------------------------------
(Updated 2011-09-08 14:01:47.339559)
Review request for shindig, Ryan Baxter, Matthew Hatem, and Stanton Sievers.
Summary
-------
Specification: http://code.google.com/p/opensocial-resources/issues/detail?id=1210
Add 2 functions to the actions feature.
1 to allow the container to register a listener for an action being run. The listener would be run whenever an action matching the id provided was run.
1 to allow the container to unregister(remove) any listener added with the previous function.
This addresses bug SHINDIG-1612.
https://issues.apache.org/jira/browse/SHINDIG-1612
Diffs
-----
http://svn.apache.org/repos/asf/shindig/trunk/features/src/main/javascript/features/actions/actions_container.js 1163664
Diff: https://reviews.apache.org/r/1689/diff
Testing
-------
If anyone has any suggestions as to how to approach adding unit tests for this, I'm all ears.
It looks like that in order to get the actions api to hit this code, by calling runAction, I'd need to somehow fake setting up a gadget site and register a fake action.
Thanks,
Dan
Re: Review Request: Implement Specification: Issue 1210: Extend actions
feature to allow container to register listeners for when actions are run
Posted by Dan Dumont <dd...@us.ibm.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/1689/
-----------------------------------------------------------
(Updated 2011-08-31 22:12:15.636913)
Review request for shindig, Ryan Baxter and Stanton Sievers.
Changes
-------
Updating from feedback from Ryan and Stanton.
Summary
-------
Specification: http://code.google.com/p/opensocial-resources/issues/detail?id=1210
Add 2 functions to the actions feature.
1 to allow the container to register a listener for an action being run. The listener would be run whenever an action matching the id provided was run.
1 to allow the container to unregister(remove) any listener added with the previous function.
This addresses bug SHINDIG-1612.
https://issues.apache.org/jira/browse/SHINDIG-1612
Diffs (updated)
-----
http://svn.apache.org/repos/asf/shindig/trunk/features/src/main/javascript/features/actions/actions_container.js 1163664
Diff: https://reviews.apache.org/r/1689/diff
Testing
-------
If anyone has any suggestions as to how to approach adding unit tests for this, I'm all ears.
It looks like that in order to get the actions api to hit this code, by calling runAction, I'd need to somehow fake setting up a gadget site and register a fake action.
Thanks,
Dan