> On 2012-03-26 18:54:24, Dan Dumont wrote: > > trunk/features/src/test/javascript/features/container.url/url_site_test.js, > > line 83 > > <https://reviews.apache.org/r/4486/diff/2/?file=95901#file95901line83> > > > > Hmm, can we replace this test with another one? Why did this fail? > > Henry Saputra wrote: > It fails because now the url_site.Close() no longer responsible to remove > the iframe child element. The task is delegated to the site_holder.dispose(). > > I dont think we have test for both site_holder and site once we do we > could add this test to the base class test cases.
ok > 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. 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. - Dan ----------------------------------------------------------- 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 > >
