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

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Geoff Lankow (:darktrojan)]

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.

Comment 1

[Geoff Lankow (:darktrojan)]

Attachment: WIP

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

Comment 2

[Geoff Lankow (:darktrojan)]

Attachment: WIP

Comment 3

[Geoff Lankow (:darktrojan)]

Attachment: WIP (comm-central)

Comment 4

[Geoff Lankow (:darktrojan)]

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.

Comment 5

[Masatoshi Kimura [:emk]]

>-    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.

Comment 6

[Geoff Lankow (:darktrojan)]

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

[Josh Matthews [:jdm]]

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!

Comment 8

[Geoff Lankow (:darktrojan)]

Attachment: v3

Comment 9

[Geoff Lankow (:darktrojan)]

Attachment: 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+.

Comment 10

[Geoff Lankow (:darktrojan)]

Attachment: c-c v2

Comment 11

[Geoff Lankow (:darktrojan)]

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?

Comment 12

[Philipp Kewisch [:Fallen][📱📆]]

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

Comment 13

[Geoff Lankow (:darktrojan)]

(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.

Comment 14

[Benjamin Smedberg]

Comment on attachment 628542 [details] [diff] [review]
v3

Yes, I trust jdm's review on this.

Comment 15

[Philipp Kewisch [:Fallen][📱📆]]

Comment on attachment 628551 [details] [diff] [review]
c-c v2

r+ on the calendar parts.

Comment 16

[[e-mail]]

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 ;-)

Comment 17

[Mark Banner (:standard8)]

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.

Comment 18

[Geoff Lankow (:darktrojan)]

Attachment: changes between v3 and v4

Comment 19

[Geoff Lankow (:darktrojan)]

https://hg.mozilla.org/mozilla-central/rev/a15d75939cd5
https://hg.mozilla.org/comm-central/rev/9e8f283a8af1

Comment 20

[Geoff Lankow (:darktrojan)]

Attachment: stray uses of QI

Comment 21

[[e-mail]]

Comment on attachment 634860 [details] [diff] [review]
stray uses of QI

localDir is used later.

Comment 22

[Geoff Lankow (:darktrojan)]

Attachment: stray uses of QI

Er, oops.

Comment 23

[Geoff Lankow (:darktrojan)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/52e51974da21

Comment 24

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/52e51974da21

Comment 25

[:aceman]

What is the target milestone of this?

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