Hi Ryan: The pattern of exposing a magical variable that's read when you call this.mixin_() seems a little odd to me. I'd prefer to have something more direct, ie have the mixin method itself take a method/function (which we could define as being bound to "this" if you like).
--j On Tue, Mar 8, 2011 at 9:36 AM, Ryan J Baxter <[email protected]> wrote: > Anyone have any objections to this? I would like to get this submitted. > > -Ryan > > Email: [email protected] > Phone: 978-899-3041 > developerWorks Profile > > > > From: [email protected] > To: [email protected], [email protected], > [email protected], > Cc: [email protected], [email protected] > Date: 03/04/2011 11:36 AM > Subject: Allow extensions to the common container by allowing other > features to provide their own namespace (issue4260049) > > > > Reviewers: mhermanto, dev_shindig.apache.org, > dev-remailer_shindig.apache.org, > > Description: > At the moment the common container cannot be extended by adding a new > namespace and still have access to the container object. The only > reasonable way to extend the common container at this moment it to add > new methods by calling .prototype or by subclassing it. Another useful > extension would be to allow features to add their own namespace to the > common container and add new methods under that common container. > However those methods may still want to reference other methods inside > the common container so they need to be able to access the common > container object itself. > > Please review this at http://codereview.appspot.com/4260049/ > > Affected files: > features/src/main/javascript/features/container/container.js > features/src/test/javascript/features/container/container_test.js > > > ### Eclipse Workspace Patch 1.0 > #P shindig-project > Index: features/src/test/javascript/features/container/container_test.js > =================================================================== > --- features/src/test/javascript/features/container/container_test.js > (revision 1078023) > +++ features/src/test/javascript/features/container/container_test.js > (working copy) > @@ -112,6 +112,22 @@ > this.assertTrue(container.sites_[2] != null); > }; > > +ContainerTest.prototype.testMixins_ = function(){ > + this.setupGadgetsRpcRegister(); > + shindig.container.Container.prototype.mixins_['test'] = > function(context){ > + return { > + "getSitesLength" : > function(){ > + return > context.sites_.length; > + } > + }; > + }; > + var container = new shindig.container.Container(); > + this.setupGadgetSite(1, {}, null); > + container.newGadgetSite(null); > + this.assertTrue(container.sites_[1] != null); > + this.assertEquals(container.sites_.length, > container.test.getSitesLength()); > +}; > + > ContainerTest.prototype.setupGadgetSite = function(id, gadgetInfo, > gadgetHolder) { > var self = this; > shindig.container.GadgetSite = function() { > @@ -137,4 +153,4 @@ > register: function() { > } > }; > -}; > +}; > \ No newline at end of file > Index: features/src/main/javascript/features/container/container.js > =================================================================== > --- features/src/main/javascript/features/container/container.js > (revision > 1078023) > +++ features/src/main/javascript/features/container/container.js (working > > copy) > @@ -113,6 +113,8 @@ > * @private > */ > this.tokenRefreshTimer_ = null; > + > + this.mixin_(); > > this.preloadFromConfig_(config); > > @@ -121,7 +123,6 @@ > this.onConstructed(config); > }; > > - > /** > * Create a new gadget site. > * @param {Element} gadgetEl HTML element into which to render. > @@ -636,3 +637,18 @@ > } > }); > }; > + > +/** > + * Adds the ability for features to extend the container with > + * their own functionality that may be specific to that feature. > + */ > +shindig.container.Container.prototype.mixins_ = {}; > + > +/** > + * Called from the constructor to add any namespace extensions. > + */ > +shindig.container.Container.prototype.mixin_ = function(){ > + for(var i in this.mixins_){ > + this[i] = new this.mixins_[i](this); > + } > +} > > > > > >
