279957 - Rebuild on model element not fully implemented

Status: RESOLVED FIXED

(Core Graveyard :: XForms, defect)

Description

[aaronr]

We need to get nsXFormsModelElement::Rebuild() to work correctly.  Allan has
already commented in the function the general things that need to happen. 
Really just need to run ::Bind on all the controls again so that the bindings
are re-generated after the MDG is cleared.  Or something along those lines.

Comment 1

[aaronr]

Well, we don't have to worry about people modifying forms via JS for a while.  I
tried to make a testcase to see if my implementation of rebuild worked and I got
the following JS exception:

Error: uncaught exception: [Exception... "Cannot modify properties of a
WrappedNative"  nsresult: "0x80570034 (NS_ERROR_XPC_CANT_MODIFY_PROP_ON_WN)" 
location: "JS frame :: file:///C:/myprogs/xml/xforms/recalculate.xhtml ::
changeValue :: line 29"  data: no]

when I tried to change the ref attribute on an output element.

Comment 2

[aaronr]

Attachment: first attempt at fix

Worked the approach out with Allan.  I started by implementing ::Bind on
elements that inherit from nsXFormsControlStub and moved the determination of
mBoundNode from ::Refresh to ::Bind since that is executed a lot less often. 
In model::Rebuild(), I clear the MDG, then I backed up mFormControls array,
cleared it, then went through the old list and ran ::Bind on all of the
controls in the list.  This will re-initialize mBoundNode and also get the
controls re-inserted into the model that they are now bound to.

Comment 3

[Allan Beaufour]

Comment on attachment 172873 [details] [diff] [review]
first attempt at fix

>+++ nsXFormsModelElement.cpp	30 Jan 2005 12:02:15 -0000
>+#include "nsXFormsControlStub.h"

nope, see below.

>@@ -418,20 +419,46 @@ nsXFormsModelElement::Rebuild()
...
>+    nsXFormsControlStub* control = NS_STATIC_CAST(nsXFormsControlStub*, 
>+                                                  (*oldFormList)[i]);

What's up with nsXFormsControlStub? bind is a funtion on nsIXFormsControl.

>-  if (!mBoundNode)
>-    return NS_OK;
>+  // Seems to be a timing problem where the first call to Bind() during
>+  // document load could fail.  We'll try one more time here.
>+  if(!mBoundNode) {
>+    Bind();
>+    NS_ENSURE_STATE(mBoundNode);
>+  }

Doesn't sound good. "Seems" should be investigated. Moreover the node binding
is optional, so you are not guaranteed a mBoundNode.

Bigger issues:
1) Isn't the Bind() function you are implementing quite the same for most of
the controls. Check whether it's possible to move the implementation to
nsXFormsControlStub.

2) You have to reattach the <bind> elements to the MDG also.

Comment 4

[aaronr]

Attachment: updated patch

incorporated Allan's comments.

checked out the timing problem.  It exists for all controls and isn't a
problem.  Bind failing to find mBoundNode because control isn't parented by a
nsIXFormsContextControl.  Eventually the select element will be appended into
the HTMLBodyElement and then Bind will work and mBoundNode will be set
accordingly.

Comment 5

[Allan Beaufour]

Comment on attachment 172934 [details] [diff] [review]
updated patch

1. You moved implementation to nsXFormsControlStub, which is fine but:

1.1. You have changed the semantics of Bind(), you always return NS_OK, you
should check/return the return-value of ResetBoundNode()!

1.2. ResetBoundNode() == Bind(), except for:
  * <group> and <switch> which saves the modelID (that'll go away with landing
of bug 280017).
  * <input> which checks its visual representaion.... which only need to be
checked in Refresh().

  So, how about making nsXFormsControlStub::Bind() { return ResetBoundNode();
};
  and make a NOOP Bind() in nsXFormsContextContainer

2. You actually change the meaning of refresh on <switch>, but as far as I can
it's ok. But *ahem* Refresh() and Process() could use a bit of pruning :)

One last go, and you're there.

Comment 6

[aaronr]

Attachment: next attempt

incorporated comments.	

Left Bind in those that need to store the model id (like switch element which
Beaufour said would be fixed in a different bug).  Also things like
ContextControl and RepeatElement who don't use Bind for mBoundNode will have
Bind stubbed out.  Otherwise everyone who needs to have mBoundNode set will use
::Bind from nsXFormsControlStub (which is most of the cases).

Comment 7

[Allan Beaufour]

Comment on attachment 173000 [details] [diff] [review]
next attempt

>+++ nsXFormsGroupElement.cpp	31 Jan 2005 20:51:35 -0000

>+nsXFormsGroupElement::Bind()
...

>+  nsCOMPtr<nsIModelElementPrivate> modelPriv = nsXFormsUtils::GetModel(mElement);
>+  nsCOMPtr<nsIDOMElement> modelElement = do_QueryInterface(modelPriv);

The modelElement is hiding in mModel, ready for action :)

nsCOMPtr<nsIDOMElement> modelElement = do_QueryInterface(mModel);

And thththththat's all folks :)

With that, r=me

Only problem is that this will probably mega-clash with bug 280017... :(

Comment 8

[aaronr]

Attachment: with change to use mModel instead of GetModel

Comment 9

[Brian Ryner (not reading)]

checked in