nsSchemaAttribute* needs to store the form attribute

RESOLVED FIXED

Status

Core Graveyard
Web Services
RESOLVED FIXED
12 years ago
29 days ago

People

(Reporter: Doron Rosenberg (IBM), Assigned: Doron Rosenberg (IBM))

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

15.47 KB, patch
peterv
: review+
peterv
: superreview+
Benjamin Smedberg
: approval1.8.1-
Details | Diff | Splinter Review
(Assignee)

Description

12 years ago
Schemas can define if attributed need to be qualified or not, which is important
for schema validation.
(Assignee)

Comment 1

12 years ago
Created attachment 195926 [details] [diff] [review]
patch
Attachment #195926 - Flags: review?(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?
(Assignee)

Comment 3

12 years ago
(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?

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); ...|.
(Assignee)

Updated

12 years ago
Attachment #195926 - Attachment is obsolete: true
Attachment #195926 - Flags: review?(peterv)
(Assignee)

Comment 5

12 years ago
Created attachment 199092 [details] [diff] [review]
patch

Indeed, makes more sense your way.  Here is the patch.
Attachment #199092 - Flags: review?(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.
Attachment #199092 - Flags: superreview+
Attachment #199092 - Flags: review?(peterv)
Attachment #199092 - Flags: review+
(Assignee)

Comment 7

12 years ago
checked in.  Probably want this for the 1.8 branch for xforms.
Whiteboard: xf-to-branch
(Assignee)

Comment 8

12 years ago
mkaply - would this be allowed into 1.8.x?

Comment 9

12 years ago
Yes, assuming this isn't built by default.
(Assignee)

Comment 10

12 years ago
Actually, this is built by default, part of webservices.

Comment 11

12 years ago
request approval. Give a really good reason :)
(Assignee)

Comment 12

12 years ago
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.
Attachment #199092 - Flags: approval1.8.1?

Comment 13

12 years ago
Comment on attachment 199092 [details] [diff] [review]
patch

Cannot change existing XPCOM interfaces on the 1.8 branch.
Attachment #199092 - Flags: approval1.8.1? → approval1.8.1-
(Assignee)

Updated

12 years ago
Status: NEW → ASSIGNED
(Assignee)

Updated

12 years ago
Status: ASSIGNED → RESOLVED
Last Resolved: 12 years ago
Resolution: --- → FIXED

Comment 14

11 years ago
removing xf-to-branch since it sounds like this will never be allowed into branch.
Whiteboard: xf-to-branch

Updated

29 days ago
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.