Last Comment Bug 329935 - refresh inefficiencies with selects, items
: refresh inefficiencies with selects, items
Status: RESOLVED FIXED
: fixed1.8.0.5, fixed1.8.1
Product: Core
Classification: Components
Component: XForms (show other bugs)
: Trunk
: x86 All
: -- normal (vote)
: ---
Assigned To: aaronr
: Stephen Pride
Mentors:
: 338969 (view as bug list)
Depends on:
Blocks: 300248
  Show dependency treegraph
 
Reported: 2006-03-09 11:16 PST by aaronr
Modified: 2006-06-06 07:01 PDT (History)
5 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
testcase (1.26 KB, application/xhtml+xml)
2006-04-05 14:59 PDT, aaronr
no flags Details
proposed fix (13.47 KB, patch)
2006-04-18 16:30 PDT, aaronr
no flags Details | Diff | Review
fix 2 (18.69 KB, patch)
2006-04-21 14:10 PDT, aaronr
bugs: review+
Details | Diff | Review
fix 3 (15.50 KB, patch)
2006-04-24 16:15 PDT, aaronr
bugs: review+
Details | Diff | Review
fix 4 (16.28 KB, patch)
2006-04-28 14:33 PDT, aaronr
allan: review-
Details | Diff | Review
fix 5 (18.12 KB, patch)
2006-05-09 12:22 PDT, aaronr
allan: review+
Details | Diff | Review

Description aaronr 2006-03-09 11:16:57 PST
On a single load of a simple xform with just a xf:select and a xf:itemset generating 3 items, xf:select's refresh gets hit 9 times.  Looking at this issue, I also noticed that the itemset is added to the deferred bind list 3 times.  So the fix to this issue needs to address why select's refresh is getting called so many times before the itemset is even done populating, and also the fact that controls are added more than once to the deferred bind list.
Comment 1 alexander :surkov 2006-03-09 17:43:08 PST
Every label ask for select refresh during its own refreshing :). 

Plus if select gets refreshes when it is refreshed then thease refreshes will not be took into account. Theoretically it can make a problem.
Comment 2 alexander :surkov 2006-03-12 17:58:28 PST
(In reply to comment #1)
> Every label ask for select refresh during its own refreshing :). 
> 

Imo if label is refreshed then it should ask for refresh parent xforms:item element. I have in view label refreshing shouldn't refresh select entirerly. Aaron, is your bug about the issue too?

Comment 3 aaronr 2006-03-13 11:13:39 PST
(In reply to comment #2)
> (In reply to comment #1)
> > Every label ask for select refresh during its own refreshing :). 
> > 
> 
> Imo if label is refreshed then it should ask for refresh parent xforms:item
> element. I have in view label refreshing shouldn't refresh select entirerly.
> Aaron, is your bug about the issue too?
> 

Since you mentioned it here, I'll try to cover that scenario with my fix. :)  I'm basically going to just put a breakpoint on the refresh for xf:select and xf:select1 and identify why they are getting called so much and group together the calls into as small a number as possible.  Like you mentioned, a refresh of a label shouldn't call directly into the select asking it to refresh but rather go through the item which contains it.  This item should go through its itemset if it has one which should only go to the select when all the items in the itemset are done.  Things like that.
Comment 4 alexander :surkov 2006-03-13 17:34:04 PST
(In reply to comment #3)

> Since you mentioned it here, I'll try to cover that scenario with my fix. :) 
> I'm basically going to just put a breakpoint on the refresh for xf:select and
> xf:select1 and identify why they are getting called so much and group together
> the calls into as small a number as possible.  Like you mentioned, a refresh of
> a label shouldn't call directly into the select asking it to refresh but rather
> go through the item which contains it.  This item should go through its itemset
> if it has one which should only go to the select when all the items in the
> itemset are done.  Things like that.
> 

If I understand right you then you propose to refresh select control entirerly when label is changed (though you want to refresh select when all labels will be loaded). But I guess it's more right to update only one control corresponding a label control (not select control entirerly). Though this way involves a some (probably bid) changes in select binding.

In any way if the bug patch involves select.xml changes then I prefer to see them based on a patch of bug 323849 because it's not always easy to extend changes to patch of mentioned bug ;).
Comment 5 alexander :surkov 2006-03-13 18:21:14 PST
One the issue is related with the bug is bug 327985 since it reduces potential calls of LabelRefreshed() mehtods.
Comment 6 aaronr 2006-04-05 14:59:48 PDT
Created attachment 217341 [details]
testcase

With this testcase, the itemset and output get added multiple times to the deferredBindList.
Comment 7 aaronr 2006-04-05 15:14:21 PDT
(In reply to comment #6)
> Created an attachment (id=217341) [edit]
> testcase
> 
> With this testcase, the itemset and output get added multiple times to the
> deferredBindList.
> 

Itemset getting added extraneously during nsXFormsItemsetElement::Refresh and output via nsXFormsOutputElement::GetValue.  I guess since we can't really anticipate the different ways that this can happen and eliminate them all, so we'd better prevent the duplication from the list end.  So I'll add a nsVoidHashSet as a memeber of model to track which controls are already in the deferred bind list and which aren't.  I can't seem to do that with the list.  Can't turn the list into a hashtable since we kind of need to process controls on the list in order so that parents will bind before children and be able to provide proper context nodes for their children.  And with large documents with a lot of controls, could be expensive to check the whole list to see if a control is already on it.  That combined with surkov's change for 327985 will help eliminate a lot of extraneous select refreshes.
Comment 8 aaronr 2006-04-18 16:30:57 PDT
Created attachment 218906 [details] [diff] [review]
proposed fix
Comment 9 Olli Pettay [:smaug] 2006-04-19 01:36:15 PDT
Would it be better to have a bit in controls which tells whether it is already in
deferred list? That way there wouldn't be need for that HashSet.
Comment 10 aaronr 2006-04-19 09:35:00 PDT
(In reply to comment #9)
> Would it be better to have a bit in controls which tells whether it is already
> in
> deferred list? That way there wouldn't be need for that HashSet.
> 

My only thought is that a flag field just for this is a slight overhead in all controls versus a hashset that only exists during form initialization.  But you are right if we really do have a need for a flag field in all controls for other purposes.  Then it wouldn't be overhead anymore.  I can go either way.  Any other opinions?
Comment 11 Allan Beaufour 2006-04-20 01:47:42 PDT
(In reply to comment #10)
> (In reply to comment #9)
> > Would it be better to have a bit in controls which tells whether it is
> already in deferred list? That way there wouldn't be need for that HashSet.
> 
> My only thought is that a flag field just for this is a slight overhead in all
> controls versus a hashset that only exists during form initialization.  But you
> are right if we really do have a need for a flag field in all controls for
> other purposes.  Then it wouldn't be overhead anymore.  I can go either way. 
> Any other opinions?

I agree with both of you :) But since we already have some flags in the item element (the only consumer of this hashset, right?), then change all the flags to PRPackedBool, and we will not have any overhead.
Comment 12 aaronr 2006-04-20 10:06:38 PDT
(In reply to comment #11)
> (In reply to comment #10)
> > (In reply to comment #9)
> > > Would it be better to have a bit in controls which tells whether it is
> > already in deferred list? That way there wouldn't be need for that HashSet.
> > 
> > My only thought is that a flag field just for this is a slight overhead in all
> > controls versus a hashset that only exists during form initialization.  But you
> > are right if we really do have a need for a flag field in all controls for
> > other purposes.  Then it wouldn't be overhead anymore.  I can go either way. 
> > Any other opinions?
> 
> I agree with both of you :) But since we already have some flags in the item
> element (the only consumer of this hashset, right?), then change all the flags
> to PRPackedBool, and we will not have any overhead.
> 


The hashset exists basically as a copy of the deferredbindlist to ensure that we don't have duplicates on the deferredbindlist.  So it will contain all sorts of controls, not just item elements.
Comment 13 aaronr 2006-04-20 16:56:40 PDT
Comment on attachment 218906 [details] [diff] [review]
proposed fix

removing review request.  Going to change it so that we put off all select/select1 refreshes, not just the ones caused by item labels wanting a select to refresh.  Really no need for a select to refresh before its children, but we still need them to bind before their children.  Currently they'll just update with stale data and just have to refresh again anyhow.
Comment 14 aaronr 2006-04-21 14:10:18 PDT
Created attachment 219362 [details] [diff] [review]
fix 2

ok, changed the fix around a little.  Now a refresh called on select/select1 may defer itself since in many cases it doesn't make sense to refresh the select before the child items (who will just refresh the select again).  And in some cases select's refresh (in XBL) will make decisions based on the contained item set, for example, even though the itemset hasn't gone through its initial bind, yet.  So I overrode ::Refresh in select and select1's to always defer their refreshes in the cases where a post refresh list exists (during deferred binding and during nsXFormsModelElement::Refresh).  I left the hashset in nsXFormsModelElement since I still don't think that it is worth introducing a flag double word into every control unless we think that there is a potential for using the flag space for other uses.
Comment 15 Olli Pettay [:smaug] 2006-04-24 07:08:46 PDT
Comment on attachment 219362 [details] [diff] [review]
fix 2

>Index: nsXFormsAtoms.cpp
>===================================================================
>RCS file: /cvsroot/mozilla/extensions/xforms/nsXFormsAtoms.cpp,v
>retrieving revision 1.18
>diff -u -8 -p -r1.18 nsXFormsAtoms.cpp
>--- nsXFormsAtoms.cpp	22 Mar 2006 18:05:48 -0000	1.18
>+++ nsXFormsAtoms.cpp	21 Apr 2006 21:03:35 -0000
>@@ -60,16 +60,17 @@ nsIAtom *nsXFormsAtoms::appearance;
> nsIAtom *nsXFormsAtoms::incremental;
> nsIAtom *nsXFormsAtoms::clazz;
> nsIAtom *nsXFormsAtoms::deferredBindListProperty;
> nsIAtom *nsXFormsAtoms::readyForBindProperty;
> nsIAtom *nsXFormsAtoms::fatalError;
> nsIAtom *nsXFormsAtoms::isInstanceDocument;
> nsIAtom *nsXFormsAtoms::instanceDocumentOwner;
> nsIAtom *nsXFormsAtoms::externalMessagesProperty;
>+nsIAtom *nsXFormsAtoms::deferredBindHashProperty;
> 
> const nsStaticAtom nsXFormsAtoms::Atoms_info[] = {
>   { "src",                      &nsXFormsAtoms::src },
>   { "bind",                     &nsXFormsAtoms::bind },
>   { "type",                     &nsXFormsAtoms::type },
>   { "readonly",                 &nsXFormsAtoms::readonly },
>   { "required",                 &nsXFormsAtoms::required },
>   { "relevant",                 &nsXFormsAtoms::relevant },
>@@ -87,16 +88,17 @@ const nsStaticAtom nsXFormsAtoms::Atoms_
>   { "appearance",               &nsXFormsAtoms::appearance },
>   { "incremental",              &nsXFormsAtoms::incremental },
>   { "class",                    &nsXFormsAtoms::clazz },
>   { "DeferredBindListProperty", &nsXFormsAtoms::deferredBindListProperty },
>   { "ReadyForBindProperty",     &nsXFormsAtoms::readyForBindProperty },
>   { "fatalError",               &nsXFormsAtoms::fatalError },
>   { "isInstanceDocument",       &nsXFormsAtoms::isInstanceDocument },
>   { "instanceDocumentOwner",    &nsXFormsAtoms::instanceDocumentOwner },
>-  { "ExternalMessagesProperty", &nsXFormsAtoms::externalMessagesProperty }
>+  { "ExternalMessagesProperty", &nsXFormsAtoms::externalMessagesProperty },
>+  { "DeferredBindHashProperty", &nsXFormsAtoms::deferredBindHashProperty }

Nit, move DeferredBindHashProperty to be right after DeferredBindListProperty.
Easier to read.

>   static NS_HIDDEN_(nsIAtom *) deferredBindListProperty;
>   static NS_HIDDEN_(nsIAtom *) readyForBindProperty;
>   static NS_HIDDEN_(nsIAtom *) fatalError;
>   static NS_HIDDEN_(nsIAtom *) isInstanceDocument;
>   static NS_HIDDEN_(nsIAtom *) instanceDocumentOwner;
>   static NS_HIDDEN_(nsIAtom *) externalMessagesProperty;
>+  static NS_HIDDEN_(nsIAtom *) deferredBindHashProperty;

Same here.


>===================================================================
>RCS file: /cvsroot/mozilla/extensions/xforms/nsXFormsItemElement.cpp,v
>retrieving revision 1.15
>diff -u -8 -p -r1.15 nsXFormsItemElement.cpp
>--- nsXFormsItemElement.cpp	12 Apr 2006 16:49:36 -0000	1.15
>+++ nsXFormsItemElement.cpp	21 Apr 2006 21:03:35 -0000
>@@ -376,19 +376,23 @@ nsXFormsItemElement::Refresh()
> {
>   if (mDoneAddingChildren) {
>     nsCOMPtr<nsIDOMNode> parent, current;
>     current = mElement;
>     do {
>       current->GetParentNode(getter_AddRefs(parent));
>       if (nsXFormsUtils::IsXFormsElement(parent, NS_LITERAL_STRING("select1")) ||
>           nsXFormsUtils::IsXFormsElement(parent, NS_LITERAL_STRING("select"))) {
>-        nsCOMPtr<nsIXFormsControl> select(do_QueryInterface(parent));
>-        if (select) {
>-          select->Refresh();
>+
>+        nsresult rv = nsXFormsModelElement::ContainerNeedsPostRefresh(parent);
>+        if (NS_FAILED(rv)) {
>+          // looks like there is no reason to delay the refresh, so we'll call
>+          // refresh on the select/select1 directly.
>+          nsCOMPtr<nsIXFormsControl> control(do_QueryInterface(parent));
>+          control->Refresh();

Why do you need to call ContainerNeedsPostRefresh here?
Select::Refresh and Select1::Refresh does that too.

>@@ -452,19 +456,23 @@ nsXFormsItemElement::LabelRefreshed()
> {
>   NS_ENSURE_STATE(mElement);
>   nsCOMPtr<nsIDOMNode> parent, current;
>   current = mElement;
>   do {
>     current->GetParentNode(getter_AddRefs(parent));
>     if (nsXFormsUtils::IsXFormsElement(parent, NS_LITERAL_STRING("select1")) ||
>         nsXFormsUtils::IsXFormsElement(parent, NS_LITERAL_STRING("select"))) {
>-      nsCOMPtr<nsIXFormsControl> select(do_QueryInterface(parent));
>-      if (select) {
>-        select->Refresh();
>+
>+      nsresult rv = nsXFormsModelElement::ContainerNeedsPostRefresh(parent);
>+      if (NS_FAILED(rv)) {
>+        // looks like there is no reason to delay the refresh, so we'll call
>+        // refresh on the select/select1 directly.
>+        nsCOMPtr<nsIXFormsControl> control(do_QueryInterface(parent));
>+        control->Refresh();

Same here.

> void
> nsXFormsModelElement::CancelPostRefresh(nsIXFormsControl* aControl)
> {
>   if (sPostRefreshList)
>     sPostRefreshList->RemoveElement(aControl);
> }

Should CancelPostRefresh remove element also from sContainerPostRefreshList?
I think it should.

With those r=me, though I might like more using a flag in controls than the hashset.
But either way is ok.
Comment 16 aaronr 2006-04-24 16:15:20 PDT
Created attachment 219678 [details] [diff] [review]
fix 3

fixed Olli's nits.  Changed to use PRPackedBool on each control instead of nsVoidHashSet.  Olli, I don't know if you want to review it again or not.
Comment 17 Allan Beaufour 2006-04-26 07:33:47 PDT
I'm slightly out of touch with the whole deferred/postRefresh business. But if the purpose of all this is to delay refreshing of an element until all child elements have refreshed, why not let the model handle it? It knows the structure.

model::refreshsubtree(control) {
  if (rebind) control->Bind()
  bool postRefresh control->WantsPostRefresh()
  if (!postRefresh && refresh) control->Refresh()
  refreshsubtree(children_of(control))
  if (postRefresh && refresh) control->Refresh()
}

?

It's very possible that I've missed the point of this whole maneuver, but please enlighten me then :)
Comment 18 aaronr 2006-04-26 12:41:00 PDT
(In reply to comment #17)
> I'm slightly out of touch with the whole deferred/postRefresh business. But if
> the purpose of all this is to delay refreshing of an element until all child
> elements have refreshed, why not let the model handle it? It knows the
> structure.
> 
> model::refreshsubtree(control) {
>   if (rebind) control->Bind()
>   bool postRefresh control->WantsPostRefresh()
>   if (!postRefresh && refresh) control->Refresh()
>   refreshsubtree(children_of(control))
>   if (postRefresh && refresh) control->Refresh()
> }
> 
> ?
> 
> It's very possible that I've missed the point of this whole maneuver, but
> please enlighten me then :)
> 

What you propose would work for a lot of the cases I'm now covering, but it would still miss one major case -> initialization.  All the refreshes that happen due to widget attaches occur after the deferredbindlist has started processing and outside of nsXFormsModelElement::Refresh.

bz mentioned in my XBL2 question that he thought that all xbl constructors (and thus widget attaches) should be done before the onload handler fired.  So now I'm looking into whether we can handle the "load" event and not process the deferredbindlist (and thus xforms-model-construct-done and xforms-ready) until then.  But I'll do that in a seperate bug.
Comment 19 aaronr 2006-04-26 14:41:43 PDT
> bz mentioned in my XBL2 question that he thought that all xbl constructors (and
> thus widget attaches) should be done before the onload handler fired.  So now
> I'm looking into whether we can handle the "load" event and not process the
> deferredbindlist (and thus xforms-model-construct-done and xforms-ready) until
> then.  But I'll do that in a seperate bug.
> 

well, looks like bz was right.  All of our widgetAttach's seem to occur before "load" is generated.  So if we take the refreshes from "ProcessDeferredBind" and take the xforms-ready generation from "MaybeNotifyCompletion" and move those into a onload handler, then I think that we could save duplicating a lot of refreshes during initialization.  We'll still need the postrefreshlist and containerpostrefresh list, though, so still need the patch for this bug.  After this bug gets closed I'll open a followup bug to investigate whether I am right about these 'saved refreshes'.

Comment 20 aaronr 2006-04-26 14:55:53 PDT
(In reply to comment #19)
> > bz mentioned in my XBL2 question that he thought that all xbl constructors (and
> > thus widget attaches) should be done before the onload handler fired.  So now
> > I'm looking into whether we can handle the "load" event and not process the
> > deferredbindlist (and thus xforms-model-construct-done and xforms-ready) until
> > then.  But I'll do that in a seperate bug.
> > 
> 
> well, looks like bz was right.  All of our widgetAttach's seem to occur before
> "load" is generated.  So if we take the refreshes from "ProcessDeferredBind"
> and take the xforms-ready generation from "MaybeNotifyCompletion" and move
> those into a onload handler, then I think that we could save duplicating a lot
> of refreshes during initialization.  We'll still need the postrefreshlist and
> containerpostrefresh list, though, so still need the patch for this bug.  After
> this bug gets closed I'll open a followup bug to investigate whether I am right
> about these 'saved refreshes'.
> 


well, I should clarify: we wouldn't be saving duplicated refreshes, but rather aborted refreshes.  Since the real work of most refreshes won't actually happen until widgetattached.  So what we'd be saving is alot of the calls into nsXFormsDelegateStub::Refresh that return because QI to nsIXFormsUIWidget fails.
Comment 21 Allan Beaufour 2006-04-27 01:05:53 PDT
(In reply to comment #19)
> well, looks like bz was right.

Of course he was :)

> All of our widgetAttach's seem to occur before
> "load" is generated.  So if we take the refreshes from "ProcessDeferredBind"
> and take the xforms-ready generation from "MaybeNotifyCompletion" and move
> those into a onload handler, then I think that we could save duplicating a lot
> of refreshes during initialization.

That seems like a good idea. Right now we hook into "DOMContentLoaded", but we probably should hook into "load" as we not depend on the XBL being attached. I'm not sure if it has futher amplications though.

> We'll still need the postrefreshlist and containerpostrefresh list, though, so
> still need the patch for this bug.

If the thing that my proposal does not fix is the initialization, and that can be solved by attaching to the load event, why do we still needs this then?
Comment 22 aaronr 2006-04-27 13:15:16 PDT
(In reply to comment #21)
> (In reply to comment #19)

> > All of our widgetAttach's seem to occur before
> > "load" is generated.  So if we take the refreshes from "ProcessDeferredBind"
> > and take the xforms-ready generation from "MaybeNotifyCompletion" and move
> > those into a onload handler, then I think that we could save duplicating a lot
> > of refreshes during initialization.
> 
> That seems like a good idea. Right now we hook into "DOMContentLoaded", but we
> probably should hook into "load" as we not depend on the XBL being attached.
> I'm not sure if it has futher amplications though.
> 

We should still hook into DOMContentLoaded to get as much work done as we can before "load".  Like the schema stuff.

> > We'll still need the postrefreshlist and containerpostrefresh list, though, so
> > still need the patch for this bug.
> 
> If the thing that my proposal does not fix is the initialization, and that can
> be solved by attaching to the load event, why do we still needs this then?
> 

I think that we'll end up still needing it.  I said that the hole was in initialization, but I guess I should have been more specific...the hole will be when a list is being walked refreshing controls and during this list processing a refresh is required of a control that wasn't previously on the refresh list.  This happens during initialization when widgetattaches occur during processdeferredbinds.  Which, as you said, would mostly fixed with the "load" handler.  But it could still be that during 'bind' or 'refresh' that a new control is created via DOM manipulation (in the xbl code or a custom control).  Also if a insert happened, then after the rebuild in the bind/refresh phase repeat and itemset could create more controls that wouldn't be in the already built control list so they'd come through refresh using the post refresh list.  I think that I have that right.
Comment 23 Allan Beaufour 2006-04-28 00:06:50 PDT
(In reply to comment #22)
> (In reply to comment #21)
> > (In reply to comment #19)
> 
> > > All of our widgetAttach's seem to occur before
> > > "load" is generated.  So if we take the refreshes from "ProcessDeferredBind"
> > > and take the xforms-ready generation from "MaybeNotifyCompletion" and move
> > > those into a onload handler, then I think that we could save duplicating a lot
> > > of refreshes during initialization.
> > 
> > That seems like a good idea. Right now we hook into "DOMContentLoaded", but we
> > probably should hook into "load" as we not depend on the XBL being attached.
> > I'm not sure if it has futher amplications though.
> > 
> 
> We should still hook into DOMContentLoaded to get as much work done as we can
> before "load".  Like the schema stuff.

Good point.

> > > We'll still need the postrefreshlist and containerpostrefresh list, though, so
> > > still need the patch for this bug.
> > 
> > If the thing that my proposal does not fix is the initialization, and that can
> > be solved by attaching to the load event, why do we still needs this then?
> > 
> 
> I think that we'll end up still needing it.  I said that the hole was in
> initialization, but I guess I should have been more specific...the hole will be
> when a list is being walked refreshing controls and during this list processing
> a refresh is required of a control that wasn't previously on the refresh list. 
> This happens during initialization when widgetattaches occur during
> processdeferredbinds.  Which, as you said, would mostly fixed with the "load"
> handler.  But it could still be that during 'bind' or 'refresh' that a new
> control is created via DOM manipulation (in the xbl code or a custom control). 
> Also if a insert happened, then after the rebuild in the bind/refresh phase
> repeat and itemset could create more controls that wouldn't be in the already
> built control list so they'd come through refresh using the post refresh list. 
> I think that I have that right.

As long as controls created are children of the currently rebinding/refreshing control, then it should work fine I think. But as we talked about on IRC, the problematic part is the XBLs and async. load. :( I have to ponder some more, which should not delay this further.
Comment 24 aaronr 2006-04-28 14:33:46 PDT
Created attachment 220178 [details] [diff] [review]
fix 4

updated to trunk to take into account the recent changes to the PostRefreshList code.  Also slight change in moving the test for isondeferredbindlist above getting the deferredbindlist to save a few cycles.
Comment 25 Allan Beaufour 2006-05-09 00:29:27 PDT
(In reply to comment #24)
> Created an attachment (id=220178) [edit]
> fix 4
> 
> updated to trunk to take into account the recent changes to the PostRefreshList
> code.  Also slight change in moving the test for isondeferredbindlist above
> getting the deferredbindlist to save a few cycles.

http://beaufour.dk/jst-review/?patch=220178

*hint* *hint* *nudge* *nudge* :-)
Comment 26 Allan Beaufour 2006-05-09 02:11:42 PDT
Comment on attachment 220178 [details] [diff] [review]
fix 4

As I've said multiple times, I really do not like this approach, but I have no better solution atm.

> Index: nsXFormsControlStub.h
> ===================================================================
> +  PRBool IsOnDeferredBindList() {return mOnDeferredBindList;}
> +
> +  void SetOnDeferredBindList(PRBool aIsOnList) {mOnDeferredBindList = aIsOnList;}
> +

Please add spaces between code and curly braces.

>    /** 
>     * Used to keep track of whether this control has any single node binding
>     * attributes.
>     */
> -  PRInt32 mBindAttrsCount;
> +  PRInt16 mBindAttrsCount;

(not really related to this bug, so feel free to push to follow up bug) 8 should be all we need right? Maybe with a safe-guard somewhere for overflow? I guess you could trigger an overflow by adding MAX(PRInt?) + 1 @ref's to control... but then again, why would you?

> +
> +  /**
> +   * Should the control appear disabled. This is f.x. used when a valid single
> +   * node binding is not pointing to an instance data node.
> +   */

Nice and descriptive comment. Says it all so elegantly. So well put ;-)

> +  PRPackedBool                        mOnDeferredBindList;

I believe you need to but the packed booleans next to each other, or they cannot get packed.

> +
> +  /**
> +   * Array of repeat-elements of which the control uses repeat-index.
> +   */
> +  nsCOMArray<nsIXFormsRepeatElement>  mIndexesUsed;
 
I had to read that comment a few times... "List of repeats that the node binding depends on. This happens when using the index() function in the binding expression." ?

(thanks for fixing my missing comments here...)

> Index: nsXFormsModelElement.cpp
> ===================================================================

> +nsresult
> +nsXFormsModelElement::ContainerNeedsPostRefresh(nsIXFormsControl* aControl)
> +{
> +  // Sometimes a child that is on the post refresh list will want to force
> +  // its containing xforms control to refresh.  Like if an xf:item's label
> +  // refreshes, it wants to reflect any of its changes in the xf:select or
> +  // xf:select1 that contains it.  This function will try to minimize that by
> +  // allowing xforms controls that function as containers (like select/select1)
> +  // to defer their refresh until most or all of its children have refreshed.
> +  // Well, at least until every control on the sPostRefreshList has been
> +  // processed.

This comment should go in the header file, since it is a description of the function itself.

> +
> +  if (sRefreshing || sPostRefreshList) {

Why a check for both? Should (sRefreshing) not be enough?

> +    if (!sContainerPostRefreshList) {
> +      sContainerPostRefreshList = new nsVoidArray();
> +      NS_ENSURE_TRUE(sContainerPostRefreshList, NS_ERROR_OUT_OF_MEMORY);
> +    }
> +
> +    if (sContainerPostRefreshList->IndexOf(aControl) < 0) {
> +      sContainerPostRefreshList->AppendElement(aControl);
> +    }
> +
> +    // return NS_OK to show that the control's refresh will be delayed, whether
> +    // as a result of this call or a previous call to this function.
> +    return NS_OK;
> +  }
> +
> +  // Looks like delaying the refresh doesn't make any sense.  But since this
> +  // function may be called from inside the control node's refresh already,
> +  // we probably shouldn't just assume that we can call the refresh here.  So
> +  // we'll just return an error to signal that we couldn't delay the refresh.
> +
> +  return NS_ERROR_FAILURE;
> +}
> +

1) Using NS_ERROR_FAILURE to return a non-error is wrong.

2) There is a lot of "waving hands" in the comments. "will try", "Looks like", etc. Remember Yoda's wise words :)

> @@ -2376,33 +2441,48 @@ nsXFormsModelElement::DeferElementBind(n
>                                         nsIXFormsControlBase *aControl)
>  {
>    nsCOMPtr<nsIDocument> doc = do_QueryInterface(aDoc);
 
>    if (!doc || !aControl) {
>      return NS_ERROR_FAILURE;
>    }
 
> +  // We are using a PRBool on each control to mark whether the control is on the
> +  // deferredBindList.  We are running into too many scenarios where a control
> +  // could be added more than once which will lead to inefficiencies because
> +  // calling bind and refresh on some controls is getting pretty expensive.
> +  // Since we need to keep the document order of the controls AND don't want
> +  // to walk the deferredBindList every time we want to check about adding a
> +  // control, this seemed like the best solution.

again not the strongest comment.

> +  nsXFormsControlStubBase *controlObj =
> +    NS_STATIC_CAST(nsXFormsControlStubBase *, aControl); 

This is bad. Either we need to store nsXFormsControlStubBase* in the model then, or you need to expose the needed functionality on the interface. I'm for the latter.

>    // always append to the end of the list.  We need to keep the elements in
> -  // document order when we process the binds later.  Otherwise we have trouble
> -  // when an element is trying to bind and should use its parent as a context
> -  // for the xpath evaluation but the parent isn't bound yet.
> +  // document order when we process the binds later.  Otherwise we have
> +  // trouble when an element is trying to bind and should use its parent as a
> +  // context for the xpath evaluation but the parent isn't bound yet.

have you actually changed anything? if not, then leave it be :)

>    deferredBindList->AppendObject(aControl);
> +  controlObj->SetOnDeferredBindList(PR_TRUE);
 
> @@ -2420,16 +2500,19 @@ nsXFormsModelElement::ProcessDeferredBin
>                       doc->GetProperty(nsXFormsAtoms::deferredBindListProperty));
 
>    if (deferredBindList) {
>      for (int i = 0; i < deferredBindList->Count(); ++i) {
>        nsIXFormsControlBase *base = deferredBindList->ObjectAt(i);
>        if (base) {
>          base->Bind();
>          base->Refresh();
> +        nsXFormsControlStubBase *controlObj =
> +          NS_STATIC_CAST(nsXFormsControlStubBase *, base); 
> +        controlObj->SetOnDeferredBindList(PR_FALSE);

Again, I do really not like this.

> Index: nsXFormsModelElement.h
> ===================================================================
 
>    static nsresult NeedsPostRefresh(nsIXFormsControl* aControl);
> +  static nsresult ContainerNeedsPostRefresh(nsIXFormsControl* aControl);

Comment please. ~the one from the function body.

> Index: nsXFormsSelect1Element.cpp
> ===================================================================
> +NS_IMETHODIMP
> +nsXFormsSelect1Element::Refresh()
> +{
> +  nsresult rv = nsXFormsModelElement::ContainerNeedsPostRefresh(this);
> +  if (NS_SUCCEEDED(rv)) {
> +    return rv;
> +  }
> +
> +  return nsXFormsDelegateStub::Refresh();
> +}

Imho, this really shows why using NS_ERROR_FAILURE is bad. The function does not help either, because the functionality is sort of both a statement and a question.
Comment 27 aaronr 2006-05-09 12:22:55 PDT
Created attachment 221480 [details] [diff] [review]
fix 5

fixes Allan's comments.
Comment 28 Allan Beaufour 2006-05-15 07:15:03 PDT
(In reply to comment #27)
> Created an attachment (id=221480) [edit]
> fix 5
> 
> fixes Allan's comments.
> 

Fixed this:
nsXFormsControlStub.h: In constructor ‘nsXFormsControlStubBase::nsXFormsControlStubBase()’:
nsXFormsControlStub.h:197: warning: ‘nsXFormsControlStubBase::mBindAttrsCount’ will be initialized after
nsXFormsControlStub.h:191: warning:   ‘PRPackedBool nsXFormsControlStubBase::mOnDeferredBindList’
nsXFormsControlStub.h:136: warning:   when initialized here

And checked in on trunk.
Comment 29 Allan Beaufour 2006-05-23 07:50:55 PDT
*** Bug 338969 has been marked as a duplicate of this bug. ***

Note You need to log in before you can comment on or make changes to this bug.