Note: There are a few cases of duplicates in user autocompletion which are being worked on.

Replace uses of nsILocalFile with nsIFile (C++ bits)

RESOLVED FIXED in mozilla16

Status

()

Core
XPCOM
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: darktrojan, Assigned: darktrojan)

Tracking

({dev-doc-needed})

Trunk
mozilla16
dev-doc-needed
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite -
in-litmus -
in-moztrap -

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(4 attachments, 5 obsolete attachments)

(Assignee)

Description

5 years ago
Erk, this is a pain as everything depends on everything else, except the JS stuff. Currently trying to figure out the easiest way to review, and how to keep the bitrot at bay.
(Assignee)

Comment 1

5 years ago
Created attachment 619279 [details] [diff] [review]
WIP

Drive-by inspections welcome. There's bound to be some whitespace inconsistencies that I've missed, and probably other stuff too.
(Assignee)

Comment 2

5 years ago
Created attachment 622183 [details] [diff] [review]
WIP
Attachment #619279 - Attachment is obsolete: true
(Assignee)

Comment 3

5 years ago
Created attachment 622186 [details] [diff] [review]
WIP (comm-central)
(Assignee)

Comment 4

5 years ago
I think this is ready to be reviewed. I'm aiming to land it at the start of the next release cycle to give downstream projects as much time as possible.

To avoid reviewer death-by-boredom, as it's mostly straight find/replace changes, I could split it by module and assign to different reviewers. Alternatively I have the bits where I actually change code as a separate (much smaller) patch from the find/replace stuff.
>-    nsCOMPtr<nsILocalFile> localFile;
>+    nsCOMPtr<nsIFile> localFile;
>     rv = NS_NewLocalFile(xpcomStr, false, getter_AddRefs(localFile));
>     NS_ENSURE_SUCCESS(rv, rv);
> 
>     file = do_QueryInterface(localFile);
Please remove localFile. nsIFile-to-nsIFile QI is meaningless.
(Assignee)

Comment 6

5 years ago
I've created a diff without the chunks that are nothing but s/nsILocalFile\b/nsIFile/g (ie. just the 'interesting' bits) - it's about a fifth of the size of the full diff. Benjamin, would you be prepared to review that?

Comment 7

5 years ago
Comment on attachment 622183 [details] [diff] [review]
WIP

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

I read through it all. Nothing looks broken, there are just some small cleanups that can be done.

::: content/base/src/nsDOMFile.cpp
@@ +562,1 @@
>      rv = NS_NewLocalFile(xpcomStr, false, getter_AddRefs(localFile));

Scrap localFile, just use the file from previously and remove the extraneous QI below.

::: content/html/content/src/nsHTMLInputElement.cpp
@@ +1099,5 @@
>      }
>  
>      if (!file) {
>        // this is no "file://", try as local file
> +      nsCOMPtr<nsIFile> localFile;

Scrap this, use file below and skip the QI.

::: dom/indexedDB/IndexedDatabaseManager.cpp
@@ +321,5 @@
>  
>    rv = directory->Append(originSanitized);
>    NS_ENSURE_SUCCESS(rv, rv);
>  
> +  directory.forget(reinterpret_cast<nsIFile**>(aDirectory));

No cast necessary.

::: dom/plugins/base/nsPluginDirServiceProvider.cpp
@@ +518,5 @@
>        if (NS_SUCCEEDED(rv) && childKey) {
>          nsAutoString path;
>          rv = childKey->ReadStringValue(NS_LITERAL_STRING("Path"), path);
>          if (NS_SUCCEEDED(rv)) {
> +          nsCOMPtr<nsIFile> localFile;

This comment is about code outside of the context shown - there's a do_QueryInterface between localFile and temp that is now redundant.

::: modules/libjar/nsJAR.cpp
@@ +265,5 @@
>    //XXX If we guarantee that rv in the case of a non-empty directory
>    //XXX is always FILE_DIR_NOT_EMPTY, we can remove
>    //XXX |rv == NS_ERROR_FAILURE| - bug 322183 needs to be completely
>    //XXX fixed before that can happen
> +  nsresult rv;

Combine this with the next line.

::: modules/libjar/nsZipArchive.cpp
@@ +260,2 @@
>    nsresult rv;
> +  rv = nsZipHandle::Init(aFile, getter_AddRefs(handle));

Combine this with the previous line.

::: profile/dirserviceprovider/src/nsProfileDirServiceProvider.cpp
@@ +118,2 @@
>    if (mSharingEnabled)
>      dirToLock = do_QueryInterface(mNonSharedProfileDir);

No QI.

@@ +118,4 @@
>    if (mSharingEnabled)
>      dirToLock = do_QueryInterface(mNonSharedProfileDir);
>    else
>      dirToLock = do_QueryInterface(mProfileDir);

No QI.

::: profile/dirserviceprovider/src/nsProfileLock.cpp
@@ +498,1 @@
>      rv = aProfileDir->Clone((nsIFile **)((void **)getter_AddRefs(lockFile)));

Please remove this shameful relic of history.

@@ +569,1 @@
>      rv = aProfileDir->Clone((nsIFile **)((void **)getter_AddRefs(oldLockFile)));

Likewise.

::: xpcom/build/nsXPCOMCID.h
@@ +53,5 @@
>   * XPCOM File
>   *   The file abstraction provides ways to obtain and access files and 
>   *   directories located on the local system. 
>   *
> + *   This contract supports the nsIFile interface and the nsIFile interface.

But what if I want the nsIFile interface? There's too much choice!

::: xpcom/string/public/nsXPCOMStrings.h
@@ +771,5 @@
>  
>    /* Conversion from UTF-16 to the native filesystem charset may result in a
>     * loss of information.  No attempt is made to protect against data loss in
>     * this case.  The native filesystem charset applies to strings passed to
> +   * the "Native" method variants on nsIFile and nsIFile. */

Once again, with feeling!
Attachment #622183 - Flags: feedback+
Keywords: dev-doc-needed
(Assignee)

Comment 8

5 years ago
Created attachment 628542 [details] [diff] [review]
v3
Attachment #622183 - Attachment is obsolete: true
(Assignee)

Comment 9

5 years ago
Created attachment 628546 [details] [diff] [review]
changes between v2 and v3

The license changes made an absolute mess of the interdiff, so here's a lovingly hand-crafted diff of the changes made since Josh's f+.
(Assignee)

Updated

5 years ago
Attachment #628542 - Attachment description: WIP 3 → v3
(Assignee)

Comment 10

5 years ago
Created attachment 628551 [details] [diff] [review]
c-c v2
Attachment #622186 - Attachment is obsolete: true
Attachment #628551 - Flags: review?(philipp)
Attachment #628551 - Flags: review?(neil)
Attachment #628551 - Flags: review?(mbanner)
Attachment #628551 - Flags: review?(bugspam.Callek)
(Assignee)

Comment 11

5 years ago
Comment on attachment 628542 [details] [diff] [review]
v3

Benjamin, Josh suggests that his f+ on the previous version (and someone reviewing the changes since) might be good enough. Is that okay with you?
Attachment #628542 - Flags: review?(benjamin)
Comment on attachment 628551 [details] [diff] [review]
c-c v2

What about the JS callers? There are 13 JS files in calendar/ that use nsILocalFile
(Assignee)

Comment 13

5 years ago
(In reply to Philipp Kewisch [:Fallen] (away until May 28th) from comment #12)
> Comment on attachment 628551 [details] [diff] [review]
> c-c v2
> 
> What about the JS callers? There are 13 JS files in calendar/ that use
> nsILocalFile

Yes there are, I'll fix them in a new bug after this lands (was going to do it here but this bug's getting big enough already). They're not affected by the changes to interfaces. Sorry, I should've been more explicit about that.
Summary: Replace uses of nsILocalFile with nsIFile → Replace uses of nsILocalFile with nsIFile (C++ bits)
Comment on attachment 628542 [details] [diff] [review]
v3

Yes, I trust jdm's review on this.
Attachment #628542 - Flags: review?(benjamin) → review+
Comment on attachment 628551 [details] [diff] [review]
c-c v2

r+ on the calendar parts.
Attachment #628551 - Flags: review?(philipp) → review+
Comment on attachment 628551 [details] [diff] [review]
c-c v2

Seems legit. Patch was mostly useless context though :-(

I assume you're going to reindent nsMsgSend.cpp etc. :-)

You fixed some of the silly code but I won't ask you to fix all of it ;-)
Attachment #628551 - Flags: review?(neil) → review+
Comment on attachment 628551 [details] [diff] [review]
c-c v2

This looks fine to me, thanks for doing the work. I'll make this an sr since you technically need that for changing interfaces. I don't think you need Callek's review as you have got Neil's.
Attachment #628551 - Flags: superreview+
Attachment #628551 - Flags: review?(mbanner)
Attachment #628551 - Flags: review?(bugspam.Callek)
(Assignee)

Comment 18

5 years ago
Created attachment 630138 [details] [diff] [review]
changes between v3 and v4
Attachment #628546 - Attachment is obsolete: true
Attachment #630138 - Flags: review?(benjamin)
Attachment #630138 - Flags: review?(benjamin) → review+
(Assignee)

Comment 19

5 years ago
https://hg.mozilla.org/mozilla-central/rev/a15d75939cd5
https://hg.mozilla.org/comm-central/rev/9e8f283a8af1
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Flags: in-testsuite-
Flags: in-moztrap-
Flags: in-litmus-
Resolution: --- → FIXED

Updated

5 years ago
Depends on: 762079
(Assignee)

Comment 20

5 years ago
Created attachment 634860 [details] [diff] [review]
stray uses of QI
Attachment #634860 - Flags: review?(neil)
Comment on attachment 634860 [details] [diff] [review]
stray uses of QI

localDir is used later.
Attachment #634860 - Flags: review?(neil) → review-
(Assignee)

Comment 22

5 years ago
Created attachment 634864 [details] [diff] [review]
stray uses of QI

Er, oops.
Attachment #634860 - Attachment is obsolete: true
Attachment #634864 - Flags: review?(neil)

Updated

5 years ago
Attachment #634864 - Flags: review?(neil) → review+
(Assignee)

Comment 23

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/52e51974da21
https://hg.mozilla.org/mozilla-central/rev/52e51974da21

Comment 25

4 years ago
What is the target milestone of this?

Also, there are still some uses of nsILocalFile in /mozilla cpp files...

Updated

4 years ago
Target Milestone: --- → mozilla16
You need to log in before you can comment on or make changes to this bug.