256744 - Integrate Novell master dependency graph code

Status: RESOLVED FIXED

(Core Graveyard :: XForms, defect)

Description

[Brian Ryner (not reading)]

Tracking bug for integrating Novell's master dependency graph code for XForms.

Comment 1

[Allan Beaufour]

Attachment: MDG Patch 1

Ok, here's a "first edition". It lacks a couple of XPath functionalities, for
which the location is a bit undecided for now. It also needs to be tied
properly together with the new design of <model> and <bind>, but for the
current code it is integrated as much as possible.

nsXFormsMDGEngine.cpp is also filled with a lot of |#ifdef
DEBUG_beaufour|-sections... these will go away later in the process :)

Comment 2

[Olli Pettay [:smaug][[e-mail]]]

Could you use 2-space indention?

Comment 3

[Allan Beaufour]

Ah, sorry. Didn't check the --expand-tabs output from diff. I'll fix it, please
take on your "ignore-indent-error" glasses on when reading :)

Comment 4

[Olli Pettay [:smaug][[e-mail]]]

And for debugging no need for DumpDocument(). 
Just use nsIDOMSerializer or nsIDOMLSSerializer.

Comment 5

[Allan Beaufour]

Attachment: MDG Patch 2

New patch for y'all :) 2-space indent, and goodbye to DumpDocument (and thus
nsXFormsUtils.h and .cpp)

Comment 6

[Brian Ryner (not reading)]

Comment on attachment 159704 [details] [diff] [review]
MDG Patch 2

>--- ../xforms.orig/nsXFormsMDG.cpp  1970-01-01 01:00:00.000000000 +0100
>+++ ./nsXFormsMDG.cpp 2004-09-21 19:41:53.000000000 +0200
>+nsresult
>+nsXFormsMDG::CreateNewChild(nsCOMPtr<nsIDOMNode> aContextNode, const nsAString& aNodeValue,
>+                            nsCOMPtr<nsIDOMNode> aBeforeNode)

nsCOMPtr parameters should generally be avoided... especially the way it's
written here, it will copy the nsCOMPtr which will result in an extra
addref/release.  Just use the raw pointer type.  This applies for several of
the methods.

>+{
>+ nsresult rv;

please two-space indent, not one.

>+
>+ nsCOMPtr<nsIDOMDocument> document;
>+ rv = aContextNode->GetOwnerDocument(getter_AddRefs(document));
>+ NS_ENSURE_SUCCESS(rv, rv);
>+
>+ nsCOMPtr<nsIDOMText> textNode;
>+ rv = document->CreateTextNode(aNodeValue, getter_AddRefs(textNode));
>+ NS_ENSURE_SUCCESS(rv, rv);
>+ 
>+ nsCOMPtr<nsIDOMNode> newNode;
>+ if (aBeforeNode) {
>+   rv = aContextNode->InsertBefore(textNode, aBeforeNode, getter_AddRefs(newNode));
>+ } else {
>+   rv = aContextNode->AppendChild(textNode, getter_AddRefs(newNode));
>+ }
>+ NS_ENSURE_SUCCESS(rv, rv);
>+
>+ return NS_OK;

I think you can just return rv here.

>+nsresult
>+nsXFormsMDG::AddMIP(ModelItemPropName aType, nsCOMPtr<nsIDOMXPathExpression> aExpression,
>+                    nsCOMPtr<nsIDOMNode> aContextNode, PRInt32 aContextPos, PRInt32 aContextSize)
>+{
>+ NS_ENSURE_ARG(aExpression);
>+ NS_ENSURE_ARG(aContextNode);
>+ 
>+ nsVoidArray deps;
>+ printf("\tTODO: Get dependencies!\n");

This should be commented out before checkin.

>+nsresult
>+nsXFormsMDG::GetNodeValue(nsCOMPtr<nsIDOMNode> aContextNode, nsAString& aNodeValue)
>+{
...

>+ case nsIDOMNode::ELEMENT_NODE:
>+   rv = aContextNode->GetFirstChild(getter_AddRefs(childNode));

How is this method going to be used? I couldn't find any callers.  Depending on
how it will be used, you may want to consider nsIDOM3Node::GetTextContent here,
which will handle cases like:

<blah>some text<!-- comment -->more text</blah>

>+   if (NS_FAILED(rv) || !childNode) {
>+     // No child
>+     aNodeValue.Assign(NS_LITERAL_STRING(""));;

aNodeValue.Truncate(0);

>+   } else {
>+     PRUint16 childType;
>+     rv = childNode->GetNodeType(&childType);
>+     NS_ENSURE_SUCCESS(rv, rv);
>+ 
>+     if (   childType == nsIDOMNode::TEXT_NODE
>+         || childType == nsIDOMNode::CDATA_SECTION_NODE) {
>+       rv = childNode->GetNodeValue(aNodeValue);
>+       NS_ENSURE_SUCCESS(rv, rv);
>+     } else {
>+       // Not a text child
>+       aNodeValue.Assign(NS_LITERAL_STRING(""));

same.

>+nsresult
>+nsXFormsMDG::SetNodeValue(nsCOMPtr<nsIDOMNode> aContextNode, nsAString& aNodeValue, PRBool aMarkNode)
>+{
...
>+ case nsIDOMNode::ELEMENT_NODE:
>+   rv = aContextNode->GetFirstChild(getter_AddRefs(childNode));
>+   NS_ENSURE_SUCCESS(rv, rv);
>+
>+   if (!childNode) {
>+     rv = CreateNewChild(aContextNode, aNodeValue);
>+     NS_ENSURE_SUCCESS(rv, rv);

You can also use nsIDOM3Node::SetTextContent here, if that would be helpful.

>+PRBool
>+nsXFormsMDG::IsValid(nsCOMPtr<nsIDOMNode> aContextNode)
>+{
>+ 
>+ PRBool valid = mEngine.Test(aContextNode, MDG_FLAG_CONSTRAINT);
>+ if (valid == PR_TRUE) {
>+   printf("TODO: nsXFormsMDG::isValid() needs Schema support\n");

remove this before checking in

>+   // valid = mSchemaHandler->ValidateNode(aContextNode);
>+ }
>+ return valid;
>+}
>+
>+PRBool
>+nsXFormsMDG::IsRelevant(nsCOMPtr<nsIDOMNode> aContextNode)
>+{
>+ PRBool valid;
>+ if (   mEngine.Test(aContextNode, MDG_FLAG_RELEVANT) == PR_TRUE
>+     && mEngine.Test(aContextNode, MDG_FLAG_INHERITED_RELEVANT) == PR_TRUE) {

It might be good to make the values of the MDG_FLAG_* constants be the actual
bits, then you could do something like:

mEngine.Test(aContextNode, MDG_FLAG_RELEVANT | MDG_FLAG_INHERITED_RELEVANT)

if you change nsXFormsMDGEngine::Test to do this:

 return (GetFlag(aDomNode) & aFlagId == aFlagId)

You also avoid doing all the bit-shifting at run-time.

I'm up to nsXFormsMDGEngine.cpp, will add more comments later.

Comment 7

[Allan Beaufour]

Attachment: MDG Patch 3

Voila, latest patch.
* Ported our XPath analyzer to get the dependencies for the XPath expressions.
It should only be seen as a "showcase". It really needs to be implemented in
the Transformiix code.
* Implemented the review comments from Brian and Aaron.

Comment 8

[Brian Ryner (not reading)]

Comment on attachment 161009 [details] [diff] [review]
MDG Patch 3

>--- ../xforms.orig/nsXFormsMDGEngine.cpp	1970-01-01 01:00:00.000000000 +0100
>+++ ./nsXFormsMDGEngine.cpp	2004-10-04 15:15:30.961126336 +0200
>+PLDHashOperator
>+nsXFormsMDGEngine::DeleteLinkedNodes(const nsISupportsHashKey::KeyType aKey, nsAutoPtr<nsXFormsMDGNode>& aNode, void* aArg)
>+{

How about:

(const nsISupports *aKey, nsXFormsMDGNode *aNode, void *aArg)

The first argument you have there is technically equivalent, but the intent was
that you write the raw pointer form as it's a lot easier.  Not sure why the
second argument actually compiles as an nsAutoPtr.

>+nsresult
>+nsXFormsMDGEngine::Finalize()
>+{
>+  nsresult rv = NS_ERROR_FAILURE;
>+  if (mNodeToFlag.Init() && mNodeToMDG.Init()) {
>+    rv = NS_OK;

No point in setting rv here (or having it at all) if you're not going to do
anything with it.

>+  }
>+  
>+  return NS_OK;
>+}
>+
>+nsresult
>+nsXFormsMDGEngine::Insert(nsIDOMNode* aContextNode, nsIDOMXPathExpression* aExpression,
>+                          const nsXFormsMDGSet* aDependencies, PRBool aDynFunc,
>+                          PRInt32 aContextPos, PRInt32 aContextSize,
>+                          ModelItemPropName aType, ModelItemPropName aDepType)
>+{
...
>+  if (newnode->HasExpr()) {
>+    // MultipleMIPs
>+    return NS_ERROR_ABORT;

What does this mean, exactly? Multiple MIPs are legal... what case is this
handling?

>+PLDHashOperator 
>+nsXFormsMDGEngine::AddStartNodes(const nsISupportsHashKey::KeyType aKey, nsXFormsMDGNode* aNode, void* aDeque)

Same comment as above regarding the first parameter type.

>+  // Initial scan for nsXFormsMDGNodes with no dependencies (count == 0)
>+  PRUint32 entries = mNodeToMDG.EnumerateRead(AddStartNodes, &sortedNodes);
>+  if (entries != mNodeToMDG.Count()) {
>+    return NS_ERROR_OUT_OF_MEMORY;
>+  }
>+
>+#ifdef DEBUG_XF_MDG
>+  printf("\tStartNodes:    %d\n", sortedNodes.GetSize());
>+#endif
>+  
>+  nsXFormsMDGNode* g;
>+  nsXFormsMDGNode* n;
>+  void* nextNode = sortedNodes.Pop();

How about:

nsXFormsMDGNode *nextNode = NS_STATIC_CAST(nsXFormsMDGNode*,
sortedNodes.Pop());

and get rid of 'g'

>+  while (nextNode != nsnull) {
>+    g = NS_STATIC_CAST(nsXFormsMDGNode*, nextNode);
>+    if (!g) {
>+      return NS_ERROR_OUT_OF_MEMORY;

static_cast always succeeds, no need for this check.

>+    }
>+    for (PRInt32 i = 0; i < g->mSuc.Count(); ++i) {
>+      n = NS_STATIC_CAST(nsXFormsMDGNode*, g->mSuc[i]);

Scope 'n' inside this for block, and maybe rename it to 'node'.

>+      if (!n) {
>+        return NS_ERROR_OUT_OF_MEMORY;

This isn't an OOM condition or likely to ever happen (you don't allow appending
a null item to this array). I would just have an NS_ASSERTION.

>+      } else { // The nodes are becomming relevant

'becoming'

>+nsresult
>+nsXFormsMDGEngine::Calculate(nsXFormsMDGSet& aValueChanged)
>+{
>+#ifdef DEBUG_XF_MDG
>+  printf("nsXFormsMDGEngine::Calculcate(aValueChanged=|%d|)\n", aValueChanged.Count());
>+#endif
>+
>+  NS_ENSURE_TRUE(aValueChanged.AddSet(mMarkedNodes), NS_ERROR_OUT_OF_MEMORY);
>+
>+  mMarkedNodes.Clear();
>+  
>+  PRBool res = PR_TRUE;
>+  
>+  if (mJustRebuilded) {
>+    mFirstCalculate = PR_TRUE;
>+  } else {
>+    mFirstCalculate = PR_FALSE;
>+  }

mFirstCalculate = mJustRebuilded;

>+// TODO: This function needs to be called 
>+nsresult
>+nsXFormsMDGEngine::MarkNode(nsIDOMNode* aDomNode)
>+{
>+  SetFlagBits(aDomNode, MDG_FLAG_ALL_DISPATCH, PR_TRUE);
>+
>+  nsXFormsMDGNode* n = GetNode(aDomNode, eModel_calculate);
>+  if (n != nsnull) {
>+    n->MarkDirty();
>+  }
>+
>+  // Add constraint to trigger validation of node 
>+  n = GetNode(aDomNode, eModel_constraint, PR_FALSE);
>+  if (n == nsnull) {
>+    n = GetNode(aDomNode, eModel_constraint, PR_TRUE);
>+    if (!n) {

Try to be consistent with (!n) for null checks, vs (n == nsnull)

>--- ../xforms.orig/nsXFormsMDGEngine.h	1970-01-01 01:00:00.000000000 +0100
>+++ ./nsXFormsMDGEngine.h	2004-10-04 15:11:22.985824312 +0200
>+  /**
>+   * True when Rebuild() has been run, but not ClearDispatchFlags()
>+   */
>+  PRBool mJustRebuilded;

Can we make this mJustRebuilt, so it doesn't distract me when I'm reading this
code? :)

>--- ../xforms.orig/nsXFormsModelElement.cpp	2004-10-02 00:06:50.000000000 +0200
>+++ ./nsXFormsModelElement.cpp	2004-10-04 15:23:36.526309208 +0200
>@@ -501,6 +497,9 @@
> 
>   NS_ASSERTION(mContent, "Wrapper is not an nsIContent, we'll crash soon");
> 
>+  nsresult rv = mMDG.Finalize();
>+  NS_ENSURE_SUCCESS(rv, rv);
>+

Finalize seems like kind of an odd name for this, since it sounds like we're
finished with something.... Init() would be more readable for me.

I'm going to skip review on the XPath stuff since it sounds like it will be
rewritten.

I'm happy with this patch other than the above remarks... fix those and I'll r+
and check it in.

Comment 9

[Allan Beaufour]

(In reply to comment #8)
> (From update of attachment 161009 [details] [diff] [review])
> >--- ../xforms.orig/nsXFormsMDGEngine.cpp	1970-01-01 01:00:00.000000000 +0100
> >+++ ./nsXFormsMDGEngine.cpp	2004-10-04 15:15:30.961126336 +0200
> >+PLDHashOperator
> >+nsXFormsMDGEngine::DeleteLinkedNodes(const nsISupportsHashKey::KeyType aKey,
nsAutoPtr<nsXFormsMDGNode>& aNode, void* aArg)
> >+{
> 
> How about:
> 
> (const nsISupports *aKey, nsXFormsMDGNode *aNode, void *aArg)
> 
> The first argument you have there is technically equivalent, but the intent was
> that you write the raw pointer form as it's a lot easier.  Not sure why the
> second argument actually compiles as an nsAutoPtr.

Hmm, the datatype needed for the Enumerate function is:
PRUint32 
   nsBaseHashtable<KeyClass, DataType, UserDataType>::Enumerate(PLDHashOperator 
   (*)(typename KeyClass::KeyType, DataType&, void*), void*) [with KeyClass = 
   nsISupportsHashKey, DataType = nsAutoPtr<nsXFormsMDGNode>, UserDataType = 
   nsXFormsMDGNode*]

Which is exactly what I have. Am I missing something here?

> >+nsresult
> >+nsXFormsMDGEngine::Finalize()
> >+{
> >+  nsresult rv = NS_ERROR_FAILURE;
> >+  if (mNodeToFlag.Init() && mNodeToMDG.Init()) {
> >+    rv = NS_OK;
> 
> No point in setting rv here (or having it at all) if you're not going to do
> anything with it.
> >+  }
> >+  
> >+  return NS_OK;
> >+}

:) Correct, "return rv;" of course.

> >+nsresult
> >+nsXFormsMDGEngine::Insert(nsIDOMNode* aContextNode, nsIDOMXPathExpression*
aExpression,
> >+                          const nsXFormsMDGSet* aDependencies, PRBool aDynFunc,
> >+                          PRInt32 aContextPos, PRInt32 aContextSize,
> >+                          ModelItemPropName aType, ModelItemPropName aDepType)
> >+{
> ...
> >+  if (newnode->HasExpr()) {
> >+    // MultipleMIPs
> >+    return NS_ERROR_ABORT;
> 
> What does this mean, exactly? Multiple MIPs are legal... what case is this
> handling?

Multiple MIP of the same type is not allowed, as in "<bind nodeset="x"
required="true()" required="false()"/>" Added a better comment.

> >+PLDHashOperator 
> >+nsXFormsMDGEngine::AddStartNodes(const nsISupportsHashKey::KeyType aKey,
nsXFormsMDGNode* aNode, void* aDeque)
> 
> Same comment as above regarding the first parameter type.

Same answer :)

> >+  // Initial scan for nsXFormsMDGNodes with no dependencies (count == 0)
> >+  PRUint32 entries = mNodeToMDG.EnumerateRead(AddStartNodes, &sortedNodes);
> >+  if (entries != mNodeToMDG.Count()) {
> >+    return NS_ERROR_OUT_OF_MEMORY;
> >+  }
> >+
> >+#ifdef DEBUG_XF_MDG
> >+  printf("\tStartNodes:    %d\n", sortedNodes.GetSize());
> >+#endif
> >+  
> >+  nsXFormsMDGNode* g;
> >+  nsXFormsMDGNode* n;
> >+  void* nextNode = sortedNodes.Pop();
> 
> How about:
> 
> nsXFormsMDGNode *nextNode = NS_STATIC_CAST(nsXFormsMDGNode*,
> sortedNodes.Pop());
> 
> and get rid of 'g'

Check.

> >+  while (nextNode != nsnull) {
> >+    g = NS_STATIC_CAST(nsXFormsMDGNode*, nextNode);
> >+    if (!g) {
> >+      return NS_ERROR_OUT_OF_MEMORY;
> 
> static_cast always succeeds, no need for this check.

I dunno why I had that check... it's gone.

> >+    }
> >+    for (PRInt32 i = 0; i < g->mSuc.Count(); ++i) {
> >+      n = NS_STATIC_CAST(nsXFormsMDGNode*, g->mSuc[i]);
> 
> Scope 'n' inside this for block, and maybe rename it to 'node'.

Hmm, I'm not to fond of having declarations inside loops. Maybe I'm too
oldfashioned? I renamed the variables.

> >+      if (!n) {
> >+        return NS_ERROR_OUT_OF_MEMORY;
> 
> This isn't an OOM condition or likely to ever happen (you don't allow appending
> a null item to this array). I would just have an NS_ASSERTION.

Check.

> >+      } else { // The nodes are becomming relevant
> 
> 'becoming'

Check.

> >+nsresult
> >+nsXFormsMDGEngine::Calculate(nsXFormsMDGSet& aValueChanged)
> >+{
> >+#ifdef DEBUG_XF_MDG
> >+  printf("nsXFormsMDGEngine::Calculcate(aValueChanged=|%d|)\n",
aValueChanged.Count());
> >+#endif
> >+
> >+  NS_ENSURE_TRUE(aValueChanged.AddSet(mMarkedNodes), NS_ERROR_OUT_OF_MEMORY);
> >+
> >+  mMarkedNodes.Clear();
> >+  
> >+  PRBool res = PR_TRUE;
> >+  
> >+  if (mJustRebuilded) {
> >+    mFirstCalculate = PR_TRUE;
> >+  } else {
> >+    mFirstCalculate = PR_FALSE;
> >+  }
> 
> mFirstCalculate = mJustRebuilded;

Wow, I'm blind :)

> >+// TODO: This function needs to be called 
> >+nsresult
> >+nsXFormsMDGEngine::MarkNode(nsIDOMNode* aDomNode)
> >+{
> >+  SetFlagBits(aDomNode, MDG_FLAG_ALL_DISPATCH, PR_TRUE);
> >+
> >+  nsXFormsMDGNode* n = GetNode(aDomNode, eModel_calculate);
> >+  if (n != nsnull) {
> >+    n->MarkDirty();
> >+  }
> >+
> >+  // Add constraint to trigger validation of node 
> >+  n = GetNode(aDomNode, eModel_constraint, PR_FALSE);
> >+  if (n == nsnull) {
> >+    n = GetNode(aDomNode, eModel_constraint, PR_TRUE);
> >+    if (!n) {
> 
> Try to be consistent with (!n) for null checks, vs (n == nsnull)

Check. Should be fixed now.

> >--- ../xforms.orig/nsXFormsMDGEngine.h	1970-01-01 01:00:00.000000000 +0100
> >+++ ./nsXFormsMDGEngine.h	2004-10-04 15:11:22.985824312 +0200
> >+  /**
> >+   * True when Rebuild() has been run, but not ClearDispatchFlags()
> >+   */
> >+  PRBool mJustRebuilded;
> 
> Can we make this mJustRebuilt, so it doesn't distract me when I'm reading this
> code? :)

:) Yes.

> >--- ../xforms.orig/nsXFormsModelElement.cpp	2004-10-02 00:06:50.000000000 +0200
> >+++ ./nsXFormsModelElement.cpp	2004-10-04 15:23:36.526309208 +0200
> >@@ -501,6 +497,9 @@
> > 
> >   NS_ASSERTION(mContent, "Wrapper is not an nsIContent, we'll crash soon");
> > 
> >+  nsresult rv = mMDG.Finalize();
> >+  NS_ENSURE_SUCCESS(rv, rv);
> >+
> 
> Finalize seems like kind of an odd name for this, since it sounds like we're
> finished with something.... Init() would be more readable for me.

Well, a question of terminology, but Init() seems like the Mozilla-way, I'll
change it.

> I'm going to skip review on the XPath stuff since it sounds like it will be
> rewritten.
> 
> I'm happy with this patch other than the above remarks... fix those and I'll r+
> and check it in.

Sounds good!

I'll fix the above, a mem leak, and then upload a new patch.

Comment 10

[Allan Beaufour]

Attachment: MDG Patch 4

Review comments integrated and mem leak fixed.

Comment 11

[Brian Ryner (not reading)]

(In reply to comment #9)
> Hmm, the datatype needed for the Enumerate function is:
> PRUint32 
>    nsBaseHashtable<KeyClass, DataType, UserDataType>::Enumerate(PLDHashOperator 
>    (*)(typename KeyClass::KeyType, DataType&, void*), void*) [with KeyClass = 
>    nsISupportsHashKey, DataType = nsAutoPtr<nsXFormsMDGNode>, UserDataType = 
>    nsXFormsMDGNode*]
> 
> Which is exactly what I have. Am I missing something here?

Well, in nsHashKeys.h, it has:

class NS_COM nsISupportsHashKey : public PLDHashEntryHdr
{
public:
  typedef nsISupports* KeyType;

So, the intent is that you can use a "natural" parameter type for the key.  For
nsISupportsHashKey, nsISupports*; for nsStringHashKey, const nsAString&.  It's a
lot more readable, and also lets you make sure the parameter type is what you
think it is.  (especially when const / references are involved)

> > >+    }
> > >+    for (PRInt32 i = 0; i < g->mSuc.Count(); ++i) {
> > >+      n = NS_STATIC_CAST(nsXFormsMDGNode*, g->mSuc[i]);
> > 
> > Scope 'n' inside this for block, and maybe rename it to 'node'.
> 
> Hmm, I'm not to fond of having declarations inside loops. Maybe I'm too
> oldfashioned? I renamed the variables.
> 

It's nice to scope variables at the smallest scope where they're used so that
you don't make a mistake using the variable later in the method.  This is the
normal coding style for mozilla.

Comment 12

[Allan Beaufour]

(In reply to comment #11)
> (In reply to comment #9)
> > Hmm, the datatype needed for the Enumerate function is:
> > PRUint32 
> >    nsBaseHashtable<KeyClass, DataType, UserDataType>::Enumerate(PLDHashOperator 
> >    (*)(typename KeyClass::KeyType, DataType&, void*), void*) [with KeyClass = 
> >    nsISupportsHashKey, DataType = nsAutoPtr<nsXFormsMDGNode>, UserDataType = 
> >    nsXFormsMDGNode*]
> > 
> > Which is exactly what I have. Am I missing something here?
> 
> Well, in nsHashKeys.h, it has:
> 
> class NS_COM nsISupportsHashKey : public PLDHashEntryHdr
> {
> public:
>   typedef nsISupports* KeyType;
> 
> So, the intent is that you can use a "natural" parameter type for the key.

Why have a typedef, and furthermore use it in function signatures, etc. when you
are not supposed to use it? I strongly disagree on that one. I think using the
typedef is the most elegant and clean way to use it.

> For
> nsISupportsHashKey, nsISupports*; for nsStringHashKey, const nsAString&.  It's a
> lot more readable, and also lets you make sure the parameter type is what you
> think it is.  (especially when const / references are involved)

Yes, and in nsBaseHashtable.h the EnumerateRead function signature is:
  typedef PLDHashOperator
    (*PR_CALLBACK EnumReadFunction)(KeyType      aKey,
                                    UserDataType aData,
                                    void*        userArg);

And this is what my compiler expects, if you can convince it otherwise go ahead.  

> > > >+    }
> > > >+    for (PRInt32 i = 0; i < g->mSuc.Count(); ++i) {
> > > >+      n = NS_STATIC_CAST(nsXFormsMDGNode*, g->mSuc[i]);
> > > 
> > > Scope 'n' inside this for block, and maybe rename it to 'node'.
> > 
> > Hmm, I'm not to fond of having declarations inside loops. Maybe I'm too
> > oldfashioned? I renamed the variables.
> > 
> 
> It's nice to scope variables at the smallest scope where they're used so that
> you don't make a mistake using the variable later in the method.  This is the
> normal coding style for mozilla.

Depending on the variable type it could also be re-initialized in each
iteration, which could be quite unnecessary and expensive. But in the case of a
pointer, no harm done there :) It is changed now. But in general I am not to
fond of it, but if it is "the Mozilla way" ... once again I'll change the old
habits. I'll change the rest of the code, but after check in, ok?

Comment 13

[Allan Beaufour]

Attachment: MDG Patch 5

* Fixed the declaration and names in nsXFormsMDGEngine::Rebuild()
* Brought up to date with current CVS
* Updated nsXFormsXPathXMLUtil to latest internal version

Comment 14

[Brian Ryner (not reading)]

Checked in, with the change of |const nsISupportsHashKey::KeyType| to
|nsISupports*| that we discussed.