331984 - input/secret/textarea should not be allowed to bind to xsd:base64Binary or xsd:hexBinary

Status: RESOLVED FIXED

(Core Graveyard :: XForms, defect)

Description

[Steve Speicher]

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

Per 1.0 2nd ed, http://www.w3.org/TR/xforms/index-all.html#ui-input
Data Binding Restrictions: Binds to any simpleContent (except xsd:base64Binary, xsd:hexBinary or any datatype derived from these).

This fails test cases: 8.1.2b, 8.1.3b, 8.1.4b

Spec isn't completely clear on what should happen, seems logical that xforms-binding-exception should be dispatched or at least message in JS console.

Testing shows:
  X-Smiles: fatal error (Java exception dump)
  formsPlayer 1: xforms-link-error ?
  Novell IE: supports validation against these types


Reproducible: Always

Steps to Reproduce:

Comment 1

[Steve Speicher]

Attachment: test case

Comment 2

[Allan Beaufour]

So I am guessing, that this will not make it to 0.6?

Comment 3

[aaronr]

I'd say we can safely slip it.  Probably not holding many people up from developing forms :-)  We could put in a quick change to the .css to do display: none for inputs with a moztype equal to these.  But we probably need to hammer out our derived type UI support to fix this properly.

Comment 4

[aaronr]

should be able to be fixed with bug 313313 and bug 316691 fixed

Comment 5

[aaronr]

(In reply to comment #4)
> should be able to be fixed with bug 313313 and bug 316691 fixed
> 

I'll fix this with the patch to bug 313313.  Please verify after that patch goes in.

Comment 6

[aaronr]

(In reply to comment #5)
> (In reply to comment #4)
> > should be able to be fixed with bug 313313 and bug 316691 fixed
> > 
> 
> I'll fix this with the patch to bug 313313.  Please verify after that patch
> goes in.
> 

The functionality piece of this will be fixed in 313313, but will still need to add the warning messages to the error console.  Will do that via this bug.

Comment 7

[Steve Speicher]

Tried out patch from bug 313313 and ran on testsuite for 8.1.2b(input), 8.1.3b(secret) and 8.1.4b(textarea), input and secret don't show but textarea shows but as invalid.  Would have expected textarea to be nonrelevant as well.

Comment 8

[alexander :surkov (:asurkov)]

(In reply to comment #7)
> Tried out patch from bug 313313 and ran on testsuite for 8.1.2b(input),
> 8.1.3b(secret) and 8.1.4b(textarea), input and secret don't show but textarea
> shows but as invalid.  Would have expected textarea to be nonrelevant as well.
> 

Specs says "Binds to xsd:string or any derived simpleContent.", therefore I guess answer is yes.

Comment 9

[aaronr]

Attachment: patch 1

Doh!  My bad.  Steve, try this patch.  Should fix your problem.  In addition to what this patch does, to finish fixing this bug I need to add code for textarea, input and secret to generate errors to the user if they bind to an unsupported type.

Comment 10

[alexander :surkov (:asurkov)]

(In reply to comment #9)
> Created an attachment (id=233671) [edit]
> patch 1
> 
> Doh!  My bad.  Steve, try this patch.  Should fix your problem.  In addition to
> what this patch does, to finish fixing this bug I need to add code for
> textarea, input and secret to generate errors to the user if they bind to an
> unsupported type.
> 

Aaron, I'm starting to like your mozType|supportedtypes approach :) since I guess for every control that has data binding restriction we will provide method that determines whether type is allowed or not. The upload acts in consistent manner. Probably "supportedtypes" is not very good name for that because I think supported means those types for what we have controls but doesn't mean binding restriction actually.

Comment 11

[alexander :surkov (:asurkov)]

(In reply to comment #10)
> 
> Aaron, I'm starting to like your mozType|supportedtypes approach :) since I
> guess for every control that has data binding restriction we will provide
> method that determines whether type is allowed or not. The upload acts in
> consistent manner. Probably "supportedtypes" is not very good name for that
> because I think supported means those types for what we have controls but
> doesn't mean binding restriction actually.
> 

Oh, I didn't say why :)
1) Since we have method to throw warning if type is not allowed then we can easy to set new attribute.
2) Then we can use this attribute to hide all controls instead of huge css rules using like your patch has.
3) And by using it we can unbound all our xbl bindings from such controls since they are not needed I guess. Or how we should act when controls has wrong type excepting its hiding?

Comment 12

[Steve Speicher]

(In reply to comment #9)
> Created an attachment (id=233671) [edit]
> patch 1
> 
> Doh!  My bad.  Steve, try this patch.  Should fix your problem.  In addition to
> what this patch does, to finish fixing this bug I need to add code for
> textarea, input and secret to generate errors to the user if they bind to an
> unsupported type.
> 
Yep works better.  Though only the "value" part of the control is not displayed, the label is still displayed...see http://www.w3.org/MarkUp/Forms/Test/XForms1.0/Edition2/Chapt8/8.1/8.1.4/8.1.4.b.xhtml
Also, get an error message that "instance did not validate"...doesn't seem like an appropriate message but maybe that will be fixed in patch part deux.

Comment 13

[aaronr]

Attachment: patch 2

NOW it works :)

Comment 14

[Steve Speicher]

(In reply to comment #13)
> Created an attachment (id=233802) [edit]
> patch 2
> 
> NOW it works :)
> 
and I verified that it NOW works :)

Comment 15

[aaronr]

(In reply to comment #11)
> (In reply to comment #10)
> > 
> > Aaron, I'm starting to like your mozType|supportedtypes approach :) since I
> > guess for every control that has data binding restriction we will provide
> > method that determines whether type is allowed or not. The upload acts in
> > consistent manner. Probably "supportedtypes" is not very good name for that
> > because I think supported means those types for what we have controls but
> > doesn't mean binding restriction actually.
> > 
> 
> Oh, I didn't say why :)
> 1) Since we have method to throw warning if type is not allowed then we can
> easy to set new attribute.

Either approach that we take, we need to throw warnings.

> 2) Then we can use this attribute to hide all controls instead of huge css
> rules using like your patch has.

That is the trade off.  Either CSS does it for us or we do it and have to override a common function for restricted controls to do the logic by using the supportedType attribute (my first approach).

> 3) And by using it we can unbound all our xbl bindings from such controls since
> they are not needed I guess. Or how we should act when controls has wrong type
> excepting its hiding?
> 

Hiding the control is the only behavior that I've seen.  Just like if you bind a control to a node that doesn't exist.

Comment 16

[aaronr]

Attachment: patch 3

Hey Alex, what do you think of this patch?  I'm using the supportedTypes approach.  But instead of that attribute name, I'm putting mozType:rejectedtype="true" on controls that are bound to nodes that are of a 'bad' type and removing that attribute if it is a 'good' type.  This patch covers upload, input, textarea, secret and range.

Let me know what you think.

Comment 17

[aaronr]

Comment on attachment 238395 [details] [diff] [review]
patch 3

I forgot to mention that this patch fixes a couple of other little bugs I noticed in my testing...a missing ',' in the disabled upload .js and a case where we were returning a bool instead of a nsresult.

Comment 18

[aaronr]

Attachment: patch 4

I made a cosmetic change to output that caused a problem, so I put it back like it was.

Comment 19

[alexander :surkov (:asurkov)]

Comment on attachment 238527 [details] [diff] [review]
patch 4


>+NS_IMETHODIMP
>+nsXFormsInputElement::Refresh()
>+{
>+  nsresult rv = nsXFormsDelegateStub::Refresh();
>+  NS_ENSURE_SUCCESS(rv, rv);
>+
>+  PRUint16 type = 0;
>+  rv = GetBoundBuiltinType(&type);
>+  NS_ENSURE_SUCCESS(rv, rv);
>+
>+  // Textareas can only be bound to types of xsd:string (or a type derived from
>+  // xsd:string).  For input and secret, the bound type can't be xsd:hexBinary
>+  // or xsd:base64Binary
>+
>+  nsAutoString localName;
>+  mElement->GetLocalName(localName);
>+  if (localName.EqualsLiteral("textarea")) {
>+    if (type == nsISchemaBuiltinType::BUILTIN_TYPE_STRING) {
>+      mElement->RemoveAttributeNS(
>+        NS_LITERAL_STRING(NS_NAMESPACE_MOZ_XFORMS_TYPE),
>+        NS_LITERAL_STRING(NS_MOZ_XFORMS_REJECTEDTYPE));
>+    
>+      return NS_OK;
>+    }
>+
>+    mElement->SetAttributeNS(NS_LITERAL_STRING(NS_NAMESPACE_MOZ_XFORMS_TYPE),
>+                             NS_LITERAL_STRING(NS_MOZ_XFORMS_REJECTEDTYPE),
>+                             NS_LITERAL_STRING("true"));
>+  
>+    nsAutoString supportedTypes(NS_LITERAL_STRING("xsd:string"));
>+    const PRUnichar *strings[] = { localName.get(), supportedTypes.get() };
>+    nsXFormsUtils::ReportError(NS_LITERAL_STRING("boundTypeErrorInclusive"),
>+                               strings, 2, mElement, mElement);
>+    return NS_OK;
>+  }

I guess it's better to provide new class for textarea. The things will be cleaner.

> NS_IMETHODIMP
>+nsXFormsRangeElement::Refresh()
>+{
>+  nsresult rv = nsXFormsDelegateStub::Refresh();
>+  NS_ENSURE_SUCCESS(rv, rv);
>+
>+  PRUint16 type = 0;
>+  rv = GetBoundBuiltinType(&type);
>+  NS_ENSURE_SUCCESS(rv, rv);
>+
>+  if (type == nsISchemaBuiltinType::BUILTIN_TYPE_DURATION ||
>+      type == nsISchemaBuiltinType::BUILTIN_TYPE_DATE ||
>+      type == nsISchemaBuiltinType::BUILTIN_TYPE_TIME ||
>+      type == nsISchemaBuiltinType::BUILTIN_TYPE_DATETIME ||
>+      type == nsISchemaBuiltinType::BUILTIN_TYPE_GYEARMONTH ||
>+      type == nsISchemaBuiltinType::BUILTIN_TYPE_GYEAR ||
>+      type == nsISchemaBuiltinType::BUILTIN_TYPE_GMONTHDAY ||
>+      type == nsISchemaBuiltinType::BUILTIN_TYPE_GDAY ||
>+      type == nsISchemaBuiltinType::BUILTIN_TYPE_GMONTH ||
>+      type == nsISchemaBuiltinType::BUILTIN_TYPE_FLOAT ||
>+      type == nsISchemaBuiltinType::BUILTIN_TYPE_DECIMAL ||
>+      type == nsISchemaBuiltinType::BUILTIN_TYPE_DOUBLE) {
>+
>+    mElement->RemoveAttributeNS(
>+      NS_LITERAL_STRING(NS_NAMESPACE_MOZ_XFORMS_TYPE),
>+      NS_LITERAL_STRING(NS_MOZ_XFORMS_REJECTEDTYPE));
>+
>+    return NS_OK;
>+  }
>+
>+  mElement->SetAttributeNS(NS_LITERAL_STRING(NS_NAMESPACE_MOZ_XFORMS_TYPE),
>+                           NS_LITERAL_STRING(NS_MOZ_XFORMS_REJECTEDTYPE),
>+                           NS_LITERAL_STRING("true"));
>+
>+  nsAutoString localName;
>+  mElement->GetLocalName(localName);
>+
>+  nsAutoString supportedTypes(
>+    NS_LITERAL_STRING("xsd:duration, xsd:date, xsd:time, xsd:dateTime, xsd:gYearMonth, xsd:gYear, xsd:gMonthDay, xsd:gDay, xsd:gMonth, xsd:float, xsd:decimal, xsd:double"));
>+  const PRUnichar *strings[] = { localName.get(), supportedTypes.get() };
>+  nsXFormsUtils::ReportError(NS_LITERAL_STRING("boundTypeErrorInclusive"),
>+                             strings, 2, mElement, mElement);
>+
>+  return NS_OK;
>+}

It will better to move most of this code to Refresh() method of, for example, into nsXFormsDelegate class. All derived classes will define method that should return is given type supported. It helps to keep the setting/removing @rejectedtype attribute and error reporting in one place. I guess the code will simplier.

I'll complile the patch later to see how it works.

Comment 20

[alexander :surkov (:asurkov)]

Comment on attachment 238527 [details] [diff] [review]
patch 4

With two comments above r+.

Comment 21

[aaronr]

(In reply to comment #19)
> (From update of attachment 238527 [details] [diff] [review] [edit])
> 
> >+NS_IMETHODIMP
> >+nsXFormsInputElement::Refresh()
> >+{
> >+  nsresult rv = nsXFormsDelegateStub::Refresh();
> >+  NS_ENSURE_SUCCESS(rv, rv);
> >+
> >+  PRUint16 type = 0;
> >+  rv = GetBoundBuiltinType(&type);
> >+  NS_ENSURE_SUCCESS(rv, rv);
> >+
> >+  // Textareas can only be bound to types of xsd:string (or a type derived from
> >+  // xsd:string).  For input and secret, the bound type can't be xsd:hexBinary
> >+  // or xsd:base64Binary
> >+
> >+  nsAutoString localName;
> >+  mElement->GetLocalName(localName);
> >+  if (localName.EqualsLiteral("textarea")) {
> >+    if (type == nsISchemaBuiltinType::BUILTIN_TYPE_STRING) {
> >+      mElement->RemoveAttributeNS(
> >+        NS_LITERAL_STRING(NS_NAMESPACE_MOZ_XFORMS_TYPE),
> >+        NS_LITERAL_STRING(NS_MOZ_XFORMS_REJECTEDTYPE));
> >+    
> >+      return NS_OK;
> >+    }
> >+
> >+    mElement->SetAttributeNS(NS_LITERAL_STRING(NS_NAMESPACE_MOZ_XFORMS_TYPE),
> >+                             NS_LITERAL_STRING(NS_MOZ_XFORMS_REJECTEDTYPE),
> >+                             NS_LITERAL_STRING("true"));
> >+  
> >+    nsAutoString supportedTypes(NS_LITERAL_STRING("xsd:string"));
> >+    const PRUnichar *strings[] = { localName.get(), supportedTypes.get() };
> >+    nsXFormsUtils::ReportError(NS_LITERAL_STRING("boundTypeErrorInclusive"),
> >+                               strings, 2, mElement, mElement);
> >+    return NS_OK;
> >+  }
> 
> I guess it's better to provide new class for textarea. The things will be
> cleaner.
> 

ok

> > NS_IMETHODIMP
> >+nsXFormsRangeElement::Refresh()
> >+{
> >+  nsresult rv = nsXFormsDelegateStub::Refresh();
> >+  NS_ENSURE_SUCCESS(rv, rv);
> >+
> >+  PRUint16 type = 0;
> >+  rv = GetBoundBuiltinType(&type);
> >+  NS_ENSURE_SUCCESS(rv, rv);
> >+
> >+  if (type == nsISchemaBuiltinType::BUILTIN_TYPE_DURATION ||
> >+      type == nsISchemaBuiltinType::BUILTIN_TYPE_DATE ||
> >+      type == nsISchemaBuiltinType::BUILTIN_TYPE_TIME ||
> >+      type == nsISchemaBuiltinType::BUILTIN_TYPE_DATETIME ||
> >+      type == nsISchemaBuiltinType::BUILTIN_TYPE_GYEARMONTH ||
> >+      type == nsISchemaBuiltinType::BUILTIN_TYPE_GYEAR ||
> >+      type == nsISchemaBuiltinType::BUILTIN_TYPE_GMONTHDAY ||
> >+      type == nsISchemaBuiltinType::BUILTIN_TYPE_GDAY ||
> >+      type == nsISchemaBuiltinType::BUILTIN_TYPE_GMONTH ||
> >+      type == nsISchemaBuiltinType::BUILTIN_TYPE_FLOAT ||
> >+      type == nsISchemaBuiltinType::BUILTIN_TYPE_DECIMAL ||
> >+      type == nsISchemaBuiltinType::BUILTIN_TYPE_DOUBLE) {
> >+
> >+    mElement->RemoveAttributeNS(
> >+      NS_LITERAL_STRING(NS_NAMESPACE_MOZ_XFORMS_TYPE),
> >+      NS_LITERAL_STRING(NS_MOZ_XFORMS_REJECTEDTYPE));
> >+
> >+    return NS_OK;
> >+  }
> >+
> >+  mElement->SetAttributeNS(NS_LITERAL_STRING(NS_NAMESPACE_MOZ_XFORMS_TYPE),
> >+                           NS_LITERAL_STRING(NS_MOZ_XFORMS_REJECTEDTYPE),
> >+                           NS_LITERAL_STRING("true"));
> >+
> >+  nsAutoString localName;
> >+  mElement->GetLocalName(localName);
> >+
> >+  nsAutoString supportedTypes(
> >+    NS_LITERAL_STRING("xsd:duration, xsd:date, xsd:time, xsd:dateTime, xsd:gYearMonth, xsd:gYear, xsd:gMonthDay, xsd:gDay, xsd:gMonth, xsd:float, xsd:decimal, xsd:double"));
> >+  const PRUnichar *strings[] = { localName.get(), supportedTypes.get() };
> >+  nsXFormsUtils::ReportError(NS_LITERAL_STRING("boundTypeErrorInclusive"),
> >+                             strings, 2, mElement, mElement);
> >+
> >+  return NS_OK;
> >+}
> 
> It will better to move most of this code to Refresh() method of, for example,
> into nsXFormsDelegate class. All derived classes will define method that should
> return is given type supported. It helps to keep the setting/removing
> @rejectedtype attribute and error reporting in one place. I guess the code will
> simplier.
> 

the reason I didn't do that is so that we wouldn't make every control go through this type of thing when it only affects 3 types of controls.  But I can do that for organizational purposes.

Patch coming up.

Comment 22

[aaronr]

(In reply to comment #19)
> (From update of attachment 238527 [details] [diff] [review] [edit])
> 
> It will better to move most of this code to Refresh() method of, for example,
> into nsXFormsDelegate class. All derived classes will define method that should
> return is given type supported. It helps to keep the setting/removing
> @rejectedtype attribute and error reporting in one place. I guess the code will
> simplier.
> 

Hey Alex, I tried to make this work, but I can't think of a smooth way to do this.  Moving the attribute setting/removing out to delegate won't be hard.  But reporting the error in delegate will be tougher because I have to know if this particular control is inclusive (only handles types x, y, and z) or exclusive (handles all type other than x, y, and z) and also what those types are.  How about if I just move the attribute setting to delegate and leave the error reporting in each control?

Comment 23

[alexander :surkov (:asurkov)]

(In reply to comment #22)
> (In reply to comment #19)
> > (From update of attachment 238527 [details] [diff] [review] [edit] [edit])
> > 
> > It will better to move most of this code to Refresh() method of, for example,
> > into nsXFormsDelegate class. All derived classes will define method that should
> > return is given type supported. It helps to keep the setting/removing
> > @rejectedtype attribute and error reporting in one place. I guess the code will
> > simplier.
> > 
> 
> Hey Alex, I tried to make this work, but I can't think of a smooth way to do
> this.  Moving the attribute setting/removing out to delegate won't be hard. 
> But reporting the error in delegate will be tougher because I have to know if
> this particular control is inclusive (only handles types x, y, and z) or
> exclusive (handles all type other than x, y, and z) and also what those types
> are.  How about if I just move the attribute setting to delegate and leave the
> error reporting in each control?
> 

Will the method of particular control like isAllowedType(in type, out isSupported, out supportedTypesString (or out errorMessage)) work here?

Comment 24

[aaronr]

(In reply to comment #23)
> (In reply to comment #22)
> > (In reply to comment #19)
> > > (From update of attachment 238527 [details] [diff] [review] [edit] [edit] [edit])
> > > 
> > > It will better to move most of this code to Refresh() method of, for example,
> > > into nsXFormsDelegate class. All derived classes will define method that should
> > > return is given type supported. It helps to keep the setting/removing
> > > @rejectedtype attribute and error reporting in one place. I guess the code will
> > > simplier.
> > > 
> > 
> > Hey Alex, I tried to make this work, but I can't think of a smooth way to do
> > this.  Moving the attribute setting/removing out to delegate won't be hard. 
> > But reporting the error in delegate will be tougher because I have to know if
> > this particular control is inclusive (only handles types x, y, and z) or
> > exclusive (handles all type other than x, y, and z) and also what those types
> > are.  How about if I just move the attribute setting to delegate and leave the
> > error reporting in each control?
> > 
> 
> Will the method of particular control like isAllowedType(in type, out
> isSupported, out supportedTypesString (or out errorMessage)) work here?
> 

I can't return a supportedTypesString for the case where the spec says to support all types except x, y, and z without hardcoding the list of all of the types.  I guess that I can return an error message, but I'll have to grab the string from the xforms.properties file and then format it...which is pretty much 1/2 of the work of nsXFormsUtils::ReportError.  I'll have the patch ready this afternoon.

Comment 25

[aaronr]

Attachment: patch 5

patch that uses surkov's comments.  Also changed ReportError to be able to take a literal string, changed it to also take a nsAString instead of a nsString, and introduced ReportErrorMessage to nsIXFormsDelegate so that we can report a literal string to the error console from xbl.  Should be good for debugging if nothing else.

Comment 26

[alexander :surkov (:asurkov)]

The mainly I asked you to put error reporting into delegate because of code dublications. But here we get the same I guess. Probably isTypeAllowed should return not error message wholy but rather list of types and error code ('exclusive' on 'inclusive'). If it saves the code lines then ok if not then probably better to leave error generation inside of isTypeAllowed() since I guess error reporting looks more fine than stringbundle operations. Please look how is better.

Comment 27

[aaronr]

Attachment: patch 6

now returns list of strings as per Alex

Comment 28

[alexander :surkov (:asurkov)]

Comment on attachment 239237 [details] [diff] [review]
patch 6

The first comment part.

>   /**
>    * Report an error
>    *
>    * @param errorMsg          The error message id
>-   *
>-   * @todo XXX this should be extended to allow for "raw strings", not
>-   * necessarily kept in bundles.
>    */
>   void reportError(in DOMString errorMsg);
>+
>+  /**
>+   * Report an error
>+   *
>+   * @param errorMsg          The literal/raw message to report
>+   */
>+  void reportErrorMessage(in DOMString errorMsg);
> };

nit: missed full stops after 'Report an error' and arguments description.

>+NS_IMETHODIMP
>+nsXFormsDelegateStub::IsTypeAllowed(PRUint16 aType, PRBool *aIsAllowed,
>+                                    nsRestrictionFlag *aRestriction,
>+                                    nsAString &aTypes)
>+{
>+  NS_ENSURE_ARG_POINTER(aIsAllowed);
>+  *aIsAllowed = PR_TRUE;
>+  return NS_OK;
>+}

Should you nulls aRestriction and truncate aTypes?

> void
> nsXFormsDelegateStub::SetMozTypeAttribute()
> {
>   NS_NAMED_LITERAL_STRING(mozTypeNs, NS_NAMESPACE_MOZ_XFORMS_TYPE);
>   NS_NAMED_LITERAL_STRING(mozType, "type");
>   NS_NAMED_LITERAL_STRING(mozTypeList, "typelist");
>+  NS_NAMED_LITERAL_STRING(mozRejectedType, "rejectedtype");
> 
>   if (mModel && mBoundNode) {
>     nsAutoString type, nsOrig;
>     if (NS_FAILED(mModel->GetTypeAndNSFromNode(mBoundNode, type, nsOrig))) {
>       mElement->RemoveAttributeNS(mozTypeNs, mozType);
>       mElement->RemoveAttributeNS(mozTypeNs, mozTypeList);
>+      mElement->RemoveAttributeNS(mozTypeNs, mozRejectedType);

When you remove mozRejectedType then control I guess should be become visible. Is it right?

Comment 29

[alexander :surkov (:asurkov)]

Comment on attachment 239237 [details] [diff] [review]
patch 6


>     // Get the list of types that this type derives from and set it as the
>     // value of the basetype attribute
>     nsresult rv = mModel->GetDerivedTypeList(type, nsOrig, attrValue);
>     if (NS_SUCCEEDED(rv)) {

>     }
>+    PRUint16 builtinType = 0;
>+    rv = GetBoundBuiltinType(&builtinType);

Is it possible the GetDerivedTypeList() return ok but GetBoundBuiltinType fail or GetDerivedTypeList faild but GetBoundBuiltinType return ok? It it is not possible then I guess something redurand with if/else statements.

>+    if (NS_SUCCEEDED(rv)) {
>+      PRBool isAllowed = PR_TRUE;
>+      nsAutoString aTypeList;

it seems it's wrong to use 'a' prefix for local variables.

The rest looks good. I'll finish review after patch compiling.

Comment 30

[aaronr]

(In reply to comment #28)
> (From update of attachment 239237 [details] [diff] [review] [edit])
> The first comment part.
> 
> >   /**
> >    * Report an error
> >    *
> >    * @param errorMsg          The error message id
> >-   *
> >-   * @todo XXX this should be extended to allow for "raw strings", not
> >-   * necessarily kept in bundles.
> >    */
> >   void reportError(in DOMString errorMsg);
> >+
> >+  /**
> >+   * Report an error
> >+   *
> >+   * @param errorMsg          The literal/raw message to report
> >+   */
> >+  void reportErrorMessage(in DOMString errorMsg);
> > };
> 
> nit: missed full stops after 'Report an error' and arguments description.

will fix in next patch

> 
> >+NS_IMETHODIMP
> >+nsXFormsDelegateStub::IsTypeAllowed(PRUint16 aType, PRBool *aIsAllowed,
> >+                                    nsRestrictionFlag *aRestriction,
> >+                                    nsAString &aTypes)
> >+{
> >+  NS_ENSURE_ARG_POINTER(aIsAllowed);
> >+  *aIsAllowed = PR_TRUE;
> >+  return NS_OK;
> >+}
> 
> Should you nulls aRestriction and truncate aTypes?

will fix in next patch.  Introduced new enum value to return...eTypes_NoRestriction and will truncate the string.

> 
> > void
> > nsXFormsDelegateStub::SetMozTypeAttribute()
> > {
> >   NS_NAMED_LITERAL_STRING(mozTypeNs, NS_NAMESPACE_MOZ_XFORMS_TYPE);
> >   NS_NAMED_LITERAL_STRING(mozType, "type");
> >   NS_NAMED_LITERAL_STRING(mozTypeList, "typelist");
> >+  NS_NAMED_LITERAL_STRING(mozRejectedType, "rejectedtype");
> > 
> >   if (mModel && mBoundNode) {
> >     nsAutoString type, nsOrig;
> >     if (NS_FAILED(mModel->GetTypeAndNSFromNode(mBoundNode, type, nsOrig))) {
> >       mElement->RemoveAttributeNS(mozTypeNs, mozType);
> >       mElement->RemoveAttributeNS(mozTypeNs, mozTypeList);
> >+      mElement->RemoveAttributeNS(mozTypeNs, mozRejectedType);
> 
> When you remove mozRejectedType then control I guess should be become visible.
> Is it right?
> 

When rejectectedType is removed, control should become visible, yes.

Comment 31

[aaronr]

(In reply to comment #29)
> (From update of attachment 239237 [details] [diff] [review] [edit])
> 
> >     // Get the list of types that this type derives from and set it as the
> >     // value of the basetype attribute
> >     nsresult rv = mModel->GetDerivedTypeList(type, nsOrig, attrValue);
> >     if (NS_SUCCEEDED(rv)) {
> 
> >     }
> >+    PRUint16 builtinType = 0;
> >+    rv = GetBoundBuiltinType(&builtinType);
> 
> Is it possible the GetDerivedTypeList() return ok but GetBoundBuiltinType fail
> or GetDerivedTypeList faild but GetBoundBuiltinType return ok? It it is not
> possible then I guess something redurand with if/else statements.

In many cases if one fails, the other will fail since the bulk of the ugly work is done by nsXFormsModelElement::WalkTypeChainInternal in both cases.  But there are plenty of other opportunities to fail during set up that one function might encounter that the other might not.  So I did it this way to be safe.

> 
> >+    if (NS_SUCCEEDED(rv)) {
> >+      PRBool isAllowed = PR_TRUE;
> >+      nsAutoString aTypeList;
> 
> it seems it's wrong to use 'a' prefix for local variables.
> 
> The rest looks good. I'll finish review after patch compiling.
> 

will fix in next patch.

Comment 32

[aaronr]

Attachment: patch 7

Comment 33

[alexander :surkov (:asurkov)]

(In reply to comment #30)

> > >+NS_IMETHODIMP
> > >+nsXFormsDelegateStub::IsTypeAllowed(PRUint16 aType, PRBool *aIsAllowed,
> > >+                                    nsRestrictionFlag *aRestriction,
> > >+                                    nsAString &aTypes)
> > >+{
> > >+  NS_ENSURE_ARG_POINTER(aIsAllowed);
> > >+  *aIsAllowed = PR_TRUE;
> > >+  return NS_OK;
> > >+}
> > 
> > Should you nulls aRestriction and truncate aTypes?
> 
> will fix in next patch.  Introduced new enum value to
> return...eTypes_NoRestriction and will truncate the string.
> 
> > 
> > > void
> > > nsXFormsDelegateStub::SetMozTypeAttribute()
> > > {
> > >   NS_NAMED_LITERAL_STRING(mozTypeNs, NS_NAMESPACE_MOZ_XFORMS_TYPE);
> > >   NS_NAMED_LITERAL_STRING(mozType, "type");
> > >   NS_NAMED_LITERAL_STRING(mozTypeList, "typelist");
> > >+  NS_NAMED_LITERAL_STRING(mozRejectedType, "rejectedtype");
> > > 
> > >   if (mModel && mBoundNode) {
> > >     nsAutoString type, nsOrig;
> > >     if (NS_FAILED(mModel->GetTypeAndNSFromNode(mBoundNode, type, nsOrig))) {
> > >       mElement->RemoveAttributeNS(mozTypeNs, mozType);
> > >       mElement->RemoveAttributeNS(mozTypeNs, mozTypeList);
> > >+      mElement->RemoveAttributeNS(mozTypeNs, mozRejectedType);
> > 
> > When you remove mozRejectedType then control I guess should be become visible.
> > Is it right?
> > 
> 
> When rejectectedType is removed, control should become visible, yes.
> 

Sorry, I should be more clear. I think control shouldn't be visible if we can't get its type since we can't guarantee control will work properly. Right?

Comment 34

[aaronr]

(In reply to comment #33)
> (In reply to comment #30)
> 
> > > >+NS_IMETHODIMP
> > > >+nsXFormsDelegateStub::IsTypeAllowed(PRUint16 aType, PRBool *aIsAllowed,
> > > >+                                    nsRestrictionFlag *aRestriction,
> > > >+                                    nsAString &aTypes)
> > > >+{
> > > >+  NS_ENSURE_ARG_POINTER(aIsAllowed);
> > > >+  *aIsAllowed = PR_TRUE;
> > > >+  return NS_OK;
> > > >+}
> > > 
> > > Should you nulls aRestriction and truncate aTypes?
> > 
> > will fix in next patch.  Introduced new enum value to
> > return...eTypes_NoRestriction and will truncate the string.
> > 
> > > 
> > > > void
> > > > nsXFormsDelegateStub::SetMozTypeAttribute()
> > > > {
> > > >   NS_NAMED_LITERAL_STRING(mozTypeNs, NS_NAMESPACE_MOZ_XFORMS_TYPE);
> > > >   NS_NAMED_LITERAL_STRING(mozType, "type");
> > > >   NS_NAMED_LITERAL_STRING(mozTypeList, "typelist");
> > > >+  NS_NAMED_LITERAL_STRING(mozRejectedType, "rejectedtype");
> > > > 
> > > >   if (mModel && mBoundNode) {
> > > >     nsAutoString type, nsOrig;
> > > >     if (NS_FAILED(mModel->GetTypeAndNSFromNode(mBoundNode, type, nsOrig))) {
> > > >       mElement->RemoveAttributeNS(mozTypeNs, mozType);
> > > >       mElement->RemoveAttributeNS(mozTypeNs, mozTypeList);
> > > >+      mElement->RemoveAttributeNS(mozTypeNs, mozRejectedType);
> > > 
> > > When you remove mozRejectedType then control I guess should be become visible.
> > > Is it right?
> > > 
> > 
> > When rejectectedType is removed, control should become visible, yes.
> > 
> 
> Sorry, I should be more clear. I think control shouldn't be visible if we can't
> get its type since we can't guarantee control will work properly. Right?
> 

I was thinking that if we can't positively identify which builtin type this control is bound to, then it should behave in its default manner which would make it visible and as if there were no type on the control at all (as far as picking which binding to use...we'll still validate against the type).  For example, if the control is bound to a type that is derived from a union type, we won't be able to figure out what the base builtin type is.  But that shouldn't mean that the control can't be used to input data that can then be validated against this type.  Afterall, we can't anticipate all possible use cases.

Comment 35

[aaronr]

checked into trunk

Comment 36

[aaronr]

checked into 1.8.0 branch on 2006/09/21

Comment 37

[aaronr]

checked into 1.8 branch on 2006/11/21