1462483 - Cleanup directory enumeration code

Status: RESOLVED FIXED

(Toolkit :: Add-ons Manager, enhancement)

Description

[Kris Maglione [:kmag]]

We have directory enumeration code duplicated in a few places, along with the unused (or unnecessary) logic for sorting the directory entries. Let's just have one helper that returns an iterator and is used everywhere.

Note: In the future, we should make this an async iterator using OS.File, but that has complications for the couple of places we sometimes need to do synchronous scans at startup, so we may need separate sync and async implementations eventually.

Comment 1

[Kris Maglione [:kmag]]

Attachment: Bug 1462483: Part 1 - Explicitly return a nsIDirectoryEnumerator from nsIFile::GetDirectoryEntries.

All of our GetDirectoryEntries implementations return a
nsIDirectoryEnumerator, and a lot of relies on this, and explicitly QIs to it.
That gets a bit ugly, and in JS code, a bit expensive. We should just return a
directory enumerator directly if that's part of the API contract.

Review commit: https://reviewboard.mozilla.org/r/244814/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/244814/

Comment 2

[Kris Maglione [:kmag]]

Attachment: Bug 1462483: Part 2 - Cleanup directory iteration code.

Review commit: https://reviewboard.mozilla.org/r/244816/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/244816/

Comment 3

[Nathan Froyd [:froydnj]]

Comment on attachment 8976727 [details]
Bug 1462483: Part 1 - Explicitly return a nsIDirectoryEnumerator from nsIFile::GetDirectoryEntries.

https://reviewboard.mozilla.org/r/244814/#review250898

Do we need to file a followup for fixing C++ callers as well?

Comment 4

[Kris Maglione [:kmag]]

(In reply to Nathan Froyd [:froydnj] from comment #3)
> Do we need to file a followup for fixing C++ callers as well?

It would probably be a good idea. Most of the existing C++ callers seem to use nsISimpleEnumerator rather than QIing to nsIDirectoryEnumerator, but switching them to the latter would save an unnecessary QI for each file they enumerate over, so it would be nice.

Comment 5

[Andrew Swan [:aswan]]

Comment on attachment 8976728 [details]
Bug 1462483: Part 2 - Cleanup directory iteration code.

https://reviewboard.mozilla.org/r/244816/#review251120

::: toolkit/mozapps/extensions/internal/XPIInstall.jsm:2817
(Diff revision 1)
> -    let dirEntries = dir.directoryEntries.QueryInterface(Ci.nsIDirectoryEnumerator);
> -    try {
> +    // eslint-disable-next-line no-unused-vars
> +    for (let file of iterDirectory(dir))
> -      if (dirEntries.nextFile)
> -        return;
> +      return;

surely there is a more straightforward way to do this?

Comment 6

[Kris Maglione [:kmag]]

Comment on attachment 8976728 [details]
Bug 1462483: Part 2 - Cleanup directory iteration code.

https://reviewboard.mozilla.org/r/244816/#review251120

> surely there is a more straightforward way to do this?

Not really.

Comment 7

[Kris Maglione [:kmag]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/8b49c4e339aeec0eb6ec4ad238dbd43adf958062
Bug 1462483: Part 1 - Explicitly return a nsIDirectoryEnumerator from nsIFile::GetDirectoryEntries. r=froydnj

https://hg.mozilla.org/integration/mozilla-inbound/rev/50e011c40415a2d3420afa686f1d2ba5b7bcbb1c
Bug 1462483: Part 2 - Cleanup directory iteration code. r=aswan

Comment 8

[Raul Gurzau (:RaulG)]

https://hg.mozilla.org/mozilla-central/rev/8b49c4e339ae
https://hg.mozilla.org/mozilla-central/rev/50e011c40415