Closed Bug 329479 Opened 15 years ago Closed 15 years ago

default namespace serialization issue per submission

Categories

(Core Graveyard :: XForms, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: surkov, Assigned: surkov)

References

Details

(Keywords: fixed1.8.0.4, fixed1.8.1)

Attachments

(2 files, 6 obsolete files)

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
Attached patch patch (obsolete) — Splinter Review
Attachment #214165 - Flags: review?(allan)
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
Attachment #214165 - Flags: review?(allan)
Attached patch patch2 (obsolete) — Splinter Review
Attachment #214165 - Attachment is obsolete: true
Attachment #214522 - Flags: review?(allan)
Attached file testcase (obsolete) —
I changed some names of local variables and changed order of namespace inheritance too.
Assignee: aaronr → surkov
(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?
(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.
(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?
Attached patch patch [allan's comment] (obsolete) — Splinter Review
Attachment #214522 - Attachment is obsolete: true
Attachment #214989 - Flags: review?(allan)
Attachment #214522 - Flags: review?(allan)
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Attached file Revised testcase
Attachment #214523 - Attachment is obsolete: true
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 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.
Attachment #214989 - Flags: review?(allan) → review-
Blocks: 327730
Attached patch patch3 (obsolete) — Splinter Review
Attachment #214989 - Attachment is obsolete: true
Attachment #215203 - Flags: review?(allan)
> 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 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
Attachment #215203 - Flags: review?(allan) → review+
Attached patch patch4 (obsolete) — Splinter Review
(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.
Attachment #215203 - Attachment is obsolete: true
Attachment #215881 - Flags: review?(doronr)
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
Attachment #215881 - Flags: review?(doronr) → review+
Attached patch patch5Splinter Review
Attachment #215881 - Attachment is obsolete: true
Checked into trunk.
Status: ASSIGNED → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
Whiteboard: xf-to-branch
Blocks: 332853
Whiteboard: xf-to-branch
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.