279187 - Hook up Schema Validation to XForms

Status: RESOLVED FIXED

(Core Graveyard :: XForms, defect)

Description

[Doron Rosenberg (IBM)]

I finally got XForms and Schema Validation to talk to eachother and got XForms
to set the eFlag_CONSTRAINT based on the validation result.

To set the eFlag_CONSTRAINT, I had to add:
nsXFormsMDGEngine::MarkNodeAsInvalid(nsIDOMNode* aContextNode, PRBool aValue)

The code I am about to attach is ugly, but I am wondering if its the right way
to go.

Comment 1

[Doron Rosenberg (IBM)]

Attachment: current code, does not contain all changes required to make it work.

Comment 2

[aaronr]

Even though this patch isn't EVERYTHING to make it work, it shows the process
that we are thinking of using right now to get/update state between the MDG and
model.  What we really need to know is if this is the best way to update the
nsXFormsNodeState (i.e ask the MDG to update the flags for us) or should the
model get the nodestate and update the flag itself.

Comment 3

[Allan Beaufour]

(In reply to comment #0)
> I finally got XForms and Schema Validation to talk to eachother and got XForms
> to set the eFlag_CONSTRAINT based on the validation result.
> 
> To set the eFlag_CONSTRAINT, I had to add:
> nsXFormsMDGEngine::MarkNodeAsInvalid(nsIDOMNode* aContextNode, PRBool aValue)
> 
> The code I am about to attach is ugly, but I am wondering if its the right way
> to go.

imho, not exactly :) It should be part of the inner workings of the MDG
revalidate-cycle, but I haven't given it much thought.

Comment 4

[Doron Rosenberg (IBM)]

The problem I ran into is that all methods to get the nodestate that can be
called from the model element return consts, so I couldn't call ->Set() on it.

Comment 5

[Doron Rosenberg (IBM)]

I added this method now:

nsresult
nsXFormsMDGEngine::SetNodeStateFlag(nsIDOMNode* aNode, PRUint16 aFlag, PRBool
aValue)
{
  nsXFormsNodeState* ns = GetNCNodeState(aNode);
  NS_ENSURE_TRUE(ns, NS_ERROR_FAILURE);

  ns->Set(aFlag, aValue);
  return NS_OK;
}

so it can be shared to set flags.

Comment 6

[Allan Beaufour]

(In reply to comment #5)
> I added this method now:
> 
> [SNIP]

Sorry, I haven't got around to this yet, but the whole idea of the const return
of the NodeState is that only the MDG determines the states, so having such a
function is against (my idea of) the design. The validation needs to be done
inside the MDG as part of the Recalculate() function, as it only needs to be
done when a node value changes.

Comment 7

[Doron Rosenberg (IBM)]

(In reply to comment #6)
> (In reply to comment #5)
> > I added this method now:
> > 
> > [SNIP]
> 
> Sorry, I haven't got around to this yet, but the whole idea of the const return
> of the NodeState is that only the MDG determines the states, so having such a
> function is against (my idea of) the design. The validation needs to be done
> inside the MDG as part of the Recalculate() function, as it only needs to be
> done when a node value changes.

Ee put a ValidateNode method in nsXFormsModelElement, so I was confused why we
needed it if the validation should go into MDG.  Should MDG link against
nsXFormsSchemaValidation and call it or should MDG validate through the model?

Comment 8

[Doron Rosenberg (IBM)]

Attachment: Another try

I was blanking out, I think I got it this time.

nsXFormsMDGEngine::Recalculate() calls nsXFormsModelElement::ValidateNode() now
and it works fine.

Note that this requires my schema validation changes that are still in the
review process, so you can't actually test it.

Comment 9

[aaronr]

If there is a dependency on schema changes, could you please note that in the
dependent bug list?  I'm having trouble keeping the schema bugs seperated in my
head.  Thanks.

Comment 10

[Doron Rosenberg (IBM)]

note that this builds fine without the code in bug 223097 if a schema is loaded
in the model.  If no schema is loaded by the model, it will throw an
NS_ERROR_SCHEMAVALIDATOR_NO_SCHEMA_LOADED.

Comment 11

[Allan Beaufour]

Comment on attachment 172389 [details] [diff] [review]
Another try

>Index: extensions/xforms/nsXFormsMDGEngine.cpp
...
>@@ -331,7 +334,7 @@
> #ifdef DEBUG_XF_MDG
>     nsAutoString domNodeName;
>     g->mContextNode->GetNodeName(domNodeName);
>-    
>+

Nit: whitespace changes.

>     printf("\tNode #%d: This=%p, Dirty=%d, DynFunc=%d, Type=%d, Count=%d, Suc=%d, CSize=%d, CPos=%d, Next=%p, domnode=%s\n",
>            i, (void*) g, g->IsDirty(), g->mDynFunc, g->mType,
>            g->mCount, g->mSuc.Count(), g->mContextSize, g->mContextPosition,
>@@ -380,17 +383,26 @@
>         rv = BooleanExpression(g, constraint);
>         NS_ENSURE_SUCCESS(rv, rv);
>       }
>-      ///
>-      /// @todo Schema validity should be checked here (XXX)
>-              
>+
>       if (ns->IsConstraint() != constraint) {
>         ns->Set(eFlag_CONSTRAINT, constraint);
>         ns->Set(eFlag_DISPATCH_VALID_CHANGED, PR_TRUE);
>         NS_ENSURE_TRUE(aChangedNodes->AddNode(g->mContextNode),
>                        NS_ERROR_FAILURE);
>       }
>+
>+      // Validate the node
>+      if (mModel) {
>+        PRBool isValid;
>+        mModel->ValidateNode(g->mContextNode, &isValid);
>+
>+        nsXFormsNodeState* ns = GetNCNodeState(g->mContextNode);
>+        NS_ENSURE_TRUE(ns, NS_ERROR_FAILURE);
>+
>+        ns->Set(eFlag_CONSTRAINT, isValid);
>+      }

Replace with:
if (mModel && constraint) {
  mModel->ValidateNode(g->mContextNode, &constraint);
}
and move it up where the original comment was.

>       break;
>-      
>+

Nit: More whitespace changes.

>Index: extensions/xforms/nsXFormsMDGEngine.h
...
>+
>+  /** The model that created the MDG*/
>+  nsIModelElementPrivate *mModel;

Nit: Add a space before '*/'

>   /**
>    * Set of nodes that are marked as changed, and should be included in
>@@ -323,7 +327,7 @@
>   /**
>    * Initializes the internal structures. Needs to be called before class is used!
>    */
>-  nsresult Init();
>+  nsresult Init(nsIModelElementPrivate *aModel);

Nit: Add a @param aModel <insert description here>, and make the ever-whining
Doxygen-user happy :)

>Index: extensions/xforms/nsXFormsModelElement.cpp
...
>+nsXFormsModelElement::ValidateNode(nsIDOMNode *aInstanceNode, PRBool *aResult)
> {
...
>+
>+  printf(" Is valid: %d \n", isValid ? 1 : 0);
>+

Remove before check in.

>+  *aResult = isValid;

Null-check aResult.

>Index: extensions/xforms/nsIModelElementPrivate.idl
...
>-  PRBool validateNode(in nsIDOMNode instanceNode);
>+  PRBool validateNode(in nsIDOMNode aInstanceNode);

I'm not sure what is the right thing here for IDLs?

I haven't tested the code at all, but I need to change something in the MDG I
think, the type will only be checked when there is a constraint on the node.

I'm sorry I haven't gotten around and hacked it together, but I thought I could
"just do" index()/<setindex> before looking at this. It proved more difficult
than I thought so it took (and takes) my time.

But with the above changes, r=me.

Comment 12

[Doron Rosenberg (IBM)]

checked in