Status

Core Graveyard
XForms
RESOLVED FIXED
5 years ago
11 months ago

People

(Reporter: imphil, Assigned: imphil)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments)

(Assignee)

Description

5 years ago
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?

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

Comment 1

5 years ago
Created attachment 595697 [details] [diff] [review]
remove executable bit from some files that don't need it
Assignee: nobody → mail
Status: NEW → ASSIGNED
Attachment #595697 - Flags: review?(surkov.alexander)
(Assignee)

Updated

5 years ago
Attachment #595696 - Flags: review?(surkov.alexander)

Comment 2

5 years ago
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
Attachment #595696 - Flags: review?(surkov.alexander) → review+

Updated

5 years ago
Attachment #595697 - Flags: review?(surkov.alexander) → review+

Comment 3

5 years ago
(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.
(Assignee)

Comment 4

5 years ago
Created attachment 599993 [details] [diff] [review]
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.
Attachment #599993 - Flags: review?(surkov.alexander)
(Assignee)

Comment 5

5 years ago
(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

5 years ago
(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

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

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

r=me
Attachment #599993 - Flags: review?(surkov.alexander) → review+
(Assignee)

Comment 8

5 years ago
http://hg.mozilla.org/xforms/rev/1ad624b6015f
http://hg.mozilla.org/xforms/rev/4d41f239277b
http://hg.mozilla.org/schema-validation/rev/2e07c216cf0c
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.