725606 - XForms for Gecko 5

Status: RESOLVED FIXED

(Core Graveyard :: XForms, defect)

Description

[Philipp Wagner [:imphil]]

Attachment: fixes-gecko-5.patch

Let's start to get XForms ready for mozilla-central. How do you want to do it? All different patches in one bug or different bugs for each gecko version?

Usually, the patches are only small changes required after refactoring in Gecko, so they should be easy to review.

Comment 1

[Philipp Wagner [:imphil]]

Attachment: remove executable bit from some files that don't need it

Comment 2

[alexander :surkov (:asurkov)]

Comment on attachment 595696 [details] [diff] [review]
fixes-gecko-5.patch

Review of attachment 595696 [details] [diff] [review]:
-----------------------------------------------------------------

r=me

::: nsXFormsModelElement.cpp
@@ +3044,5 @@
>  }
>  
>  nsresult
> +nsXFormsModelElement::GetBuiltinTypesNames(PRUint16            aType,
> +                                           nsTArray<nsString> *aNameArray)

you might want to follow Mozilla style:

(PRUint16 aType,
 nsTArray<nsString>* aNameArray)

@@ +3148,5 @@
>  }
>  
>  nsresult
> +nsXFormsModelElement::WalkTypeChainInternal(nsISVSchemaType    *aType,
> +                                            PRBool              aFindRootBuiltin,

btw: at some point PRBool has gone, it makes sense to use bool instead

@@ +3362,1 @@
>        constructorString.Append(tempString);

do you really need to copy string to append it to anther string?

@@ +3557,3 @@
>      nsCOMPtr<nsIDOMElement> el;
>      nsresult rv = NS_OK;
> +    for (PRUint32 i=0; i<mPendingInlineSchemas.Length(); ++i) {

while you're here please fix style: space between operator and operand

Comment 3

[alexander :surkov (:asurkov)]

(In reply to Philipp Wagner [:imphil] from comment #0)
> Created attachment 595696 [details] [diff] [review]
> fixes-gecko-5.patch
> 
> Let's start to get XForms ready for mozilla-central. How do you want to do
> it? All different patches in one bug or different bugs for each gecko
> version?

Probably patches for each gecko version like you did. But I don't mind.

Comment 4

[Philipp Wagner [:imphil]]

Attachment: schema-validation: Fixes for Gecko 5

Before I check this in, let's get schema-validation ready for Gecko 5 as well. The patch contains nothing special, just a few core changes that need to be done in schema-validation as well.

Comment 5

[Philipp Wagner [:imphil]]

(In reply to alexander :surkov from comment #2)
> Comment on attachment 595696 [details] [diff] [review]
> fixes-gecko-5.patch
> 
> Review of attachment 595696 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r=me
> 
> ::: nsXFormsModelElement.cpp
> @@ +3044,5 @@
> >  }
> >  
> >  nsresult
> > +nsXFormsModelElement::GetBuiltinTypesNames(PRUint16            aType,
> > +                                           nsTArray<nsString> *aNameArray)
> 
> you might want to follow Mozilla style:
> 
> (PRUint16 aType,
>  nsTArray<nsString>* aNameArray)

This is the style used throughout the file, so I guess it's best to keep it consistent with the other methods.

> @@ +3148,5 @@
> >  }
> >  
> >  nsresult
> > +nsXFormsModelElement::WalkTypeChainInternal(nsISVSchemaType    *aType,
> > +                                            PRBool              aFindRootBuiltin,
> 
> btw: at some point PRBool has gone, it makes sense to use bool instead

this is coming in a later patch.

> @@ +3362,1 @@
> >        constructorString.Append(tempString);
> 
> do you really need to copy string to append it to anther string?

no, changed.

> @@ +3557,3 @@
> >      nsCOMPtr<nsIDOMElement> el;
> >      nsresult rv = NS_OK;
> > +    for (PRUint32 i=0; i<mPendingInlineSchemas.Length(); ++i) {
> 
> while you're here please fix style: space between operator and operand

changed.

Comment 6

[alexander :surkov (:asurkov)]

(In reply to Philipp Wagner [:imphil] from comment #5)
> > >  nsresult
> > > +nsXFormsModelElement::GetBuiltinTypesNames(PRUint16            aType,
> > > +                                           nsTArray<nsString> *aNameArray)
> > 
> > you might want to follow Mozilla style:
> > 
> > (PRUint16 aType,
> >  nsTArray<nsString>* aNameArray)
> 
> This is the style used throughout the file, so I guess it's best to keep it
> consistent with the other methods.

up to you but it must be annoying to keep in mind specific file styles when you hack. Personally I fix the style in code I touch.

Comment 7

[alexander :surkov (:asurkov)]

Comment on attachment 599993 [details] [diff] [review]
schema-validation: Fixes for Gecko 5

Review of attachment 599993 [details] [diff] [review]:
-----------------------------------------------------------------

r=me

Comment 8

[Philipp Wagner [:imphil]]

http://hg.mozilla.org/xforms/rev/1ad624b6015f
http://hg.mozilla.org/xforms/rev/4d41f239277b
http://hg.mozilla.org/schema-validation/rev/2e07c216cf0c