> 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?
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. > 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. 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. - Henry ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/4486/#review6352 ----------------------------------------------------------- On 2012-03-26 18:44:47, Henry Saputra wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/4486/ > ----------------------------------------------------------- > > (Updated 2012-03-26 18:44:47) > > > 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/site_holder.js > 1305481 > trunk/features/src/test/javascript/features/container.url/url_site_test.js > 1305481 > trunk/features/src/main/javascript/features/container.site/site.js 1305481 > > trunk/features/src/main/javascript/features/container.site.url/url_holder.js > 1305481 > > 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 > > 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 > >
