Repeat is slow slow slow right now. One obvious thing that we can optimize is the refreshing part, we do re-recreate the content on each refresh, that is unnecessary. We just need to adjust the number of children to the size of the nodeset, and then update the context nodes for each child. Let's XBLize repeat first though (bug 306247).

Here's the idea. Instead of re-creating the unrolled content all the time, the content is just adjusted to the new "row count" on refresh, and the context is changed. The same "refresh children" concept is used for updating the content, by always making repeats refresh their children, and by letting the contextContainers figure out whether they change context or not. A simple textcase with 200 controls in a repeat, and then adding a new goes from around 5 secs to 0.8 sec. Trunk, debug, etc., but should still be noticable in optimized build. I still have a few unknowns around, including at least one problem for nested repeats (of course).

There's a lot of small parts to this, but the two main parts are: 1) Change nsIXFormsControl::Bind() to return a boolean that tells whether the model whether context has changed, and it needs to rebind children. This allows a) the repeat to signal that children should always be rebound, and b) the contextcontainers to signal whether their context have actually changed. 2) No more "first delete all and then recreate repeat rows". I keep a mCurrentRowCount, that contains the current number of unrolled repeat rows, and then just add or delete rows as necessary on refresh (UnrollRows()). When row count is up to date and template content is generated (InsertTemplateContent(), all rows have their context set/reset (context size, context position, and bound node). Because of 1), content will only be refreshed if something actually changes. Small bits (some of these can be moved to seperate bugs if you would rather like that): * Moved model-construct-done to after ProcessDeferredBinds() -- because that where it should be :) * do not set model attribute on contextcontainer. It will use the generic GetContext() to figure it out through mModel (which is set through BindToModel()). * itemset contextcontainer construction change, so context is right when children are inserted. * I've moved the needed rebinding in nsXFormsModelElement::Rebuild() to the refresh part of things, no need to have the same logic twice. This is controlled by mRebindAllControls (renamed from mNeedsRefresh). * Changed model and repeat booleans to packed booleans

This should be fixed by bug 329849

(In reply to comment #3) > This should be fixed by bug 329849 heh. I got lost in my tabs, and added it the wrong way around :). Naturally, _this_ should fix bug 329849.

>Moved model-construct-done to after ProcessDeferredBinds() -- because that > where it should be according to 4.2.2, the binding of all form controls happens as the default action of xforms-model-construct-done. So xforms-model-construct-done needs to fire before the bindings take place.

(In reply to comment #5) > >Moved model-construct-done to after ProcessDeferredBinds() -- because that > > where it should be > > according to 4.2.2, the binding of all form controls happens as the default > action of xforms-model-construct-done. So xforms-model-construct-done needs to > fire before the bindings take place. Heh. You are right. I was a bit quick on the trigger there. What I tried to obtain was that the processdeferredbinds is run before intializecontrols (which actually also should run as part of the construct-done processing imho, but that's a nit).

Allan, will you post a new patch, or should I review this one? You mentioned that you'd noticed some bug in it, related to focus.

(In reply to comment #7) > Allan, will you post a new patch, or should I review this one? > You mentioned that you'd noticed some bug in it, related to focus. No, that was unreleated. That is bug 336596.

Comment on attachment 220004 [details] [diff] [review] Patch >- NS_ENSURE_SUCCESS(rv, rv); >+ // XXX: Refresh still fails for some controls, for some reason. >+ // NS_ENSURE_SUCCESS(rv, rv); Open a bug for this and add a reference to that bug to here, fix comment #5 and could you attach a testcase, which shows the better performance.

(In reply to comment #9) > (From update of attachment 220004 [details] [diff] [review] [edit]) > > >- NS_ENSURE_SUCCESS(rv, rv); > >+ // XXX: Refresh still fails for some controls, for some reason. > >+ // NS_ENSURE_SUCCESS(rv, rv); > > Open a bug for this and add a reference to that bug to here, Opened bug 336608. > fix comment #5 Hmm, I believe that the ProcessDeferredBinds call is in the correct place in the patch. (It is calling control->Bind(), which is slightly wrong, it should be control->BindToModel().) It only needs to be called by one model, and it needs to be called before initializing controls in general. > and could you attach a testcase, which shows the better performance. Coming up.

As stated in comment 10, I think ProcessDeferredBinds is where it should be, although it is doing slightly too much work.

Comment on attachment 220892 [details] [diff] [review] Updated w/smaug's comments >diff -X patch-excludes -uprN -U8 xforms/nsXFormsContextContainer.cpp xforms.repopt/nsXFormsContextContainer.cpp >--- xforms/nsXFormsContextContainer.cpp 2006-05-04 17:51:26.000000000 +0200 >+++ xforms.repopt/nsXFormsContextContainer.cpp 2006-04-25 16:12:35.000000000 +0200 >@@ -249,22 +260,21 @@ nsXFormsContextContainer::GetContext(nsA > *aContextSize = mContextSize; > > return NS_OK; > } > > // nsIXFormsControl > > NS_IMETHODIMP >-nsXFormsContextContainer::Bind() >+nsXFormsContextContainer::Bind(PRBool *aContextChanged) > { >- >- nsresult rv = BindToModel(); >- NS_ENSURE_SUCCESS(rv, rv); >- >+ NS_ENSURE_ARG(aContextChanged); >+ *aContextChanged = mContextIsDirty; >+ mContextIsDirty = PR_FALSE; > return NS_OK; > } > nit: To be consistent, I think that we tend to use NS_ENSURE_ARG_POINTER for return buffers. Couple of other places in this patch you use NS_ENSURE_ARG, too. >diff -X patch-excludes -uprN -U8 xforms/nsXFormsModelElement.cpp xforms.repopt/nsXFormsModelElement.cpp >--- xforms/nsXFormsModelElement.cpp 2006-04-27 18:11:20.000000000 +0200 >+++ xforms.repopt/nsXFormsModelElement.cpp 2006-05-04 16:45:38.000000000 +0200 >@@ -1999,23 +1997,16 @@ nsXFormsModelElement::MaybeNotifyComplet > // DOMContentLoaded. > for (i = 0; i < models->Count(); ++i) { > nsXFormsModelElement *model = > NS_STATIC_CAST(nsXFormsModelElement *, models->ElementAt(i)); > if (!model->mDocumentLoaded || !model->IsComplete()) > return; > } > >- // Okay, dispatch xforms-model-construct-done and xforms-ready events! >- for (i = 0; i < models->Count(); ++i) { >- nsXFormsModelElement *model = >- NS_STATIC_CAST(nsXFormsModelElement *, models->ElementAt(i)); >- nsXFormsUtils::DispatchEvent(model->mElement, eEvent_ModelConstructDone); >- } >- > // validate the instance documents becauar we want schemaValidation to add > // schema type properties from the schema file unto our instance document > // elements. We don't care about the validation results. > if (mInstanceDocuments) { > PRUint32 instCount; > mInstanceDocuments->GetLength(&instCount); > if (instCount) { > nsCOMPtr<nsIDOMDocument> document; >@@ -2039,29 +2030,37 @@ nsXFormsModelElement::MaybeNotifyComplet > } > } > } > } > } > > nsXFormsModelElement::ProcessDeferredBinds(domDoc); > >+ // Okay, dispatch xforms-model-construct-done >+ for (i = 0; i < models->Count(); ++i) { >+ nsXFormsModelElement *model = >+ NS_STATIC_CAST(nsXFormsModelElement *, models->ElementAt(i)); >+ nsXFormsUtils::DispatchEvent(model->mElement, eEvent_ModelConstructDone); >+ } >+ I see in a couple of comments that you say that ProcessDeferredBinds is in the correct place in the patch, but how can that be? If all control binds happen in the default handling of xforms-model-construct-done (as noted in comment #5), how can these binds happen before the event is even dispatched? >diff -X patch-excludes -uprN -U8 xforms/nsXFormsRepeatElement.cpp xforms.repopt/nsXFormsRepeatElement.cpp >--- xforms/nsXFormsRepeatElement.cpp 2006-05-04 14:14:21.000000000 +0200 >+++ xforms.repopt/nsXFormsRepeatElement.cpp 2006-05-04 16:39:30.000000000 +0200 >+nsresult >+nsXFormsRepeatElement::UnrollRows(nsIDOMXPathResult *aNodeset) >+ // STEP 4: Insert template content into newly created rows >+ nsCOMPtr<nsIDOMNodeList> containerList; >+ rv = anon->GetChildNodes(getter_AddRefs(containerList)); >+ NS_ENSURE_SUCCESS(rv, rv); >+ for (PRUint32 i = mCurrentRowCount; i < mMaxIndex; ++i) { >+ nsCOMPtr<nsIDOMNode> container; >+ rv = containerList->Item(i, getter_AddRefs(container)); >+ NS_ENSURE_SUCCESS(rv, rv); >+ >+ rv = InsertTemplateContent(container); >+ NS_ENSURE_SUCCESS(rv, rv); > } >+ >+ mCurrentRowCount = mMaxIndex; >+ return NS_OK; >+} With this new approach couldn't you end up with a problem if the form author has changed the repeat's DOM in between repeat refreshing? So rows 1 -> mCurrentRowCount-1 could have a template that is different than mCurrentRowCount -> mMaxIndex? I'm just thinking that is a problem that we could have after this patch goes in that we don't have now. rest of the patch looks good. But I'd like to know why you think xforms-model-construct-done behavior is per spec in the patch and if you think the different template issue is worth worrying about before giving an r+.

(In reply to comment #13) > (From update of attachment 220892 [details] [diff] [review] [edit]) > I see in a couple of comments that you say that ProcessDeferredBinds is in the > correct place in the patch, but how can that be? If all control binds happen > in the default handling of xforms-model-construct-done (as noted in comment > #5), how can these binds happen before the event is even dispatched? I tried elaborating in comment 10. I see ProcessDeferredBinds as "register controls with model", not an actual binding. The controls on the list are controls that has had their binding to the _model_ deferred. Not so much the actually setting up of bound node, etc. So ProcessDeferredBinds should actually just do a BindToModel() instead of "full Bind()". But I thought that was nitpicking it too much. Especially since it is not possible to cancel model-construct-done. > With this new approach couldn't you end up with a problem if the form author > has changed the repeat's DOM in between repeat refreshing? So rows 1 -> > mCurrentRowCount-1 could have a template that is different than > mCurrentRowCount -> mMaxIndex? I'm just thinking that is a problem that we > could have after this patch goes in that we don't have now. You are right, that is new (wrong) behaviour :( Problem is how I should find out? Either we need to listen for DOM Mutation events which afaik is a terribly bad idea in Gecko 1.8.x, and we have the same problem as for bug 273706. A better way could be to somehow figure out when a rebuild is happening. So if a user adds something to the DOM it is possible to do a rebuild to get things working properly again. That said, current behaviour is not exactly neat either because of bug 273706.

(In reply to comment #14) > (In reply to comment #13) > > (From update of attachment 220892 [details] [diff] [review] [edit] [edit]) > > I see in a couple of comments that you say that ProcessDeferredBinds is in the > > correct place in the patch, but how can that be? If all control binds happen > > in the default handling of xforms-model-construct-done (as noted in comment > > #5), how can these binds happen before the event is even dispatched? > > I tried elaborating in comment 10. I see ProcessDeferredBinds as "register > controls with model", not an actual binding. The controls on the list are > controls that has had their binding to the _model_ deferred. Not so much the > actually setting up of bound node, etc. > > So ProcessDeferredBinds should actually just do a BindToModel() instead of > "full Bind()". But I thought that was nitpicking it too much. Especially since > it is not possible to cancel model-construct-done. > True, you can't cancel it. But if the user wanted to do something just prior to the binding, they could in any other processor (assuming they behave properly, of course) by listening for the xforms-model-construct-done event and now that wouldn't work in our processor. But maybe I'm nitpicking too much. Because you can also make the same argument that it could be equally valuable to know that when model-construct-done is fired that all the nodes are bound. > > With this new approach couldn't you end up with a problem if the form author > > has changed the repeat's DOM in between repeat refreshing? So rows 1 -> > > mCurrentRowCount-1 could have a template that is different than > > mCurrentRowCount -> mMaxIndex? I'm just thinking that is a problem that we > > could have after this patch goes in that we don't have now. > > You are right, that is new (wrong) behaviour :( Problem is how I should find > out? Either we need to listen for DOM Mutation events which afaik is a terribly > bad idea in Gecko 1.8.x, and we have the same problem as for bug 273706. > > A better way could be to somehow figure out when a rebuild is happening. So if > a user adds something to the DOM it is possible to do a rebuild to get things > working properly again. > > That said, current behaviour is not exactly neat either because of bug 273706. > Well, what if we just take the attitude that we will NOT honor DOM changes to the repeat template? If we are willing to take that stand, then you just need to clone the nodes from the first context container into all subsequent context containers. Or probably better (to cover the case where someone deletes all of nodes in the nodeset...and thus all of the context containers...and then does an insert) we could cache the initial template in a display:none div and clone that when we need a context container.

(In reply to comment #15) > (In reply to comment #14) > > (In reply to comment #13) > > > (From update of attachment 220892 [details] [diff] [review] [edit] [edit] [edit]) > > > I see in a couple of comments that you say that ProcessDeferredBinds is in the > > > correct place in the patch, but how can that be? If all control binds happen > > > in the default handling of xforms-model-construct-done (as noted in comment > > > #5), how can these binds happen before the event is even dispatched? > > > > I tried elaborating in comment 10. I see ProcessDeferredBinds as "register > > controls with model", not an actual binding. The controls on the list are > > controls that has had their binding to the _model_ deferred. Not so much the > > actually setting up of bound node, etc. > > > > So ProcessDeferredBinds should actually just do a BindToModel() instead of > > "full Bind()". But I thought that was nitpicking it too much. Especially since > > it is not possible to cancel model-construct-done. > > > > True, you can't cancel it. But if the user wanted to do something just prior > to the binding, they could in any other processor (assuming they behave > properly, of course) by listening for the xforms-model-construct-done event and > now that wouldn't work in our processor. But maybe I'm nitpicking too much. > Because you can also make the same argument that it could be equally valuable > to know that when model-construct-done is fired that all the nodes are bound. No, I believe it is important, and I think it is good that you keep me adhering to the spec. (albeit it is nitpicking, but I excel in that too :) ). So we should expose BindToModel then, and do that on ProcessDeferredBind. Would that be ok? > > > With this new approach couldn't you end up with a problem if the form author > > > has changed the repeat's DOM in between repeat refreshing? So rows 1 -> > > > mCurrentRowCount-1 could have a template that is different than > > > mCurrentRowCount -> mMaxIndex? I'm just thinking that is a problem that we > > > could have after this patch goes in that we don't have now. > > > > You are right, that is new (wrong) behaviour :( Problem is how I should find > > out? Either we need to listen for DOM Mutation events which afaik is a terribly > > bad idea in Gecko 1.8.x, and we have the same problem as for bug 273706. > > > > A better way could be to somehow figure out when a rebuild is happening. So if > > a user adds something to the DOM it is possible to do a rebuild to get things > > working properly again. > > > > That said, current behaviour is not exactly neat either because of bug 273706. > > > > Well, what if we just take the attitude that we will NOT honor DOM changes to > the repeat template? Priority-wise dynamic DOM changes are low on my list, especially since it's not handled in the spec. so the author is on his own. That said, I would like to support as much as we can. But I would rather take that in a followup bug, as this patch already messes with enough. > If we are willing to take that stand, then you just need > to clone the nodes from the first context container into all subsequent context > containers. Or probably better (to cover the case where someone deletes all of > nodes in the nodeset...and thus all of the context containers...and then does > an insert) we could cache the initial template in a display:none div and clone > that when we need a context container. Well, that is what both current code and the patch does :)

> No, I believe it is important, and I think it is good that you keep me adhering > to the spec. (albeit it is nitpicking, but I excel in that too :) ). So we > should expose BindToModel then, and do that on ProcessDeferredBind. Would that > be ok? So you want to call BindToModel from ProcessDeferredBind instead of calling Bind()? Then when would Bind() be called during initialization? Or would you call both? > > If we are willing to take that stand, then you just need > > to clone the nodes from the first context container into all subsequent context > > containers. Or probably better (to cover the case where someone deletes all of > > nodes in the nodeset...and thus all of the context containers...and then does > > an insert) we could cache the initial template in a display:none div and clone > > that when we need a context container. > > Well, that is what both current code and the patch does :) > Doh! You are of course right. I guess I hadn't looked at repeat since it got XBL'ized.

(In reply to comment #17) > > No, I believe it is important, and I think it is good that you keep me adhering > > to the spec. (albeit it is nitpicking, but I excel in that too :) ). So we > > should expose BindToModel then, and do that on ProcessDeferredBind. Would that > > be ok? > > So you want to call BindToModel from ProcessDeferredBind instead of calling > Bind()? Then when would Bind() be called during initialization? Or would you > call both? Only BTM, Bind is called from InitializeControls(). My idea was that we would then only have one place where we initialize controls (ie. InitializeControls), and the ProcessDeferredBinds() just takes care of the "after-registering of postponed controls to the model".

(In reply to comment #18) > (In reply to comment #17) > > > No, I believe it is important, and I think it is good that you keep me adhering > > > to the spec. (albeit it is nitpicking, but I excel in that too :) ). So we > > > should expose BindToModel then, and do that on ProcessDeferredBind. Would that > > > be ok? > > > > So you want to call BindToModel from ProcessDeferredBind instead of calling > > Bind()? Then when would Bind() be called during initialization? Or would you > > call both? > > Only BTM, Bind is called from InitializeControls(). > > My idea was that we would then only have one place where we initialize controls > (ie. InitializeControls), and the ProcessDeferredBinds() just takes care of the > "after-registering of postponed controls to the model". > So?

(In reply to comment #19) > (In reply to comment #18) > > (In reply to comment #17) > > > > No, I believe it is important, and I think it is good that you keep me adhering > > > > to the spec. (albeit it is nitpicking, but I excel in that too :) ). So we > > > > should expose BindToModel then, and do that on ProcessDeferredBind. Would that > > > > be ok? > > > > > > So you want to call BindToModel from ProcessDeferredBind instead of calling > > > Bind()? Then when would Bind() be called during initialization? Or would you > > > call both? > > > > Only BTM, Bind is called from InitializeControls(). > > > > My idea was that we would then only have one place where we initialize controls > > (ie. InitializeControls), and the ProcessDeferredBinds() just takes care of the > > "after-registering of postponed controls to the model". > > > > So? > Yep, I know. I've been too tempted by my small reviews lately :-) I'll get to it today.

(In reply to comment #19) > (In reply to comment #18) > > (In reply to comment #17) > > > > No, I believe it is important, and I think it is good that you keep me adhering > > > > to the spec. (albeit it is nitpicking, but I excel in that too :) ). So we > > > > should expose BindToModel then, and do that on ProcessDeferredBind. Would that > > > > be ok? > > > > > > So you want to call BindToModel from ProcessDeferredBind instead of calling > > > Bind()? Then when would Bind() be called during initialization? Or would you > > > call both? > > > > Only BTM, Bind is called from InitializeControls(). > > > > My idea was that we would then only have one place where we initialize controls > > (ie. InitializeControls), and the ProcessDeferredBinds() just takes care of the > > "after-registering of postponed controls to the model". > > > > So? > Sorry that it took me so long to respond. I think that I now fully understand where you are headed and the distinction that you are making between when the control is bound to the model and when it is bound to the instance node. Sounds fine to me. Maybe add a comment just prior to the call to ProcessDeferredBind so that we keep that distinction clear in the code. Also I was re-reading through the patch to refresh my memory and I noticed that in nsXFormsItemSet you are appending the item to the anonymous content before you append the context container to the item. I'd think that you'd want to make sure that the contextcontainer was constructed successfully and insert that into the item before inserting the item into the anonymous content. Otherwise we might end up with an empty item under the anonymous content. Not really that big of a deal though because if something goes wrong either way we are still in a world of hurt :-)

Comment on attachment 220892 [details] [diff] [review] Updated w/smaug's comments removing my review request, will review new patch with BindToModel in ProcessDeferredBind when it is ready.

(In reply to comment #21) > Sorry that it took me so long to respond. I think that I now fully understand > where you are headed and the distinction that you are making between when the > control is bound to the model and when it is bound to the instance node. > Sounds fine to me. Np, I was not exactly being clear about what my aim/plan was either. So my bad. > Maybe add a comment just prior to the call to ProcessDeferredBind so that we > keep that distinction clear in the code. I will. > Also I was re-reading through the patch to refresh my memory and I noticed that > in nsXFormsItemSet you are appending the item to the anonymous content before > you append the context container to the item. I'd think that you'd want to > make sure that the contextcontainer was constructed successfully and insert > that into the item before inserting the item into the anonymous content. > Otherwise we might end up with an empty item under the anonymous content. Not > really that big of a deal though because if something goes wrong either way we > are still in a world of hurt :-) Hmm, I did that change for reason.... If I remember correctly, it was to assure that the contextcontainer would be able to find the parent in the model's form controls when it was inserted into it. It also seems more logical I think. But maybe it was only needed before the "final version" of the patch.... I'd rather leave it be. As you say, if it goes wrong an empty item is not our biggest problem :)

(In reply to comment #23) > (In reply to comment #21) > > Maybe add a comment just prior to the call to ProcessDeferredBind so that we > > keep that distinction clear in the code. > > I will. Oops, that I forgot in the previous patch. Here's a patch with both BindToModel() and improved comments.

Comment on attachment 222586 [details] [diff] [review] Patch with new comments thanks for the added comments! r=me

Fixed on trunk.

Comment on attachment 222586 [details] [diff] [review] Patch with new comments >+ for (PRUint32 i = 0; i < mMaxIndex; ++i) { >... >+ for (PRUint32 i = mCurrentRowCount; i < mMaxIndex; ++i) { VC6 doesn't like this :-\

Comment on attachment 223002 [details] [diff] [review] vc6 fix The first two occurrances are actually inside the if/else block scope so that VC6 correctly compiles them. Only the ones in lines 815 and 846 conflict.

(In reply to comment #30) > (From update of attachment 223002 [details] [diff] [review] [edit]) > The first two occurrances are actually inside the if/else block scope so that > VC6 correctly compiles them. Only the ones in lines 815 and 846 conflict. head-up-my-...

(In reply to comment #31) > Created an attachment (id=223141) [edit] > Less braindead vc6 fix Fixed on trunk.