Thanks much for the reply Dan.
Ah no problem so far. We are not using the buffer feature but Just re
visiting the flow in planning to do so. Also The cc spec is going to be out
of incubation for 2.5 I believe, so I am trying to fix and cleanup the code
as much as possible for Shindig final next release.
I think calling Site.getActiveSiteHolder should return the currently active
holder which should be the currentGadgetHolder_ because its a public
function that can be called via gadget site.
My JS foo is not super but checking the loadingGadgetHolder_ if exist in
the getActiveSiteHolder function is probably done to cover scenario where
the GadgetSite used the first time and somehow the JavaScript interrupted
before finishing rendering the gadget (which I am not sure how this could
happen).
As for cleaning up, we could just do this in site.js:
/**
* Closes this site.
*/
osapi.container.Site.prototype.close = function() {
this.disposeSiteHolders();
};
/**
* Dispose the internal site holders.
* @abstract
*/
osapi.container.GadgetSite.prototype.disposeSiteHolders = function() {
throw new Error("This method must be implemented by a subclass.");
}
and in gadget_site.js:
/**
* @inheritDoc
*/
osapi.container.GadgetSite.prototype.close = function() {
osapi.container.Site.prototype.close.call(this); // super.close();
};
/**
* @inheritDoc
*/
osapi.container.GadgetSite.prototype.disposeSiteHolders = function() {
if (this.loadingGadgetHolder_) {
this.loadingGadgetHolder_.dispose();
}
if (this.currentGadgetHolder_) {
this.currentGadgetHolder_.dispose();
}
}
and move deletion of iframe to GadgetHolder.js:
/**
* @inheritDoc
*/
osapi.container.GadgetHolder.prototype.dispose = function() {
this.gadgetInfo_ = null;
if (this.el_ && this.el_.firstChild) {
this.el_.removeChild(this.el_.firstChild);
}
};
The reason is that GadgetSite is the actual class that holds both gadget
holders so it makes sense that GadgetSite class to do the cleaning for both
holders and removing of child element of the holder element has to happen
in GagdetHolder since its the class that actually uses the iframe HTML
parent element.
Sorry about questions on this but do really appreciate the inputs and
comments
- Henry
On Friday, March 23, 2012, Dan Dumont <ddum...@us.ibm.com> wrote:
> AFAIK, the reason getActiveSiteHolder prefers the loading holder is so
> that operations will get that while a gadget is still loading.
> After render it goes away and the result should always be the
> currentGadgetHolder_
>
> The dispose check to get it again is just for safety so that if that edge
> case happens in a gadget we know we've cleaned up both holders in the
> site.
> Are you running into an issue in this code? I curious what's prompting
> the questions :)
>
>
> I think we might be able to move this bit:
> if (this.loadingGadgetEl_ && this.loadingGadgetEl_.firstChild) {
> this.loadingGadgetEl_.removeChild(this.loadingGadgetEl_.firstChild);
> }
>
> inside the holder's dispose method to clean things up a bit. I would need
> to see what that does to the other non-loading holder.
>
>
>
> From: Henry Saputra <henry.sapu...@gmail.com>
> To: dev@shindig.apache.org,
> Date: 03/23/2012 03:12 AM
> Subject: Re: Question about gagdet_site.js GadgetSite.close impl
> for close() function
>
>
>
> I see.
>
> So the getActiveSiteHolder code currently looks like this:
>
> /**
> * @inheritDoc
> */
> osapi.container.GadgetSite.prototype.getActiveSiteHolder = function() {
> return this.loadingGadgetHolder_ || this.currentGadgetHolder_;
> };
>
> This code is weird because it should be the other way:
>
> return this.currentGadgetHolder_ || this.loadingGadgetHolder_;
>
> because after GadgetSite.onRender() is called and replacing
> currentGadgetHolder_ with loadingGadgetHolder_ which makes it active.
>
> So the close() function in gadget_site.js file could look like:
>
> /**
> * @inheritDoc
> */
> osapi.container.GadgetSite.prototype.close = function() {
> osapi.container.Site.prototype.close.call(this); // super.close();
> if (this.loadingGadgetEl_ && this.loadingGadgetEl_.firstChild) {
> this.loadingGadgetEl_.removeChild(this.loadingGadgetEl_.firstChild);
> }
>
> // dispose of our other holder if any
> if(loadingGadgetHolder_) {
> loadingGadgetHolder_.dispose();
> }
> };
>
> The odds of GadgetSite.onRender() has not finished executing when
> someone call GadgetSite.close() I think is pretty rare. So in most
> typical scenarios there will be only one instance of gadget holder
> active.
>
>
> Thoughts?
>
> - Henry
>
>
> On Thu, Mar 22, 2012 at 8:52 PM, Dan Dumont <ddum...@us.ibm.com> wrote:
>> Yes it's the same as an if check. The reason the code is duplicated is
> because gadget sites have 2 holders.
>> The second time it's called it should return the other one.
>>
>> -----Henry Saputra <henry.sapu...@gmail.com> wrote: -----
>>
>> =================
>> To: dev@shindig.apache.org@IBMUS@LOTUS
>> From: Henry Saputra <henry.sapu...@gmail.com>
>> Date: 03/22/2012 06:52PM
>> Cc: IBMUS@LOTUS
>> Subject: Question about gagdet_site.js GadgetSite.close impl for
> close() function
>> =======================
>> In the gadget_site.js file there is this function:
>>
>> /**
>> * @inheritDoc
>> */
>> osapi.container.GadgetSite.prototype.close = function() {
>> osapi.container.Site.prototype.close.call(this); // super.close();
>> if (this.loadingGadgetEl_ && this.loadingGadgetEl_.firstChild) {
>> this.loadingGadgetEl_.removeChild(this.loadingGadgetEl_.firstChild);
>> }
>>
>> // dispose of our other holder.
>> var holder = this.getActiveSiteHolder();
>> holder && holder.dispose();
>> };
>>
>>
>> The parent site.js has this:
>>
>>
>> /**
>> * Closes this site.
>> */
>> osapi.container.Site.prototype.close = function() {
>> if (this.el_ && this.el_.firstChild) {
>> this.el_.removeChild(this.el_.firstChild);
>> }
>>
>> var holder = this.getActiveSiteHolder();
>> holder && holder.dispose();
>> };
>>
>>
>> If am not sure what " holder && holder.dispose();" should do. I think
>> its trying to do:
>>
>> if(holder) {
>> holder.dispose();
>> }
>>
>> And the GadgetSite should not repeat the code I suppose.
>>
>> - Henry
>>
>>
>
>
>
