779407 - Remove usage of nsILocalFile in Add-ons Manager

Status: RESOLVED FIXED

(Toolkit :: Add-ons Manager, defect)

Description

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Bug 682360 merged the nsIFile and nsILocalFile interfaces - now nsILocalFile is just an empty interface that inherits from nsIFile. I'd like to cleanup the Add-ons Manager code to remove any usage of nsILocalFile, so it's just using nsIFile directly.

Relevant code is at: /toolkit/mozapps/extensions/

I think the most common pattern is:
  whatever.QueryInterface(Ci.nsILocalFile);
Where |whatever| is a nsIFile. In which case, we can remove the QueryInterface() call, and clean up the code as it makes sense for the individual case.

Comment 1

[Mark Capella [:capella]]

Attachment: Patch (v1)

Well, here's the first try. I removed all QI's except the one associated with the .getNext() as discussed in IRC. I didn't see any situations where the QI was tied to a nsIDirectoryEnumerator though... maybe I missed something?

fyi - The short / targeted version of the XPCShell tests was a life saver !

Let me know if there was more you saw as being required / desired in the scope of this bug and I'll do what I can -- evil minion, mark

Comment 2

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Comment on attachment 649013 [details] [diff] [review]
Patch (v1)

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

Grea, thanks :) Just a couple of fix-ups.

::: toolkit/mozapps/extensions/XPIProvider.jsm
@@ +1849,5 @@
>            var xpiEntries = stageDirEntry.directoryEntries
>                                          .QueryInterface(Ci.nsIDirectoryEnumerator);
>            while (xpiEntries.hasMoreElements()) {
>              let file = xpiEntries.nextFile;
> +            if (!(file instanceof Ci.nsIFile))

This is one of the examples of what I meant about nsIDirectoryEnumerator. Bug 772238 changes the code for most of these - but not this one!

.nextFile can only be a nsIFile, so you should just completely remove this check.

@@ +1927,5 @@
>        let seenFiles = [];
>        let entries = stagingDir.directoryEntries
>                                .QueryInterface(Ci.nsIDirectoryEnumerator);
>        while (entries.hasMoreElements()) {
> +        let stageDirEntry = entries.getNext().QueryInterface(Ci.nsIFile);

This is removed in bug 772238.

@@ +2119,5 @@
>                             .QueryInterface(Ci.nsIDirectoryEnumerator);
>      let entry;
>      while (entry = entries.nextFile) {
>        // Should never happen really
> +      if (!(entry instanceof Ci.nsIFile))

Ditto.

@@ +6226,5 @@
>     * Reads a directory linked to in a file.
>     *
>     * @param   file
>     *          The file containing the directory path
> +   * @return  a nsIFile object representing the linked directory.

Nit: Since you're already changing this comment, would be good to capitalize the sentence.

@@ +6275,5 @@
>                                   .QueryInterface(Ci.nsIDirectoryEnumerator);
>      let entry;
>      while (entry = entries.nextFile) {
>        // Should never happen really
> +      if (!(entry instanceof Ci.nsIFile))

This is removed in bug 772238.

@@ +6537,5 @@
>     * Gets the directory that the add-on with the given ID is installed in.
>     *
>     * @param  aId
>     *         The ID of the add-on
> +   * @return the nsIFile

Nit: Capitalize?

Comment 3

[Mark Capella [:capella]]

Nice. I'll hold this until bug 772238 lands, then rebase, and re-post  it for review @ that time.

Comment 4

[Mark Capella [:capella]]

Attachment: Patch (v2)

Rebased, to include changes done for bug 772238, plus few commented nits and a trailing whitespace fix-up to keep splinter happy.

Comment 5

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Comment on attachment 651925 [details] [diff] [review]
Patch (v2)

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

Good to go :)

Comment 6

[Blair McBride [:Unfocused] (UNAVAILABLE)]

https://hg.mozilla.org/integration/fx-team/rev/cdca67c9cf4b

Comment 7

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/mozilla-central/rev/cdca67c9cf4b