329479 - default namespace serialization issue per submission

Status: RESOLVED FIXED

(Core Graveyard :: XForms, defect)

Description

[alexander :surkov (:asurkov)]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9a1) Gecko/20051212 Firefox/1.6a1
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9a1) Gecko/20051212 Firefox/1.6a1

The intitialiy problem was described in bug 326994. I can see it is in the following behaviour: we should support #default value of @includenamespaceprefixes and we shouldn't add default namespaces from top elements (like instance, model and so on).

Reproducible: Always

Comment 1

[alexander :surkov (:asurkov)]

Attachment: patch

Comment 2

[Allan Beaufour]

Comment on attachment 214165 [details] [diff] [review]
patch

(In reply to comment #1)
> Created an attachment (id=214165) [edit]
> patch

Please add some testcases, including an explanation of current (wrong) behaviour, and what is right behaviour.

Then ask for review again

Comment 3

[alexander :surkov (:asurkov)]

Attachment: patch2

Comment 4

[alexander :surkov (:asurkov)]

Attachment: testcase

Comment 5

[alexander :surkov (:asurkov)]

I changed some names of local variables and changed order of namespace inheritance too.

Comment 6

[Allan Beaufour]

(In reply to comment #4)
> Created an attachment (id=214523) [edit]
> testcase

Nice testcase, but I do not agree with it.
"The default behavior is that every namespace node is serialized according to the rules of the XML output method, so that at least one namespace declaration appears in the serialized XML for each in-scope namespace."
[http://www.w3.org/TR/2005/PER-xforms-20051006/slice11.html#serialize-xml]

That means the default namespace too. So xmlns="http://www.w3.org/1999/xhtml" will be on all submit documents except for submit3. Agree?

Comment 7

[Allan Beaufour]

(In reply to comment #6)
> (In reply to comment #4)
> > Created an attachment (id=214523) [edit]
> > testcase
> 
> Nice testcase, but I do not agree with it.
> "The default behavior is that every namespace node is serialized according to
> the rules of the XML output method, so that at least one namespace declaration
> appears in the serialized XML for each in-scope namespace."
> [http://www.w3.org/TR/2005/PER-xforms-20051006/slice11.html#serialize-xml]
> 
> That means the default namespace too. So xmlns="http://www.w3.org/1999/xhtml"
> will be on all submit documents except for submit3. Agree?

Not on submit2 either, so I guess my only problem is with submit1.

Comment 8

[alexander :surkov (:asurkov)]

(In reply to comment #7)
> (In reply to comment #6)
> > (In reply to comment #4)
> > > Created an attachment (id=214523) [edit]
> > > testcase
> > 
> > Nice testcase, but I do not agree with it.
> > "The default behavior is that every namespace node is serialized according to
> > the rules of the XML output method, so that at least one namespace declaration
> > appears in the serialized XML for each in-scope namespace."
> > [http://www.w3.org/TR/2005/PER-xforms-20051006/slice11.html#serialize-xml]
> > 
> > That means the default namespace too. So xmlns="http://www.w3.org/1999/xhtml"
> > will be on all submit documents except for submit3. Agree?
> 
> Not on submit2 either, so I guess my only problem is with submit1.
> 

It's good question. The things what confuse me are:
1) I can't manage default namespace declaration inclusion. I guess it's w3c omission.
2) What default namespace declaration should be included (topest or lowest)?
3) There are a cases when default namespace are not included (f.x. submit2 or submit3)
4) I can't see utility of default namespace inclusion.

Really we need in default namespace inheritance from top elements? What do other xforms processors do?

Comment 9

[alexander :surkov (:asurkov)]

Attachment: patch [allan's comment]

Comment 10

[Allan Beaufour]

Attachment: Revised testcase

Comment 11

[alexander :surkov (:asurkov)]

The bug 327730 is related with the one. I guess we can mark it as dublicate though I'm not sure completely (please look at second testcase from the mentioned bug).

Comment 12

[Allan Beaufour]

Comment on attachment 214989 [details] [diff] [review]
patch [allan's comment]

Please use -p when you diff

> diff -uNra mozilla.orig/extensions/xforms/nsXFormsSubmissionElement.cpp mozilla/extensions/xforms/nsXFormsSubmissionElement.cpp

> @@ -912,24 +898,57 @@
>        NS_ENSURE_SUCCESS(rv, rv);
>      }
 
> -    // handle namespace on main document
> -    rv = AddNameSpaces(newDocElm, node, prefixHash);
> +    // get the document element of the document we are going to submit
> +    nsCOMPtr<nsIDOMElement> submDocElm;
> +    submDoc->GetDocumentElement(getter_AddRefs(submDocElm));

null-check submDocElm

> +
> +    // if submission document has empty default namespace attribute and if 
> +    // @includenamespaceprifixes attribute doesn't contain "#default" value then
> +    // we should remove default namespace attribute (see the specs 11.3).
> +    PRBool hasXMLNSAttr = PR_FALSE;
> +    submDocElm->HasAttributeNS(kXMLNSNameSpaceURI, NS_LITERAL_STRING("xmlns"),
> +                               &hasXMLNSAttr);

Skip this step, and just do GetAttributeNS and check whether it is empty.

> -    // handle namespaces on the xforms:instance
> -    rv = AddNameSpaces(newDocElm, instanceNode, prefixHash);
> -    NS_ENSURE_SUCCESS(rv, rv);
> +    // first handle the main document

? this comment should just be killed right, and the comment below moved up here?

> +    nsCOMPtr<nsIDOMDocument> mainDoc;
> +    mElement->GetOwnerDocument(getter_AddRefs(mainDoc));
> +    NS_ENSURE_TRUE(mainDoc, NS_ERROR_UNEXPECTED);
> +
> +    nsCOMPtr<nsIDOMElement> mainDocElm;
> +    mainDoc->GetDocumentElement(getter_AddRefs(mainDocElm));
> +    nsCOMPtr<nsIDOMNode> mainDocNode(do_QueryInterface(mainDocElm));
> +    NS_ENSURE_TRUE(mainDocNode, NS_ERROR_UNEXPECTED);
 
> -    // handle namespaces on the root element of the instance document
> -    doc->GetDocumentElement(getter_AddRefs(docElm));
> -    rv = AddNameSpaces(newDocElm, docElm, prefixHash);
> +    // handle namespace on main document
> +    rv = AddNameSpaces(submDocElm, mainDocNode, prefixHash);
>      NS_ENSURE_SUCCESS(rv, rv);
 
> @@ -1055,18 +1074,25 @@
>        attrNode->GetLocalName(localName);
>        attrNode->GetNodeValue(value);
 
> -      attrName.AssignLiteral("xmlns");
> -      // xmlns:foo, not xmlns=
>        if (!localName.EqualsLiteral("xmlns")) {
> -        // If "includenamespaceprefixes" attribute is present, don't add
> -        // namespace unless it appears in hash
> -        if (aPrefixHash && !aPrefixHash->Contains(localName))
> -          continue;
> -        attrName.AppendLiteral(":");
> -        attrName.Append(localName);
> +        if (!aPrefixHash || aPrefixHash->Contains(localName)) {
> +          nsAutoString attrName(NS_LITERAL_STRING("xmlns:"));
> +          attrName.Append(localName);
> +          aTarget->SetAttributeNS(kXMLNSNameSpaceURI, attrName, value);
> +        }
> +      } else if (!value.IsEmpty()) {
> +        // if aTarget default namespace uri isn't empty and it hasn't default
> +        // namespace attribute then we should add it.
> +        aTarget->GetNamespaceURI(nsURI);
> +        if (!nsURI.IsEmpty()) {
> +          PRBool hasDefaultNSAttr;
> +          aTarget->HasAttributeNS(kXMLNSNameSpaceURI, NS_LITERAL_STRING("xmlns"), 
> +                                  &hasDefaultNSAttr);
> +          if (!hasDefaultNSAttr) {
> +            aTarget->SetAttributeNS(kXMLNSNameSpaceURI, localName, value);
> +          }
> +        }

Can you not end up by doing the last if () section a lot of times? The first time it succeeds, you do not need to do it anymore. For all runs of AddNameSpaces().

Please fix the W and L errors:
http://beaufour.dk/jst-review/?patch=214989

BTW. Our namespace handling in general is sub-optimal... I just filed bug 330557.

Comment 13

[alexander :surkov (:asurkov)]

Attachment: patch3

Comment 14

[alexander :surkov (:asurkov)]

> Can you not end up by doing the last if () section a lot of times? The first
> time it succeeds, you do not need to do it anymore. For all runs of
> AddNameSpaces().

I don't prefer modification of arguments list of AddNameSpaces(). I don't see how it can be done by other way. Maybe isn't the approach so ugly? Especially the code will be executed one time only on host document element almost for all cases :).

Comment 15

[Allan Beaufour]

Comment on attachment 215203 [details] [diff] [review]
patch3

>+    instDoc = do_QueryInterface(data);
>+    NS_ENSURE_TRUE(instDoc, NS_ERROR_UNEXPECTED);

I prefer NS_ENSURE_STATE(instDoc), but I can see that the above is used other places in the file... it's up to you.

r=me

Comment 16

[alexander :surkov (:asurkov)]

Attachment: patch4

(In reply to comment #15)
> (From update of attachment 215203 [details] [diff] [review] [edit])
> >+    instDoc = do_QueryInterface(data);
> >+    NS_ENSURE_TRUE(instDoc, NS_ERROR_UNEXPECTED);
> 
> I prefer NS_ENSURE_STATE(instDoc), but I can see that the above is used other
> places in the file... it's up to you.
> 

Fixed.

Comment 17

[Doron Rosenberg (IBM)]

Comment on attachment 215881 [details] [diff] [review]
patch4

> 
>-    // handle namespace on main document
>-    rv = AddNameSpaces(newDocElm, node, prefixHash);
>+    // get the document element of the document we are going to submit
>+    nsCOMPtr<nsIDOMElement> submDocElm;
>+    submDoc->GetDocumentElement(getter_AddRefs(submDocElm));
>+    NS_ENSURE_STATE(submDocElm);
>+
>+    // if submission document has empty default namespace attribute and if
>+    // @includenamespaceprifixes attribute doesn't contain "#default" value then
>+    // we should remove default namespace attribute (see the specs 11.3).

prefix, not prifix

Comment 18

[alexander :surkov (:asurkov)]

Attachment: patch5

Comment 19

[Allan Beaufour]

Checked into trunk.