Closed Bug 1486311 Opened Last year Closed Last year

GetDirectoryEntries() should be passed a nsIDirectoryEnumerator

Categories

(Core :: XPCOM, enhancement)

enhancement
Not set

Tracking

()

RESOLVED FIXED
mozilla63
Tracking Status
firefox63 --- fixed

People

(Reporter: aceman, Assigned: aceman)

References

Details

Attachments

(1 file, 1 obsolete file)

In https://hg.mozilla.org/mozilla-central/rev/062e4138bfde of bug 1484496 it was changed that a nsIDirectoryEnumerator is to be passed to nsIFile->GetDirectoryEntries().

It seems this was missed at least at https://searchfox.org/mozilla-central/rev/7df3c5baab8a65a6c2cfc6e8a504b9d3fe3184c3/xpcom/io/nsLocalFileWin.cpp#2047.

Found by Thunderbird tests, see bug 1485820 comment 102.
Attached patch 1486311.patch (obsolete) — Splinter Review
These are the potentially left over occurrences. Compiled on Linux locally.
Please push to c-c and m-c try and see if the change makes sense at all places.
Attachment #9004102 - Flags: review?(kmaglione+bmo)
This fixes the test crash we see on Windows in Thunderbird. I haven't done M-C try pushes in ages, so I hope Kris can do that.
Thanks for testing.
I wonder what is the assert/crash good for, if the tests pass fine in opt builds. Is the problem real and are the opt builds silently not working?
Blocks: 1485820
(In reply to :aceman from comment #3)
> I wonder what is the assert/crash good for, if the tests pass fine in opt
> builds. Is the problem real and are the opt builds silently not working?

It's mostly a sanity check. In this particular case, it doesn't matter much, since we just happen to have two nsISimpleEnumerator vtable entries in this class. It matters in the case of bad casts and missing QueryInterface entries, though.
Comment on attachment 9004102 [details] [diff] [review]
1486311.patch

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

::: xpcom/io/nsIFile.idl
@@ +349,5 @@
>      [binaryname(DirectoryEntriesImpl)]
>      readonly attribute nsIDirectoryEnumerator directoryEntries;
>  
>      %{C++
> +/* XXX: still needed for anything?

Please remove this comment. Feel free to just remove the function, though.
Attachment #9004102 - Flags: review?(kmaglione+bmo) → review+
Thanks Kris. Aceman is on holidays and will return in a week. Could you please make this adjustment and get it landed. Thanks in advance. If not, I can do it.
Attached patch 1486311.patchSplinter Review
Addressed review comment. Carrying forward Kris' r+.
(Author Aceman on vacation.)
Attachment #9004102 - Attachment is obsolete: true
Attachment #9004640 - Flags: review+
Keywords: checkin-needed
Pushed by dluca@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d40ab2ee6023
convert remaining nsIFile::GetDirectoryEntries() callers to pass in a nsIDirectoryEnumerator. r=kmag
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/d40ab2ee6023
Status: ASSIGNED → RESOLVED
Closed: Last year
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
Comment on attachment 9004640 [details] [diff] [review]
1486311.patch

>     [binaryname(DirectoryEntriesImpl)]
>     readonly attribute nsIDirectoryEnumerator directoryEntries;
> 
>     %{C++
>-    nsresult GetDirectoryEntries(nsISimpleEnumerator** aOut)
>-    {
>-      nsCOMPtr<nsIDirectoryEnumerator> dirEnum;
>-      nsresult rv = GetDirectoryEntries(getter_AddRefs(dirEnum));
>-      dirEnum.forget(aOut);
>-      return rv;
>-    };
>-
>     nsresult GetDirectoryEntries(nsIDirectoryEnumerator** aOut)
>     {
>       return GetDirectoryEntriesImpl(aOut);
>     };
>     %}
Now that the function isn't being overloaded any more you can remove all of the gubbins (keeping just the second line of course).
You need to log in before you can comment on or make changes to this bug.