Closed Bug 779407 Opened 9 years ago Closed 9 years ago

Remove usage of nsILocalFile in Add-ons Manager

Categories

(Toolkit :: Add-ons Manager, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla17

People

(Reporter: Unfocused, Assigned: capella)

References

Details

(Whiteboard: [good first bug][mentor=bmcbride@mozilla.com][lang=js])

Attachments

(1 file, 1 obsolete file)

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.
Assignee: nobody → markcapella
Status: NEW → ASSIGNED
Attached patch Patch (v1) (obsolete) — Splinter Review
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
Attachment #649013 - Flags: review?(bmcbride)
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?
Attachment #649013 - Flags: review?(bmcbride) → review-
Nice. I'll hold this until bug 772238 lands, then rebase, and re-post  it for review @ that time.
Depends on: 772238
Attached patch Patch (v2)Splinter Review
Rebased, to include changes done for bug 772238, plus few commented nits and a trailing whitespace fix-up to keep splinter happy.
Attachment #649013 - Attachment is obsolete: true
Attachment #651925 - Flags: review?(bmcbride)
Comment on attachment 651925 [details] [diff] [review]
Patch (v2)

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

Good to go :)
Attachment #651925 - Flags: review?(bmcbride) → review+
https://hg.mozilla.org/mozilla-central/rev/cdca67c9cf4b
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla17
You need to log in before you can comment on or make changes to this bug.