Remove recur parameter from nsIFile::Contains()

RESOLVED FIXED in mozilla32

Status

()

defect
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: gfritzsche, Assigned: ikolupaev)

Tracking

unspecified
mozilla32
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [good first bug][lang=C++][mentor=alessio.placitelli@mail.com] requires windows and unix)

Attachments

(1 attachment, 3 obsolete attachments)

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

5 years ago
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.
Whiteboard: [good first bug][lang=C++] requires windows and unix
Whiteboard: [good first bug][lang=C++] requires windows and unix → [good first bug][lang=C++][mentor=alessio.placitelli@mail.com] requires windows and unix

Comment 2

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

Comment 4

5 years ago
Posted patch First trial commit of bugfix (obsolete) — Splinter Review
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
Flags: needinfo?(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!
Assignee: nobody → ikolupaev
Status: NEW → ASSIGNED
Flags: needinfo?(azasypkin)
Attachment #8412886 - Flags: feedback?(alessio.placitelli)
Assignee

Comment 6

5 years ago
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
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 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 :)
Attachment #8412886 - Flags: feedback?(alessio.placitelli) → feedback-
Assignee

Comment 9

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

Comment 11

5 years ago
Improved patch attached. Build tested on windows and Ubuntu. Looking forward for feedback.
Attachment #8412886 - Attachment is obsolete: true
Flags: needinfo?(alessio.placitelli)
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 ).
Attachment #8416914 - Flags: feedback+
Flags: needinfo?(alessio.placitelli)
I've pushed the changes to the tryserver: https://tbpl.mozilla.org/?tree=Try&rev=888ae156d59c
Assignee

Comment 14

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

Thanks,
Igor
Attachment #8416914 - Attachment is obsolete: true
(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!
(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!
Hello Igor!

Did you make any progress on this bug? Please let me know if you need any additional information ;-)
Flags: needinfo?(ikolupaev)
Assignee

Comment 18

5 years ago
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
Flags: needinfo?(ikolupaev)
Assignee

Comment 19

5 years ago
Name and date are added to the patch file manually.
Attachment #8417049 - Attachment is obsolete: true
Attachment #8419173 - Flags: review?(alessio.placitelli)
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.
Attachment #8419173 - Flags: review?(alessio.placitelli) → review?(benjamin)
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.
Attachment #8419173 - Flags: review?(benjamin) → review+
Assignee

Comment 23

5 years ago
Doc is updated. 

Should I do anything else with this issue?

And thanks everybody for support me. Plan to do more contributions for Mozilla.
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.
Assignee

Comment 25

5 years ago
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.
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.
https://hg.mozilla.org/mozilla-central/rev/07b29d7938d3
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla32
You need to log in before you can comment on or make changes to this bug.