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