997141 - Remove recur parameter from nsIFile::Contains()

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Georg Fritzsche [:gfritzsche]]

nsIFile::Contains() has a second parameter, recur, that is always ignored (since Gecko 1.7?) and treated as if true.

We should just remove it.

Comment 1

[Benjamin Smedberg]

IDL declaration:
http://hg.mozilla.org/mozilla-central/annotate/dd50745d7f35/xpcom/io/nsIFile.idl#l317

Windows implementation:
http://hg.mozilla.org/mozilla-central/annotate/dd50745d7f35/xpcom/io/nsLocalFileWin.cpp#l3026

*nix implementation (including mac):
http://hg.mozilla.org/mozilla-central/annotate/dd50745d7f35/xpcom/io/nsLocalFileUnix.cpp#l1599

Testing patches will probably require building on both Windows and Linux or Mac.

You will also have to change the c++ callers not to pass the extra argument. I don't know whether DXR can give you a good list of callers or not, I asked at https://ask.mozilla.org/question/536/can-dxr-give-me-all-the-c-callers-of-nsifilecontains/

You don't have to do anything with callers in JS: the extra parameter will simply be ignored. However if you'd like to go through the tree and remove the extra parameters from JS callsites I'd happily review that as well.

Comment 2

[teardrop.iit]

Hi Alessio, 

I would like to work on this bug, I have already set up mozilla-central repo. I have successfully compiled it as well. Please let me know how to get started with the codebase.

Comment 3

[Alessio Placitelli [:Dexter]]

Hi and welcome :)

Some guidelines on how to fix this bug are available in "Comment 1". You could start by removing the "recur" parameter from the nsIFile::Contains method in the IDL file (xpcom/io/nsIFile.idl) and from the related comment. Be sure to update the UUID for the IDL as well.

Then change the signature of the implementing methods in xpcom/io/nsLocalFileWin.cpp and xpcom/io/nsLocalFileUnix.cpp

Once you've modified the source tree and checked that it compiles and works, you can submit a patch here. If you are unsure about how to generate a path containing your modifications, have a look here:

https://developer.mozilla.org/en-US/docs/Mercurial_FAQ#How_can_I_generate_a_patch_for_somebody_else_to_check-in_for_me.3F

Please do not hesitate to ask if you need more information or if you have any specific question!

Comment 4

[Igor Kolupaev]

Attachment: First trial commit of bugfix

Hi there,

this is my first try inspired by Mozilla meetup held by Aleh Zasypkin (azasypkin).

Will be happy to get feedback from you, Mozillians :) 

Thanks,
Igor

Comment 5

[Oleg Zasypkin [:azasypkin]]

(In reply to Igor Kolupaev from comment #4)
> Created attachment 8412886 [details] [diff] [review]
> First trial commit of bugfix
> 
> Hi there,
> 
> this is my first try inspired by Mozilla meetup held by Aleh Zasypkin
> (azasypkin).
> 
> Will be happy to get feedback from you, Mozillians :) 
> 
> Thanks,
> Igor

That's exciting! As this bug has a mentor and still doesn't have assignee, I'm assigning it to you and asking for feedback from mentor.

Please, also see comment 3 for some guidance from Alessio and feel free to ask any questions you have!

Just a small hint that may be useful here: you can use http://mxr.mozilla.org/mozilla-central/ to find code fragments, usages and so on.

Thanks for your contribution!

Comment 6

[Igor Kolupaev]

Oleg, tnx for you message and the link. Definitely useful resource. 

Btw I have build just windows part yesterday. Now I'm going to check if Linux build is working after fixings.

Thanks,
Igor

Comment 7

[Alessio Placitelli [:Dexter]]

Hi Igor,

thank you very much for your contribution and welcome to the community!

I just have a few suggestions:

- Update the comment in the IDL file to reflect the changes in the modified method (see: http://hg.mozilla.org/mozilla-central/annotate/dd50745d7f35/xpcom/io/nsIFile.idl#l315 )?

- You might want to be a little more explicative in your commit message. A good commit message could look like: "Bug 997141 - Removed the recur parameter from nsIFile::Contains()"

- Make sure your patch compiles on all the supported platforms. To aid you with this, I've pushed your patch to the try server: it will compile and test your patch on several platforms. It seems to fail on some platforms. See:

https://tbpl.mozilla.org/?tree=Try&rev=6c7a96b647f2

Please do not hesitate to ask if you have more questions :)

Comment 8

[Alessio Placitelli [:Dexter]]

Comment on attachment 8412886 [details] [diff] [review]
First trial commit of bugfix

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

That's a good start indeed but, unfortunately, the patch breaks the compilation on all the platforms (see the try-serv). Try to fix the compilation erros and follow the suggestions I gave in the other comment :)

Comment 9

[Igor Kolupaev]

Alessio,

Hm for some reason (my fault) changes in nsNetUtil.h have not included to the diff file. Ok I need extra time to make new diff file which includes this changes. Is that ok if I continue with this issue in next 1-3 days or I have to do that faster?

Also just to be sure, Is this correct repo what I cloned? https://hg.mozilla.org/mozilla-central/ 

Thanks for you feedback.

Comment 10

[Alessio Placitelli [:Dexter]]

Igor,

no problem! Just attach the updated whenever you're done, no rush. 

By the way yes, that's the correct repo :-)

(In reply to Igor Kolupaev from comment #9)
> Alessio,
> 
> Hm for some reason (my fault) changes in nsNetUtil.h have not included to
> the diff file. Ok I need extra time to make new diff file which includes
> this changes. Is that ok if I continue with this issue in next 1-3 days or I
> have to do that faster?
> 
> Also just to be sure, Is this correct repo what I cloned?
> https://hg.mozilla.org/mozilla-central/ 
> 
> Thanks for you feedback.

Comment 11

[Igor Kolupaev]

Attachment: Bug 997141 - Remove recur parameter from nsIFile::Contains()

Improved patch attached. Build tested on windows and Ubuntu. Looking forward for feedback.

Comment 12

[Alessio Placitelli [:Dexter]]

Comment on attachment 8416914 [details] [diff] [review]
Bug 997141 - Remove recur parameter from nsIFile::Contains()

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

The patch looks good to me, Igor! Just take care of those things and add a proper commit message (which seems to be missing in this patch) and you're good to go!

::: xpcom/io/nsIFile.idl
@@ +312,3 @@
>  
>      /**
>       *  Will determine if inFile is a descendant of this file

Now that the |recur| parameter has been remove, how does the function behave? Does it look in the subdirectories too by default? You should state that in this comment.

@@ +315,2 @@
>       */
> +    boolean contains(in nsIFile inFile );

There's an extra space between inFile and ).

Comment 13

[Alessio Placitelli [:Dexter]]

I've pushed the changes to the tryserver: https://tbpl.mozilla.org/?tree=Try&rev=888ae156d59c

Comment 14

[Igor Kolupaev]

Attachment: Bug 997141 - Removed the recur parameter from nsIFile::Contains(). Fixed patch

Fixed path is attached. 
description of the function is extended, commit message is added and space is deleted from the function declaration.

Thanks,
Igor

Comment 15

[Oleg Zasypkin [:azasypkin]]

(In reply to Igor Kolupaev from comment #14)
> Created attachment 8417049 [details] [diff] [review]
> Bug 997141 - Removed the recur parameter from nsIFile::Contains(). Fixed
> patch
> 
> Fixed path is attached. 
> description of the function is extended, commit message is added and space
> is deleted from the function declaration.
> 
> Thanks,
> Igor

Just my two cents :) You probably would like to add your username to the patch in addition to commit message(you can check other patches in bugzilla as example and see link from comment 3 for details). And once Try is green (looks like it is!) set review? flag with Alessio as reviewer for the patch to get r+!

Thanks!

Comment 16

[Alessio Placitelli [:Dexter]]

(In reply to Oleg Zasypkin [:azasypkin] from comment #15)
> (In reply to Igor Kolupaev from comment #14)
> > Created attachment 8417049 [details] [diff] [review]
> > Bug 997141 - Removed the recur parameter from nsIFile::Contains(). Fixed
> > patch
> > 
> > Fixed path is attached. 
> > description of the function is extended, commit message is added and space
> > is deleted from the function declaration.
> > 
> > Thanks,
> > Igor
> 
> Just my two cents :) You probably would like to add your username to the
> patch in addition to commit message(you can check other patches in bugzilla
> as example and see link from comment 3 for details). And once Try is green
> (looks like it is!) set review? flag with Alessio as reviewer for the patch
> to get r+!
> 
> Thanks!

Thank you Oleg, you're totally right for the name! As for the reviewer, I think that somebody responsible for the Core::XPCOM (https://wiki.mozilla.org/Modules/Core#XPCOM) should acknowledge the patch as well (r? :bsmedberg ?).

Igor, just update the patch after setting your name as explained here (https://developer.mozilla.org/en-US/docs/Mercurial_FAQ#How_can_I_generate_a_patch_for_somebody_else_to_check-in_for_me.3F) and then you'll be good to go :) Keep up the great work!

Comment 17

[Alessio Placitelli [:Dexter]]

Hello Igor!

Did you make any progress on this bug? Please let me know if you need any additional information ;-)

Comment 18

[Igor Kolupaev]

Alessio,

Yeah, I'm a little bit stuck with mq hg ext and the name. But I will handle that in next 24h and will attach updated patch. If I will have questions -- I'll let you or somebody know here or in IRC.

Thanks,
Igor

Comment 19

[Igor Kolupaev]

Attachment: Bug 997141 - Removed the recur parameter from nsIFile::Contains(). Name and date added

Name and date are added to the patch file manually.

Comment 20

[Alessio Placitelli [:Dexter]]

Comment on attachment 8419173 [details] [diff] [review]
Bug 997141 - Removed the recur parameter from nsIFile::Contains(). Name and date added

Hi Igor,

thank you for your contribution!

The owner/peer of the module containing nsIFile should have the final word on the patch you submitted, to make sure everything is all right.

In this case it should be Benjamin Smedberg (:bsmedberg): I'm appointing him as the reviewer.

Comment 21

[Benjamin Smedberg]

Comment on attachment 8419173 [details] [diff] [review]
Bug 997141 - Removed the recur parameter from nsIFile::Contains(). Name and date added

Great job. This looks ready to go.

Comment 22

[Benjamin Smedberg]

If you have time, please also update https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XPCOM/Reference/Interface/nsIFile#contains%28%29

Comment 23

[Igor Kolupaev]

Doc is updated. 

Should I do anything else with this issue?

And thanks everybody for support me. Plan to do more contributions for Mozilla.

Comment 24

[Alessio Placitelli [:Dexter]]

Great job Igor! And thanks for updating the Docs as well. One minor thing: the documentation still reports the "recur" parameter in the function signature. Would you like to take care of that as well?

(In reply to Igor Kolupaev from comment #23)
> Doc is updated. 
> 
> Should I do anything else with this issue?
> 
> And thanks everybody for support me. Plan to do more contributions for
> Mozilla.

Comment 25

[Igor Kolupaev]

Ouch, my bad. Fixed.

(In reply to Alessio Placitelli [:Dexter] from comment #24)
> Great job Igor! And thanks for updating the Docs as well. One minor thing:
> the documentation still reports the "recur" parameter in the function
> signature. Would you like to take care of that as well?
> 
> (In reply to Igor Kolupaev from comment #23)
> > Doc is updated. 
> > 
> > Should I do anything else with this issue?
> > 
> > And thanks everybody for support me. Plan to do more contributions for
> > Mozilla.

Comment 26

[Alessio Placitelli [:Dexter]]

Thanks!

(In reply to Igor Kolupaev from comment #25)
> Ouch, my bad. Fixed.
> 
> (In reply to Alessio Placitelli [:Dexter] from comment #24)
> > Great job Igor! And thanks for updating the Docs as well. One minor thing:
> > the documentation still reports the "recur" parameter in the function
> > signature. Would you like to take care of that as well?
> > 
> > (In reply to Igor Kolupaev from comment #23)
> > > Doc is updated. 
> > > 
> > > Should I do anything else with this issue?
> > > 
> > > And thanks everybody for support me. Plan to do more contributions for
> > > Mozilla.

Comment 27

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/07b29d7938d3

Comment 28

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/07b29d7938d3