> On 2012-03-26 18:54:24, Dan Dumont wrote: > > trunk/features/src/main/javascript/features/container.site.gadget/gadget_site.js, > > line 140 > > <https://reviews.apache.org/r/4486/diff/2/?file=95897#file95897line140> > > > > I didn't get the reasoning behind changing this. > > > > One scenario I can think of is a period of time where the loading > > holder is showing the content of the site because the current holder (which > > should probably be called the 'main' holder or something) is still loading > > (nothing yet to render) > > > > The completion of the load, iirc, removed the loading holder, and then > > all calls would route correctly to the current holder. > > > > With this new logic you may get the current holder before it is ready. > > Henry Saputra wrote: > Thanks for the review Dan. > > With the way single threaded JS work I dont think that scenario ever > happen. The rendering flow will not yield until it injects the iframe DOM to > the parent HTML element is dont synchronously. > Until the load is complete, which will replace the current holder with > loading one and remove the prev current holder, calls to getActiveHolder > should return the "active" one which is the whatever current holder is. > > Unless I misunderstood the meaning of active, the loading holder is not > active until it becomes the current holder. Please correct me if I am wrong > here, in other env where it supports yield to user action, there could be > scenario where the render flow is interrupted such that the gadget site is in > the state where the GadgetSite.getActiveHolder is called when it has both > loadingGadgetHolder_ and currentGadgetHolder_ but I dont see how it could be > happen in browser scenario. > > Dan Dumont wrote: > Darn, forgot to hit publish on this. Sorry for the delay. > > Hmm. Are you sure execution control is not given up at all in between > dom injection and iframe onload event? (it might be for most browsers, but > iirc, opera gives an execution thread to each iframe on a page). > > In any case, for UX purposes there should probably be another event (done > in the gadget to signal everything is ready) so that containers can choose to > gracefully handle horribly broken gadgets with the loading holder. > > If you disagree that it would be a desirable thing to do down the road, > then I guess I'm fine with the reordering. Does leaving the order the way it > was cause any problems, or were you just trying to clarify the return? > > It might be best some time in the future to spend some time setting up a > loading flag that can be used to return the appropriate holders, as well as > introducing some new lifecycle events to key off of when switching the > holders out.
For the sake of the cleanup of the close() flow of site classes, I will revert the ordering change for now because I havent encountered problem with the original ordering. I will create separate issue to clarify the intent of the GadgetSite .getActiveHolder function. - Henry ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/4486/#review6352 ----------------------------------------------------------- On 2012-03-26 23:21:33, Henry Saputra wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/4486/ > ----------------------------------------------------------- > > (Updated 2012-03-26 23:21:33) > > > Review request for shindig and Dan Dumont. > > > Summary > ------- > > As part of continuing refactoring of the site and holder code, this is > another cleanup to simplify the close flow: > 1. Move the responsibility of delete first element of the gadget HTML element > wrapper to site_holder > 2. The GadgetSite.getActiveSiteHolder now check for currentGadgetHolder_ then > loadingGadgetHolder_ because its a public API that should modify currently > active holder. > 3. osapi.container.SiteHolder.prototype.dispose now responsible to remove > child element. > 4. osapi.container.GadgetHolder.prototype.dispose and > osapi.container.UrlHolder.prototype.dispose need to call parent function > > > Diffs > ----- > > > trunk/features/src/main/javascript/features/container.site.gadget/gadget_holder.js > 1305481 > > trunk/features/src/main/javascript/features/container.site.gadget/gadget_site.js > 1305481 > > trunk/features/src/main/javascript/features/container.site.url/url_holder.js > 1305481 > trunk/features/src/main/javascript/features/container.site/site.js 1305481 > trunk/features/src/main/javascript/features/container.site/site_holder.js > 1305481 > trunk/features/src/test/javascript/features/container.url/url_site_test.js > 1305481 > > Diff: https://reviews.apache.org/r/4486/diff > > > Testing > ------- > > Modify the jsunit and pass the unit tests. Run it through common container > test page. > > > Thanks, > > Henry > >
