A gadget lifecycle registry for the container

Allows container components to register themselves and react to preload/navigate/close/unload gadget events. 

Allows features that rely on feature parameters(e.g. opensearch, actions) to find out when gadgets are added, and react to their contributions.

[igor1x, 2011-06-03 11:27:36]

Mike, not sure if you saw this, so just wanted to make sure. This is the code
change to go along with this update to the container spec:
http://code.google.com/p/opensocial-resources/issues/detail?id=1185

[andrewedavis, 2011-06-07 01:57:23]

Paul, Mike,

Can we move this code review along?  we have Declarative actions and
opensearch patches all dependent on this before we can send them out for
review.

--Andrew

On Fri, Jun 3, 2011 at 7:27 AM, <igor1x@gmail.com> wrote:

> Reviewers: mhermanto1,
>
> Message:
> Mike, not sure if you saw this, so just wanted to make sure. This is the
> code change to go along with this update to the container spec:
> http://code.google.com/p/opensocial-resources/issues/detail?id=1185
>
> Description:
> Allows container components to register themselves and react to
> preload/navigate/close/unload gadget events.
>
> Allows features that rely on feature parameters(e.g. opensearch,
> actions) to find out when gadgets are added, and react to their
> contributions.
>
> Please review this at http://codereview.appspot.com/4536097/
>
> Affected files:
>  features/src/main/javascript/features/container/container.js
>
>
> Index: features/src/main/javascript/features/container/container.js
> ===================================================================
> --- features/src/main/javascript/features/container/container.js
>  (revision 1130245)
> +++ features/src/main/javascript/features/container/container.js
>  (working copy)
> @@ -29,8 +29,14 @@
>  */
>  osapi.container.Container = function(opt_config) {
>   var config = opt_config || {};
> -
> +
>   /**
> +   * A list of object to be notified when gadgets are preloaded, navigated
> to or closed.
> +   * @type {Array} array of callback objects, all of which have an
> "preloaded", "navigated", "closed" and "unloaded" methods.
> +   * @private
> +   */
> +  this.gadgetLifecycleCallbacks_ = [];
> +  /**
>    * A JSON list of preloaded gadget URLs.
>    * @type {Object}
>    * @private
> @@ -173,6 +179,7 @@
>   this.refreshService_();
>
>   var self = this;
> +  var selfSite=site;
>   // TODO: Lifecycle, add ability for current gadget to cancel nav.
>   site.navigateTo(gadgetUrl, viewParams, renderParams, function(gadgetInfo)
> {
>     // TODO: Navigate to error screen on primary gadget load failure
> @@ -184,6 +191,8 @@
>     } else if
> (gadgetInfo[osapi.container.MetadataResponse.NEEDS_TOKEN_REFRESH]) {
>       self.scheduleRefreshTokens_();
>     }
> +
> +    self.applyLifecycleCallbacks_("navigated", selfSite);
>     callback(gadgetInfo);
>   });
>  };
> @@ -198,10 +207,31 @@
>   site.close();
>   delete this.sites_[id];
>   this.unscheduleRefreshTokens_();
> +  this.applyLifecycleCallbacks_("closed", site);
>  };
>
>
>  /**
> + * Add a callback to be called when one or more gadgets are preloaded,
> navigated to or closed.
> + * @param {Object} callback object to call back when a gadget is
> preloaded, navigated to or closed. called via preloaded, navigated and
> closed methods
> + */
> +osapi.container.Container.prototype.addGadgetLifecycleCallback =
> function(lifeCycleCallback) {
> +  this.gadgetLifecycleCallbacks_.push(lifeCycleCallback);
> +};
> +
> +/**
> + * remove a lifecycle callback previously registered with the container
> + * @param {Object} callback object to be removed
> + */
> +osapi.container.Container.prototype.removeGadgetLifecycleCallback =
> function(lifeCycleCallback) {
> +  for (index in this.gadgetLifecycleCallbacks_) {
> +     if(this.gadgetLifecycleCallbacks_[index]==lifeCycleCallback) {
> +       this.gadgetLifecycleCallbacks_.splice(index,1);
> +     }
> +   }
> +};
> +
> +/**
>  * Pre-load one gadget metadata information. More details on
> preloadGadgets().
>  * @param {string} gadgetUrl gadget URI to preload.
>  * @param {function(Object)=} opt_callback function to call upon data
> receive.
> @@ -226,7 +256,9 @@
>   this.refreshService_();
>   this.service_.getGadgetMetadata(request, function(response) {
>     self.addPreloadGadgets_(response);
> +    self.applyLifecycleCallbacks_("preloaded", response);
>     callback(response);
> +
>   });
>  };
>
> @@ -248,6 +280,7 @@
>   for (var i = 0; i < gadgetUrls.length; i++) {
>     var url = gadgetUrls[i];
>     delete this.preloadedGadgetUrls_[url];
> +    this.applyLifecycleCallbacks_("unloaded", url);
>   }
>  };
>
> @@ -624,3 +657,19 @@
>     }
>   });
>  };
> +
> +
> +/**
> + * invokes methods on the gadget lifecycle callback registered with the
> container.
> + * @param {methodName} name of the callback method to be called.
> + * @param {data} data to be passed to the callback method
> + * @private
> + */
> +
>
+osapi.container.Container.prototype.applyLifecycleCallbacks_=function(methodName,
> data) {
> +  for (index in this.gadgetLifecycleCallbacks_) {
> +    if (this.gadgetLifecycleCallbacks_[index][methodName]!=null) {
> +      this.gadgetLifecycleCallbacks_[index][methodName](data);
> +    }
> +  }
> +};
>
>
>

[plindner1, 2011-06-07 09:58:58]

take a spin through this.  I'll do some basic cleanups on container.js to clean
up some of the chaff for when you run gjslint..

thanks!

http://codereview.appspot.com/4536097/diff/1/features/src/main/javascript/fea...
File features/src/main/javascript/features/container/container.js (right):

http://codereview.appspot.com/4536097/diff/1/features/src/main/javascript/fea...
features/src/main/javascript/features/container/container.js:35: * @type {Array}
array of callback objects, all of which have an "preloaded", "navigated",
"closed" and "unloaded" methods.
Array of what? 

I think you want functions that take optional objects.

http://codereview.appspot.com/4536097/diff/1/features/src/main/javascript/fea...
features/src/main/javascript/features/container/container.js:38:
this.gadgetLifecycleCallbacks_ = [];
style: newline between statement/comment

http://codereview.appspot.com/4536097/diff/1/features/src/main/javascript/fea...
features/src/main/javascript/features/container/container.js:182: var
selfSite=site;
style: spaces around =, != here and throughout

http://codereview.appspot.com/4536097/diff/1/features/src/main/javascript/fea...
features/src/main/javascript/features/container/container.js:664: * @param
{methodName} name of the callback method to be called.
methodName is not a type.  I think you want {string}

http://codereview.appspot.com/4536097/diff/1/features/src/main/javascript/fea...
features/src/main/javascript/features/container/container.js:665: * @param
{data} data to be passed to the callback method
data is not a type.  Maybe {Object}

is data optional?  If it is rename to opt_data and use {Object?}

See http://code.google.com/closure/compiler/docs/js-for-compiler.html

[igor1x, 2011-06-07 15:14:14]

Some clarifications.

http://codereview.appspot.com/4536097/diff/1/features/src/main/javascript/fea...
File features/src/main/javascript/features/container/container.js (right):

http://codereview.appspot.com/4536097/diff/1/features/src/main/javascript/fea...
features/src/main/javascript/features/container/container.js:35: * @type {Array}
array of callback objects, all of which have an "preloaded", "navigated",
"closed" and "unloaded" methods.
On 2011/06/07 09:58:58, plindner1 wrote:
> Array of what? 
> 
> I think you want functions that take optional objects.

Each element of the array would look like this:

var containerCallback = new Object();
  containerCallback.preloaded=function(data){};
  containerCallback.closed=function(data){};
  containerCallback.navigated=function(data){};
  containerCallback.unloaded=unloaded(data){};

http://codereview.appspot.com/4536097/diff/1/features/src/main/javascript/fea...
features/src/main/javascript/features/container/container.js:665: * @param
{data} data to be passed to the callback method
On 2011/06/07 09:58:58, plindner1 wrote:
> data is not a type.  Maybe {Object}
> 
> is data optional?  If it is rename to opt_data and use {Object?}
> 

> See http://code.google.com/closure/compiler/docs/js-for-compiler.html

Changing to {Object}--it is a required parameter.

[igor1x, 2011-06-07 15:37:26]

updated with Paul's feedback.

[mhermanto, 2011-06-07 17:29:36]

http://codereview.appspot.com/4536097/diff/1/features/src/main/javascript/fea...
File features/src/main/javascript/features/container/container.js (right):

http://codereview.appspot.com/4536097/diff/1/features/src/main/javascript/fea...
features/src/main/javascript/features/container/container.js:35: * @type {Array}
array of callback objects, all of which have an "preloaded", "navigated",
"closed" and "unloaded" methods.
Can we do, onPreloaded, onNavigated, onCLosed, onUnloaded? More semantically
in-line with event handling. Some callbacks require GadgetSite, Url,
MetadataResponse, etc. Please document.

http://codereview.appspot.com/4536097/diff/1/features/src/main/javascript/fea...
features/src/main/javascript/features/container/container.js:38:
this.gadgetLifecycleCallbacks_ = [];
Instead of an array, how about a map so 1) you can do direct removal, 2) name
your callback, ie: this.gadgetLifecycleCallbacks_ = {};, 3) avoid commonly-named
callbacks. Read comment below.

http://codereview.appspot.com/4536097/diff/1/features/src/main/javascript/fea...
features/src/main/javascript/features/container/container.js:195:
self.applyLifecycleCallbacks_("navigated", selfSite);
We should probably constant "onNavigated" to constant.js as
osapi.container.CallbackType.ON_NAVIGATED. So, this will be --
self.applyLifecycleCallbacks_(osapi.container.CallbackType.ON_NAVIGATED,
selfSite);

http://codereview.appspot.com/4536097/diff/1/features/src/main/javascript/fea...
features/src/main/javascript/features/container/container.js:210:
this.applyLifecycleCallbacks_("closed", site);
Ditto

http://codereview.appspot.com/4536097/diff/1/features/src/main/javascript/fea...
features/src/main/javascript/features/container/container.js:215: * Add a
callback to be called when one or more gadgets are preloaded, navigated to or
closed.
style nit: 80 chars/line. Please follow current style when in doubt. This
includes spacing.

http://codereview.appspot.com/4536097/diff/1/features/src/main/javascript/fea...
features/src/main/javascript/features/container/container.js:219:
this.gadgetLifecycleCallbacks_.push(lifeCycleCallback);
With map, this will be --
this.gadgetLifecycleCallbacks_[name] = lifeCycleCallback;
Perhaps, you want a check --
if (this.gadgetLifecycleCallbacks_[name]) { <cannot insert> }

http://codereview.appspot.com/4536097/diff/1/features/src/main/javascript/fea...
features/src/main/javascript/features/container/container.js:227: for (index in
this.gadgetLifecycleCallbacks_) {
With map, all these stmts can be --
delete this.gadgetLifecycleCallbacks_[name];

http://codereview.appspot.com/4536097/diff/1/features/src/main/javascript/fea...
features/src/main/javascript/features/container/container.js:669:
osapi.container.Container.prototype.applyLifecycleCallbacks_=function(methodName,
data) {
nit, spacing before and after =

http://codereview.appspot.com/4536097/diff/1/features/src/main/javascript/fea...
features/src/main/javascript/features/container/container.js:670: for (index in
this.gadgetLifecycleCallbacks_) {
With map, s/index/name/. And you can perhaps also have debugging statements,
like, "now running callback <name>". Not suggesting to have one here.

http://codereview.appspot.com/4536097/diff/1/features/src/main/javascript/fea...
features/src/main/javascript/features/container/container.js:671: if
(this.gadgetLifecycleCallbacks_[index][methodName]!=null) {
Sufficient to just say --
var method = this.gadgetLifecycleCallbacks_[index][methodName];
if (method) method(data);

[igor1x, 2011-06-07 20:20:23]

Updated with Mike's comments.

[mhermanto, 2011-06-08 05:20:48]

LGTM++

Mostly stylistic comments. Please correct, re-upload change, and I can help
commit

http://codereview.appspot.com/4536097/diff/11001/features/src/main/javascript...
File features/src/main/javascript/features/container/container.js (right):

http://codereview.appspot.com/4536097/diff/11001/features/src/main/javascript...
features/src/main/javascript/features/container/container.js:34: * A list of
objects containing functions to be invoked when gadgets are preloaded,
navigated, closed or unloaded. Sample object:
Major nit, comments should also respect max 80chars/line. Same throughout.

http://codereview.appspot.com/4536097/diff/11001/features/src/main/javascript...
features/src/main/javascript/features/container/container.js:36: *  
containerCallback[osapi.container.CallbackType.ON_PRELOADED]=function(metadataResponse){};
nit, spaces after/before =

http://codereview.appspot.com/4536097/diff/11001/features/src/main/javascript...
features/src/main/javascript/features/container/container.js:43:
this.gadgetLifecycleCallbacks_ = {};
I don't see the rest of the implementation. Codereview says "error: old chunk
mismatch". Re-try by Re-uploading?

http://codereview.appspot.com/4536097/diff/11001/features/src/main/javascript...
features/src/main/javascript/features/container/container.js:216: 
Safe/ok to call this callback AFTER site.close()'ed ?

http://codereview.appspot.com/4536097/diff/11001/features/src/main/javascript...
features/src/main/javascript/features/container/container.js:228: 
nit, spaces after/before =.

http://codereview.appspot.com/4536097/diff/11001/features/src/main/javascript...
features/src/main/javascript/features/container/container.js:675: 
nit, probably don't need this space.

[igor1x, 2011-06-08 13:47:07]

Added Mike's changes. Moved the closed callback up in the closeGadget code.

[igor1x, 2011-06-08 14:04:07]

Still having issues with chunk mismatch in code review, will try to figure
it out.
On Jun 8, 2011 9:47 AM, <igor1x@gmail.com> wrote:
> Added Mike's changes. Moved the closed callback up in the closeGadget
> code.
>
> http://codereview.appspot.com/4536097/

[plindner1, 2011-06-08 15:08:29]

try uploading to reviews.apache.org...


On Wed, Jun 8, 2011 at 7:04 AM, Igor Belakovskiy <igor1x@gmail.com> wrote:

> Still having issues with chunk mismatch in code review, will try to figure
> it out.
> On Jun 8, 2011 9:47 AM, <igor1x@gmail.com> wrote:
> > Added Mike's changes. Moved the closed callback up in the closeGadget
> > code.
> >
> > http://codereview.appspot.com/4536097/
>

[bull_alum.mit.edu, 2011-06-08 15:53:00]

Fixed it.

On Wed, Jun 8, 2011 at 11:08 AM, Paul Lindner <plindner@google.com> wrote:

> try uploading to reviews.apache.org...
>
>
> On Wed, Jun 8, 2011 at 7:04 AM, Igor Belakovskiy <igor1x@gmail.com> wrote:
>
>> Still having issues with chunk mismatch in code review, will try to figure
>> it out.
>> On Jun 8, 2011 9:47 AM, <igor1x@gmail.com> wrote:
>> > Added Mike's changes. Moved the closed callback up in the closeGadget
>> > code.
>> >
>> > http://codereview.appspot.com/4536097/
>>
>
>

[mhermanto, 2011-06-09 00:11:57]

Committed as r1133607.

On 2011/06/08 15:53:00, bull_alum.mit.edu wrote:
> Fixed it.
> 
> On Wed, Jun 8, 2011 at 11:08 AM, Paul Lindner <mailto:plindner@google.com>
wrote:
> 
> > try uploading to http://reviews.apache.org...
> >
> >
> > On Wed, Jun 8, 2011 at 7:04 AM, Igor Belakovskiy <mailto:igor1x@gmail.com>
wrote:
> >
> >> Still having issues with chunk mismatch in code review, will try to figure
> >> it out.
> >> On Jun 8, 2011 9:47 AM, <mailto:igor1x@gmail.com> wrote:
> >> > Added Mike's changes. Moved the closed callback up in the closeGadget
> >> > code.
> >> >
> >> > http://codereview.appspot.com/4536097/
> >>
> >
> >