Yeah we can remove it, it just help me doing less typing =)
I will set a review for this.
Thanks a lot Dan.
- Henry
On Fri, Mar 23, 2012 at 11:29 AM, Dan Dumont <ddum...@us.ibm.com> wrote:
> No problem, and it doesn't sound like a bad idea. 1 comment.
>
> /**
> * @inheritDoc
> */
> osapi.container.GadgetSite.prototype.close = function() {
> osapi.container.Site.prototype.close.call(this); // super.close();
> };
>
> You don't need to define this at all if you're only calling super.
>
> Were you going to set up a review?
>
>
>
>
> From: Henry Saputra <henry.sapu...@gmail.com>
> To: "dev@shindig.apache.org" <dev@shindig.apache.org>,
> Date: 03/23/2012 02:23 PM
> Subject: Question about gagdet_site.js GadgetSite.close impl for
> close() function
>
>
>
> 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
>>>
>>>
>>
>>
>>
>
