308372 - nsSchemaAttribute* needs to store the form attribute

Status: RESOLVED FIXED

(Core Graveyard :: Web Services, defect)

Description

[Doron Rosenberg (IBM)]

Schemas can define if attributed need to be qualified or not, which is important
for schema validation.

Comment 1

[Doron Rosenberg (IBM)]

Attachment: patch

Comment 2

[Peter Van der Beken [:peterv]]

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

>Index: extensions/webservices/public/nsISchema.idl
>===================================================================
>Index: extensions/webservices/schema/src/nsSchema.cpp
>===================================================================

>+    if (attributeFormDefault.EqualsLiteral("qualified")) {
>+      mAttributeFormDefaultQualified = PR_TRUE;
>+    } else {
>+      // default is unqualified
>+      mAttributeFormDefaultQualified = PR_FALSE;
>+    }

I'd make that:
    mAttributeFormDefaultQualified =
attributeFormDefault.EqualsLiteral("qualified");

I wonder whether we want to expose that as attributeFormQualified on
nsISchemaAttribute. Wouldn't adding targetNamespace to nsISchemaAttribute be
enough?

Comment 3

[Doron Rosenberg (IBM)]

(In reply to comment #2)
> (From update of attachment 195926 [details] [diff] [review] [edit])
> >Index: extensions/webservices/public/nsISchema.idl
> >===================================================================
> >Index: extensions/webservices/schema/src/nsSchema.cpp
> >===================================================================
> 
> >+    if (attributeFormDefault.EqualsLiteral("qualified")) {
> >+      mAttributeFormDefaultQualified = PR_TRUE;
> >+    } else {
> >+      // default is unqualified
> >+      mAttributeFormDefaultQualified = PR_FALSE;
> >+    }
> 
> I'd make that:
>     mAttributeFormDefaultQualified =
> attributeFormDefault.EqualsLiteral("qualified");
> 
> I wonder whether we want to expose that as attributeFormQualified on
> nsISchemaAttribute. Wouldn't adding targetNamespace to nsISchemaAttribute be
> enough?
> 

So if the attribute is not to be qualified, we would set an empty
targetNamepace?  Would increase the size of the object (string vs boolean), right?

Would also require us to figure the right namespace on parsing of the schema.  I
think that would work fine too, not sure if we really care about the size increase?

Comment 4

[Peter Van der Beken [:peterv]]

I'm not talking about how to store the value (a boolean seems fine to me), but
how to expose it. When getting the boolean value in this patch you always have
access to the concrete classes, so there's no real need for it to be on the
interface. You didn't add the code that actually uses this API, but it seems to
me that when using this API a |readonly attribute AString targetNamespace| on
nsISchemaAttribute would be more useful. nsSchemaAttribute::GetTargetNamespace
would just be |if (mAttributeFormQualified && mSchema) return
mSchema->GetTargetNamespace(aTargetNamespace); ...|.

Comment 5

[Doron Rosenberg (IBM)]

Attachment: patch

Indeed, makes more sense your way.  Here is the patch.

Comment 6

[Peter Van der Beken [:peterv]]

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

>Index: extensions/webservices/public/nsISchema.idl
>===================================================================

>+[scriptable, uuid(236ce1d7-213e-42e7-96ad-79f4cb3282a9)]
> interface nsISchema : nsISchemaComponent {
>   /* Is this necessary? */
>   readonly attribute AString schemaNamespace;
>+  readonly attribute boolean attributeFormDefaultQualified;

No need for this (you have IsAttributeFormDefaultQualified), unless you need this outside of schema code?

>Index: extensions/webservices/schema/src/nsSchema.cpp
>===================================================================

>@@ -58,20 +58,33 @@ nsSchema::nsSchema(nsISchemaCollection* 

>+    if (attributeFormDefault.EqualsLiteral("qualified")) {
>+      mAttributeFormDefaultQualified = PR_TRUE;
>+    } else {
>+      // default is unqualified
>+      mAttributeFormDefaultQualified = PR_FALSE;
>+    }

    // default is unqualified
    mAttributeFormDefaultQualified =
      attributeFormDefault.EqualsLiteral("qualified");

>Index: extensions/webservices/schema/src/nsSchemaAttributes.cpp
>===================================================================

>@@ -155,20 +155,32 @@ nsSchemaAttribute::GetFixedValue(nsAStri

>+/* readonly attribute AString targetNamespace */
>+NS_IMETHODIMP
>+nsSchemaAttribute::GetQualifiedNamespace(nsAString & aTargetNamespace)
>+{
>+  if (mSchema && mAttributeFormQualified)
>+    mSchema->GetTargetNamespace(aTargetNamespace);
>+  else
>+    aTargetNamespace.AssignLiteral("");

Truncate()

>@@ -183,20 +195,28 @@ nsSchemaAttribute::SetConstraints(const 

>+NS_IMETHODIMP

Just nsresult

>+nsSchemaAttribute::SetAttributeFormQualified(PRBool aAttributeFormQualified)

>@@ -316,38 +336,58 @@ nsSchemaAttributeRef::GetFixedValue(nsAS

>+NS_IMETHODIMP
>+nsSchemaAttributeRef::GetQualifiedNamespace(nsAString & aTargetNamespace)
>+{
>+  if (mSchema && mAttributeFormQualified)
>+    mSchema->GetTargetNamespace(aTargetNamespace);
>+  else
>+    aTargetNamespace.AssignLiteral("");

Truncate()

>+NS_IMETHODIMP

Just nsresult

>+nsSchemaAttributeRef::SetAttributeFormQualified(PRBool aAttributeFormQualified)

>Index: extensions/webservices/schema/src/nsSchemaLoader.cpp
>===================================================================

>@@ -2784,57 +2784,82 @@ nsSchemaLoader::ProcessAttributeComponen

>+    // set the qualified form
>+    if (formValue.EqualsLiteral("qualified")) {
>+      attributeRef->SetAttributeFormQualified(PR_TRUE);
>+    } else if (formValue.EqualsLiteral("unqualified")) {


I think the style in these files is:

}
else {


>+      attributeRef->SetAttributeFormQualified(PR_FALSE);
>+    } else {

}
else {

>+      // get default
>+      PRBool defaultvalue;
>+      aSchema->GetAttributeFormDefaultQualified(&defaultvalue);

    PRBool defaultvalue = aSchema->IsAttributeFormDefaultQualified();


>+    // set the qualified form
>+    if (formValue.EqualsLiteral("qualified")) {
>+      attributeInst->SetAttributeFormQualified(PR_TRUE);
>+    } else if (formValue.EqualsLiteral("unqualified")) {

}
else if {

>+      attributeInst->SetAttributeFormQualified(PR_FALSE);
>+    } else {

}
else {

>+      // get default
>+      PRBool defaultvalue;
>+      aSchema->GetAttributeFormDefaultQualified(&defaultvalue);

    PRBool defaultvalue = aSchema->IsAttributeFormDefaultQualified();

>Index: extensions/webservices/schema/src/nsSchemaPrivate.h
>===================================================================

>+  PRBool IsAttributeFormDefaultQualified() { return mAttributeFormDefaultQualified; }

Make it |PRBool IsAttributeFormDefaultQualified() const|.

>@@ -428,52 +430,56 @@ public:

>+  NS_IMETHOD SetAttributeFormQualified(PRBool aAttributeFormQualified);

Just nsresult.

>+  NS_IMETHOD SetAttributeFormQualified(PRBool aAttributeFormQualified);

Just nsresult.

Comment 7

[Doron Rosenberg (IBM)]

checked in.  Probably want this for the 1.8 branch for xforms.

Comment 8

[Doron Rosenberg (IBM)]

mkaply - would this be allowed into 1.8.x?

Comment 9

[Mike Kaply [:mkaply]]

Yes, assuming this isn't built by default.

Comment 10

[Doron Rosenberg (IBM)]

Actually, this is built by default, part of webservices.

Comment 11

[Mike Kaply [:mkaply]]

request approval. Give a really good reason :)

Comment 12

[Doron Rosenberg (IBM)]

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

XForms will need this, and since 1.8.x will be the next release, would be nice to get this.  Low risk, the code itself is only used by webservices in release builds and won't break anything.

Comment 13

[Benjamin Smedberg]

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

Cannot change existing XPCOM interfaces on the 1.8 branch.

Comment 14

[aaronr]

removing xf-to-branch since it sounds like this will never be allowed into branch.