Closed Bug 749930 Opened 8 years ago Closed 8 years ago

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

Categories

(Core :: XPCOM, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla16

People

(Reporter: darktrojan, Assigned: darktrojan)

References

Details

(Keywords: dev-doc-needed)

Attachments

(4 files, 5 obsolete files)

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.
Attached patch WIP (obsolete) — Splinter Review
Drive-by inspections welcome. There's bound to be some whitespace inconsistencies that I've missed, and probably other stuff too.
Attached patch WIP (obsolete) — Splinter Review
Attachment #619279 - Attachment is obsolete: true
Attached patch WIP (comm-central) (obsolete) — Splinter Review
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.
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 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+
Attached patch v3Splinter Review
Attachment #622183 - Attachment is obsolete: true
Attached patch changes between v2 and v3 (obsolete) — Splinter Review
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+.
Attachment #628542 - Attachment description: WIP 3 → v3
Attached patch c-c v2Splinter Review
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)
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
(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)
Attachment #628546 - Attachment is obsolete: true
Attachment #630138 - Flags: review?(benjamin)
Attachment #630138 - Flags: review?(benjamin) → review+
https://hg.mozilla.org/mozilla-central/rev/a15d75939cd5
https://hg.mozilla.org/comm-central/rev/9e8f283a8af1
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Flags: in-testsuite-
Flags: in-moztrap-
Flags: in-litmus-
Resolution: --- → FIXED
Depends on: 762079
Attached patch stray uses of QI (obsolete) — Splinter Review
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-
Attached patch stray uses of QISplinter Review
Er, oops.
Attachment #634860 - Attachment is obsolete: true
Attachment #634864 - Flags: review?(neil)
Attachment #634864 - Flags: review?(neil) → review+
What is the target milestone of this?

Also, there are still some uses of nsILocalFile in /mozilla cpp files...
Target Milestone: --- → mozilla16
You need to log in before you can comment on or make changes to this bug.