Last Comment Bug 725606 - XForms for Gecko 5
: XForms for Gecko 5
Status: RESOLVED FIXED
:
Product: Core Graveyard
Classification: Graveyard
Component: XForms (show other bugs)
: Trunk
: All All
: -- normal (vote)
: ---
Assigned To: Philipp Wagner [:imphil]
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-02-09 03:04 PST by Philipp Wagner [:imphil]
Modified: 2016-07-15 14:46 PDT (History)
1 user (show)
See Also:
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
fixes-gecko-5.patch (20.08 KB, patch)
2012-02-09 03:04 PST, Philipp Wagner [:imphil]
surkov.alexander: review+
Details | Diff | Splinter Review
remove executable bit from some files that don't need it (551 bytes, patch)
2012-02-09 03:05 PST, Philipp Wagner [:imphil]
surkov.alexander: review+
Details | Diff | Splinter Review
schema-validation: Fixes for Gecko 5 (29.98 KB, patch)
2012-02-23 06:59 PST, Philipp Wagner [:imphil]
surkov.alexander: review+
Details | Diff | Splinter Review

Description Philipp Wagner [:imphil] 2012-02-09 03:04:21 PST
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.
Comment 1 Philipp Wagner [:imphil] 2012-02-09 03:05:01 PST
Created attachment 595697 [details] [diff] [review]
remove executable bit from some files that don't need it
Comment 2 alexander :surkov 2012-02-09 05:10:02 PST
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 2012-02-09 05:12:07 PST
(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] 2012-02-23 06:59:31 PST
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.
Comment 5 Philipp Wagner [:imphil] 2012-02-23 07:00:53 PST
(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 2012-02-23 19:12:32 PST
(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 2012-02-23 19:43:03 PST
Comment on attachment 599993 [details] [diff] [review]
schema-validation: Fixes for Gecko 5

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

r=me

Note You need to log in before you can comment on or make changes to this bug.